From bcc0edeecb817e7459fb5ccdbd0362a28469f616 Mon Sep 17 00:00:00 2001 From: Anastasios Date: Wed, 12 Aug 2026 09:58:39 +0300 Subject: [PATCH] =?UTF-8?q?feat:=205=20generic=20harness=20fixes=20?= =?UTF-8?q?=E2=80=94=20HermesRunner,=20judge=20decontamination,=20B1-B4=20?= =?UTF-8?q?defects?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cherry-picked from hermes-evolution-lab onto clean upstream/main. Fixes: - HermesRunner: model-agnostic agent runner - Judge decontamination: prevent model-under-test from grading itself - B1: baseline skill execution arm for candidate-vs-baseline comparison - B2: raw output retention, infrastructure return codes - B3: model/provider provenance in evidence metadata - B4: self-judging confound guard at CLI level 763 tests pass, 1 skipped. 19 files, +2,907/-49 lines. --- skill_eval/agent_runner.py | 99 +++- skill_eval/cli.py | 100 ++++ skill_eval/compare.py | 5 +- skill_eval/eval_schemas.py | 20 +- skill_eval/functional.py | 372 +++++++++++- skill_eval/grading.py | 131 ++++- skill_eval/hermes_runner.py | 443 +++++++++++++++ skill_eval/trigger.py | 19 + tests/fixtures/hermes_output/README.md | 101 ++++ tests/fixtures/hermes_output/smoke-runs.md | 139 +++++ .../usage_custom_shim_zero_cost.json | 18 + .../hermes_output/usage_deepseek_v4_pro.json | 18 + .../hermes_output/usage_kimi_quota_403.json | 18 + tests/test_functional.py | 322 ++++++++++- tests/test_grading_judge_runner.py | 196 +++++++ tests/test_hermes_runner.py | 533 ++++++++++++++++++ tests/test_judge_wiring.py | 299 ++++++++++ tests/test_runner_capabilities.py | 120 ++++ 18 files changed, 2906 insertions(+), 47 deletions(-) create mode 100644 skill_eval/hermes_runner.py create mode 100644 tests/fixtures/hermes_output/README.md create mode 100644 tests/fixtures/hermes_output/smoke-runs.md create mode 100644 tests/fixtures/hermes_output/usage_custom_shim_zero_cost.json create mode 100644 tests/fixtures/hermes_output/usage_deepseek_v4_pro.json create mode 100644 tests/fixtures/hermes_output/usage_kimi_quota_403.json create mode 100644 tests/test_grading_judge_runner.py create mode 100644 tests/test_hermes_runner.py create mode 100644 tests/test_judge_wiring.py create mode 100644 tests/test_runner_capabilities.py diff --git a/skill_eval/agent_runner.py b/skill_eval/agent_runner.py index 9628c73..6401ecf 100644 --- a/skill_eval/agent_runner.py +++ b/skill_eval/agent_runner.py @@ -21,6 +21,52 @@ LOG = logging.getLogger("skill_eval.agent_runner") +# --------------------------------------------------------------------------- +# Infrastructure return codes +# --------------------------------------------------------------------------- +# +# These belong to the runner contract, not to any one backend: every runner +# reports them and `functional.py` reads them to keep a harness failure out of +# the skill's pass rate. A real CLI exit status is >= 0, so the negative space +# is free for sentinels — and POSIX already uses it the same way (subprocess +# returns -N when a child dies on signal N), which is why "any rc < 0 is +# infrastructure" is a safe rule downstream. +# +# They MUST stay distinct. Collapsing them onto a shared -1 makes a provider +# timeout indistinguishable from a missing binary, and both then read as skill +# failure. +RC_TIMEOUT = -1 +RC_LAUNCH_FAILED = -2 +# The provider refused or aborted the request while the CLI itself exited +# cleanly. This is the only infra class a return code cannot express on its +# own: `hermes -z` exits 0 and prints the provider's error where the assistant's +# answer belongs, so the runner has to synthesise the code from out-of-band +# evidence (its usage file). Without it, a quota exhaustion is scored as a +# skill that failed its assertions — MVE AC-6's exact prohibition. +RC_PROVIDER_ERROR = -3 + + +# --------------------------------------------------------------------------- +# Run provenance +# --------------------------------------------------------------------------- +# +# The key inside `token_counts` under which a runner reports WHO served the +# call — model, provider, session id, cost status. It rides in `token_counts` +# because that dict is already threaded from `run_prompt` through +# `parse_output` into every execution-metrics block; a parallel channel would +# have to be plumbed through the same four call sites and could drift out of +# step with the counts it describes. +# +# Underscore-prefixed so it cannot collide with a token bucket, and every +# consumer reads counts by explicit key (`counts.get("input_tokens", 0)`), so a +# non-integer value here is never summed as spend. +# +# It is a runner-contract constant rather than a Hermes-local one for the same +# reason the RC_* codes are: `evolve_evidence.py` reads it for whichever runner +# it was handed, and must not import a backend to do so. +RUN_META_KEY = "_meta" + + # --------------------------------------------------------------------------- # Abstract interface # --------------------------------------------------------------------------- @@ -80,9 +126,39 @@ def total_tokens(self, token_counts: dict) -> int: """Return total token consumption from a token_counts dict. Default: input_tokens + output_tokens (cache tokens excluded). + + Runners whose backend reports token classes outside this pair (e.g. + reasoning tokens) must override this, or that spend becomes invisible + to cost-efficiency comparison. """ return token_counts.get("input_tokens", 0) + token_counts.get("output_tokens", 0) + def supports_trigger_eval(self) -> bool: + """Whether this runner exposes structured tool events for trigger eval. + + Default True preserves existing behaviour. Runners emitting only plain + text must override to False so the harness reports the gap instead of + inferring activation from prose. + """ + return True + + def config_snapshot(self) -> dict: + """Return the non-secret configuration that shaped this runner's runs. + + This lands verbatim in an evidence document that gets committed and read + by people who were not in the room, so an implementation MUST return + only values that are safe to publish: never an API key, a token, or any + other credential. `evolve_evidence` filters credential-shaped keys as + well, but a runner that relies on that filter is one rename away from a + leak that cannot be undone. + + Defaults to `{}` rather than raising: every runner has to answer the + question, or each consumer needs an `hasattr` guard and a runner that + simply never implemented the hook becomes indistinguishable from one + that genuinely has no configuration. + """ + return {} + # --------------------------------------------------------------------------- # Errors @@ -207,10 +283,16 @@ def run_prompt( except subprocess.TimeoutExpired: elapsed = time.monotonic() - start LOG.debug("Claude timed out after %ds", timeout) - return "", f"Timed out after {timeout}s", -1, elapsed + return "", f"Timed out after {timeout}s", RC_TIMEOUT, elapsed except FileNotFoundError: elapsed = time.monotonic() - start LOG.debug("Claude CLI not found on PATH") + # Known wart: this should be RC_LAUNCH_FAILED, but the -1 is part of + # ClaudeRunner's published contract (tests assert it), so changing + # it belongs in its own change. Diagnosis is the only casualty — + # functional.py treats every rc < 0 as infrastructure, so a missing + # binary is still excluded from the pass rate, just labelled less + # precisely than a HermesRunner launch failure would be. return "", "claude CLI not found", -1, elapsed def parse_output(self, raw: str) -> dict: @@ -345,3 +427,18 @@ def get_runner(name: str = "claude") -> AgentRunner: # Register the built-in ClaudeRunner register_runner("claude", ClaudeRunner) + + +def _register_optional_runners() -> None: + """Self-register bundled optional runners. + + Guarded so a failure here can never break the zero-dependency audit path, + which must keep working with no agent CLI installed at all. + """ + try: + import skill_eval.hermes_runner # noqa: F401 (import registers) + except Exception as exc: # pragma: no cover - defensive + LOG.debug("Hermes runner unavailable: %s", exc) + + +_register_optional_runners() diff --git a/skill_eval/cli.py b/skill_eval/cli.py index c7287d9..94cc193 100644 --- a/skill_eval/cli.py +++ b/skill_eval/cli.py @@ -154,6 +154,64 @@ def main(argv: list[str] | None = None) -> int: snapshot_parser.add_argument("--version", type=str, default=None, help="Version label (default: auto from metadata)") + # evolve-check command — deterministic two-split promotion gate + evolve_parser = subparsers.add_parser( + "evolve-check", + help="Gate a candidate on held-in/held-out evidence (never promotes)", + ) + evolve_parser.add_argument("evidence_path", help="Path to the evidence JSON file") + evolve_parser.add_argument("--lineage", type=str, default=None, + help="Append the decision to this append-only JSONL ledger") + evolve_parser.add_argument("--tolerance", type=float, default=None, + help="Permitted regression per split (default: 0.0)") + evolve_parser.add_argument("--min-improvement", type=float, default=None, + help="Improvement one split must clear (default: 0.01)") + evolve_parser.add_argument("--allow-small-n", action="store_true", + help="Permit fewer than 3 runs per split (discouraged)") + + # evolve-evidence command — produces what evolve-check consumes + evidence_parser = subparsers.add_parser( + "evolve-evidence", + help="Run a frozen suite through both arms and emit gate-ready evidence", + ) + evidence_parser.add_argument("--skill", required=True, + help="Path to the skill directory under test") + evidence_parser.add_argument("--suite", required=True, + help="Path to the frozen suite JSON") + evidence_parser.add_argument("--baseline", type=str, default=None, + help="Path to the incumbent skill the candidate " + "must beat. Without it the baseline is the " + "absence of any skill, and the gate answers " + "'is this better than nothing?' — a question " + "every non-empty first version passes and " + "which says nothing about whether v7 beats " + "v6. Roughly doubles the provider spend.") + evidence_parser.add_argument("--out", required=True, + help="Path to write the evidence JSON") + evidence_parser.add_argument("--agent", type=str, default="claude", + help="Registered agent runner to execute tasks (default: claude)") + evidence_parser.add_argument("--judge-agent", type=str, default=None, + help="Runner that grades assertions the deterministic " + "matchers defer (default: none, and a suite that " + "needs one is refused)") + evidence_parser.add_argument("--force-self-judge", action="store_true", + help="Permit --judge-agent to name the same runner " + "as --agent. Refused by default: the model " + "would grade its own output and the measured " + "delta would blend skill transfer with judge " + "leniency. The override records the confound " + "in the evidence rather than hiding it.") + evidence_parser.add_argument("--runs", type=int, default=3, + help="Runs per split (default: 3)") + evidence_parser.add_argument("--candidate-id", type=str, default=None, + help="Candidate id (default: @)") + evidence_parser.add_argument("--lineage", type=str, default=None, + help="Append the decision to this append-only JSONL ledger") + evidence_parser.add_argument("--allow-small-n", action="store_true", + help="Permit fewer than 3 runs per split (discouraged)") + evidence_parser.add_argument("--timeout", type=int, default=120, + help="Timeout per agent invocation in seconds (default: 120)") + # regression command (Phase 2) regression_parser = subparsers.add_parser("regression", help="Check for regressions against baseline") @@ -179,6 +237,12 @@ def main(argv: list[str] | None = None) -> int: help="Timeout per claude invocation in seconds (default: 120)") functional_parser.add_argument("--agent", type=str, default="claude", help="Agent runner to use (default: claude)") + functional_parser.add_argument("--judge-agent", type=str, default=None, + help="Runner that grades non-deterministic " + "assertions (default: none — they are " + "reported unevaluated). Never defaults " + "to --agent: a model grading its own " + "output inflates its own score.") # trigger command (Phase 3) trigger_parser = subparsers.add_parser("trigger", @@ -346,6 +410,41 @@ def main(argv: list[str] | None = None) -> int: from skill_eval.regression import save_snapshot return save_snapshot(args.skill_path, version=args.version) + elif args.command == "evolve-check": + from skill_eval.evolution_gate import ( + DEFAULT_MIN_IMPROVEMENT, + DEFAULT_TOLERANCE, + run_evolve_check, + ) + return run_evolve_check( + args.evidence_path, + lineage_path=args.lineage, + tolerance=DEFAULT_TOLERANCE if args.tolerance is None else args.tolerance, + min_improvement=( + DEFAULT_MIN_IMPROVEMENT + if args.min_improvement is None + else args.min_improvement + ), + allow_small_n=args.allow_small_n, + ) + + elif args.command == "evolve-evidence": + from skill_eval.evolve_evidence import run_evolve_evidence + return run_evolve_evidence( + skill_path=args.skill, + suite_path=args.suite, + out_path=args.out, + baseline_path=args.baseline, + agent=args.agent, + runs=args.runs, + judge_agent=args.judge_agent, + candidate_id=args.candidate_id, + lineage_path=args.lineage, + allow_small_n=args.allow_small_n, + timeout=args.timeout, + force_self_judge=args.force_self_judge, + ) + elif args.command == "regression": from skill_eval.regression import check_regression return check_regression(args.skill_path, baseline_path=args.baseline, @@ -362,6 +461,7 @@ def main(argv: list[str] | None = None) -> int: dry_run=args.dry_run, timeout=args.timeout, agent=args.agent, + judge_agent=args.judge_agent, ) elif args.command == "trigger": diff --git a/skill_eval/compare.py b/skill_eval/compare.py index 5acd1f9..4a3eec7 100644 --- a/skill_eval/compare.py +++ b/skill_eval/compare.py @@ -183,7 +183,10 @@ def _run_single_skill( parsed = runner.parse_output(stdout) text = parsed["text"] - assertion_results, pass_rate = grade_output(text, eval_case.assertions, timeout=timeout) + # Judge with the executing runner -- see functional.py for why (blocker B1). + assertion_results, pass_rate = grade_output( + text, eval_case.assertions, timeout=timeout, judge_runner=runner, + ) return { "pass_rate": pass_rate, diff --git a/skill_eval/eval_schemas.py b/skill_eval/eval_schemas.py index c85d58c..16ce2d1 100644 --- a/skill_eval/eval_schemas.py +++ b/skill_eval/eval_schemas.py @@ -68,12 +68,30 @@ def from_dict(cls, data: dict) -> "GradingResult": @dataclass class RunPairResult: - """Paired with-skill / without-skill results for one eval case run.""" + """Paired with-skill / without-skill results for one eval case run. + + `infrastructure_error` is set when an arm failed to execute at all — a + provider timeout, a CLI that would not launch, a child killed by signal. + It maps the affected arm ("with_skill" / "without_skill") to a + {kind, rc, detail} record. Such a pair is reported but kept out of the + aggregate means: a harness failure produced no evidence about the skill, + and scoring it as 0% would understate the skill by exactly the harness's + own flakiness. + + `raw_streams` carries the untouched subprocess output of both arms — + `{arm: {"stdout": str, "stderr": str}}` — and is populated only when the + caller asks for it, because the benchmark path writes every pair into + `benchmark.json` and a full stream-json transcript per arm would dwarf the + report it lives in. The evidence path asks for it: there, the streams are + the thing being retained, and they leave for a JSONL ledger of their own. + """ eval_id: str run_index: int with_skill: Optional[dict] = None without_skill: Optional[dict] = None delta_pass_rate: float = 0.0 + infrastructure_error: Optional[dict] = None + raw_streams: Optional[dict] = None def to_dict(self) -> dict: return asdict(self) diff --git a/skill_eval/functional.py b/skill_eval/functional.py index 7613563..d13dd8d 100644 --- a/skill_eval/functional.py +++ b/skill_eval/functional.py @@ -10,23 +10,78 @@ import json import logging import math +import re import shutil import sys import tempfile import time from pathlib import Path -from typing import Optional +from typing import Any, Optional from skill_eval.cost import estimate_eval_cost, format_cost from skill_eval.eval_schemas import ( EvalCase, AssertionResult, GradingResult, RunPairResult, BenchmarkReport, ) -from skill_eval.agent_runner import AgentRunner, AgentNotAvailableError, get_runner -from skill_eval.grading import grade_output +from skill_eval.agent_runner import ( + RC_LAUNCH_FAILED, + RC_PROVIDER_ERROR, + RC_TIMEOUT, + AgentRunner, + AgentNotAvailableError, + get_runner, +) +from skill_eval.grading import NO_JUDGE, grade_output LOG = logging.getLogger("skill_eval.functional") +# --------------------------------------------------------------------------- +# Infrastructure failure detection +# --------------------------------------------------------------------------- +# +# A run that never executed carries no evidence about the skill. Scoring it as +# 0% does not measure the skill — it measures the harness's own flakiness and +# subtracts that from the skill's score. So the runner's return code is +# consumed here, and a failed arm is reported but kept out of every mean. +# +# HARD signal (the return code, authoritative): any rc < 0. A real CLI exit +# status is >= 0, and POSIX already uses the negative space this way — +# `subprocess` returns -N for a child killed by signal N — so "rc < 0 means the +# process did not complete normally" holds for every backend, not only for the +# two named sentinels. A POSITIVE rc is deliberately left alone: that is the +# agent running and choosing to exit non-zero, which IS skill-relevant. +_RC_KINDS = { + RC_TIMEOUT: "timeout", + RC_LAUNCH_FAILED: "launch_failed", + # Synthesised by a runner that saw the CLI exit 0 while its own telemetry + # said the request never completed. It is a distinct kind because the + # operator's fix differs: a timeout means wait or raise the limit, a + # provider error means the account/route is broken. + RC_PROVIDER_ERROR: "provider_error", +} + +# SOFT signal (the output text, advisory only): rc == 0 but the body reads like +# a provider error rather than an answer. This cannot be authoritative — a +# skill under test may legitimately print "403" or discuss rate limits, and +# dropping those runs would silently shrink the sample in a way the operator +# never sees. A match therefore warns and annotates; it never excludes. +_PROVIDER_ERROR_PHRASES = re.compile( + r"(usage limit|rate limit|quota exceeded|insufficient[ _]quota|overloaded" + r"|service unavailable|too many requests|authentication[ _]error" + r"|invalid[ _]api[ _]key|permission[ _]denied)", + re.IGNORECASE, +) + +# Bare HTTP status codes are far too common in ordinary output to match on their +# own, so a code counts only when its own line also carries error vocabulary. +_PROVIDER_ERROR_CODES = re.compile(r"\b(401|403|429|500|502|503|529)\b") +_ERROR_CONTEXT = re.compile( + r"\b(error|errno|failed|forbidden|unauthorized|denied|exceeded|limit" + r"|unavailable|overloaded|retry|refused)\b", + re.IGNORECASE, +) + + # --------------------------------------------------------------------------- # Public entry point # --------------------------------------------------------------------------- @@ -40,6 +95,7 @@ def run_functional_eval( dry_run: bool = False, timeout: int = 120, agent: str = "claude", + judge_agent: Optional[str] = None, ) -> int: """Run functional evaluation on a skill. @@ -52,6 +108,12 @@ def run_functional_eval( dry_run: If True, load and validate evals but do not execute. timeout: Timeout per claude invocation in seconds. agent: Name of the registered agent runner (default: "claude"). + judge_agent: Name of the runner that grades assertions the + deterministic matchers defer. ``None`` (the default) means no + judge runs at all and those assertions are reported unevaluated. + It deliberately does NOT fall back to `agent`: a model grading its + own output inflates its own score, and the inflation cannot be + separated from real skill transfer afterwards. Returns: Exit code: 0 = passed, 1 = failed, 2 = error. @@ -94,6 +156,26 @@ def run_functional_eval( print(f"Error: {e}", file=sys.stderr) return 2 + # Resolve the judge — only when one was actually asked for. An + # unresolvable judge is fatal rather than a downgrade to NO_JUDGE: the + # operator asked for judged assertions, and quietly not judging them would + # depress the score for a reason found only in a log line. + judge_runner = NO_JUDGE + if judge_agent: + try: + judge_runner = get_runner(judge_agent) + judge_runner.check_available() + except (KeyError, AgentNotAvailableError) as e: + print(f"Error resolving --judge-agent {judge_agent!r}: {e}", file=sys.stderr) + return 2 + if judge_agent == agent: + LOG.warning( + "--judge-agent %r is the same runner as --agent %r: the model " + "under test is grading its own output, so any score it " + "produces blends skill transfer with judge leniency", + judge_agent, agent, + ) + # Execute eval pairs skill_name = path.name frontmatter = _read_skill_name(path) @@ -108,6 +190,7 @@ def run_functional_eval( for run_idx in range(runs_per_eval): pair, with_grading, without_grading = _execute_eval_pair( eval_case, path, evals_dir, run_idx, timeout, runner=runner, + judge_runner=judge_runner, ) all_pairs.append(pair) all_grading.append(with_grading) @@ -116,6 +199,7 @@ def run_functional_eval( # Aggregate benchmark report = _aggregate_benchmark( skill_name, str(path), eval_cases, all_pairs, all_grading, runs_per_eval, + runner=runner, ) # Write benchmark.json @@ -229,6 +313,71 @@ def _read_skill_name(skill_path: Path) -> Optional[str]: return None +def _classify_rc(rc: int, stderr: str) -> Optional[dict]: + """Map a runner return code to an infrastructure-error record, or None. + + Returns None for rc >= 0 — a normally-exited process, whose output is real + evidence about the skill even when the exit status is non-zero. + """ + if rc >= 0: + return None + kind = _RC_KINDS.get(rc) + if kind is None: + # Negative but not a named sentinel: subprocess reports -N for a child + # killed by signal N. An OOM kill or SIGTERM is infrastructure too. + kind = "signal" + return { + "kind": kind, + "rc": rc, + "detail": (stderr or "").strip()[:500], + } + + +def _detect_provider_error_text(text: str) -> Optional[dict]: + """Look for provider-error prose in an output that exited cleanly. + + Advisory only. Returns a {match, snippet} record or None; the caller warns + and annotates but must not exclude the run — see the module-level note. + """ + if not text: + return None + + phrase = _PROVIDER_ERROR_PHRASES.search(text) + if phrase: + return {"match": phrase.group(1), "snippet": text.strip()[:200]} + + for line in text.splitlines(): + code = _PROVIDER_ERROR_CODES.search(line) + if code and _ERROR_CONTEXT.search(line): + return {"match": code.group(1), "snippet": line.strip()[:200]} + return None + + +def _build_execution_metrics( + parsed: dict, + infra: Optional[dict], + pair_excluded: bool, + suspected: Optional[dict], +) -> dict: + """Assemble execution_metrics, carrying the infrastructure verdict along. + + `pair_excluded` is a property of the PAIR, not the arm: the comparison is + with-vs-without, so a surviving arm whose partner died has no counterpart + to be compared against and must leave the means with it. + """ + metrics = { + "tool_calls": len(parsed["tool_calls"]), + "token_counts": parsed["token_counts"], + } + if infra: + metrics["infrastructure_error"] = infra + if pair_excluded: + metrics["excluded_from_means"] = True + if suspected: + metrics["suspected_infrastructure_error"] = suspected + return metrics + + def _execute_eval_pair( eval_case: EvalCase, skill_path: Path, @@ -236,8 +385,24 @@ def _execute_eval_pair( run_index: int, timeout: int, runner: Optional[AgentRunner] = None, + judge_runner: Any = NO_JUDGE, + capture_raw: bool = False, ) -> tuple[RunPairResult, GradingResult, GradingResult]: - """Run an eval case with and without the skill, grade both outputs.""" + """Run an eval case with and without the skill, grade both outputs. + + `judge_runner` defaults to NO_JUDGE, never to `runner`. The default has to + be the safe one: a caller who forgets the argument must end up with an + unjudged assertion they can see in the report, not a self-graded one they + cannot detect afterwards. + + `capture_raw` attaches each arm's untouched stdout/stderr to the returned + pair. It is off by default because the benchmark path serialises every + pair into one report, and on for the evidence path, whose whole claim is + that a verdict can be re-derived from the output it was read off. What + grading keeps is not that output: `GradingResult.raw_output` is the parsed + answer, truncated at 2000 characters, so a verdict resting on the envelope + around it or on text past the cut has no record at all. + """ if runner is None: runner = get_runner("claude") @@ -308,12 +473,63 @@ def _execute_eval_pair( ) without_parsed = runner.parse_output(without_stdout) - # Grade both outputs with_text = with_parsed["text"] without_text = without_parsed["text"] - with_results, with_pass_rate = grade_output(with_text, eval_case.assertions, timeout=timeout) - without_results, without_pass_rate = grade_output(without_text, eval_case.assertions, timeout=timeout) + # Consume the return codes. Before this, `with_rc`/`without_rc` were + # unpacked and never read again, so a 620s DeepSeek timeout reached grading + # as empty output and was averaged in as "the skill produced nothing". + with_infra = _classify_rc(with_rc, with_stderr) + without_infra = _classify_rc(without_rc, without_stderr) + + infra: dict = {} + if with_infra: + infra["with_skill"] = with_infra + if without_infra: + infra["without_skill"] = without_infra + + pair_excluded = bool(infra) + if pair_excluded: + LOG.warning( + "Infrastructure failure on %s[run %d]: %s — pair excluded from means", + eval_case.id, run_index, + ", ".join(f"{arm}={rec['kind']}(rc={rec['rc']})" for arm, rec in infra.items()), + ) + + # Advisory pass over arms that DID exit cleanly. Never affects exclusion. + with_suspected = None if with_infra else _detect_provider_error_text(with_text) + without_suspected = None if without_infra else _detect_provider_error_text(without_text) + for arm, suspected in (("with_skill", with_suspected), ("without_skill", without_suspected)): + if suspected: + LOG.warning( + "Run %s[%d] %s exited 0 but its output matches provider-error " + "pattern %r — counted as a real result, verify manually: %s", + eval_case.id, run_index, arm, suspected["match"], suspected["snippet"], + ) + + # Grade both outputs. An arm that never executed is not graded: there is + # nothing to judge, and spending a judge call on it would bill for noise. + # + # Both arms share whatever judge the caller chose, so the with/without + # delta stays internally consistent. What is deliberately NOT done here is + # defaulting that judge to `runner`: it closed the Claude leak (blocker B1) + # by making every model grade itself, which trades a known contaminant for + # one that hides inside the headline cross-model number. + if with_infra: + with_results, with_pass_rate = [], 0.0 + else: + with_results, with_pass_rate = grade_output( + with_text, eval_case.assertions, timeout=timeout, + judge_runner=judge_runner, + ) + + if without_infra: + without_results, without_pass_rate = [], 0.0 + else: + without_results, without_pass_rate = grade_output( + without_text, eval_case.assertions, timeout=timeout, + judge_runner=judge_runner, + ) with_grading = GradingResult( eval_id=eval_case.id, @@ -321,10 +537,9 @@ def _execute_eval_pair( assertion_results=[r.to_dict() for r in with_results], pass_rate=with_pass_rate, summary=f"With skill: {with_pass_rate:.0%} assertions passed", - execution_metrics={ - "tool_calls": len(with_parsed["tool_calls"]), - "token_counts": with_parsed["token_counts"], - }, + execution_metrics=_build_execution_metrics( + with_parsed, with_infra, pair_excluded, with_suspected, + ), timing={"elapsed_seconds": with_elapsed}, raw_output=with_text[:2000], ) @@ -335,10 +550,9 @@ def _execute_eval_pair( assertion_results=[r.to_dict() for r in without_results], pass_rate=without_pass_rate, summary=f"Without skill: {without_pass_rate:.0%} assertions passed", - execution_metrics={ - "tool_calls": len(without_parsed["tool_calls"]), - "token_counts": without_parsed["token_counts"], - }, + execution_metrics=_build_execution_metrics( + without_parsed, without_infra, pair_excluded, without_suspected, + ), timing={"elapsed_seconds": without_elapsed}, raw_output=without_text[:2000], ) @@ -348,7 +562,17 @@ def _execute_eval_pair( run_index=run_index, with_skill=with_grading.to_dict(), without_skill=without_grading.to_dict(), - delta_pass_rate=with_pass_rate - without_pass_rate, + # A broken pair has no measured delta. Reporting 0.0 - 0.0 = 0.0 is + # the honest value only because the pair is also excluded downstream; + # it must never be read as "the skill made no difference". + delta_pass_rate=0.0 if pair_excluded else with_pass_rate - without_pass_rate, + infrastructure_error=infra or None, + # Verbatim, and in particular not truncated: the cut is what made the + # retained text unable to support the verdict beside it. + raw_streams={ + "with_skill": {"stdout": with_stdout, "stderr": with_stderr}, + "without_skill": {"stdout": without_stdout, "stderr": without_stderr}, + } if capture_raw else None, ) return pair, with_grading, without_grading @@ -361,11 +585,37 @@ def _aggregate_benchmark( pairs: list[RunPairResult], gradings: list[GradingResult], runs_per_eval: int, + *, + runner: AgentRunner, ) -> BenchmarkReport: - """Compute aggregated benchmark statistics and 4-dimension scores.""" + """Compute aggregated benchmark statistics and 4-dimension scores. + + `runner` is required, not optional: token totals must come from + `runner.total_tokens()` so that backends reporting classes outside + input/output (e.g. reasoning tokens) are counted. A default here would let + a new caller silently reinstate the under-count it replaced. + """ - with_gradings = [g for g in gradings if "With skill" in g.summary] - without_gradings = [g for g in gradings if "Without skill" in g.summary] + all_with = [g for g in gradings if "With skill" in g.summary] + all_without = [g for g in gradings if "Without skill" in g.summary] + + # Runs whose infrastructure failed are reported but not measured. Every + # aggregate below — pass rates, tokens, tool calls, cost — reads the + # filtered lists, so a harness failure cannot leak into any of them + # through a metric someone forgot to filter. + with_gradings = [g for g in all_with if not _is_excluded(g)] + without_gradings = [g for g in all_without if not _is_excluded(g)] + + infrastructure = _summarize_infrastructure( + all_with, all_without, with_gradings, without_gradings, + ) + if infrastructure["excluded_runs"]: + LOG.warning( + "%d of %d runs excluded from the aggregate as infrastructure " + "failures (%s); %d pair(s) actually measured", + infrastructure["excluded_runs"], len(all_with) + len(all_without), + infrastructure["kinds"] or "unclassified", infrastructure["graded_pairs"], + ) # Pass rates with_pass_rates = [g.pass_rate for g in with_gradings] @@ -396,13 +646,14 @@ def _aggregate_benchmark( for g in without_gradings ] - # Token counts — total (input + output) + # Token counts — total, as defined by the executing runner. HermesRunner + # adds reasoning tokens here; the base runner sums input + output. with_total_tokens = [ - _total_tokens(g.execution_metrics.get("token_counts", {})) + runner.total_tokens(g.execution_metrics.get("token_counts", {})) for g in with_gradings ] without_total_tokens = [ - _total_tokens(g.execution_metrics.get("token_counts", {})) + runner.total_tokens(g.execution_metrics.get("token_counts", {})) for g in without_gradings ] @@ -416,8 +667,14 @@ def _aggregate_benchmark( style_score = mean_with # Use pass rate as proxy (style assertions are a subset) efficiency_score = _compute_efficiency(with_total_tokens, without_total_tokens, with_pass_rates, without_pass_rates) - # Overall pass: skill must outperform or match no-skill - passed = mean_with >= mean_without and mean_with >= 0.5 + # Overall pass: skill must outperform or match no-skill — and there must be + # at least one measured pair. Without the first clause a benchmark where + # every run timed out reports mean 0.0 vs 0.0 and could be read as a tie. + passed = ( + infrastructure["graded_pairs"] > 0 + and mean_with >= mean_without + and mean_with >= 0.5 + ) mean_with_total = _mean(with_total_tokens) mean_without_total = _mean(without_total_tokens) @@ -448,6 +705,7 @@ def _aggregate_benchmark( "input_tokens": round(_mean(with_input_tokens) - _mean(without_input_tokens), 1), "tool_calls": round(_mean(with_tools) - _mean(without_tools), 1), }, + "infrastructure": infrastructure, } # Cost-efficiency Pareto classification @@ -524,9 +782,48 @@ def _compute_efficiency( return min(1.0, max(0.0, ratio / (1.0 + ratio) * 2)) -def _total_tokens(token_counts: dict) -> int: - """Return input_tokens + output_tokens (cache tokens excluded).""" - return token_counts.get("input_tokens", 0) + token_counts.get("output_tokens", 0) +def _is_excluded(grading: GradingResult) -> bool: + """True when a run must not enter any aggregate mean.""" + return bool(grading.execution_metrics.get("excluded_from_means")) + + +def _summarize_infrastructure( + all_with: list[GradingResult], + all_without: list[GradingResult], + kept_with: list[GradingResult], + kept_without: list[GradingResult], +) -> dict: + """Account for what was dropped, and why. + + An exclusion the report does not mention is indistinguishable from a run + that never happened, which would hide a broken harness behind a healthy + looking score. The counts are therefore part of the report, not just a log + line. + """ + kinds: dict[str, int] = {} + for g in all_with + all_without: + err = g.execution_metrics.get("infrastructure_error") + if err: + kind = err.get("kind", "unknown") + kinds[kind] = kinds.get(kind, 0) + 1 + + return { + "with_skill_errors": sum( + 1 for g in all_with if g.execution_metrics.get("infrastructure_error") + ), + "without_skill_errors": sum( + 1 for g in all_without if g.execution_metrics.get("infrastructure_error") + ), + "suspected_errors": sum( + 1 for g in all_with + all_without + if g.execution_metrics.get("suspected_infrastructure_error") + ), + "excluded_runs": sum(1 for g in all_with + all_without if _is_excluded(g)), + # Pairs are the unit of comparison, so the honest sample size is the + # number of pairs with both arms intact. + "graded_pairs": min(len(kept_with), len(kept_without)), + "kinds": kinds, + } def _mean(values: list[float | int]) -> float: @@ -567,6 +864,27 @@ def _print_functional_report(report: BenchmarkReport) -> None: print(f" Delta: {sign}{dp:.1%}") print(f"{'─' * w}") + # Infrastructure section — printed only when something was dropped, but + # never suppressed: the rates above are computed on the surviving pairs, + # and the operator has to be told how many of those there actually were. + infra = rs.get("infrastructure") or {} + if infra.get("excluded_runs") or infra.get("suspected_errors"): + print(f" Infrastructure:") + print(f" Measured pairs: {infra.get('graded_pairs', 0)}") + print(f" Excluded runs: {infra.get('excluded_runs', 0)}" + f" (with: {infra.get('with_skill_errors', 0)}," + f" without: {infra.get('without_skill_errors', 0)})") + kinds = infra.get("kinds") or {} + if kinds: + print(f" Failure kinds: " + + ", ".join(f"{k}={v}" for k, v in sorted(kinds.items()))) + if infra.get("suspected_errors"): + print(f" Suspected (kept):{infra['suspected_errors']}" + f" → exited 0 but output looks like a provider error") + if not infra.get("graded_pairs"): + print(f" ⚠ No pair completed — the rates above measure nothing.") + print(f"{'─' * w}") + # Token Usage section (skip when all zeros, e.g. dry-run) ws_total = ws.get("mean_total_tokens", 0) wos_total = wos.get("mean_total_tokens", 0) diff --git a/skill_eval/grading.py b/skill_eval/grading.py index 39372a0..d5a3ce0 100644 --- a/skill_eval/grading.py +++ b/skill_eval/grading.py @@ -1,17 +1,65 @@ """Deterministic + LLM assertion grading for functional evaluation. Assertions are natural-language strings from evals.json. Deterministic patterns -are tried first; ambiguous assertions fall back to an LLM judge via the claude CLI. +are tried first; ambiguous assertions fall back to an LLM judge. + +The judge is selected by the caller via ``judge_runner``, which has three +states rather than two: + +* a runner -- that runner judges, and nothing else is imported; +* ``None`` -- the historical claude-CLI path, unchanged, so every existing + caller stays byte-compatible; +* ``NO_JUDGE`` -- no model judges anything. Ambiguous assertions are reported + as unevaluated instead of being handed to whichever model happened to be + nearby. + +The third state exists because ``None`` cannot express it. A caller that +wants "no judge" and passes ``None`` gets Claude, which is the very leak this +parameter was added to close. """ from __future__ import annotations import json import re -from typing import Optional +from typing import TYPE_CHECKING, Optional from skill_eval.eval_schemas import AssertionResult +if TYPE_CHECKING: # pragma: no cover - typing only + from skill_eval.agent_runner import AgentRunner + + +# --------------------------------------------------------------------------- +# The "explicitly nobody" judge +# --------------------------------------------------------------------------- + +class _NoJudge: + """Sentinel meaning: no LLM judge is configured, and none may be invented. + + Distinct from ``None`` on purpose. ``None`` is the historical default and + resolves to Claude; a caller evaluating some other model who passes it gets + a cross-model contaminated score. Distinct from the executing runner on + purpose too: letting the model under test grade its own output blends skill + transfer with judge leniency, and no later analysis can separate them + (MVE section 3, exclusion X4). + """ + + __slots__ = () + + def __repr__(self) -> str: # pragma: no cover - debugging aid + return "NO_JUDGE" + + +NO_JUDGE = _NoJudge() + +_NO_JUDGE_EVIDENCE = ( + "No LLM judge is configured, so this assertion was never evaluated. " + "It is reported unsatisfied because an ungraded assertion is not " + "evidence of success. Pass --judge-agent to grade it with a " + "pinned judge, or rewrite it as a deterministic assertion." +) + # --------------------------------------------------------------------------- # Public entry point @@ -21,9 +69,21 @@ def grade_output( output: str, assertions: list[str], timeout: int = 60, + judge_runner: Optional["AgentRunner"] = None, ) -> tuple[list[AssertionResult], float]: """Grade an output string against a list of assertion strings. + Args: + output: The agent output under evaluation. + assertions: Assertion strings to check. + timeout: Per-judge-call timeout in seconds. + judge_runner: Runner used for the LLM fallback, or ``NO_JUDGE`` to + disable LLM grading entirely. When omitted the historical Claude + path is used unchanged. Callers evaluating a non-Claude agent + MUST pass a runner or ``NO_JUDGE``, otherwise the task is executed + by one model and judged by another — a contaminated cross-model + number (blocker B1). + Returns: Tuple of (list of AssertionResult, pass_rate as 0.0-1.0). """ @@ -37,9 +97,12 @@ def grade_output( else: deferred.append(assertion) - # Batch LLM grading for remaining assertions + # Batch LLM grading for remaining assertions. Deterministic-only inputs + # never reach a model, so no judge is spun up for them. if deferred: - llm_results = _llm_grade(output, deferred, timeout=timeout) + llm_results = _llm_grade( + output, deferred, timeout=timeout, judge_runner=judge_runner, + ) results.extend(llm_results) if not results: @@ -209,25 +272,71 @@ def _llm_grade( output: str, assertions: list[str], timeout: int = 60, + judge_runner=None, ) -> list[AssertionResult]: - """Grade ambiguous assertions using the claude CLI as an LLM judge. + """Grade ambiguous assertions with an LLM judge. + + Args: + judge_runner: An ``AgentRunner`` to judge with. When ``None`` (the + default) the historical claude-CLI path is used unchanged, so + existing callers keep byte-identical behaviour. - Falls back to passed=False with explanatory evidence if claude is unavailable. + Passing a judge_runner is what keeps cross-model evaluation honest: without + it, ``--agent hermes`` executed the task through Hermes but judged it with + Claude, contaminating every number it produced. + + Falls back to passed=False with explanatory evidence if the judge is + unavailable. """ - try: - from skill_eval._claude import check_claude_available, run_claude_prompt - check_claude_available() - except Exception: + # Checked before anything else: NO_JUDGE must not reach a check_available() + # probe, an import, or a model call. "No judge" that still touches the + # network is not no judge. + if judge_runner is NO_JUDGE: return [ AssertionResult( text=a, passed=False, - evidence="claude CLI not available; cannot evaluate ambiguous assertion", + evidence=_NO_JUDGE_EVIDENCE, method="llm", + confidence=0.0, + uncertain=True, ) for a in assertions ] + judge_name = "claude" + if judge_runner is None: + try: + from skill_eval._claude import check_claude_available, run_claude_prompt + check_claude_available() + except Exception: + return [ + AssertionResult( + text=a, + passed=False, + evidence="claude CLI not available; cannot evaluate ambiguous assertion", + method="llm", + ) + for a in assertions + ] + else: + judge_name = type(judge_runner).__name__ + try: + judge_runner.check_available() + except Exception: + return [ + AssertionResult( + text=a, + passed=False, + evidence=f"{judge_name} not available; cannot evaluate ambiguous assertion", + method="llm", + ) + for a in assertions + ] + + def run_claude_prompt(prompt, timeout=timeout): # noqa: F811 + return judge_runner.run_prompt(prompt, timeout=timeout) + # Build a structured prompt for batch evaluation assertions_block = "\n".join(f" {i+1}. {a}" for i, a in enumerate(assertions)) # Truncate output to avoid exceeding context limits diff --git a/skill_eval/hermes_runner.py b/skill_eval/hermes_runner.py new file mode 100644 index 0000000..7f19828 --- /dev/null +++ b/skill_eval/hermes_runner.py @@ -0,0 +1,443 @@ +"""Hermes Agent runner — model-agnostic execution for skill evaluation. + +`HermesRunner` shells out to the Hermes Agent CLI in one-shot mode. Because +Hermes itself routes to DeepSeek, Claude, Gemini, Qwen, or a local model, a +single runner gives skill_eval cross-model coverage without one adapter per +provider. + +Stdlib only, matching `agent_runner.py`'s zero-dependency guarantee. + +Three facts were verified against Hermes Agent v0.20.0 source and drive the +implementation below: + +1. `--source` does NOT exist on the top-level `-z` path (only on `hermes chat`). + Passing it aborts the run with `unrecognized arguments`. +2. `hermes -z` emits the final assistant text as plain text — no stream-json, + therefore no structured tool events. Trigger evaluation is unsupported here + rather than faked. +3. Token telemetry arrives only via `--usage-file`, whose field names differ + from skill_eval's and which carries a `reasoning_tokens` field with no + skill_eval equivalent. See `_USAGE_FIELD_MAP` and `total_tokens`. +""" + +from __future__ import annotations + +import json +import logging +import os +import shutil +import subprocess +import tempfile +import time +from pathlib import Path +from typing import Optional + +from skill_eval.agent_runner import ( + RC_LAUNCH_FAILED, + RC_PROVIDER_ERROR, + RC_TIMEOUT, + RUN_META_KEY, + AgentNotAvailableError, + AgentRunner, + register_runner, +) + +LOG = logging.getLogger("skill_eval.hermes_runner") + + +# RC_TIMEOUT / RC_LAUNCH_FAILED / RC_PROVIDER_ERROR are re-exported from +# agent_runner, where they now live: `functional.py` has to read them for every +# backend, so they are a runner-contract constant rather than a Hermes-local +# detail. The names stay importable from here for existing callers. +__all__ = [ + "HermesRunner", + "RC_TIMEOUT", + "RC_LAUNCH_FAILED", + "RC_PROVIDER_ERROR", + "DEFAULT_TIMEOUT", +] + +# DeepSeek V4 Pro can sit queued until a provider-side kill at ~10 minutes. A +# 120s default would score that latency as a skill failure. +DEFAULT_TIMEOUT = 620 + +# Hermes usage-file field -> skill_eval token_counts field. +# `reasoning_tokens` deliberately keeps its own name: it has no skill_eval +# counterpart and must not be silently folded into another bucket. +_USAGE_FIELD_MAP = { + "input_tokens": "input_tokens", + "output_tokens": "output_tokens", + "cache_read_tokens": "cache_read_input_tokens", + "cache_write_tokens": "cache_creation_input_tokens", + "reasoning_tokens": "reasoning_tokens", +} + +_ZERO_COUNTS = { + "input_tokens": 0, + "output_tokens": 0, + "cache_read_input_tokens": 0, + "cache_creation_input_tokens": 0, + "reasoning_tokens": 0, +} + +# Usage-file fields naming WHO served the call. `hermes` is a router — the same +# `--agent hermes` covers DeepSeek, Claude, Gemini and a local shim — so without +# these the evidence names the router and nothing else, and two runs served by +# two different models are indistinguishable after the fact. +# +# The mapping is identity today and written out anyway: a rename upstream then +# costs one line here instead of surfacing as a silent `None` in every evidence +# document produced afterwards. +_META_FIELDS = ("model", "provider", "session_id", "cost_status") + +_NULL_META = {name: None for name in _META_FIELDS} + + +def _empty_counts() -> dict: + """Return a fresh zeroed counts dict carrying its own null `_meta` block. + + A function rather than another module-level constant because `_meta` is a + nested dict and `dict(...)` copies shallowly: every run would otherwise + share one provenance block, and the second run would silently rewrite the + provenance of the first. + """ + counts = dict(_ZERO_COUNTS) + counts[RUN_META_KEY] = dict(_NULL_META) + return counts + + +def _map_meta(data: dict) -> dict: + """Project a usage object's provenance fields, as strings or None. + + Only a non-blank string counts as an answer. A quota-refused run writes + `"model": null` (see `tests/fixtures/hermes_output/usage_kimi_quota_403.json`) + and schema drift could put an object there; both mean *the runner could not + say*, and a document that named a model on that basis would be + indistinguishable from one that measured it. + """ + meta = dict(_NULL_META) + for name in _META_FIELDS: + value = data.get(name) + if isinstance(value, str) and value.strip(): + meta[name] = value.strip() + return meta + +_SKILL_INSTRUCTION = ( + "A skill file named SKILL.md is present in your current working directory. " + "Read it first and follow its instructions for this task.\n\n" +) + + +def _env(name: str) -> Optional[str]: + """Read an env var, treating blank as unset.""" + value = os.environ.get(name, "").strip() + return value or None + + +def _load_usage(path: str) -> dict: + """Return the usage file's raw object, or an empty dict. + + Absence and corruption are both normal: the upstream writer swallows its + own exceptions, so a missing file means "no telemetry", never "the run + failed". Callers must treat `{}` as *no evidence either way*. + """ + try: + with open(path, "r", encoding="utf-8") as fh: + data = json.load(fh) + except (FileNotFoundError, json.JSONDecodeError, OSError) as exc: + LOG.debug("Usage file unavailable (%s): %s", path, exc) + return {} + return data if isinstance(data, dict) else {} + + +def _status_failure_reason(data: dict) -> Optional[str]: + """Describe why the usage file says this run did not succeed, or None. + + `hermes -z` exits **0** when the provider refuses the request and prints + the provider's error where the assistant's answer belongs (reproduced + across three sessions — `tests/fixtures/hermes_output/smoke-runs.md`). The + exit status therefore cannot express this failure at all; these two flags + are the only in-band evidence, and discarding them is what let a quota + exhaustion be graded as a skill that failed its assertions. + + Only an explicit boolean counts. Every upstream field is `result.get(...)` + so `None` is the ordinary shape of a field the CLI could not fill in, and a + string would be schema drift — neither is evidence of failure, and reading + either as one would drop healthy runs out of every mean. + """ + completed = data.get("completed") + failed = data.get("failed") + if completed is False or failed is True: + return ( + f"hermes reported an unsuccessful run in its usage file " + f"(completed={completed!r}, failed={failed!r}) while the CLI itself " + f"exited cleanly; classified as infrastructure, not skill failure" + ) + return None + + +class HermesRunner(AgentRunner): + """Agent runner for the Hermes Agent CLI (one-shot mode). + + Configuration is entirely environment-driven so the evaluation harness has + no Hermes-specific config surface of its own: + + SKILL_EVAL_HERMES_BIN default: hermes + SKILL_EVAL_HERMES_PROVIDER optional --provider + SKILL_EVAL_HERMES_MODEL optional -m + SKILL_EVAL_HERMES_REASONING optional --reasoning + SKILL_EVAL_HERMES_TOOLSETS optional -t + SKILL_EVAL_HERMES_PROFILE_HOME optional subprocess HERMES_HOME + SKILL_EVAL_HERMES_TIMEOUT default: 620 + """ + + def __init__(self) -> None: + # Populated by run_prompt from the usage file; parse_output reads it + # because `-z` carries no inline telemetry. + self.last_token_counts: dict = _empty_counts() + + # -- Configuration ------------------------------------------------------ + + @property + def cli_name(self) -> str: + return _env("SKILL_EVAL_HERMES_BIN") or "hermes" + + def default_timeout(self) -> int: + raw = _env("SKILL_EVAL_HERMES_TIMEOUT") + if raw is None: + return DEFAULT_TIMEOUT + try: + return int(raw) + except ValueError: + LOG.warning("Invalid SKILL_EVAL_HERMES_TIMEOUT=%r; using %d", raw, DEFAULT_TIMEOUT) + return DEFAULT_TIMEOUT + + def check_available(self) -> None: + if shutil.which(self.cli_name) is None: + raise AgentNotAvailableError( + "hermes", + "Install from https://hermes-agent.nousresearch.com/install.sh " + "or set SKILL_EVAL_HERMES_BIN to its path.", + ) + + def supports_trigger_eval(self) -> bool: + """`hermes -z` yields no structured tool events, so activation is unmeasurable.""" + return False + + # -- Command construction ---------------------------------------------- + + def _build_prompt(self, prompt: str, skill_path: Optional[str]) -> str: + """Reference the workspace skill copy; never inline file contents. + + Inlining SKILL.md risks dragging adjacent files (including .env) into + the argv, so the instruction points at the already-copied file instead. + """ + if not skill_path: + return prompt + if not (Path(skill_path) / "SKILL.md").is_file(): + return prompt + return _SKILL_INSTRUCTION + prompt + + def _build_cmd(self, prompt: str, usage_path: str) -> list[str]: + cmd = [ + self.cli_name, + "-z", prompt, + "--cli", + "--no-restore-cwd", + # Isolation: keep MEMORY.md, USER.md, SOUL.md, AGENTS.md and the + # profile's skill catalog out of the measurement. + "--ignore-rules", + "--ignore-user-config", + "--usage-file", usage_path, + ] + # NOTE: `--source` is intentionally absent — see module docstring. + + provider = _env("SKILL_EVAL_HERMES_PROVIDER") + if provider: + cmd.extend(["--provider", provider]) + + model = _env("SKILL_EVAL_HERMES_MODEL") + if model: + cmd.extend(["-m", model]) + + reasoning = _env("SKILL_EVAL_HERMES_REASONING") + if reasoning: + cmd.extend(["--reasoning", reasoning]) + + toolsets = _env("SKILL_EVAL_HERMES_TOOLSETS") + if toolsets: + cmd.extend(["-t", toolsets]) + + return cmd + + def _build_env(self) -> dict: + env = dict(os.environ) + profile_home = _env("SKILL_EVAL_HERMES_PROFILE_HOME") + if profile_home: + # Scoped to the subprocess only; the parent process keeps its own + # HERMES_HOME so evaluation never mutates the live profile. + env["HERMES_HOME"] = profile_home + return env + + # -- Usage-file telemetry ---------------------------------------------- + + def read_usage_file(self, path: str) -> dict: + """Map a Hermes usage file into skill_eval token counts plus provenance. + + The returned dict is token buckets alongside a `_meta` block naming the + model, provider, session and cost status that served the call. `_meta` + rides inside the counts because that dict is already threaded from + `run_prompt` through `parse_output` into every execution-metrics block; + a parallel channel would need the same four call sites plumbed and could + drift out of step with the counts it describes. Nothing sums the counts + blindly — every consumer reads them by explicit key — so a dict in there + is never added to spend. + + Degrades to zeros and a null `_meta` for a missing or malformed file: + the upstream writer swallows its own exceptions, so absence is a normal + outcome and must not be reported as an evaluation failure. + """ + return self._map_counts(_load_usage(path)) + + @staticmethod + def _map_counts(data: dict) -> dict: + """Project an already-loaded usage object onto skill_eval's fields.""" + counts = _empty_counts() + for hermes_key, skill_eval_key in _USAGE_FIELD_MAP.items(): + value = data.get(hermes_key) + # Every upstream field is `result.get(...)`, so None is expected. + counts[skill_eval_key] = int(value) if isinstance(value, (int, float)) else 0 + counts[RUN_META_KEY] = _map_meta(data) + return counts + + def config_snapshot(self) -> dict: + """Report the knobs that shaped this runner's invocations. + + Every value is read back from a `SKILL_EVAL_HERMES_*` variable this + class already consults to build its command line, which is what makes + the snapshot safe to publish: the runner never reads a credential + variable at all. Provider auth lives in the subprocess environment, + which is deliberately not enumerated here — a snapshot that dumped + `os.environ` would put a key in a committed file the first time anyone + exported one. + """ + return { + "cli_name": self.cli_name, + "provider": _env("SKILL_EVAL_HERMES_PROVIDER"), + "model": _env("SKILL_EVAL_HERMES_MODEL"), + "reasoning": _env("SKILL_EVAL_HERMES_REASONING"), + "toolsets": _env("SKILL_EVAL_HERMES_TOOLSETS"), + "profile_home": _env("SKILL_EVAL_HERMES_PROFILE_HOME"), + "timeout": self.default_timeout(), + } + + def total_tokens(self, token_counts: dict) -> int: + """Include reasoning tokens, which the base implementation omits. + + DeepSeek V4 Pro runs with thinking always on. Excluding those tokens + would make Hermes look systematically cheaper than Claude in + `classify_cost_efficiency()`, manufacturing a PARETO_BETTER verdict out + of an accounting gap. + """ + return ( + token_counts.get("input_tokens", 0) + + token_counts.get("output_tokens", 0) + + token_counts.get("reasoning_tokens", 0) + ) + + # -- Execution ---------------------------------------------------------- + + def run_prompt( + self, + prompt: str, + skill_path: Optional[str] = None, + workspace_dir: Optional[str] = None, + timeout: int = 120, + output_format: str = "text", + ) -> tuple[str, str, int, float]: + """Invoke `hermes -z` and return (stdout, stderr, returncode, elapsed). + + `output_format` is accepted for interface compatibility but ignored: + one-shot Hermes emits plain text only. + """ + # The caller's 120s default predates DeepSeek's queue behaviour; only + # an explicit non-default override should shorten our window. + effective_timeout = timeout if timeout != 120 else self.default_timeout() + + fd, usage_path = tempfile.mkstemp(prefix="hermes_usage_", suffix=".json") + os.close(fd) + os.unlink(usage_path) # let Hermes create it; absence is meaningful + + cmd = self._build_cmd(self._build_prompt(prompt, skill_path), usage_path) + self.last_token_counts = _empty_counts() + + LOG.debug("Hermes command: %s", " ".join(cmd[:2] + [""] + cmd[3:])) + + start = time.monotonic() + try: + result = subprocess.run( + cmd, + capture_output=True, + text=True, + timeout=effective_timeout, + cwd=workspace_dir, + env=self._build_env(), + ) + elapsed = time.monotonic() - start + # One read, two consumers: the token counts and the run-status + # flags come out of the same object, so they can never disagree + # about which run they describe. + usage = _load_usage(usage_path) + self.last_token_counts = self._map_counts(usage) + LOG.debug("Hermes exited %d in %.1fs", result.returncode, elapsed) + + reason = _status_failure_reason(usage) + if reason is not None: + # stdout is preserved deliberately: the provider's error text is + # the evidence for this classification, and `functional.py` + # keeps it in the run record. stderr carries the reason because + # that is what `_classify_rc` surfaces as the failure detail. + LOG.warning("Hermes run reclassified as infrastructure: %s", reason) + return result.stdout, reason, RC_PROVIDER_ERROR, elapsed + + return result.stdout, result.stderr, result.returncode, elapsed + + except subprocess.TimeoutExpired: + elapsed = time.monotonic() - start + # Written even on failure paths upstream, so spend is still real. + self.last_token_counts = self.read_usage_file(usage_path) + return "", f"Timed out after {effective_timeout}s", RC_TIMEOUT, elapsed + + except (FileNotFoundError, PermissionError, OSError) as exc: + elapsed = time.monotonic() - start + return "", f"hermes CLI could not be launched: {exc}", RC_LAUNCH_FAILED, elapsed + + finally: + try: + os.unlink(usage_path) + except OSError: + pass + + def parse_output(self, raw: str) -> dict: + """Normalize plain-text one-shot output into the shared contract. + + Tool calls stay empty by design: `-z` exposes no structured events, and + inferring them from prose would fabricate telemetry. + """ + text = raw or "" + counts = dict(self.last_token_counts) + # The `_meta` block is copied, not shared: `last_token_counts` survives + # until the next run_prompt, and a caller mutating the dict it was + # handed would rewrite the provenance of a run already written to the + # records ledger. + meta = counts.get(RUN_META_KEY) + counts[RUN_META_KEY] = dict(meta) if isinstance(meta, dict) else dict(_NULL_META) + return { + "events": [], + "tool_calls": [], + "text": text, + "token_counts": counts, + } + + +register_runner("hermes", HermesRunner) diff --git a/skill_eval/trigger.py b/skill_eval/trigger.py index 36c8a89..69763fb 100644 --- a/skill_eval/trigger.py +++ b/skill_eval/trigger.py @@ -86,6 +86,25 @@ def run_trigger_eval( print(f"Error: {e}", file=sys.stderr) return 2 + # Capability guard. Trigger detection reads structured tool-use events out + # of stream-json output; a runner whose CLI emits only plain text has + # nothing to detect and would score every query at 0% activation -- an + # artifact of the missing telemetry, indistinguishable in the report from a + # skill that genuinely never fired. Refuse before spending model calls and + # before writing a report that would read as a real finding. + if not runner.supports_trigger_eval(): + print( + f"Error: agent {agent!r} does not support trigger evaluation.\n" + " Trigger detection requires structured tool-use events " + "(stream-json); this agent emits plain text only.\n" + " A run would report 0% activation for every query regardless of " + "the skill -- refusing rather than emitting a fabricated result.\n" + " Use --agent claude for trigger eval, or evaluate this agent " + "with the audit/functional commands instead.", + file=sys.stderr, + ) + return 2 + # Read skill name skill_name = _read_skill_name(path) or path.name diff --git a/tests/fixtures/hermes_output/README.md b/tests/fixtures/hermes_output/README.md new file mode 100644 index 0000000..d7ba022 --- /dev/null +++ b/tests/fixtures/hermes_output/README.md @@ -0,0 +1,101 @@ +# Hermes CLI output fixture + +Real, unedited output from the Hermes Agent CLI one-shot (`-z`) path. Captured +before any parser test was written, so the parser is shaped by observed +behaviour rather than assumed behaviour. + +## Provenance + +| Field | Value | +|---|---| +| Captured (UTC) | `2026-08-10T03:05:44Z` → `2026-08-10T03:05:46Z` | +| Elapsed | ~2 s | +| CLI version | `Hermes Agent v0.20.0 (2026.8.3)` | +| CLI Python | 3.11.15 | +| Binary | `/Users/anastasios/.local/bin/hermes` (wrapper → `~/.hermes/hermes-agent/venv/bin/python`) | +| `HERMES_HOME` | default (`~/.hermes`) — the current shell's profile, no override | +| Configured model | `k3` | +| Configured provider | `kimi-coding` (Kimi / Kimi Coding Plan) | +| Host | macOS 26.4 | +| Exit code | **0** | +| Secrets | none present; the captured text contains only a public pricing URL | + +## Command + +``` +hermes -z "reply exactly: HELLO" \ + --cli \ + --no-restore-cwd \ + --ignore-rules \ + --ignore-user-config \ + --usage-file /tmp/skill-eval-usage.json +``` + +## Observed stdout + +One line, terminated by a single `\n` (verified with `od -c`): + +``` +HTTP 403: You've reached your usage limit for this billing cycle. Your quota will be refreshed in the next cycle. To continue now, purchase extra usage or upgrade your plan: https://www.kimi.com/code/#pricing +``` + +## Observed stderr + +Empty (0 bytes). + +## Observed usage file + +```json +{ + "estimated_cost_usd": null, + "cost_status": null, + "cost_source": null, + "input_tokens": null, + "output_tokens": null, + "cache_read_tokens": null, + "cache_write_tokens": null, + "reasoning_tokens": null, + "total_tokens": null, + "api_calls": 1, + "model": null, + "provider": null, + "session_id": null, + "completed": false, + "failed": true, + "service_tier": null +} +``` + +## What this capture establishes + +This capture is a **failed provider call**, not a successful completion. At +capture time the only authenticated provider on this host was Kimi, and its +subscription quota was exhausted. That makes the fixture more valuable than a +happy-path capture would have been, because it pins down four facts: + +1. **`hermes -z` exits `0` on a provider failure.** The non-zero exit code a + caller would expect never arrives. +2. **The provider error is delivered on stdout, in the position where the + assistant's answer belongs.** Nothing in the text stream distinguishes it + from a real reply. +3. **stderr stays empty**, so it carries no independent failure signal. +4. **The usage file is still written**, with every token field `null`, and it + is the *only* place the failure is represented — via `"completed": false` + and `"failed": true`. + +Consequence for `skill_eval.hermes_runner`: `read_usage_file()` maps only the +five token fields and drops `completed` / `failed`, so a quota-exhausted run +reaches the grader as `returncode=0`, `text="HTTP 403: ..."`, and all-zero +token counts — indistinguishable from a real but wrong answer. See the +"Capability boundary" section of `docs/hermes-runner.md`. + +Every `null` token field in this capture is also the direct justification for +`read_usage_file()` coercing non-numeric values to `0` rather than assuming the +keys hold numbers. + +## Reproducing + +Re-run the command above. On a host whose provider still has quota the stdout +will contain the model's reply instead, `completed` will be `true`, and the +token fields will be populated integers. Both shapes are in scope for the +parser. diff --git a/tests/fixtures/hermes_output/smoke-runs.md b/tests/fixtures/hermes_output/smoke-runs.md new file mode 100644 index 0000000..7ca6661 --- /dev/null +++ b/tests/fixtures/hermes_output/smoke-runs.md @@ -0,0 +1,139 @@ +# HermesRunner smoke runs + +Real executions of `skill_eval.hermes_runner.HermesRunner`, driving the actual +`hermes` binary as a subprocess. No mocks, no fabricated values — every number +below is copied from the run output. + +## Environment + +| Field | Value | +|---|---| +| Run (UTC) | `2026-08-10T03:0x` (same session as the CLI fixture) | +| CLI version | `Hermes Agent v0.20.0 (2026.8.3)` | +| `SKILL_EVAL_HERMES_BIN` | `/Users/anastasios/.local/bin/hermes` (set because `hermes` is not on this shell's `PATH`) | +| `HERMES_HOME` | default (`~/.hermes`) — current shell's provider, no separate profile | +| Provider / model | `kimi-coding` / `k3` | +| Timeout passed | `620` | + +Runner self-report before execution: + +``` +cli_name : /Users/anastasios/.local/bin/hermes +default_timeout : 620 +supports_trigger_eval: False +``` + +## Run 1 — no-skill smoke + +`HermesRunner().run_prompt("reply: OK", timeout=620)` + +| Field | Value | +|---|---| +| returncode | `0` | +| elapsed | `2.24 s` | +| text | `"HTTP 403: You've reached your usage limit for this billing cycle. Your quota will be refreshed in the next cycle. To continue now, purchase extra usage or upgrade your plan: https://www.kimi.com/code/#pricing"` | +| stderr | `""` | +| token_counts | `{"cache_creation_input_tokens": 0, "cache_read_input_tokens": 0, "input_tokens": 0, "output_tokens": 0, "reasoning_tokens": 0}` | +| `total_tokens()` | `0` | +| tool_calls / events | `[]` / `[]` | + +## Run 2 — with-skill smoke + +Workspace built the way `functional.py` builds it: a fresh temp directory with +`tests/fixtures/eval-skill/SKILL.md` copied in, then +`run_prompt("reply: OK", skill_path=..., workspace_dir=..., timeout=620)`. + +Injection checks, both passing: + +``` +injected prompt prefix present: True # prompt begins "A skill file named SKILL.md ..." +workspace contains: ['SKILL.md'] +``` + +| Field | Value | +|---|---| +| returncode | `0` | +| elapsed | `2.10 s` | +| text | identical HTTP 403 string as Run 1 | +| stderr | `""` | +| token_counts | all zeros (as Run 1) | +| `total_tokens()` | `0` | +| tool_calls / events | `[]` / `[]` | + +## Honest reading of these results + +**What is verified.** The runner's own machinery works end to end against the +real binary: it resolves the CLI, builds the isolation flag set, launches the +subprocess in the given workspace, honours the 620 s timeout default, creates +and reads back the usage file, applies the skill-instruction prefix only when +`SKILL.md` is actually present, and normalises output into the shared +`{events, tool_calls, text, token_counts}` contract without raising. + +**What is NOT verified.** Neither run obtained a model completion. The only +authenticated provider on this host (Kimi) is quota-exhausted, and no other +provider is configured — `hermes status` reports every other API key as *not +set* and every OAuth provider as *not logged in*, and no local backend +(`ollama`, `llama-server`, `lms`) is installed. Consequently these runs do +**not** demonstrate: correct extraction of real assistant text, non-zero token +accounting, `reasoning_tokens` mapping against live data, or any cross-model +comparison. Those remain unproven until a provider with quota is available. + +**Defect surfaced.** Both runs returned `returncode=0` while carrying a +provider error as their `text`. A grader consuming this sees a successful run +whose answer happens to be wrong. The `failed: true` / `completed: false` flags +that would expose it are present in the usage file and were discarded by +`read_usage_file()`. Detailed in `docs/hermes-runner.md` → *Capability +boundary*. Not fixed at the time of this capture: `hermes_runner.py` was +Task-1–3 code and that task was documentation-only. + +**Defect since fixed.** `run_prompt()` now consumes both flags and returns +`RC_PROVIDER_ERROR` (`-3`) for exactly this shape, so re-running the smokes +above today yields `rc=-3` rather than `rc=0`, and `functional.py` excludes the +pair from every mean instead of scoring it 0 %. The captured artifacts in this +directory are unchanged — they are the raw evidence the fix was built against, +and `usage_kimi_quota_403.json` is the byte-exact regression pin. + +## Independent re-verification (three sessions) + +The capture above was re-run from two further separate sessions to check that +it is a stable property of this host rather than a transient one. Command, +flags, and binary identical to `README.md` each time. + +| | Session 1 | Session 2 | Session 3 | +|---|---|---|---| +| Time (UTC) | `2026-08-10T03:05Z` | `2026-08-10T03:10Z` | `2026-08-10T06:2xZ` | +| CLI version | v0.20.0 (2026.8.3) | unchanged | unchanged | +| Exit code | `0` | `0` | `0` | +| stdout | HTTP 403 quota string | byte-identical | byte-identical (209 bytes) | +| stderr | empty | empty | empty (0 bytes) | +| usage file | all tokens `null`, `completed:false`, `failed:true` | identical | identical | + +Session 3 also re-ran both `HermesRunner` smokes end to end and reproduced Run 1 +and Run 2 exactly: `returncode=0`, elapsed `2.11 s` / `2.02 s`, identical HTTP +403 text in both arms, empty stderr, all-zero `token_counts`, `total_tokens()=0`, +empty `tool_calls` / `events`, and the skill-injection checks still passing +(`injection applied: True`, workspace `['SKILL.md']`). Runner self-report was +again `default_timeout=620`, `supports_trigger_eval=False`. + +`hermes status` at each re-run still reports Kimi as the only configured +provider: every other API key *not set*, every OAuth provider *not logged in*. + +Two conclusions. First, the fixture is **reproducible across three independent +sessions**, not a one-off — the `returncode=0`-on-provider-failure defect is a +stable behaviour of `hermes -z` v0.20.0, which is what makes it worth +documenting as a capability boundary rather than a flake. Second, **cross-model +evidence remains blocked** on this host for the same reason as before; no +cross-model comparison has been performed, and none is claimed anywhere in this +package. + +## Reproducing + +```bash +export SKILL_EVAL_HERMES_BIN=/Users/anastasios/.local/bin/hermes +.venv/bin/python - <<'PY' +from skill_eval.hermes_runner import HermesRunner +r = HermesRunner(); r.check_available() +out, err, rc, el = r.run_prompt("reply: OK", timeout=620) +print(rc, repr(out.strip()), r.parse_output(out)["token_counts"]) +PY +``` diff --git a/tests/fixtures/hermes_output/usage_custom_shim_zero_cost.json b/tests/fixtures/hermes_output/usage_custom_shim_zero_cost.json new file mode 100644 index 0000000..5f61194 --- /dev/null +++ b/tests/fixtures/hermes_output/usage_custom_shim_zero_cost.json @@ -0,0 +1,18 @@ +{ + "estimated_cost_usd": 0.0, + "cost_status": "unknown", + "cost_source": "none", + "input_tokens": 0, + "output_tokens": 0, + "cache_read_tokens": 0, + "cache_write_tokens": 0, + "reasoning_tokens": 0, + "total_tokens": 0, + "api_calls": 1, + "model": "claude-fable-5", + "provider": "custom", + "session_id": "20260810_060227_485585", + "completed": true, + "failed": false, + "service_tier": null +} diff --git a/tests/fixtures/hermes_output/usage_deepseek_v4_pro.json b/tests/fixtures/hermes_output/usage_deepseek_v4_pro.json new file mode 100644 index 0000000..0023ff4 --- /dev/null +++ b/tests/fixtures/hermes_output/usage_deepseek_v4_pro.json @@ -0,0 +1,18 @@ +{ + "estimated_cost_usd": 7.627e-05, + "cost_status": "estimated", + "cost_source": "official_docs_snapshot", + "input_tokens": 30, + "output_tokens": 22, + "cache_read_tokens": 12160, + "cache_write_tokens": 0, + "reasoning_tokens": 20, + "total_tokens": 12212, + "api_calls": 1, + "model": "deepseek-v4-pro", + "provider": "deepseek", + "session_id": "20260810_060335_c48b70", + "completed": true, + "failed": false, + "service_tier": null +} diff --git a/tests/fixtures/hermes_output/usage_kimi_quota_403.json b/tests/fixtures/hermes_output/usage_kimi_quota_403.json new file mode 100644 index 0000000..a424f92 --- /dev/null +++ b/tests/fixtures/hermes_output/usage_kimi_quota_403.json @@ -0,0 +1,18 @@ +{ + "estimated_cost_usd": null, + "cost_status": null, + "cost_source": null, + "input_tokens": null, + "output_tokens": null, + "cache_read_tokens": null, + "cache_write_tokens": null, + "reasoning_tokens": null, + "total_tokens": null, + "api_calls": 1, + "model": null, + "provider": null, + "session_id": null, + "completed": false, + "failed": true, + "service_tier": null +} diff --git a/tests/test_functional.py b/tests/test_functional.py index dfbf5eb..c4edf2f 100644 --- a/tests/test_functional.py +++ b/tests/test_functional.py @@ -22,6 +22,7 @@ EvalCase, GradingResult, RunPairResult, BenchmarkReport, ) from skill_eval.agent_runner import AgentNotAvailableError, ClaudeRunner +from skill_eval.hermes_runner import HermesRunner, RC_TIMEOUT, RC_LAUNCH_FAILED FIXTURES = Path(__file__).parent / "fixtures" @@ -163,7 +164,7 @@ def test_basic_aggregation(self): execution_metrics={"tool_calls": 2, "token_counts": {"input_tokens": 80, "output_tokens": 40}}, ), ] - report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1) + report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1, runner=ClaudeRunner()) assert report.skill_name == "test" assert report.eval_count == 1 assert report.passed is True # 0.8 >= 0.6 and >= 0.5 @@ -186,7 +187,7 @@ def test_failing_aggregation(self): execution_metrics={"tool_calls": 2, "token_counts": {"input_tokens": 80, "output_tokens": 40}}, ), ] - report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1) + report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1, runner=ClaudeRunner()) assert report.passed is False # 0.3 < 0.5 @@ -344,7 +345,7 @@ def test_run_summary_token_fields(self): execution_metrics={"tool_calls": 2, "token_counts": {"input_tokens": 980, "output_tokens": 290}}, ), ] - report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1) + report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1, runner=ClaudeRunner()) rs = report.run_summary # New token fields present @@ -623,7 +624,7 @@ def test_aggregate_includes_cost_efficiency(self): execution_metrics={"tool_calls": 2, "token_counts": {"input_tokens": 1200, "output_tokens": 300}}, ), ] - report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1) + report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1, runner=ClaudeRunner()) ce = report.run_summary.get("cost_efficiency") assert ce is not None assert ce["classification"] == "PARETO_BETTER" # quality +0.2, cost down @@ -648,7 +649,7 @@ def test_cost_efficiency_missing_when_without_tokens_zero(self): execution_metrics={"tool_calls": 1, "token_counts": {"input_tokens": 0, "output_tokens": 0}}, ), ] - report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1) + report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1, runner=ClaudeRunner()) assert "cost_efficiency" not in report.run_summary def test_cost_efficiency_tradeoff(self): @@ -667,7 +668,7 @@ def test_cost_efficiency_tradeoff(self): execution_metrics={"tool_calls": 2, "token_counts": {"input_tokens": 1000, "output_tokens": 400}}, ), ] - report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1) + report = _aggregate_benchmark("test", "/tmp", cases, pairs, gradings, 1, runner=ClaudeRunner()) ce = report.run_summary["cost_efficiency"] assert ce["classification"] == "TRADEOFF" @@ -733,3 +734,312 @@ def test_cost_efficiency_section_hidden_when_no_data(self, capsys): _print_functional_report(report) captured = capsys.readouterr() assert "Cost Efficiency:" not in captured.out + + +class TestRunnerTokenAccounting: + """Regression: _aggregate_benchmark must route totals through the runner. + + Defect 5.1 — functional.py defined its own module-level `_total_tokens()` + (input + output only) instead of calling `runner.total_tokens()`. That made + every reasoning token invisible to the cost-efficiency comparison, so a + HermesRunner benchmark (DeepSeek V4 Pro, thinking always on) reported the + same cost_delta_pct as a runner that spent nothing on reasoning. + """ + + @staticmethod + def _gradings(with_counts: dict, without_counts: dict): + return [ + GradingResult( + eval_id="t1", run_index=0, pass_rate=0.8, + summary="With skill: 80% assertions passed", + execution_metrics={"tool_calls": 1, "token_counts": with_counts}, + ), + GradingResult( + eval_id="t1", run_index=0, pass_rate=0.8, + summary="Without skill: 80% assertions passed", + execution_metrics={"tool_calls": 1, "token_counts": without_counts}, + ), + ] + + def _report(self, runner): + cases = [EvalCase(id="t1", prompt="test")] + pairs = [RunPairResult(eval_id="t1", run_index=0, delta_pass_rate=0.0)] + gradings = self._gradings( + {"input_tokens": 100, "output_tokens": 50, "reasoning_tokens": 500}, + {"input_tokens": 100, "output_tokens": 50, "reasoning_tokens": 0}, + ) + return _aggregate_benchmark( + "test", "/tmp", cases, pairs, gradings, 1, runner=runner, + ) + + def test_reasoning_tokens_change_cost_delta(self): + """The same gradings must score differently under a reasoning-aware runner.""" + base = self._report(ClaudeRunner()) + hermes = self._report(HermesRunner()) + + base_ce = base.run_summary["cost_efficiency"]["cost_delta_pct"] + hermes_ce = hermes.run_summary["cost_efficiency"]["cost_delta_pct"] + + # Base runner sees 150 vs 150 — the 500 reasoning tokens are invisible. + assert base_ce == 0.0 + # HermesRunner sees 650 vs 150 — a 333% cost increase. + assert hermes_ce == pytest.approx(333.3, abs=0.1) + assert hermes_ce != base_ce + + def test_reasoning_tokens_counted_in_mean_totals(self): + """mean_total_tokens must include reasoning tokens under HermesRunner.""" + report = self._report(HermesRunner()) + assert report.run_summary["with_skill"]["mean_total_tokens"] == 650.0 + assert report.run_summary["without_skill"]["mean_total_tokens"] == 150.0 + assert report.run_summary["delta"]["total_tokens"] == 500.0 + + def test_base_runner_matches_legacy_behaviour(self): + """ClaudeRunner totals stay input+output — no behaviour change for Claude.""" + report = self._report(ClaudeRunner()) + assert report.run_summary["with_skill"]["mean_total_tokens"] == 150.0 + assert report.run_summary["without_skill"]["mean_total_tokens"] == 150.0 + + +class TestInfrastructureReturnCodes: + """Defect 5.2 — infrastructure return codes were unpacked and discarded. + + `_execute_eval_pair` destructured `with_rc` / `without_rc` from + `runner.run_prompt()` and never referenced them again. A 620s DeepSeek + timeout (RC_TIMEOUT) or a missing CLI (RC_LAUNCH_FAILED) therefore arrived + as empty output, graded 0%, and was averaged into the pass rate as though + the skill had been asked and produced nothing. Infrastructure failure and + skill failure are not the same measurement and must not share a mean. + """ + + def _eval_case(self): + return EvalCase( + id="infra-case", + prompt="Summarize the CSV file.", + assertions=["contains 'name'", "contains 'age'"], + ) + + def _runner(self): + runner = MagicMock(spec=ClaudeRunner) + runner.parse_output.side_effect = ClaudeRunner().parse_output + return runner + + def _run(self, runner): + return _execute_eval_pair( + self._eval_case(), + FIXTURES / "eval-skill", + FIXTURES / "eval-skill" / "evals", + run_index=0, + timeout=30, + runner=runner, + ) + + # -- Hard infrastructure failures ------------------------------------ + + def test_timeout_marks_run_as_infrastructure_error(self): + """RC_TIMEOUT on the with-skill arm must be recorded, not scored.""" + runner = self._runner() + runner.run_prompt.side_effect = [ + ("", "Timed out after 620s", RC_TIMEOUT, 620.0), + (_make_stream_json("name age city"), "", 0, 1.0), + ] + pair, with_g, without_g = self._run(runner) + + assert pair.infrastructure_error is not None + assert pair.infrastructure_error["with_skill"]["kind"] == "timeout" + assert pair.infrastructure_error["with_skill"]["rc"] == RC_TIMEOUT + assert "without_skill" not in pair.infrastructure_error + + assert with_g.execution_metrics["infrastructure_error"]["kind"] == "timeout" + assert with_g.execution_metrics["excluded_from_means"] is True + # The healthy arm is excluded too: the pair is the unit of comparison. + assert without_g.execution_metrics["excluded_from_means"] is True + assert "infrastructure_error" not in without_g.execution_metrics + + def test_launch_failure_marks_run_as_infrastructure_error(self): + """RC_LAUNCH_FAILED must be distinguishable from a timeout.""" + runner = self._runner() + runner.run_prompt.side_effect = [ + (_make_stream_json("name age city"), "", 0, 1.0), + ("", "hermes CLI could not be launched: [Errno 2]", RC_LAUNCH_FAILED, 0.1), + ] + pair, with_g, without_g = self._run(runner) + + assert pair.infrastructure_error["without_skill"]["kind"] == "launch_failed" + assert pair.infrastructure_error["without_skill"]["rc"] == RC_LAUNCH_FAILED + assert without_g.execution_metrics["infrastructure_error"]["rc"] == RC_LAUNCH_FAILED + + def test_signal_death_is_infrastructure_not_skill_failure(self): + """subprocess returns -N for signal N; SIGKILL is not a skill failure.""" + runner = self._runner() + runner.run_prompt.side_effect = [ + ("", "", -9, 12.0), + (_make_stream_json("name age"), "", 0, 1.0), + ] + pair, with_g, _ = self._run(runner) + assert pair.infrastructure_error["with_skill"]["kind"] == "signal" + assert with_g.execution_metrics["infrastructure_error"]["rc"] == -9 + + def test_errored_arm_is_not_graded(self): + """No judge call should be spent on an arm that never ran.""" + runner = self._runner() + runner.run_prompt.side_effect = [ + ("", "Timed out after 620s", RC_TIMEOUT, 620.0), + (_make_stream_json("name age city"), "", 0, 1.0), + ] + with patch("skill_eval.functional.grade_output") as mock_grade: + mock_grade.return_value = ([], 1.0) + self._run(runner) + assert mock_grade.call_count == 1 + + def test_healthy_pair_is_unflagged(self): + """Regression guard: a clean pair must carry no infrastructure marks.""" + stream = _make_stream_json("name age city") + runner = self._runner() + runner.run_prompt.side_effect = [(stream, "", 0, 1.0), (stream, "", 0, 1.2)] + pair, with_g, without_g = self._run(runner) + + assert pair.infrastructure_error is None + assert "infrastructure_error" not in with_g.execution_metrics + assert with_g.execution_metrics.get("excluded_from_means", False) is False + assert without_g.execution_metrics.get("excluded_from_means", False) is False + + # -- Exclusion from the aggregate means ------------------------------- + + @staticmethod + def _grading(arm: str, pass_rate: float, *, broken: bool = False, excluded: bool = False, + tokens: tuple = (100, 50)): + metrics = { + "tool_calls": 1, + "token_counts": {"input_tokens": tokens[0], "output_tokens": tokens[1]}, + } + if broken: + metrics["infrastructure_error"] = {"kind": "timeout", "rc": RC_TIMEOUT, "detail": "x"} + if excluded: + metrics["excluded_from_means"] = True + label = "With skill" if arm == "with" else "Without skill" + return GradingResult( + eval_id="t1", run_index=0, pass_rate=pass_rate, + summary=f"{label}: {pass_rate:.0%} assertions passed", + execution_metrics=metrics, + ) + + def test_infrastructure_runs_excluded_from_pass_rate_mean(self): + """A timed-out pair must not drag the mean toward zero.""" + gradings = [ + self._grading("with", 1.0), + self._grading("without", 0.0), + # Broken pair: with-skill timed out, both arms excluded. + self._grading("with", 0.0, broken=True, excluded=True), + self._grading("without", 0.0, excluded=True), + ] + report = _aggregate_benchmark( + "t", "/tmp", [EvalCase(id="t1", prompt="p")], + [RunPairResult(eval_id="t1", run_index=0)], gradings, 2, + runner=ClaudeRunner(), + ) + # Only the healthy pair counts: 1.0, not the 0.5 a naive mean would give. + assert report.run_summary["with_skill"]["mean_pass_rate"] == 1.0 + assert report.run_summary["without_skill"]["mean_pass_rate"] == 0.0 + + def test_infrastructure_counts_reported(self): + """The report must say how many runs were dropped, and why.""" + gradings = [ + self._grading("with", 1.0), + self._grading("without", 0.0), + self._grading("with", 0.0, broken=True, excluded=True), + self._grading("without", 0.0, excluded=True), + ] + report = _aggregate_benchmark( + "t", "/tmp", [EvalCase(id="t1", prompt="p")], + [RunPairResult(eval_id="t1", run_index=0)], gradings, 2, + runner=ClaudeRunner(), + ) + infra = report.run_summary["infrastructure"] + assert infra["with_skill_errors"] == 1 + assert infra["without_skill_errors"] == 0 + assert infra["excluded_runs"] == 2 + assert infra["graded_pairs"] == 1 + assert infra["kinds"] == {"timeout": 1} + + def test_all_runs_infrastructure_never_reports_passed(self): + """A benchmark where nothing executed must not be scored as a pass.""" + gradings = [ + self._grading("with", 0.0, broken=True, excluded=True), + self._grading("without", 0.0, broken=True, excluded=True), + ] + report = _aggregate_benchmark( + "t", "/tmp", [EvalCase(id="t1", prompt="p")], + [RunPairResult(eval_id="t1", run_index=0)], gradings, 1, + runner=ClaudeRunner(), + ) + assert report.passed is False + assert report.run_summary["infrastructure"]["graded_pairs"] == 0 + + def test_tokens_from_errored_runs_excluded(self): + """A timed-out run's token spend must not enter the cost comparison.""" + gradings = [ + self._grading("with", 1.0), + self._grading("without", 1.0), + # The dead run burned a huge prompt before dying — excluding it has + # to be observable, so give it token counts nothing else shares. + self._grading("with", 0.0, broken=True, excluded=True, tokens=(9000, 9000)), + self._grading("without", 0.0, excluded=True, tokens=(7000, 7000)), + ] + report = _aggregate_benchmark( + "t", "/tmp", [EvalCase(id="t1", prompt="p")], + [RunPairResult(eval_id="t1", run_index=0)], gradings, 2, + runner=ClaudeRunner(), + ) + # One healthy run per arm at 100 in + 50 out. + assert report.run_summary["with_skill"]["mean_total_tokens"] == 150.0 + assert report.run_summary["without_skill"]["mean_input_tokens"] == 100.0 + + # -- Soft signal: rc==0 but the text is a provider error -------------- + + def test_provider_error_text_flagged_as_suspected(self): + """rc==0 with a usage-limit body is suspicious but not proof.""" + body = _make_stream_json("Error: usage limit reached for this API key") + runner = self._runner() + runner.run_prompt.side_effect = [(body, "", 0, 2.0), (_make_stream_json("name age"), "", 0, 1.0)] + pair, with_g, _ = self._run(runner) + + suspected = with_g.execution_metrics["suspected_infrastructure_error"] + assert "usage limit" in suspected["match"].lower() + # Suspicion alone must NOT drop the run — a skill may legitimately + # print "403" and dropping it would silently shrink the sample. + assert pair.infrastructure_error is None + assert with_g.execution_metrics.get("excluded_from_means", False) is False + + def test_provider_error_text_logged(self, caplog): + """The operator has to be able to see it in the log.""" + import logging + body = _make_stream_json("HTTP 429 rate limit exceeded") + runner = self._runner() + runner.run_prompt.side_effect = [(body, "", 0, 2.0), (_make_stream_json("ok"), "", 0, 1.0)] + with caplog.at_level(logging.WARNING, logger="skill_eval.functional"): + self._run(runner) + assert any("rate limit" in r.message.lower() or "rate limit" in str(r.args).lower() + for r in caplog.records) + + def test_clean_output_not_flagged_as_suspected(self): + """Ordinary prose must not trip the provider-error patterns.""" + stream = _make_stream_json("The CSV has columns: name, age, city") + runner = self._runner() + runner.run_prompt.side_effect = [(stream, "", 0, 1.0), (stream, "", 0, 1.0)] + _, with_g, without_g = self._run(runner) + assert "suspected_infrastructure_error" not in with_g.execution_metrics + assert "suspected_infrastructure_error" not in without_g.execution_metrics + + +class TestReturnCodeSentinels: + """The sentinels are a runner-contract constant, not a Hermes detail.""" + + def test_canonical_definition_is_shared(self): + from skill_eval.agent_runner import RC_TIMEOUT as BASE_TIMEOUT + from skill_eval.agent_runner import RC_LAUNCH_FAILED as BASE_LAUNCH + assert BASE_TIMEOUT == RC_TIMEOUT == -1 + assert BASE_LAUNCH == RC_LAUNCH_FAILED == -2 + + def test_sentinels_stay_distinct(self): + """Conflating them makes a provider timeout look like a missing binary.""" + assert RC_TIMEOUT != RC_LAUNCH_FAILED diff --git a/tests/test_grading_judge_runner.py b/tests/test_grading_judge_runner.py new file mode 100644 index 0000000..c9d5e48 --- /dev/null +++ b/tests/test_grading_judge_runner.py @@ -0,0 +1,196 @@ +"""Judge decontamination: LLM fallback grading must follow the chosen runner. + +Blocker B1 from docs/research/fable-plan-review.md. Before this change, +`_llm_grade` imported `skill_eval._claude` directly, so +`skill-eval functional --agent hermes` executed the task through +Hermes but silently judged it with Claude. Any cross-model number produced +that way is contaminated. + +The contract: + * `judge_runner` omitted -> existing Claude path, byte-for-byte (compat pin). + * `judge_runner` provided -> that runner judges; `_claude` is never imported. +""" + +import json +import sys +from unittest.mock import MagicMock, patch + +import pytest + +from skill_eval.agent_runner import AgentRunner +from skill_eval.grading import _llm_grade, grade_output + + +AMBIGUOUS = "demonstrates good judgement" + + +def _judge_payload(passed: bool = True, evidence: str = "judged") -> str: + return json.dumps( + [{"index": 1, "passed": passed, "confidence": 0.9, "evidence": evidence}] + ) + + +class _RecordingJudge(AgentRunner): + """Minimal in-process runner that records how it was invoked.""" + + def __init__(self, payload: str = "", rc: int = 0) -> None: + self.payload = payload or _judge_payload() + self.rc = rc + self.calls: list[dict] = [] + + def check_available(self) -> None: + return None + + def run_prompt(self, prompt, skill_path=None, workspace_dir=None, + timeout=120, output_format="text"): + self.calls.append({"prompt": prompt, "timeout": timeout}) + return (self.payload, "", self.rc, 0.01) + + def parse_output(self, raw: str) -> dict: + return {"events": [], "tool_calls": [], "text": raw, + "token_counts": {"input_tokens": 0, "output_tokens": 0}} + + +class TestBackwardCompatibility: + """Omitting judge_runner must not change existing Claude behaviour.""" + + def test_omitted_judge_runner_still_uses_claude(self): + with patch("skill_eval._claude.check_claude_available"), \ + patch("skill_eval._claude.run_claude_prompt", + return_value=(_judge_payload(), "", 0, 0.01)) as claude: + results = _llm_grade("some output", [AMBIGUOUS], timeout=30) + + assert claude.called, "omitted judge_runner must fall back to Claude" + assert len(results) == 1 + assert results[0].method == "llm" + + def test_omitted_judge_runner_degrades_when_claude_absent(self): + with patch("skill_eval._claude.check_claude_available", + side_effect=RuntimeError("no claude")): + results = _llm_grade("some output", [AMBIGUOUS]) + + assert len(results) == 1 + assert results[0].passed is False + + +class TestJudgeRunnerDecontamination: + """An explicit judge_runner must fully displace the Claude path.""" + + def test_explicit_judge_runner_never_imports_claude(self): + judge = _RecordingJudge() + # Poison the Claude module so any use raises loudly. + with patch("skill_eval._claude.check_claude_available", + side_effect=AssertionError("Claude judge leaked")), \ + patch("skill_eval._claude.run_claude_prompt", + side_effect=AssertionError("Claude judge leaked")): + results = _llm_grade("some output", [AMBIGUOUS], judge_runner=judge) + + assert len(judge.calls) == 1, "judge_runner must be the one invoked" + assert len(results) == 1 + assert results[0].passed is True + assert results[0].method == "llm" + + def test_judge_runner_verdict_is_parsed_not_defaulted(self): + judge = _RecordingJudge(payload=_judge_payload(passed=False, + evidence="missing section")) + results = _llm_grade("out", [AMBIGUOUS], judge_runner=judge) + + assert results[0].passed is False + assert "missing section" in results[0].evidence + + def test_judge_runner_failure_is_reported_not_silently_passed(self): + judge = _RecordingJudge(payload="", rc=1) + results = _llm_grade("out", [AMBIGUOUS], judge_runner=judge) + + assert results[0].passed is False + + def test_judge_runner_receives_the_timeout(self): + judge = _RecordingJudge() + _llm_grade("out", [AMBIGUOUS], timeout=45, judge_runner=judge) + + assert judge.calls[0]["timeout"] == 45 + + +class TestNoJudgeSentinel: + """`NO_JUDGE` is the third state: explicitly nobody. + + `judge_runner=None` cannot express it -- None already means "use the + historical Claude path", which is precisely the leak B1 closed. So a + caller that wants no judge at all needs a value that is neither a runner + nor None, or it silently gets Claude. + """ + + def test_no_judge_never_imports_claude(self): + from skill_eval.grading import NO_JUDGE + + with patch("skill_eval._claude.check_claude_available", + side_effect=AssertionError("Claude judge leaked")): + results = _llm_grade("out", [AMBIGUOUS], judge_runner=NO_JUDGE) + + assert len(results) == 1 + + def test_no_judge_fails_the_assertion_rather_than_passing_it(self): + from skill_eval.grading import NO_JUDGE + + results = _llm_grade("out", [AMBIGUOUS], judge_runner=NO_JUDGE) + + assert results[0].passed is False, ( + "an ungraded assertion must never be scored as satisfied" + ) + + def test_no_judge_marks_the_result_uncertain(self): + """"Nobody looked" and "the judge said no" must be distinguishable.""" + from skill_eval.grading import NO_JUDGE + + results = _llm_grade("out", [AMBIGUOUS], judge_runner=NO_JUDGE) + + assert results[0].uncertain is True + assert results[0].confidence == 0.0 + assert "judge" in results[0].evidence.lower() + + def test_no_judge_is_forwarded_through_grade_output(self): + from skill_eval.grading import NO_JUDGE + + with patch("skill_eval._claude.check_claude_available", + side_effect=AssertionError("Claude judge leaked")): + results, pass_rate = grade_output("out", [AMBIGUOUS], + judge_runner=NO_JUDGE) + + assert pass_rate == 0.0 + assert results[0].method == "llm" + + def test_deterministic_assertions_still_grade_without_a_judge(self): + """NO_JUDGE must not disable the deterministic path it never used.""" + from skill_eval.grading import NO_JUDGE + + results, pass_rate = grade_output( + "hello world", ["contains 'hello'"], judge_runner=NO_JUDGE, + ) + + assert pass_rate == 1.0 + assert results[0].method == "deterministic" + + +class TestGradeOutputForwarding: + """grade_output is the public entry point; it must forward the judge.""" + + def test_grade_output_forwards_judge_runner(self): + judge = _RecordingJudge() + with patch("skill_eval._claude.check_claude_available", + side_effect=AssertionError("Claude judge leaked")): + results, pass_rate = grade_output("out", [AMBIGUOUS], + judge_runner=judge) + + assert len(judge.calls) == 1 + assert pass_rate == 1.0 + assert len(results) == 1 + + def test_deterministic_assertions_never_reach_the_judge(self): + """A deterministic matcher must short-circuit before any model call.""" + judge = _RecordingJudge() + results, pass_rate = grade_output("hello world", ["contains 'hello'"], + judge_runner=judge) + + assert judge.calls == [], "deterministic path must not call a judge" + assert pass_rate == 1.0 + assert results[0].method == "deterministic" diff --git a/tests/test_hermes_runner.py b/tests/test_hermes_runner.py new file mode 100644 index 0000000..f345d59 --- /dev/null +++ b/tests/test_hermes_runner.py @@ -0,0 +1,533 @@ +"""Tests for skill_eval.hermes_runner — model-agnostic Hermes AgentRunner. + +Every CLI flag asserted here was verified against the live Hermes Agent v0.20.0 +parser (`hermes_cli/_parser.py`) and `hermes_cli/oneshot.py::_write_usage_file`. +Notably `--source` does NOT exist on the top-level `-z` path; asserting its +absence is a regression guard, not a style preference. +""" + +import json +import subprocess +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from skill_eval.agent_runner import ( + RUN_META_KEY, + AgentNotAvailableError, + AgentRunner, + ClaudeRunner, + get_runner, +) +from skill_eval.hermes_runner import ( + RC_LAUNCH_FAILED, + RC_TIMEOUT, + HermesRunner, +) + + +# --------------------------------------------------------------------------- +# Registration + contract +# --------------------------------------------------------------------------- + +class TestRegistration: + def test_registered_as_hermes(self): + """Importing the module registers 'hermes' in the shared registry.""" + runner = get_runner("hermes") + assert isinstance(runner, HermesRunner) + + def test_is_agent_runner_subclass(self): + assert issubclass(HermesRunner, AgentRunner) + + def test_claude_runner_still_registered(self): + """Adding a runner must not disturb the existing default.""" + assert isinstance(get_runner("claude"), ClaudeRunner) + + +class TestCheckAvailable: + def test_raises_when_cli_absent(self): + runner = HermesRunner() + with patch("shutil.which", return_value=None): + with pytest.raises(AgentNotAvailableError) as exc: + runner.check_available() + assert exc.value.agent_name == "hermes" + + def test_does_not_raise_when_cli_present(self): + runner = HermesRunner() + with patch("shutil.which", return_value="/usr/local/bin/hermes"): + runner.check_available() # must not raise + + +# --------------------------------------------------------------------------- +# Command construction — verified flags only +# --------------------------------------------------------------------------- + +def _cmd_from_run(runner, **kwargs): + """Run run_prompt against a mocked subprocess and return the argv list.""" + mock_result = MagicMock(stdout="ok", stderr="", returncode=0) + with patch("subprocess.run", return_value=mock_result) as mock_run: + runner.run_prompt("do the thing", **kwargs) + return mock_run.call_args[0][0] + + +class TestCommandConstruction: + def test_uses_oneshot_and_isolation_flags(self, monkeypatch): + monkeypatch.delenv("SKILL_EVAL_HERMES_PROVIDER", raising=False) + monkeypatch.delenv("SKILL_EVAL_HERMES_MODEL", raising=False) + cmd = _cmd_from_run(HermesRunner()) + + assert cmd[0] == "hermes" + assert "-z" in cmd + assert "do the thing" in cmd + assert "--cli" in cmd + assert "--no-restore-cwd" in cmd + assert "--ignore-rules" in cmd + assert "--ignore-user-config" in cmd + + def test_never_passes_source_flag(self): + """`--source` exists only on `hermes chat`; on -z it aborts the run.""" + cmd = _cmd_from_run(HermesRunner()) + assert "--source" not in cmd + + def test_always_requests_usage_file(self): + cmd = _cmd_from_run(HermesRunner()) + assert "--usage-file" in cmd + path = cmd[cmd.index("--usage-file") + 1] + assert path.endswith(".json") + + def test_optional_env_flags_applied(self, monkeypatch): + monkeypatch.setenv("SKILL_EVAL_HERMES_BIN", "/opt/hermes") + monkeypatch.setenv("SKILL_EVAL_HERMES_PROVIDER", "deepseek") + monkeypatch.setenv("SKILL_EVAL_HERMES_MODEL", "deepseek-v4-pro") + monkeypatch.setenv("SKILL_EVAL_HERMES_REASONING", "none") + monkeypatch.setenv("SKILL_EVAL_HERMES_TOOLSETS", "file,terminal") + + cmd = _cmd_from_run(HermesRunner()) + + assert cmd[0] == "/opt/hermes" + assert cmd[cmd.index("--provider") + 1] == "deepseek" + assert cmd[cmd.index("-m") + 1] == "deepseek-v4-pro" + assert cmd[cmd.index("--reasoning") + 1] == "none" + assert cmd[cmd.index("-t") + 1] == "file,terminal" + + def test_omits_optional_flags_when_unset(self, monkeypatch): + for var in ( + "SKILL_EVAL_HERMES_PROVIDER", + "SKILL_EVAL_HERMES_MODEL", + "SKILL_EVAL_HERMES_REASONING", + "SKILL_EVAL_HERMES_TOOLSETS", + ): + monkeypatch.delenv(var, raising=False) + + cmd = _cmd_from_run(HermesRunner()) + + assert "--provider" not in cmd + assert "-m" not in cmd + assert "--reasoning" not in cmd + assert "-t" not in cmd + + +class TestSkillInjection: + def test_with_skill_points_at_workspace_copy(self, tmp_path): + skill = tmp_path / "myskill" + skill.mkdir() + (skill / "SKILL.md").write_text("---\nname: myskill\n---\n\nDo X.") + + cmd = _cmd_from_run(HermesRunner(), skill_path=str(skill)) + prompt = cmd[cmd.index("-z") + 1] + + assert "SKILL.md" in prompt + assert "do the thing" in prompt + + def test_without_skill_prompt_is_unmodified(self): + cmd = _cmd_from_run(HermesRunner()) + assert cmd[cmd.index("-z") + 1] == "do the thing" + + def test_never_embeds_env_or_credentials(self, tmp_path): + """Injection must reference the file, never inline secret-bearing content.""" + skill = tmp_path / "myskill" + skill.mkdir() + (skill / "SKILL.md").write_text("---\nname: myskill\n---\nbody") + (skill / ".env").write_text("SECRET_TOKEN=sk-must-not-leak") + + cmd = _cmd_from_run(HermesRunner(), skill_path=str(skill)) + + assert "sk-must-not-leak" not in " ".join(cmd) + + +# --------------------------------------------------------------------------- +# Infrastructure failure separation (Fable B4 / AC-6) +# --------------------------------------------------------------------------- + +class TestInfraFailureSeparation: + def test_timeout_and_launch_failure_use_distinct_codes(self): + """A skill must never be blamed for infra; the two classes must differ.""" + assert RC_TIMEOUT != RC_LAUNCH_FAILED + + def test_timeout_returns_rc_timeout(self): + runner = HermesRunner() + with patch("subprocess.run", side_effect=subprocess.TimeoutExpired("hermes", 620)): + stdout, stderr, rc, elapsed = runner.run_prompt("x", timeout=620) + assert rc == RC_TIMEOUT + assert "620" in stderr + assert elapsed >= 0 + + def test_missing_binary_returns_rc_launch_failed(self): + runner = HermesRunner() + with patch("subprocess.run", side_effect=FileNotFoundError()): + stdout, stderr, rc, _ = runner.run_prompt("x") + assert rc == RC_LAUNCH_FAILED + assert "hermes" in stderr.lower() + + def test_default_timeout_survives_deepseek_queue_kill(self, monkeypatch): + """DeepSeek can idle to a ~10min provider kill; 120s would misread it.""" + monkeypatch.delenv("SKILL_EVAL_HERMES_TIMEOUT", raising=False) + assert HermesRunner().default_timeout() >= 620 + + def test_timeout_env_override(self, monkeypatch): + monkeypatch.setenv("SKILL_EVAL_HERMES_TIMEOUT", "900") + assert HermesRunner().default_timeout() == 900 + + def test_success_passes_through(self): + runner = HermesRunner() + mock_result = MagicMock(stdout="answer", stderr="", returncode=0) + with patch("subprocess.run", return_value=mock_result): + stdout, stderr, rc, _ = runner.run_prompt("x") + assert (stdout, rc) == ("answer", 0) + + def test_workspace_dir_forwarded_as_cwd(self, tmp_path): + runner = HermesRunner() + mock_result = MagicMock(stdout="", stderr="", returncode=0) + with patch("subprocess.run", return_value=mock_result) as mock_run: + runner.run_prompt("x", workspace_dir=str(tmp_path)) + assert mock_run.call_args.kwargs["cwd"] == str(tmp_path) + + def test_profile_home_scopes_subprocess_env_only(self, tmp_path, monkeypatch): + monkeypatch.setenv("SKILL_EVAL_HERMES_PROFILE_HOME", str(tmp_path)) + runner = HermesRunner() + mock_result = MagicMock(stdout="", stderr="", returncode=0) + with patch("subprocess.run", return_value=mock_result) as mock_run: + runner.run_prompt("x") + assert mock_run.call_args.kwargs["env"]["HERMES_HOME"] == str(tmp_path) + + +# --------------------------------------------------------------------------- +# Output parsing — `hermes -z` emits plain text, not stream-json +# --------------------------------------------------------------------------- + +class TestParseOutput: + def test_empty_output_is_zeroed_not_none(self): + parsed = HermesRunner().parse_output("") + assert parsed["text"] == "" + assert parsed["events"] == [] + assert parsed["tool_calls"] == [] + # Every bucket, and only the buckets: `_meta` shares the dict but is + # provenance, not spend, and is asserted on separately below. + for key, value in parsed["token_counts"].items(): + if key == RUN_META_KEY: + continue + assert isinstance(value, int), key + assert value == 0 + + def test_plain_text_is_preserved(self): + parsed = HermesRunner().parse_output("The answer is 42.\n") + assert "The answer is 42." in parsed["text"] + + def test_tool_calls_empty_no_structured_events(self): + """-z gives no structured tool events; claiming otherwise fakes telemetry.""" + parsed = HermesRunner().parse_output("some text\nmore text") + assert parsed["tool_calls"] == [] + + def test_token_counts_expose_skill_eval_key_names(self): + counts = HermesRunner().parse_output("x")["token_counts"] + assert "input_tokens" in counts + assert "output_tokens" in counts + assert "cache_read_input_tokens" in counts + assert "cache_creation_input_tokens" in counts + + +# --------------------------------------------------------------------------- +# Usage-file mapping — verified schema from oneshot.py::_write_usage_file +# --------------------------------------------------------------------------- + +class TestUsageFileMapping: + def test_maps_hermes_field_names_to_skill_eval_names(self, tmp_path): + usage = tmp_path / "usage.json" + usage.write_text(json.dumps({ + "input_tokens": 100, + "output_tokens": 50, + "cache_read_tokens": 30, + "cache_write_tokens": 10, + "reasoning_tokens": 200, + "total_tokens": 390, + "model": "deepseek-v4-pro", + "provider": "deepseek", + })) + + counts = HermesRunner().read_usage_file(str(usage)) + + assert counts["input_tokens"] == 100 + assert counts["output_tokens"] == 50 + assert counts["cache_read_input_tokens"] == 30 + assert counts["cache_creation_input_tokens"] == 10 + assert counts["reasoning_tokens"] == 200 + + def test_null_values_coerce_to_zero(self, tmp_path): + """Every field is result.get(...) upstream, so None is expected, not exceptional.""" + usage = tmp_path / "usage.json" + usage.write_text(json.dumps({ + "input_tokens": None, + "output_tokens": None, + "cache_read_tokens": None, + "cache_write_tokens": None, + "reasoning_tokens": None, + })) + + counts = HermesRunner().read_usage_file(str(usage)) + + for key, value in counts.items(): + if key == RUN_META_KEY: + continue + assert isinstance(value, int), key + assert value == 0 + + def test_absent_file_is_normal_not_an_error(self, tmp_path): + """The upstream writer swallows exceptions, so no file is a valid outcome.""" + counts = HermesRunner().read_usage_file(str(tmp_path / "nope.json")) + assert counts["input_tokens"] == 0 + assert counts["reasoning_tokens"] == 0 + + def test_malformed_json_degrades_to_zeros(self, tmp_path): + usage = tmp_path / "usage.json" + usage.write_text("{not valid json") + counts = HermesRunner().read_usage_file(str(usage)) + assert counts["output_tokens"] == 0 + + def test_run_prompt_populates_tokens_from_usage_file(self, tmp_path): + """The end-to-end path must surface real tokens, not zeros.""" + runner = HermesRunner() + captured = {} + + def fake_run(cmd, **kwargs): + path = cmd[cmd.index("--usage-file") + 1] + captured["path"] = path + with open(path, "w") as fh: + json.dump({ + "input_tokens": 11, + "output_tokens": 22, + "reasoning_tokens": 33, + }, fh) + return MagicMock(stdout="done", stderr="", returncode=0) + + with patch("subprocess.run", side_effect=fake_run): + runner.run_prompt("x") + + counts = runner.last_token_counts + assert counts["input_tokens"] == 11 + assert counts["output_tokens"] == 22 + assert counts["reasoning_tokens"] == 33 + + def test_usage_file_is_cleaned_up(self, tmp_path): + runner = HermesRunner() + seen = {} + + def fake_run(cmd, **kwargs): + seen["path"] = cmd[cmd.index("--usage-file") + 1] + return MagicMock(stdout="", stderr="", returncode=0) + + import os + with patch("subprocess.run", side_effect=fake_run): + runner.run_prompt("x") + assert not os.path.exists(seen["path"]) + + +# --------------------------------------------------------------------------- +# Provenance — WHICH model served the run +# --------------------------------------------------------------------------- +# +# The usage file is the only place Hermes says which model answered. Without +# it the evidence document records `agent: "hermes"` — the name of the router, +# not of the thing that was measured — and two runs served by DeepSeek and +# Claude are indistinguishable after the fact. + +FIXTURES = Path(__file__).parent / "fixtures" / "hermes_output" + + +class TestUsageFileProvenance: + def test_read_usage_file_carries_model_and_provider(self, tmp_path): + usage = tmp_path / "usage.json" + usage.write_text(json.dumps({ + "input_tokens": 100, + "model": "deepseek-v4-pro", + "provider": "deepseek", + "session_id": "20260810_060335_c48b70", + "cost_status": "estimated", + })) + + meta = HermesRunner().read_usage_file(str(usage))[RUN_META_KEY] + + assert meta["model"] == "deepseek-v4-pro" + assert meta["provider"] == "deepseek" + assert meta["session_id"] == "20260810_060335_c48b70" + assert meta["cost_status"] == "estimated" + + def test_real_usage_fixture_is_read_the_same_way(self): + """Pin against a captured file, not only against a hand-written one.""" + meta = HermesRunner().read_usage_file( + str(FIXTURES / "usage_deepseek_v4_pro.json") + )[RUN_META_KEY] + assert meta["model"] == "deepseek-v4-pro" + assert meta["provider"] == "deepseek" + + def test_null_provenance_stays_none_never_a_guess(self, tmp_path): + """A quota-refused run reports nulls; inventing a model would be a lie.""" + meta = HermesRunner().read_usage_file( + str(FIXTURES / "usage_kimi_quota_403.json") + )[RUN_META_KEY] + assert meta["model"] is None + assert meta["provider"] is None + + def test_absent_file_yields_a_meta_block_of_nones(self, tmp_path): + """The key is always present: a missing key would read as 'never asked'.""" + meta = HermesRunner().read_usage_file(str(tmp_path / "nope.json"))[RUN_META_KEY] + assert set(meta) == {"model", "provider", "session_id", "cost_status"} + assert all(value is None for value in meta.values()) + + def test_non_string_provenance_is_dropped_not_coerced_to_junk(self, tmp_path): + usage = tmp_path / "usage.json" + usage.write_text(json.dumps({"model": {"nested": "object"}, "provider": []})) + meta = HermesRunner().read_usage_file(str(usage))[RUN_META_KEY] + assert meta["model"] is None + assert meta["provider"] is None + + def test_provenance_travels_through_parse_output(self): + runner = HermesRunner() + runner.last_token_counts = { + "input_tokens": 1, + RUN_META_KEY: {"model": "claude-fable-5", "provider": "anthropic", + "session_id": "s1", "cost_status": "unknown"}, + } + assert runner.parse_output("x")["token_counts"][RUN_META_KEY]["model"] == ( + "claude-fable-5" + ) + + def test_parse_output_meta_is_a_copy_not_a_shared_reference(self): + """Two parses of two runs must not be able to overwrite each other.""" + runner = HermesRunner() + parsed = runner.parse_output("x") + parsed["token_counts"][RUN_META_KEY]["model"] = "tampered" + assert runner.last_token_counts[RUN_META_KEY]["model"] is None + + def test_run_prompt_captures_provenance_end_to_end(self): + runner = HermesRunner() + + def fake_run(cmd, **kwargs): + with open(cmd[cmd.index("--usage-file") + 1], "w") as fh: + json.dump({ + "input_tokens": 11, + "model": "deepseek-v4-pro", + "provider": "deepseek", + "session_id": "20260810_060335_c48b70", + }, fh) + return MagicMock(stdout="done", stderr="", returncode=0) + + with patch("subprocess.run", side_effect=fake_run): + runner.run_prompt("x") + + meta = runner.last_token_counts[RUN_META_KEY] + assert meta["model"] == "deepseek-v4-pro" + assert meta["provider"] == "deepseek" + + def test_meta_does_not_disturb_token_arithmetic(self, tmp_path): + """`_meta` rides inside token_counts; it must never be summed as spend.""" + usage = tmp_path / "usage.json" + usage.write_text(json.dumps({ + "input_tokens": 10, "output_tokens": 20, "reasoning_tokens": 30, + "model": "deepseek-v4-pro", + })) + counts = HermesRunner().read_usage_file(str(usage)) + assert HermesRunner().total_tokens(counts) == 60 + + +class TestConfigSnapshot: + def test_reports_the_knobs_that_shaped_the_run(self, monkeypatch): + monkeypatch.setenv("SKILL_EVAL_HERMES_MODEL", "deepseek-v4-pro") + monkeypatch.setenv("SKILL_EVAL_HERMES_PROVIDER", "deepseek") + monkeypatch.setenv("SKILL_EVAL_HERMES_TIMEOUT", "900") + + snapshot = HermesRunner().config_snapshot() + + assert snapshot["model"] == "deepseek-v4-pro" + assert snapshot["provider"] == "deepseek" + assert snapshot["timeout"] == 900 + + def test_carries_no_credentials(self, monkeypatch): + """The snapshot lands in a committed evidence file; a leaked key is forever.""" + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-do-not-leak") + monkeypatch.setenv("DEEPSEEK_API_KEY", "sk-deepseek-do-not-leak") + monkeypatch.setenv("SKILL_EVAL_HERMES_MODEL", "deepseek-v4-pro") + + blob = json.dumps(HermesRunner().config_snapshot()) + + assert "do-not-leak" not in blob + assert not any("KEY" in key.upper() or "TOKEN" in key.upper() + for key in HermesRunner().config_snapshot()) + + def test_is_json_serialisable(self): + """It is embedded in the evidence document, which is dumped to JSON.""" + json.dumps(HermesRunner().config_snapshot()) + + def test_base_runner_snapshot_is_empty_not_missing(self): + """Every runner answers the question; unknown config is {} , not AttributeError.""" + assert ClaudeRunner().config_snapshot() == {} + + +# --------------------------------------------------------------------------- +# Cost accounting — the silent Pareto-corruption guard +# --------------------------------------------------------------------------- + +class TestTotalTokens: + def test_includes_reasoning_tokens(self): + """Base class drops reasoning; DeepSeek thinking spend must not vanish.""" + counts = {"input_tokens": 100, "output_tokens": 50, "reasoning_tokens": 500} + assert HermesRunner().total_tokens(counts) == 650 + + def test_reasoning_heavy_run_is_not_scored_cheaper_than_claude(self): + """Regression guard for a fabricated PARETO_BETTER verdict.""" + hermes_counts = {"input_tokens": 100, "output_tokens": 50, "reasoning_tokens": 500} + claude_counts = {"input_tokens": 100, "output_tokens": 50} + + hermes_total = HermesRunner().total_tokens(hermes_counts) + claude_total = ClaudeRunner().total_tokens(claude_counts) + + assert hermes_total > claude_total + + def test_missing_reasoning_key_defaults_to_zero(self): + assert HermesRunner().total_tokens({"input_tokens": 5, "output_tokens": 5}) == 10 + + def test_empty_dict_is_zero(self): + assert HermesRunner().total_tokens({}) == 0 + + def test_claude_arithmetic_is_unchanged(self): + """Pin the existing contract: cache and reasoning excluded for Claude.""" + counts = { + "input_tokens": 100, + "output_tokens": 50, + "cache_read_input_tokens": 30, + "reasoning_tokens": 999, + } + assert ClaudeRunner().total_tokens(counts) == 150 + + +# --------------------------------------------------------------------------- +# Capability boundaries — fail honestly rather than fake a signal +# --------------------------------------------------------------------------- + +class TestCapabilities: + def test_trigger_eval_unsupported(self): + """No structured tool events on -z, so activation cannot be detected.""" + assert HermesRunner().supports_trigger_eval() is False + + def test_claude_supports_trigger_eval(self): + assert ClaudeRunner().supports_trigger_eval() is True diff --git a/tests/test_judge_wiring.py b/tests/test_judge_wiring.py new file mode 100644 index 0000000..55b326d --- /dev/null +++ b/tests/test_judge_wiring.py @@ -0,0 +1,299 @@ +"""The judge a call site selects must be the judge the operator asked for. + +`grade_output` accepting a `judge_runner` argument does not by itself close +blocker B1 -- the orchestrators have to forward it. That gap existed once: +`grade_output` had the parameter while `functional.py` and `compare.py` still +called it positionally, so `--agent hermes` executed through Hermes and was +judged by Claude exactly as before, with a green test suite. + +Closing B1 by forwarding the *executing* runner then traded that leak for a +worse one: each model judged its own output, so a cross-model transfer number +blended skill transfer with judge leniency and the two are inseparable after +the fact (MVE S3 exclusion X4). `functional` therefore has three states, not +two, and these tests pin all three: + + * `--judge-agent NAME` -> that runner judges, and only that runner. + * `--judge-agent` unset -> NO_JUDGE. No model is asked to judge anything: + not Claude, and not the model under test. + * never -> a silent fallback to either one. + +These tests pin the wiring rather than the signature. +""" + +import json +from pathlib import Path +from unittest.mock import patch + +import pytest + +from skill_eval.agent_runner import AgentRunner + + +class _MarkerRunner(AgentRunner): + """A runner whose identity can be asserted on downstream.""" + + def __init__(self, text: str = "produced output") -> None: + self.text = text + + def check_available(self) -> None: + return None + + def run_prompt(self, prompt, skill_path=None, workspace_dir=None, + timeout=120, output_format="text"): + return (self.text, "", 0, 0.01) + + def parse_output(self, raw: str) -> dict: + return {"events": [], "tool_calls": [], "text": self.text, + "token_counts": {"input_tokens": 1, "output_tokens": 1}} + + +@pytest.fixture +def eval_case(): + from skill_eval.eval_schemas import EvalCase + return EvalCase( + id="case-1", + prompt="do the thing", + # Ambiguous on purpose: a deterministic assertion would short-circuit + # before any judge is selected and the test would pass vacuously. + assertions=["demonstrates good judgement"], + files=[], + ) + + +def _capture_judges(seen: list[object]): + def _capture(output, assertions, timeout=60, judge_runner=None): + seen.append(judge_runner) + return ([], 1.0) + return _capture + + +class TestFunctionalNeverSelfJudges: + """The model under test must never be handed its own homework to grade.""" + + def test_default_is_no_judge_not_the_executing_runner(self, eval_case, tmp_path): + from skill_eval.functional import _execute_eval_pair + from skill_eval.grading import NO_JUDGE + + runner = _MarkerRunner() + seen: list[object] = [] + + with patch("skill_eval.functional.grade_output", + side_effect=_capture_judges(seen)): + _execute_eval_pair( + eval_case, tmp_path, tmp_path, 0, timeout=30, runner=runner, + ) + + assert seen, "grade_output was never called" + assert all(j is NO_JUDGE for j in seen), ( + "with no judge configured functional.py must pass NO_JUDGE; " + f"got {seen!r}" + ) + assert not any(j is runner for j in seen), ( + "the executing runner was forwarded as its own judge -- that is " + "the self-judging confound, not a fix for it" + ) + + def test_explicit_judge_runner_is_the_one_forwarded(self, eval_case, tmp_path): + from skill_eval.functional import _execute_eval_pair + + runner = _MarkerRunner() + judge = _MarkerRunner("judge output") + seen: list[object] = [] + + with patch("skill_eval.functional.grade_output", + side_effect=_capture_judges(seen)): + _execute_eval_pair( + eval_case, tmp_path, tmp_path, 0, timeout=30, + runner=runner, judge_runner=judge, + ) + + assert seen, "grade_output was never called" + assert all(j is judge for j in seen), ( + f"the explicitly configured judge must be used; got {seen!r}" + ) + + def test_no_judge_does_not_reach_claude_either(self, eval_case, tmp_path): + """NO_JUDGE means no model at all -- not a fallback to the old path.""" + from unittest.mock import MagicMock + + from skill_eval.functional import _execute_eval_pair + + check = MagicMock() + run = MagicMock(return_value=("[]", "", 0, 0.01)) + + with patch("skill_eval._claude.check_claude_available", check), \ + patch("skill_eval._claude.run_claude_prompt", run): + _execute_eval_pair( + eval_case, tmp_path, tmp_path, 0, timeout=30, + runner=_MarkerRunner(), + ) + + assert check.call_count == 0 and run.call_count == 0, ( + "an unset --judge-agent fell through to the Claude judge" + ) + + def test_unjudged_assertion_fails_visibly_rather_than_passing( + self, eval_case, tmp_path + ): + """An assertion nobody graded is not a pass. + + It is also not an ordinary failure: `uncertain` marks it as a gap in + the measurement so a reader can tell "the judge said no" from "no + judge was asked", which a bare passed=False cannot. + """ + from skill_eval.functional import _execute_eval_pair + + _, with_g, _ = _execute_eval_pair( + eval_case, tmp_path, tmp_path, 0, timeout=30, runner=_MarkerRunner(), + ) + + assert with_g.pass_rate == 0.0 + result = with_g.assertion_results[0] + assert result["passed"] is False + assert result["uncertain"] is True + assert "judge" in result["evidence"].lower() + + +class TestRunFunctionalEvalResolvesJudge: + """`--judge-agent` is resolved once, at the orchestrator, and only if set.""" + + def _evals(self, tmp_path): + evals = tmp_path / "evals.json" + evals.write_text(json.dumps([{ + "id": "case-1", "prompt": "do it", + "assertions": ["demonstrates good judgement"], + }])) + return evals + + def test_judge_agent_unset_never_calls_get_runner_twice(self, tmp_path): + from skill_eval.functional import run_functional_eval + + runner = _MarkerRunner() + with patch("skill_eval.functional.get_runner", + return_value=runner) as get: + run_functional_eval( + str(tmp_path), evals_path=str(self._evals(tmp_path)), + output_path=str(tmp_path / "benchmark.json"), agent="claude", + ) + + assert get.call_count == 1, ( + "no judge was requested, so no judge runner may be constructed; " + f"get_runner called {get.call_count} times" + ) + + def test_judge_agent_set_resolves_that_runner(self, tmp_path): + from skill_eval.functional import run_functional_eval + + runner = _MarkerRunner() + judge = _MarkerRunner("judge") + asked: list[str] = [] + + def _get(name): + asked.append(name) + return judge if name == "claude" else runner + + with patch("skill_eval.functional.get_runner", side_effect=_get): + run_functional_eval( + str(tmp_path), evals_path=str(self._evals(tmp_path)), + output_path=str(tmp_path / "benchmark.json"), + agent="hermes", judge_agent="claude", + ) + + assert asked == ["hermes", "claude"], ( + f"execution and judge runners resolved wrongly: {asked!r}" + ) + + def test_unknown_judge_agent_is_an_error_not_a_fallback(self, tmp_path): + from skill_eval.functional import run_functional_eval + + def _get(name): + if name == "nope": + raise KeyError("No agent runner registered as 'nope'") + return _MarkerRunner() + + with patch("skill_eval.functional.get_runner", side_effect=_get): + rc = run_functional_eval( + str(tmp_path), evals_path=str(self._evals(tmp_path)), + output_path=str(tmp_path / "benchmark.json"), + agent="claude", judge_agent="nope", + ) + + assert rc == 2, "an unresolvable judge must abort, not degrade silently" + + +class TestJudgeAgentReachesTheCLI: + def test_cli_forwards_judge_agent(self, tmp_path): + from skill_eval.cli import main + + with patch("skill_eval.functional.run_functional_eval", + return_value=0) as run: + main(["functional", str(tmp_path), "--judge-agent", "claude"]) + + assert run.call_args.kwargs["judge_agent"] == "claude" + + def test_cli_default_is_none(self, tmp_path): + from skill_eval.cli import main + + with patch("skill_eval.functional.run_functional_eval", + return_value=0) as run: + main(["functional", str(tmp_path)]) + + assert run.call_args.kwargs["judge_agent"] is None + + +class TestCompareForwardsJudge: + def test_executing_runner_is_the_judge(self, eval_case, tmp_path): + from skill_eval.compare import _run_single_skill + + runner = _MarkerRunner() + seen: list[object] = [] + + def _capture(output, assertions, timeout=60, judge_runner=None): + seen.append(judge_runner) + return ([], 1.0) + + with patch("skill_eval.compare.grade_output", side_effect=_capture): + _run_single_skill( + eval_case, tmp_path, tmp_path, timeout=30, runner=runner, + ) + + assert seen, "grade_output was never called" + assert all(j is runner for j in seen), ( + "compare.py must forward the executing runner as the judge; " + f"got {seen!r}" + ) + + +class TestNoClaudeLeakEndToEnd: + """The whole point: a non-Claude run must never touch the Claude judge. + + Asserting on *non-invocation* rather than on a poisoned exception is + deliberate. `_llm_grade` wraps its Claude import in a broad + ``except Exception`` and degrades to ``passed=False``, so a raising stub is + swallowed and the leak still scores 0.0 -- identical to the clean path. + An earlier version of this test did exactly that and passed with the + wiring reverted, i.e. it proved nothing. + """ + + def test_non_claude_run_never_calls_the_claude_judge(self, eval_case, tmp_path): + from unittest.mock import MagicMock + + from skill_eval.functional import _execute_eval_pair + + runner = _MarkerRunner() + check = MagicMock() + run = MagicMock(return_value=("[]", "", 0, 0.01)) + + with patch("skill_eval._claude.check_claude_available", check), \ + patch("skill_eval._claude.run_claude_prompt", run): + _execute_eval_pair( + eval_case, tmp_path, tmp_path, 0, timeout=30, runner=runner, + ) + + assert check.call_count == 0, ( + "Claude availability was probed during a non-Claude run -- " + "the judge leaked" + ) + assert run.call_count == 0, ( + "Claude was invoked as judge during a non-Claude run" + ) diff --git a/tests/test_runner_capabilities.py b/tests/test_runner_capabilities.py new file mode 100644 index 0000000..05bbb0e --- /dev/null +++ b/tests/test_runner_capabilities.py @@ -0,0 +1,120 @@ +"""Runner capability gating for trigger evaluation. + +Trigger eval detects skill activation by inspecting structured tool-use +events in the agent's stream-json output. `hermes -z` emits only the final +content block as plain text, so there is nothing to detect. Running trigger +eval against Hermes would silently report 0% activation for every query -- +a fabricated result that looks like a real finding. + +The runner declares the capability; the orchestrator refuses up front rather +than producing a meaningless report. +""" + +import json +from unittest.mock import MagicMock, patch + +import pytest + +from skill_eval.agent_runner import AgentRunner, ClaudeRunner +from skill_eval.hermes_runner import HermesRunner + + +class TestCapabilityDeclaration: + def test_base_class_provides_a_default(self): + """Existing third-party runners must not break on the new method.""" + assert hasattr(AgentRunner, "supports_trigger_eval") + + def test_claude_supports_trigger_eval(self): + assert ClaudeRunner().supports_trigger_eval() is True + + def test_hermes_does_not_support_trigger_eval(self): + assert HermesRunner().supports_trigger_eval() is False + + def test_default_is_supported_for_unknown_runners(self): + """A runner that says nothing keeps the pre-existing behaviour.""" + + class _Silent(AgentRunner): + def check_available(self): return None + def run_prompt(self, prompt, skill_path=None, workspace_dir=None, + timeout=120, output_format="text"): + return ("", "", 0, 0.0) + def parse_output(self, raw): return {} + + assert _Silent().supports_trigger_eval() is True + + +class TestTriggerEvalGuard: + """run_trigger_eval must refuse before spending any model calls.""" + + @pytest.fixture + def queries_file(self, tmp_path): + skill = tmp_path / "skill" + (skill / "evals").mkdir(parents=True) + (skill / "SKILL.md").write_text( + "---\nname: demo\ndescription: demo skill\n---\n\n# Demo\n" + ) + (skill / "evals" / "eval_queries.json").write_text(json.dumps([ + {"query": "do the thing", "should_trigger": True}, + {"query": "unrelated question", "should_trigger": False}, + ])) + return skill + + def test_exits_2_when_runner_lacks_support(self, queries_file): + from skill_eval.trigger import run_trigger_eval + + # check_available is stubbed so a missing binary can never be mistaken + # for the capability guard -- that false green hid this bug once already. + runner = HermesRunner() + with patch.object(runner, "check_available"), \ + patch.object(runner, "run_prompt", + return_value=("", "", 0, 0.0)), \ + patch("skill_eval.trigger.get_runner", return_value=runner): + rc = run_trigger_eval(str(queries_file), agent="hermes") + + assert rc == 2, "unsupported trigger eval must exit 2, not 0 or 1" + + def test_runs_no_queries_when_unsupported(self, queries_file): + """The guard must fire before any subprocess is launched.""" + from skill_eval.trigger import run_trigger_eval + + runner = HermesRunner() + with patch.object(runner, "check_available"), \ + patch.object(runner, "run_prompt", + return_value=("", "", 0, 0.0)) as run_prompt, \ + patch("skill_eval.trigger.get_runner", return_value=runner): + run_trigger_eval(str(queries_file), agent="hermes") + + assert run_prompt.call_count == 0, "no model calls may be spent" + + def test_writes_no_misleading_report(self, queries_file): + """A 0%-activation report would read as a real finding. Refuse instead. + + Observed for real before the guard existed: a live Hermes run spent + two subprocesses and emitted "Trigger precision: 0.0%" -- an artifact + of stream-json being absent, not of the skill failing to activate. + """ + from skill_eval.trigger import run_trigger_eval + + runner = HermesRunner() + with patch.object(runner, "check_available"), \ + patch.object(runner, "run_prompt", + return_value=("", "", 0, 0.0)), \ + patch("skill_eval.trigger.get_runner", return_value=runner): + run_trigger_eval(str(queries_file), agent="hermes") + + report = queries_file / "evals" / "trigger_report.json" + assert not report.exists(), "must not emit a fabricated 0% report" + + def test_supported_runner_is_not_blocked(self, queries_file): + """Claude must still reach the normal path.""" + from skill_eval.trigger import run_trigger_eval + + runner = ClaudeRunner() + with patch.object(runner, "check_available"), \ + patch.object(runner, "run_prompt", + return_value=("", "", 0, 0.0)), \ + patch("skill_eval.trigger.get_runner", return_value=runner): + rc = run_trigger_eval(str(queries_file), agent="claude", + runs_per_query=1) + + assert rc != 2, "supported runners must not hit the capability guard"