From f90d0d0f206ec1e5cbd2b2ae8ad95e8d93f5b1b5 Mon Sep 17 00:00:00 2001 From: jon-devlapaz Date: Tue, 1 Sep 2026 12:13:05 -0500 Subject: [PATCH 1/4] feat: make the paired evaluator harness-agnostic The runner now takes skill, tasks, harness binary, and opaque model id with no Codex special case. Adapters own argv, listing, and auth copy. Live traces stay in gitignored .eval-output/. Co-authored-by: Cursor --- .eval-output/README.md | 37 ++ .gitignore | 7 +- AGENTS.md | 3 - README.md | 98 +++-- skills/skill-eval-loop/SKILL.md | 67 ++-- skills/skill-eval-loop/agents/openai.yaml | 2 +- .../references/promotion-workflow.md | 6 +- .../scripts/harnesses/__init__.py | 11 +- .../scripts/harnesses/antigravity.py | 37 +- .../skill-eval-loop/scripts/harnesses/base.py | 24 ++ .../scripts/harnesses/claude.py | 6 + .../scripts/harnesses/codex.py | 3 + .../scripts/harnesses/cursor_agent.py | 61 +++- .../scripts/harnesses/hermes.py | 34 +- .../skill-eval-loop/scripts/harnesses/muse.py | 33 +- .../skill-eval-loop/scripts/harnesses/pi.py | 36 +- .../scripts/skill_eval_loop.py | 139 ++++--- tests/fixtures/simple-fake-agy | 17 + tests/fixtures/simple-fake-cursor-agent | 21 ++ tests/fixtures/simple-fake-muse | 10 + tests/fixtures/simple-fake-pi | 18 + tests/helpers.py | 4 + tests/test_harnesses.py | 341 +++++++++++++++++- 23 files changed, 867 insertions(+), 148 deletions(-) create mode 100644 .eval-output/README.md create mode 100755 tests/fixtures/simple-fake-agy create mode 100755 tests/fixtures/simple-fake-cursor-agent create mode 100755 tests/fixtures/simple-fake-muse create mode 100755 tests/fixtures/simple-fake-pi diff --git a/.eval-output/README.md b/.eval-output/README.md new file mode 100644 index 0000000..8e9c791 --- /dev/null +++ b/.eval-output/README.md @@ -0,0 +1,37 @@ +# Local evaluation artifacts + +This directory is gitignored except for this README. + +Put live `--output` and `--output` calibration directories here: + +```text +.eval-output// +``` + +Or use any path outside the repository. Do not commit run directories. + +## What to keep and inspect + +Retained evidence for a paired run: + +- `run.json`, `config.json`, `tasks.jsonl` +- `task-/trial-NNN/report.json` and `report.md` +- `control/` and `treatment/`: `response.md`, `trace.jsonl`, `stderr.txt` +- judge artifacts under `judge-NNN/` and `pairwise-NNN/` when a rubric ran + +Those files are the evaluation record. Read them locally. Share only after +review; they can contain prompts, model output, and harness metadata. + +## What is not evidence + +Harness runtime homes are copied or created for the process, then deleted +when the run finishes cleanly: + +- `cursor-home/` (includes `chats/**/store.db`) +- `codex-home/` (may include a copied `auth.json` during the run) +- `claude-home/`, `hermes-home/`, `agy-home/` +- per-invocation `home/` directories + +If those directories are still here, the run was interrupted or predates +cleanup. Do not commit them. SQLite chat databases are especially noisy in +`git status` and are not part of the comparison report. diff --git a/.gitignore b/.gitignore index 5f66a0e..c7f2628 100644 --- a/.gitignore +++ b/.gitignore @@ -1,7 +1,12 @@ .agents/ -.eval-runs/ .pytest_cache/ .ruff_cache/ __pycache__/ *.py[cod] node_modules/ + +# Live evaluation artifacts. Inspect AI ignores logs/; Cursor agent-trace +# ignores .agent-trace/. Keep harness traces and Cursor chat DBs local. +.eval-output/* +!.eval-output/README.md +.eval-runs/ diff --git a/AGENTS.md b/AGENTS.md index a6224d4..16a2253 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,8 +4,5 @@ Follow the maintainability principles in [ZEN.md](ZEN.md). -## Next change -Harness adapters, judging, and domain tests now have owners. `skill_eval_loop.py` is still ~2k lines; load models still serialize to dicts before the rest of the pipeline. Do not split further until a later change needs a new owner. -The remaining evidence gate is Task 10: a repeated-trial promotion run on an independently controlled, human-labeled holdout. Do not invent that holdout here. diff --git a/README.md b/README.md index 90418e8..54eb896 100644 --- a/README.md +++ b/README.md @@ -1,10 +1,14 @@ # skill-eval-loop -`skill-eval-loop` is a self-contained Python 3 Agent Skill that measures -whether explicitly applying one local skill changes task outcomes. The control -receives the original task. The treatment receives the exact hashed skill's -`SKILL.md` instructions in its prompt, with the installed payload available for -referenced files. The runner retains the raw evidence and a comparison report. +`skill-eval-loop` measures one question: for this skill, these JSONL tasks, +this CLI harness, and this opaque model id, does injecting `SKILL.md` change +the outcome versus the same prompt with no skill? + +The runner does not talk to a model vendor. It runs whatever binary +`--harness` / `--harness-bin` names, passes `--model` through unchanged, and +keeps traces. `--harness script` is the escape hatch for any other program. +Control gets the raw task. Treatment gets the hashed skill instructions. +Reports are derived from retained evidence. ## Install @@ -21,8 +25,15 @@ The public launcher requires Python 3 and no package installation: ```bash EVALUATOR="$PWD/.agents/skills/skill-eval-loop/scripts/skill-eval-loop" "$EVALUATOR" healthcheck +"$EVALUATOR" models --harness pi --harness-bin /absolute/path/to/pi ``` +Copy `--model` and `--judge-model` from that harness's listing. If the listing +is non-empty, `run` and `calibrate` reject ids that are not on it. An empty +listing does not reject. For a quality-complete rubric run, use the same +`--harness` for student and judge: a different `--judge-harness` can mark a +run independent, but that calibration cannot bind. + ## Run an evaluation Create a JSONL task file. Every non-empty line needs a unique, path-safe `id`, @@ -38,9 +49,9 @@ Run a side-effect-free plan before a live invocation: "$EVALUATOR" run \ --skill /absolute/path/to/target-skill \ --tasks /absolute/path/to/tasks.jsonl \ - --output /absolute/path/to/fresh-run \ - --harness codex \ - --harness-bin /absolute/path/to/codex \ + --output "$PWD/.eval-output/fresh-run" \ + --harness pi \ + --harness-bin /absolute/path/to/pi \ --model exact-model-id \ --trials 1 \ --timeout-seconds 300 \ @@ -71,20 +82,20 @@ development evidence, not a secret client holdout. For rubric tasks, also pass `--judge-model` with a different exact model identifier and `--calibration /absolute/path/to/calibration.json` from an -accepted calibrate run. The runner judges each condition only after -deterministic gates pass. A valid same-provider judgment is -`provisional_non_independent`; a timeout, failed gate, malformed response, or +accepted calibrate run on that same harness pair. The runner judges each +condition only after deterministic gates pass. A valid same-harness judgment +is `provisional_non_independent`; a timeout, failed gate, malformed response, or identity mismatch is `unknown`. A missing trace-reported model is unattested, not a quality unknown. Omitting `--calibration` is allowed, but a rubric run then remains quality-incomplete and cannot exit `0`. -The runner invokes Codex sequentially in read-only mode, emitting invocation -progress to stderr. Odd trials run control first; even trials run treatment -first. The evaluator injects the exact `SKILL.md` text itself, so treatment -exposure does not depend on model-side discovery. Target, judge, and calibration -invocations share one lifecycle that uses cleaned OS-temporary workspaces outside -the evaluator repository. It retains `run.json`, the -planned configuration, tasks, condition responses, traces, stderr, and a +The runner invokes the configured harness sequentially in read-only mode, +emitting invocation progress to stderr. Odd trials run control first; even +trials run treatment first. The evaluator injects the exact `SKILL.md` text +itself, so treatment exposure does not depend on model-side discovery. Target, +judge, and calibration invocations share one lifecycle that uses cleaned +OS-temporary workspaces outside the evaluator repository. It retains `run.json`, +the planned configuration, tasks, condition responses, traces, stderr, and a JSON/Markdown report for every pair. `runner_valid` means the runner held its declared variables, isolation checks, @@ -92,7 +103,7 @@ and treatment activation. It is not a general quality claim. Read both transcrip interpreting `treatment_only`, `both_pass`, `control_only`, or `both_fail`. JSON and Markdown reports expose evaluator-recorded instruction delivery plus -optional trace telemetry when Codex also reads the installed skill, rolled-up timing and token usage, +optional trace telemetry when the harness also reads the installed skill, rolled-up timing and token usage, calibration (`not_run`, or `accepted` plus `fixtures_sha256` when a bound calibration is supplied), every judged dimension, `quality_status`, and `quality_outcome`. Deterministic-only reports say semantic quality was not @@ -110,9 +121,9 @@ Calibrate the pairwise judge against versioned human-labeled ```bash python3 skills/skill-eval-loop/scripts/skill_eval_loop.py calibrate \ --fixtures /absolute/path/to/calibration/v1.json \ - --output /absolute/path/to/fresh-calibration \ - --harness codex \ - --harness-bin /absolute/path/to/codex \ + --output "$PWD/.eval-output/fresh-calibration" \ + --harness pi \ + --harness-bin /absolute/path/to/pi \ --model exact-model-id \ --judge-model exact-judge-model-id \ --dry-run @@ -134,17 +145,38 @@ owner. ## Boundaries -The minimum runner supports Codex, deterministic graders, a provisional -same-provider rubric judge, blinded pairwise comparison, human-labeled -calibration fixtures, and hash-bound two-reviewer promotion evidence. It records -operator-supplied cost; it does not discover pricing, provide an independent -automated judge, authenticate human identities, run in parallel, discover -providers, or adapt other harnesses. - -Live evaluation is a trusted local-operator workflow. The configured harness -and Codex executable can read the run-local Codex credentials and therefore -must be trusted. This project does not sandbox hostile executables. Keep raw -run directories local and inspect them before sharing any evidence. +The supported path is: list enumerable model ids, use those ids on the harness +that will call them, keep student and judge on the same harness, calibrate, +dry-run, live-run, and read the retained traces. Deterministic graders can +complete without a judge. + +Adapters exist for Codex, Claude Code, Cursor Agent, Muse, Hermes, Pi, +Antigravity, and a custom script. CI proves mechanics with fake Codex and +`script`. Local live dogfood on 2026-09-01 succeeded on cursor-agent, pi, +muse, antigravity, hermes, and codex. Claude is installed but not logged in +on this machine (`claude auth status` reports `loggedIn: false`); the adapter +treats that JSON error as a failed invocation rather than a model answer. +`models` enumerates a harness when that CLI can list ids (Muse catalog, +Cursor Agent `--list-models`, Pi `--list-models`, Antigravity `agy models`). +An empty listing does not reject an id. `--harness script` wraps any other +binary. + +`--promotion` plus `prepare-review` / `finalize-review` implement a human +review workflow. They do not prove an independent holdout or complete a +promotion claim. Tasks run in empty temp workspaces, so repository-editing +evals are not quality evidence. + +The runner records operator-supplied cost. It does not pick models, discover +providers, price calls, authenticate reviewers, or run in parallel. + +Live evaluation is a trusted local-operator workflow. Write `--output` under +the gitignored `.eval-output/` directory or outside the repo. The runner +retains reports, responses, traces, and stderr. Harness homes (`cursor-home`, +`codex-home`, and Cursor `chats/**/store.db`) are deleted after a clean run; +leftovers from interrupted runs are still not git material. The configured +harness executable can read local credentials and therefore must be trusted. +This project does not sandbox hostile executables. Inspect raw runs locally +before sharing any evidence. ## Development diff --git a/skills/skill-eval-loop/SKILL.md b/skills/skill-eval-loop/SKILL.md index afd2c31..5beb0e9 100644 --- a/skills/skill-eval-loop/SKILL.md +++ b/skills/skill-eval-loop/SKILL.md @@ -1,6 +1,6 @@ --- name: skill-eval-loop -description: Run a paired, evidence-retaining Codex evaluation of one local Agent Skill against a no-skill control. Use when measuring whether a skill improves JSONL-defined task outcomes, validating a skill with deterministic graders, or comparing control and treatment responses. +description: Run a paired, evidence-retaining evaluation of one local Agent Skill against a no-skill control. Use when measuring whether a skill improves JSONL-defined task outcomes, validating a skill with deterministic graders, or comparing control and treatment responses. --- # Skill Eval Loop @@ -19,6 +19,22 @@ EVALUATOR=/absolute/path/to/skill-eval-loop/scripts/skill-eval-loop "$EVALUATOR" healthcheck ``` +List model ids a harness can enumerate before choosing `--model` or +`--judge-model`. If the listing is non-empty, `run` and `calibrate` dry-runs +reject ids that are not on it. An empty listing does not reject. Muse reads +its local catalog; Cursor Agent and Pi call `--list-models`; Antigravity calls +`agy models`. Other adapters currently return an empty listing. `--harness +script` wraps any other binary. + +For a quality-complete rubric run, use the same `--harness` for student and +judge. A different `--judge-harness` can mark a live run `independent`, but +`calibrate` then cannot bind: binding requires +`provisional_non_independent` cases. + +```bash +"$EVALUATOR" models --harness pi --harness-bin /absolute/path/to/pi +``` + Use newline-delimited JSON tasks. Each task needs a path-safe `id`, a non-empty `prompt`, and at least one grader. @@ -64,15 +80,15 @@ missing-suite error is the precondition for this coordinator workflow. ## Plan the exact run Pass absolute paths and a fresh output directory. Dry-run validates consumed -inputs, resolves the Codex executable, hashes the skill and tasks, and creates +inputs, resolves the harness executable, hashes the skill and tasks, and creates neither run artifacts nor provider calls. ```bash "$EVALUATOR" run \ --skill /absolute/path/to/target-skill \ - --output /absolute/path/to/fresh-run \ - --harness codex \ - --harness-bin /absolute/path/to/codex \ + --output /absolute/path/to/.eval-output/fresh-run \ + --harness pi \ + --harness-bin /absolute/path/to/pi \ --model exact-model-id \ --judge-model exact-judge-model-id \ --calibration /absolute/path/to/fresh-calibration/calibration.json \ @@ -95,10 +111,8 @@ the planned run. The retained fixture path must still exist at its recorded absolute path with the same SHA-256 hash. Omitting `--calibration` is allowed, but a rubric run then remains quality-incomplete and cannot exit `0`. -The judge must differ from the runner model. An OpenAI model judging another -OpenAI model is explicitly same-provider evidence, not an independent judgment. -A recommended OpenAI-only pair is `--model gpt-5.6-terra --judge-model -gpt-5.6-sol`. +The judge must differ from the runner model. Same-provider judging is +same-provider evidence, not an independent judgment. The default evaluation role is `development`. Treat any suite visible to the skill author or repeatedly used during hill-climbing as development or @@ -111,9 +125,9 @@ calibration, and repeated trials: "$EVALUATOR" run \ --skill /absolute/path/to/target-skill \ --tasks /absolute/path/to/operator-controlled-holdout.jsonl \ - --output /absolute/path/to/fresh-promotion-run \ - --harness codex \ - --harness-bin /absolute/path/to/codex \ + --output /absolute/path/to/.eval-output/fresh-promotion-run \ + --harness pi \ + --harness-bin /absolute/path/to/pi \ --model exact-model-id \ --judge-model exact-judge-model-id \ --calibration /absolute/path/to/fresh-calibration/calibration.json \ @@ -141,9 +155,9 @@ with rationale. The judge sees anonymized `A`/`B` text only. ```bash "$EVALUATOR" calibrate \ --fixtures /absolute/path/to/calibration/v1.json \ - --output /absolute/path/to/fresh-calibration \ - --harness codex \ - --harness-bin /absolute/path/to/codex \ + --output /absolute/path/to/.eval-output/fresh-calibration \ + --harness pi \ + --harness-bin /absolute/path/to/pi \ --model exact-model-id \ --judge-model exact-judge-model-id \ --dry-run @@ -176,7 +190,7 @@ Run the identical command without `--dry-run`. The runner: `SKILL.md` instructions before that task; - runs sequentially, alternating control-first and treatment-first by trial; - emits invocation progress to stderr and stops after a detected infrastructure failure; -- invokes Codex in read-only mode; +- invokes the configured harness with that adapter's argv; - retains response, trace, stderr, execution metadata, and reports; - records treatment instruction delivery and requires deterministic gates before any rubric judge; @@ -188,7 +202,7 @@ runner may show `both_pass`, `both_fail`, `control_only`, or `treatment_only`. Read both responses before making a quality claim. For rubric tasks, inspect each condition's `rubric_judgments`, the pair's -`pairwise` evidence, and `dimension_results`. A successful Codex judgment is +`pairwise` evidence, and `dimension_results`. A successful same-harness judgment is labeled `provisional_non_independent`. A timeout, failed deterministic gate, malformed response, mismatched judge identity, or identical runner and judge model produces `unknown`. If the trace does not report a model, the requested @@ -204,7 +218,7 @@ dimension favors the condition opposing the overall winner, or the restored winner condition. A tied dimension does not contradict an overall winner. An overall winner is never a quality pass when a dimension is unknown or disagrees. Evaluator-owned treatment injection is `observed`; optional trace -telemetry records whether Codex also opened the installed `SKILL.md`. Delivery +telemetry records whether the harness also opened the installed `SKILL.md`. Delivery proves exposure, not faithful compliance. A bound accepted calibration records `calibration_status: accepted` and `fixtures_sha256` in `run.json` and every pair report. Without a binding, @@ -220,14 +234,15 @@ degenerate, unavailable, or hash-drifted supplied calibration is runner-invalid. Read `run.json` followed by each pair's `report.json`, `report.md`, responses, traces, and stderr. Confirm the control lacks the target skill, the treatment contains the source hash, and any trace-reported model identity agrees with the -requested model. - -A live run creates `$output/codex-home` and points Codex at that directory. -If `~/.codex/auth.json` exists, it is copied there for the process. The entire -run-local Codex home is removed afterward; it is not retained evidence. Use only a trusted -harness: it can read the run-local credential file, and this evaluator is not -a sandbox for hostile executables. Keep raw runs local and inspect them before -sharing. Host Codex skills are not part of the intervention. +requested model. Write `--output` under the repo's gitignored `.eval-output/` +directory or outside the repository. + +A live run lets the adapter prepare a run-local home and env, then deletes that +home afterward; it is not retained evidence. Codex copies `~/.codex/auth.json` +into `CODEX_HOME` when present. Use only a trusted harness: it can read local +credentials, and this evaluator is not a sandbox for hostile executables. Keep +raw runs local and inspect them before sharing. Host-installed harness skills +are not part of the intervention. Treat injection of the exact hashed payload's `SKILL.md` instructions as the intervention. The control receives the original task; the treatment receives diff --git a/skills/skill-eval-loop/agents/openai.yaml b/skills/skill-eval-loop/agents/openai.yaml index 0e5fb68..323b25a 100644 --- a/skills/skill-eval-loop/agents/openai.yaml +++ b/skills/skill-eval-loop/agents/openai.yaml @@ -1,4 +1,4 @@ interface: display_name: "Skill Eval Loop" - short_description: "Run paired Codex skill diagnostics" + short_description: "Run paired skill-vs-control diagnostics" default_prompt: "Use $skill-eval-loop to compare one local skill against a no-skill control with a dry-run and retained evidence." diff --git a/skills/skill-eval-loop/references/promotion-workflow.md b/skills/skill-eval-loop/references/promotion-workflow.md index 3b999c1..6704337 100644 --- a/skills/skill-eval-loop/references/promotion-workflow.md +++ b/skills/skill-eval-loop/references/promotion-workflow.md @@ -28,9 +28,9 @@ accepted calibration and inspect the dry-run plan before authorizing live calls. "$EVALUATOR" run \ --skill /absolute/path/to/target-skill \ --tasks /absolute/custodian/path/holdout.jsonl \ - --output /absolute/path/to/fresh-promotion-run \ - --harness codex \ - --harness-bin /absolute/path/to/codex \ + --output /absolute/path/to/.eval-output/fresh-promotion-run \ + --harness pi \ + --harness-bin /absolute/path/to/pi \ --model exact-runner-model \ --judge-model exact-judge-model \ --calibration /absolute/path/to/calibration.json \ diff --git a/skills/skill-eval-loop/scripts/harnesses/__init__.py b/skills/skill-eval-loop/scripts/harnesses/__init__.py index 7327d03..9fbd157 100644 --- a/skills/skill-eval-loop/scripts/harnesses/__init__.py +++ b/skills/skill-eval-loop/scripts/harnesses/__init__.py @@ -3,7 +3,7 @@ from __future__ import annotations from harnesses.antigravity import AntigravityAdapter -from harnesses.base import BaseHarnessAdapter, TraceResult, is_infrastructure_failure +from harnesses.base import BaseHarnessAdapter, TraceResult, is_infrastructure_failure, reject_unknown_model from harnesses.claude import ClaudeAdapter from harnesses.codex import CodexAdapter, parse_trace, prepare_run_codex_home, trace_value from harnesses.cursor_agent import CursorAgentAdapter @@ -32,12 +32,8 @@ def get_harness_adapter(name: str) -> BaseHarnessAdapter: return adapter_class() -def resolve_harness(executable_or_name: str, executable: str | None = None) -> tuple[str, str]: - if executable is None and executable_or_name not in SUPPORTED_HARNESSES: - adapter = get_harness_adapter("codex") - return adapter.resolve(executable_or_name) - adapter = get_harness_adapter(executable_or_name) - return adapter.resolve(executable) +def resolve_harness(name: str, executable: str | None = None) -> tuple[str, str]: + return get_harness_adapter(name).resolve(executable) __all__ = [ @@ -48,6 +44,7 @@ def resolve_harness(executable_or_name: str, executable: str | None = None) -> t "is_infrastructure_failure", "parse_trace", "prepare_run_codex_home", + "reject_unknown_model", "resolve_harness", "trace_value", ] diff --git a/skills/skill-eval-loop/scripts/harnesses/antigravity.py b/skills/skill-eval-loop/scripts/harnesses/antigravity.py index d1ee73c..f42e8a0 100644 --- a/skills/skill-eval-loop/scripts/harnesses/antigravity.py +++ b/skills/skill-eval-loop/scripts/harnesses/antigravity.py @@ -1,16 +1,47 @@ -"""Antigravity CLI harness adapter.""" +"""Antigravity (`agy`) CLI harness adapter.""" from __future__ import annotations from pathlib import Path +import subprocess -from harnesses.base import BaseHarnessAdapter, isolated_home +from harnesses.base import BaseHarnessAdapter, ModelListing, isolated_home + + +def parse_models_table(text: str) -> tuple[str, ...]: + models: set[str] = set() + for line in text.splitlines(): + stripped = line.strip() + if not stripped or stripped.lower().startswith( + ("error", "usage", "failed", "warning", "flags") + ): + continue + token = stripped.split()[0].strip() + if token: + models.add(token) + return tuple(sorted(models)) class AntigravityAdapter(BaseHarnessAdapter): name = "antigravity" default_executable = "agy" + def list_models(self, executable: str) -> ModelListing: + try: + completed = subprocess.run( + [executable, "models"], + text=True, + capture_output=True, + timeout=15, + check=False, + ) + except (OSError, subprocess.TimeoutExpired): + return ModelListing() + models = parse_models_table(completed.stdout) + if not models: + return ModelListing() + return ModelListing(models=models, source="cli") + def prepare_environment(self, output_dir: Path) -> tuple[dict[str, str], Path | None]: return isolated_home(output_dir, "agy-home", "GEMINI_HOME") @@ -25,4 +56,4 @@ def build_command( timeout_seconds: int, skill_name: str = "", ) -> list[str]: - return [executable, "--headless", "-p", prompt, "--model", model] + return [executable, "--model", model, "--print", prompt] diff --git a/skills/skill-eval-loop/scripts/harnesses/base.py b/skills/skill-eval-loop/scripts/harnesses/base.py index cfff502..ce7418b 100644 --- a/skills/skill-eval-loop/scripts/harnesses/base.py +++ b/skills/skill-eval-loop/scripts/harnesses/base.py @@ -9,6 +9,12 @@ from typing import Any +@dataclass(frozen=True) +class ModelListing: + models: tuple[str, ...] = () + source: str = "unavailable" + + INFRASTRUCTURE_FAILURE_MARKERS = ( "failed to lookup address information", "error sending request", @@ -80,6 +86,9 @@ def resolve(self, executable: str | None) -> tuple[str, str]: def prepare_environment(self, output_dir: Path) -> tuple[dict[str, str], Path | None]: return noop_env() + def invocation_env(self, invocation_dir: Path) -> dict[str, str]: + return {} + def cleanup_environment(self, home_dir: Path | None) -> None: if home_dir is not None and home_dir.exists(): shutil.rmtree(home_dir, ignore_errors=True) @@ -108,3 +117,18 @@ def parse_trace( def is_infrastructure_failure(self, message: str) -> bool: return is_infrastructure_failure(message) + + def list_models(self, executable: str) -> ModelListing: + return ModelListing() + + +def reject_unknown_model(listing: ModelListing, model: str, label: str, harness: str) -> None: + if not listing.models: + return + if model in listing.models: + return + available = ", ".join(listing.models) + raise ValueError( + f"{label} {model!r} is not available on harness {harness!r}; " + f"available models ({listing.source}): {available}" + ) diff --git a/skills/skill-eval-loop/scripts/harnesses/claude.py b/skills/skill-eval-loop/scripts/harnesses/claude.py index 0872793..15bf39c 100644 --- a/skills/skill-eval-loop/scripts/harnesses/claude.py +++ b/skills/skill-eval-loop/scripts/harnesses/claude.py @@ -48,6 +48,12 @@ def parse_trace( try: data = json.loads(text) if isinstance(data, dict): + if data.get("is_error"): + result.failure_message = str( + data.get("result") or data.get("error") or "harness reported an error" + ).strip() + result.response = "" + return result.as_dict() result.response = str( data.get("result", data.get("text", data.get("response", text))) ).strip() diff --git a/skills/skill-eval-loop/scripts/harnesses/codex.py b/skills/skill-eval-loop/scripts/harnesses/codex.py index cb5c150..b881b18 100644 --- a/skills/skill-eval-loop/scripts/harnesses/codex.py +++ b/skills/skill-eval-loop/scripts/harnesses/codex.py @@ -88,6 +88,9 @@ def prepare_environment(self, output_dir: Path) -> tuple[dict[str, str], Path | home = prepare_run_codex_home(output_dir) return {"CODEX_HOME": str(home)}, home + def invocation_env(self, invocation_dir: Path) -> dict[str, str]: + return {"HOME": str(invocation_dir / "home")} + def build_command( self, *, diff --git a/skills/skill-eval-loop/scripts/harnesses/cursor_agent.py b/skills/skill-eval-loop/scripts/harnesses/cursor_agent.py index fc1807f..ea99f60 100644 --- a/skills/skill-eval-loop/scripts/harnesses/cursor_agent.py +++ b/skills/skill-eval-loop/scripts/harnesses/cursor_agent.py @@ -5,15 +5,68 @@ import json from pathlib import Path import shutil +import subprocess from typing import Any -from harnesses.base import BaseHarnessAdapter, TraceResult, isolated_home +from harnesses.base import BaseHarnessAdapter, ModelListing, TraceResult, isolated_home + + +def looks_like_model_id(token: str) -> bool: + if token == "auto": + return True + if not token or not all(char.isalnum() or char in "._-[]=" for char in token): + return False + return any(char in "-._" for char in token) or any(char.isdigit() for char in token) + + +def parse_cli_model_ids(text: str) -> tuple[str, ...]: + cleaned = text.strip() + if not cleaned: + return () + try: + parsed = json.loads(cleaned) + except json.JSONDecodeError: + parsed = None + models: set[str] = set() + if isinstance(parsed, list): + for item in parsed: + if isinstance(item, str) and looks_like_model_id(item.strip()): + models.add(item.strip()) + elif isinstance(item, dict): + model_id = item.get("id") or item.get("model") or item.get("model_id") + if isinstance(model_id, str) and looks_like_model_id(model_id.strip()): + models.add(model_id.strip()) + return tuple(sorted(models)) + for line in cleaned.splitlines(): + line = line.strip() + if not line or line.lower().startswith(("failed", "error", "usage", "warning", "available", "tip")): + continue + token = line.split()[0].strip(",:") + if looks_like_model_id(token): + models.add(token) + return tuple(sorted(models)) class CursorAgentAdapter(BaseHarnessAdapter): name = "cursor-agent" default_executable = "cursor-agent" + def list_models(self, executable: str) -> ModelListing: + try: + completed = subprocess.run( + [executable, "--list-models"], + text=True, + capture_output=True, + timeout=15, + check=False, + ) + except (OSError, subprocess.TimeoutExpired): + return ModelListing() + models = parse_cli_model_ids(completed.stdout) + if not models: + return ModelListing() + return ModelListing(models=models, source="cli") + def prepare_environment(self, output_dir: Path) -> tuple[dict[str, str], Path | None]: env, home = isolated_home(output_dir, "cursor-home", "CURSOR_CONFIG_DIR") try: @@ -65,6 +118,12 @@ def parse_trace( try: data = json.loads(text) if isinstance(data, dict): + if data.get("is_error"): + result.failure_message = str( + data.get("result") or data.get("error") or "harness reported an error" + ).strip() + result.response = "" + return result.as_dict() result.response = str( data.get("result", data.get("text", data.get("output", text))) ).strip() diff --git a/skills/skill-eval-loop/scripts/harnesses/hermes.py b/skills/skill-eval-loop/scripts/harnesses/hermes.py index eb83b9a..ec6a9c5 100644 --- a/skills/skill-eval-loop/scripts/harnesses/hermes.py +++ b/skills/skill-eval-loop/scripts/harnesses/hermes.py @@ -3,8 +3,10 @@ from __future__ import annotations from pathlib import Path +import shutil +from typing import Any -from harnesses.base import BaseHarnessAdapter, isolated_home +from harnesses.base import BaseHarnessAdapter, TraceResult, isolated_home class HermesAdapter(BaseHarnessAdapter): @@ -12,7 +14,18 @@ class HermesAdapter(BaseHarnessAdapter): default_executable = "hermes" def prepare_environment(self, output_dir: Path) -> tuple[dict[str, str], Path | None]: - return isolated_home(output_dir, "hermes-home", "HERMES_HOME") + env, home = isolated_home(output_dir, "hermes-home", "HERMES_HOME") + try: + host = Path.home() / ".hermes" + for name in ("config.yaml", ".env", "auth.json"): + source = host / name + if source.is_file(): + target = home / name + shutil.copyfile(source, target) + target.chmod(0o600) + except OSError: + pass + return env, home def build_command( self, @@ -25,4 +38,19 @@ def build_command( timeout_seconds: int, skill_name: str = "", ) -> list[str]: - return [executable, "chat", "-q", prompt, "--model", model] + return [executable, "chat", "-Q", "-q", prompt, "--model", model] + + def parse_trace( + self, + trace_path: Path, + stderr_path: Path, + skill_name: str = "", + ) -> dict[str, Any]: + text = trace_path.read_text(encoding="utf-8") if trace_path.exists() else "" + kept: list[str] = [] + for line in text.splitlines(): + stripped = line.strip() + if not stripped or stripped.startswith("⚠") or stripped.lower().startswith("session_id:"): + continue + kept.append(stripped) + return TraceResult(response="\n".join(kept).strip()).as_dict() diff --git a/skills/skill-eval-loop/scripts/harnesses/muse.py b/skills/skill-eval-loop/scripts/harnesses/muse.py index f7769d2..6d009db 100644 --- a/skills/skill-eval-loop/scripts/harnesses/muse.py +++ b/skills/skill-eval-loop/scripts/harnesses/muse.py @@ -6,13 +6,44 @@ from pathlib import Path from typing import Any -from harnesses.base import BaseHarnessAdapter, TraceResult +from harnesses.base import BaseHarnessAdapter, ModelListing, TraceResult + + +def muse_catalog_dir() -> Path: + return Path.home() / ".local" / "share" / "muse" / "model-catalog" + + +def catalog_model_ids(catalog_dir: Path) -> tuple[str, ...]: + if not catalog_dir.is_dir(): + return () + models: set[str] = set() + for path in sorted(catalog_dir.glob("*.json")): + try: + payload = json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + continue + rows = payload.get("rows") if isinstance(payload, dict) else None + if not isinstance(rows, list): + continue + for row in rows: + if not isinstance(row, dict): + continue + model_id = row.get("model_id") + if isinstance(model_id, str) and model_id.strip(): + models.add(model_id.strip()) + return tuple(sorted(models)) class MuseAdapter(BaseHarnessAdapter): name = "muse" default_executable = "muse" + def list_models(self, executable: str) -> ModelListing: + models = catalog_model_ids(muse_catalog_dir()) + if not models: + return ModelListing() + return ModelListing(models=models, source="local_catalog") + def build_command( self, *, diff --git a/skills/skill-eval-loop/scripts/harnesses/pi.py b/skills/skill-eval-loop/scripts/harnesses/pi.py index d770b95..4dbb0e0 100644 --- a/skills/skill-eval-loop/scripts/harnesses/pi.py +++ b/skills/skill-eval-loop/scripts/harnesses/pi.py @@ -3,14 +3,46 @@ from __future__ import annotations from pathlib import Path +import subprocess -from harnesses.base import BaseHarnessAdapter +from harnesses.base import BaseHarnessAdapter, ModelListing + + +def parse_list_models_table(text: str) -> tuple[str, ...]: + models: set[str] = set() + for line in text.splitlines(): + stripped = line.strip() + if not stripped or stripped.lower().startswith("provider"): + continue + parts = stripped.split() + if len(parts) < 2: + continue + provider, model_id = parts[0], parts[1] + if provider and model_id: + models.add(f"{provider}/{model_id}") + return tuple(sorted(models)) class PiAdapter(BaseHarnessAdapter): name = "pi" default_executable = "pi" + def list_models(self, executable: str) -> ModelListing: + try: + completed = subprocess.run( + [executable, "--list-models"], + text=True, + capture_output=True, + timeout=15, + check=False, + ) + except (OSError, subprocess.TimeoutExpired): + return ModelListing() + models = parse_list_models_table(completed.stdout) + if not models: + return ModelListing() + return ModelListing(models=models, source="cli") + def build_command( self, *, @@ -22,4 +54,4 @@ def build_command( timeout_seconds: int, skill_name: str = "", ) -> list[str]: - return [executable, "-p", prompt, "--model", model] + return [executable, "--print", "--model", model, "--", prompt] diff --git a/skills/skill-eval-loop/scripts/skill_eval_loop.py b/skills/skill-eval-loop/scripts/skill_eval_loop.py index 034f837..67a1620 100644 --- a/skills/skill-eval-loop/scripts/skill_eval_loop.py +++ b/skills/skill-eval-loop/scripts/skill_eval_loop.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -"""Run a paired Codex skill evaluation with retained deterministic evidence.""" +"""Run a paired skill evaluation with retained deterministic evidence.""" from __future__ import annotations @@ -58,15 +58,39 @@ from harnesses import ( # noqa: E402 SUPPORTED_HARNESSES, get_harness_adapter, - prepare_run_codex_home, resolve_harness, ) -from harnesses.codex import parse_trace as parse_trace # noqa: E402 +from harnesses.base import reject_unknown_model # noqa: E402 MAX_TASK_BYTES = 4 * 1024 * 1024 +def require_harness_models( + *, + harness: str, + executable: str, + model: str, + judge_harness: str, + judge_executable: str, + judge_model: str, +) -> None: + reject_unknown_model( + get_harness_adapter(harness).list_models(executable), + model, + "model", + harness, + ) + if not judge_model: + return + reject_unknown_model( + get_harness_adapter(judge_harness).list_models(judge_executable), + judge_model, + "judge-model", + judge_harness, + ) + + def error(message: str) -> None: print(f"ERROR: {message}", file=sys.stderr) @@ -387,7 +411,7 @@ def reject_tasks_inside_skill(skill: Path, tasks_path: Path, promotion: bool) -> def build_plan(arguments: argparse.Namespace) -> dict[str, Any]: - target_harness = getattr(arguments, "harness", "codex") + target_harness = arguments.harness if target_harness not in SUPPORTED_HARNESSES: supported = ", ".join(sorted(SUPPORTED_HARNESSES)) raise ValueError(f"unsupported harness {target_harness!r}; supported harnesses are: {supported}") @@ -423,6 +447,14 @@ def build_plan(arguments: argparse.Namespace) -> dict[str, Any]: judge_executable, judge_version = resolve_harness( judge_harness, getattr(arguments, "judge_harness_bin", None) or arguments.harness_bin ) + require_harness_models( + harness=target_harness, + executable=executable, + model=arguments.model, + judge_harness=judge_harness, + judge_executable=judge_executable, + judge_model=arguments.judge_model, + ) paired_trials = len(tasks) * arguments.trials target_invocations = paired_trials * 2 judge_invocations = rubrics * arguments.trials * 3 @@ -479,11 +511,6 @@ def copy_skill_payload(source: Path, destination: Path) -> None: target.chmod(path.stat().st_mode & 0o777) -def discard_runtime_home(home: Path) -> None: - if home.exists(): - shutil.rmtree(home, ignore_errors=True) - - def workspace_target(workspace: Path, relative: str) -> Path: root = workspace.resolve() target = (root / relative).resolve() @@ -569,28 +596,15 @@ def grade(task: dict[str, Any], workspace: Path, response: str) -> dict[str, Any class HarnessRuntime: """Own the shared harness process, workspace, and evidence lifecycle.""" - def __init__(self, codex_directory: Path, configuration: dict[str, Any]) -> None: - self.codex_directory = codex_directory + def __init__(self, output_dir: Path, configuration: dict[str, Any]) -> None: + self.output_dir = output_dir self.configuration = dict(configuration) - self.target_harness = self.configuration.get("harness") or "codex" - self.judge_harness = ( - self.configuration.get("judge_harness") - or self.configuration.get("harness") - or "codex" - ) + self.target_harness = self.configuration["harness"] + self.judge_harness = self.configuration.get("judge_harness") or self.target_harness self.target_adapter = get_harness_adapter(self.target_harness) self.judge_adapter = get_harness_adapter(self.judge_harness) - base_dir = ( - codex_directory.parent - if codex_directory.name.endswith("-home") - else codex_directory - ) - self.target_env, self.target_home = self.target_adapter.prepare_environment(base_dir) - self.judge_env, self.judge_home = self.judge_adapter.prepare_environment(base_dir) - if self.target_harness == "codex": - self.target_env["CODEX_HOME"] = str(codex_directory) - if self.judge_harness == "codex": - self.judge_env["CODEX_HOME"] = str(codex_directory) + self.target_env, self.target_home = self.target_adapter.prepare_environment(output_dir) + self.judge_env, self.judge_home = self.judge_adapter.prepare_environment(output_dir) def cleanup(self) -> None: self.target_adapter.cleanup_environment(self.target_home) @@ -628,7 +642,6 @@ def _invoke( response_name = "response.md" if target_role else "response.txt" response_path = invocation_dir / response_name environment = os.environ.copy() - environment.pop("OPENAI_API_KEY", None) path_parts = environment.get("PATH", "").split(os.pathsep) clean_parts = [p for p in path_parts if ".pyenv/shims" not in p] if "/opt/homebrew/bin" not in clean_parts: @@ -636,12 +649,7 @@ def _invoke( if "/usr/bin" not in clean_parts: clean_parts.append("/usr/bin") environment["PATH"] = os.pathsep.join(clean_parts) - if adapter.name == "codex": - environment.update( - { - "HOME": str(invocation_dir / "home"), - } - ) + environment.update(adapter.invocation_env(invocation_dir)) adapter_env = self.target_env if target_role else self.judge_env environment.update(adapter_env) if target_role: @@ -668,6 +676,7 @@ def _invoke( arguments, cwd=workspace, env=environment, + stdin=subprocess.DEVNULL, stdout=trace, stderr=stderr, timeout=self.configuration["timeout_seconds"], @@ -686,7 +695,12 @@ def _invoke( response_path.write_text(observed["response"], encoding="utf-8") reported_model = observed["actual_model"] model_matches = reported_model == model if reported_model else None - status = "timed_out" if timed_out else ("completed" if exit_code == 0 else "failed") + if timed_out: + status = "timed_out" + elif exit_code == 0 and not (observed.get("failure_message") and not observed.get("response")): + status = "completed" + else: + status = "failed" failure_reason = ( "infrastructure_failed" if exit_code != 0 and adapter.is_infrastructure_failure(observed.get("failure_message", "")) @@ -841,9 +855,6 @@ def invoke_judge( return result, invocation["response"] -CodexRuntime = HarnessRuntime - - def deterministic_comparison(control: str, treatment: str) -> str: if "not_scored" in {control, treatment}: return "not_scored" @@ -1153,10 +1164,8 @@ def run_live(plan: dict[str, Any]) -> dict[str, Any]: raise ValueError(f"output directory already exists: {output}") skill_name = skill.name output.mkdir(parents=True) - codex_directory = output / "codex-home" + runtime = HarnessRuntime(output, configuration) try: - codex_directory = prepare_run_codex_home(output) - runtime = CodexRuntime(codex_directory, configuration) write_json( output / "config.json", {"mode": "live", "configuration": configuration, "counts": plan["counts"]}, @@ -1273,11 +1282,11 @@ def run_live(plan: dict[str, Any]) -> dict[str, Any]: write_json(output / "run.json", result) return result finally: - discard_runtime_home(codex_directory) + runtime.cleanup() def build_calibration_plan(arguments: argparse.Namespace) -> dict[str, Any]: - target_harness = getattr(arguments, "harness", "codex") + target_harness = arguments.harness if target_harness not in SUPPORTED_HARNESSES: supported = ", ".join(sorted(SUPPORTED_HARNESSES)) raise ValueError(f"unsupported harness {target_harness!r}; supported harnesses are: {supported}") @@ -1293,6 +1302,14 @@ def build_calibration_plan(arguments: argparse.Namespace) -> dict[str, Any]: judge_executable, judge_version = resolve_harness( judge_harness, getattr(arguments, "judge_harness_bin", None) or arguments.harness_bin ) + require_harness_models( + harness=target_harness, + executable=executable, + model=arguments.model, + judge_harness=judge_harness, + judge_executable=judge_executable, + judge_model=arguments.judge_model, + ) return { "valid": True, "mode": "dry_run", @@ -1382,10 +1399,8 @@ def run_calibrate(plan: dict[str, Any]) -> dict[str, Any]: if output.exists(): raise ValueError(f"output directory already exists: {output}") output.mkdir(parents=True) - codex_directory = output / "codex-home" + runtime = HarnessRuntime(output, configuration) try: - codex_directory = prepare_run_codex_home(output) - runtime = HarnessRuntime(codex_directory, configuration) write_json( output / "config.json", {"mode": "calibrate", "configuration": configuration, "counts": plan["counts"]}, @@ -1434,7 +1449,7 @@ def run_calibrate(plan: dict[str, Any]) -> dict[str, Any]: write_json(output / "calibration.json", result) return result finally: - discard_runtime_home(codex_directory) + runtime.cleanup() def calibration_exit_code(result: dict[str, Any]) -> int: @@ -1466,6 +1481,7 @@ def healthcheck(arguments: argparse.Namespace) -> int: "skill_dir": str(root), "commands": [ "healthcheck", + "models", "run", "calibrate", "prepare-review", @@ -1477,6 +1493,27 @@ def healthcheck(arguments: argparse.Namespace) -> int: return 0 if not missing else 1 +def list_harness_models(arguments: argparse.Namespace) -> int: + harness = arguments.harness + if harness not in SUPPORTED_HARNESSES: + supported = ", ".join(sorted(SUPPORTED_HARNESSES)) + raise ValueError(f"unsupported harness {harness!r}; supported harnesses are: {supported}") + executable, version = resolve_harness(harness, arguments.harness_bin) + listing = get_harness_adapter(harness).list_models(executable) + print_json( + { + "valid": True, + "harness": harness, + "executable": executable, + "harness_version": version, + "enumerable": bool(listing.models), + "source": listing.source, + "models": list(listing.models), + } + ) + return 0 + + def run(arguments: argparse.Namespace) -> int: plan = build_plan(arguments) if arguments.dry_run: @@ -1968,6 +2005,12 @@ def parser() -> argparse.ArgumentParser: health = commands.add_parser("healthcheck", help="validate the installed skill") health.add_argument("--skill-dir") health.set_defaults(handler=healthcheck) + models_parser = commands.add_parser( + "models", help="list model ids a harness can enumerate locally" + ) + models_parser.add_argument("--harness", required=True) + models_parser.add_argument("--harness-bin") + models_parser.set_defaults(handler=list_harness_models) run_parser = commands.add_parser("run", help="plan or run a paired evaluation") run_parser.add_argument("--skill", required=True) run_parser.add_argument("--tasks") diff --git a/tests/fixtures/simple-fake-agy b/tests/fixtures/simple-fake-agy new file mode 100755 index 0000000..45ffcd9 --- /dev/null +++ b/tests/fixtures/simple-fake-agy @@ -0,0 +1,17 @@ +#!/bin/sh +set -eu + +if [ "${1:-}" = "--version" ]; then + printf 'simple-fake-agy 1.0\n' + exit 0 +fi + +if [ "${1:-}" = "models" ]; then + printf '%s\n' \ + "gemini-3.7-flash-high Gemini 3.7 Flash (High)" \ + "claude-sonnet-4-6 Claude Sonnet 4.6 (Thinking)" + exit 0 +fi + +printf 'unexpected invocation\n' >&2 +exit 9 diff --git a/tests/fixtures/simple-fake-cursor-agent b/tests/fixtures/simple-fake-cursor-agent new file mode 100755 index 0000000..31dad91 --- /dev/null +++ b/tests/fixtures/simple-fake-cursor-agent @@ -0,0 +1,21 @@ +#!/bin/sh +set -eu + +if [ "${1:-}" = "--version" ]; then + printf 'simple-fake-cursor-agent 1.0\n' + exit 0 +fi + +if [ "${1:-}" = "--list-models" ]; then + printf '%s\n' \ + "Available models" \ + "" \ + "claude-sonnet-5-medium - Claude Sonnet 5 Medium" \ + "gpt-5.6-sol-medium - GPT-5.6 Sol Medium" \ + "" \ + "Tip: pass --model " + exit 0 +fi + +printf 'unexpected invocation\n' >&2 +exit 9 diff --git a/tests/fixtures/simple-fake-muse b/tests/fixtures/simple-fake-muse new file mode 100755 index 0000000..021c5c3 --- /dev/null +++ b/tests/fixtures/simple-fake-muse @@ -0,0 +1,10 @@ +#!/bin/sh +set -eu + +if [ "${1:-}" = "--version" ]; then + printf 'simple-fake-muse 1.0\n' + exit 0 +fi + +printf 'unexpected invocation\n' >&2 +exit 9 diff --git a/tests/fixtures/simple-fake-pi b/tests/fixtures/simple-fake-pi new file mode 100755 index 0000000..a7d660d --- /dev/null +++ b/tests/fixtures/simple-fake-pi @@ -0,0 +1,18 @@ +#!/bin/sh +set -eu + +if [ "${1:-}" = "--version" ]; then + printf 'simple-fake-pi 1.0\n' + exit 0 +fi + +if [ "${1:-}" = "--list-models" ]; then + printf '%s\n' \ + "provider model" \ + "openai-codex gpt-5.6-sol" \ + "google gemini-2.5-flash" + exit 0 +fi + +printf 'unexpected invocation\n' >&2 +exit 9 diff --git a/tests/helpers.py b/tests/helpers.py index d6bfee0..5c92868 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -10,6 +10,10 @@ EVALUATOR = ROOT / "skills" / "skill-eval-loop" / "scripts" / "skill_eval_loop.py" LAUNCHER = ROOT / "skills" / "skill-eval-loop" / "scripts" / "skill-eval-loop" FAKE_CODEX = ROOT / "tests" / "fixtures" / "simple-fake-codex" +FAKE_MUSE = ROOT / "tests" / "fixtures" / "simple-fake-muse" +FAKE_CURSOR_AGENT = ROOT / "tests" / "fixtures" / "simple-fake-cursor-agent" +FAKE_PI = ROOT / "tests" / "fixtures" / "simple-fake-pi" +FAKE_AGY = ROOT / "tests" / "fixtures" / "simple-fake-agy" CALIBRATION_FIXTURES = ROOT / "tests" / "fixtures" / "calibration" / "v1.json" diff --git a/tests/test_harnesses.py b/tests/test_harnesses.py index 9d75407..5a52578 100644 --- a/tests/test_harnesses.py +++ b/tests/test_harnesses.py @@ -10,7 +10,11 @@ from helpers import ( EVALUATOR, + FAKE_AGY, FAKE_CODEX, + FAKE_CURSOR_AGENT, + FAKE_MUSE, + FAKE_PI, LAUNCHER, ROOT, EvaluatorTestCase, @@ -24,7 +28,7 @@ def test_healthcheck_reports_python_commands(self) -> None: self.assertEqual(result.returncode, 0, result.stderr) self.assertEqual( json.loads(result.stdout)["commands"], - ["healthcheck", "run", "calibrate", "prepare-review", "finalize-review"], + ["healthcheck", "models", "run", "calibrate", "prepare-review", "finalize-review"], ) @@ -287,7 +291,7 @@ def test_live_run_marks_model_mismatch_invalid_and_preserves_evidence(self) -> N self.assertTrue((output / "task-choice" / "trial-001" / "control" / "trace.jsonl").is_file()) - def test_all_codex_roles_use_cleaned_workspaces_outside_retained_output(self) -> None: + def test_all_harness_roles_use_cleaned_workspaces_outside_retained_output(self) -> None: with tempfile.TemporaryDirectory() as temporary: root = Path(temporary) cwd_log = root / "role-cwds.txt" @@ -309,7 +313,7 @@ def test_all_codex_roles_use_cleaned_workspaces_outside_retained_output(self) -> self.assertTrue(all(not workspace.exists() for workspace in workspaces)) - def test_codex_runtime_is_the_shared_target_and_judge_test_surface(self) -> None: + def test_harness_runtime_is_the_shared_target_and_judge_test_surface(self) -> None: spec = importlib.util.spec_from_file_location("skill_eval_loop_runtime", EVALUATOR) self.assertIsNotNone(spec) self.assertIsNotNone(spec.loader) @@ -320,12 +324,13 @@ def test_codex_runtime_is_the_shared_target_and_judge_test_surface(self) -> None root = Path(temporary) skill = self.make_skill(root) pair_dir = root / "retained" / "task-choice" / "trial-001" - codex_home = root / "codex-home" - codex_home.mkdir() + output = root / "run-output" + output.mkdir() cwd_log = root / "runtime-cwds.txt" - runtime = evaluator.CodexRuntime( - codex_home, + runtime = evaluator.HarnessRuntime( + output, { + "harness": "codex", "harness_executable": str(FAKE_CODEX), "model": "runner-model", "judge_model": "judge-model", @@ -404,7 +409,9 @@ def test_trace_records_successful_target_skill_read_as_activation(self) -> None: encoding="utf-8", ) - observed = evaluator.parse_trace(trace, skill_name="target-skill") + observed = evaluator.get_harness_adapter("codex").parse_trace( + trace, trace, skill_name="target-skill" + ) self.assertTrue(observed["skill_accessed"]) @@ -440,7 +447,9 @@ def test_trace_records_skill_read_when_later_compound_command_fails(self) -> Non encoding="utf-8", ) - observed = evaluator.parse_trace(trace, skill_name="target-skill") + observed = evaluator.get_harness_adapter("codex").parse_trace( + trace, trace, skill_name="target-skill" + ) self.assertTrue(observed["skill_accessed"]) @@ -469,10 +478,77 @@ def test_trace_does_not_treat_skill_directory_listing_as_activation(self) -> Non encoding="utf-8", ) - observed = evaluator.parse_trace(trace, skill_name="target-skill") + observed = evaluator.get_harness_adapter("codex").parse_trace( + trace, trace, skill_name="target-skill" + ) self.assertFalse(observed["skill_accessed"]) + def test_claude_json_error_is_a_failure_not_a_response(self) -> None: + spec = importlib.util.spec_from_file_location("evaluator_mod", EVALUATOR) + self.assertIsNotNone(spec) + self.assertIsNotNone(spec.loader) + evaluator = importlib.util.module_from_spec(spec) + spec.loader.exec_module(evaluator) + + with tempfile.TemporaryDirectory() as temporary: + trace = Path(temporary) / "trace.jsonl" + trace.write_text( + json.dumps( + { + "is_error": True, + "result": "Not logged in · Please run /login", + "type": "result", + } + ), + encoding="utf-8", + ) + observed = evaluator.get_harness_adapter("claude").parse_trace( + trace, trace, skill_name="target-skill" + ) + self.assertEqual(observed["response"], "") + self.assertIn("Not logged in", observed["failure_message"]) + + def test_hermes_copies_host_config_into_the_run_home(self) -> None: + spec = importlib.util.spec_from_file_location("evaluator_mod", EVALUATOR) + self.assertIsNotNone(spec) + self.assertIsNotNone(spec.loader) + evaluator = importlib.util.module_from_spec(spec) + spec.loader.exec_module(evaluator) + + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + host = root / "user-home" / ".hermes" + host.mkdir(parents=True) + (host / "config.yaml").write_text("model: auto\n", encoding="utf-8") + (host / ".env").write_text("FREELLM_API_KEY=test-key\n", encoding="utf-8") + output = root / "run-output" + with patch.dict(os.environ, {"HOME": str(root / "user-home")}): + env, home = evaluator.get_harness_adapter("hermes").prepare_environment(output) + self.assertEqual(env["HERMES_HOME"], str(home)) + self.assertEqual((home / "config.yaml").read_text(encoding="utf-8"), "model: auto\n") + self.assertEqual((home / ".env").read_text(encoding="utf-8"), "FREELLM_API_KEY=test-key\n") + + def test_hermes_parse_drops_scanner_warnings(self) -> None: + spec = importlib.util.spec_from_file_location("evaluator_mod", EVALUATOR) + self.assertIsNotNone(spec) + self.assertIsNotNone(spec.loader) + evaluator = importlib.util.module_from_spec(spec) + spec.loader.exec_module(evaluator) + + with tempfile.TemporaryDirectory() as temporary: + trace = Path(temporary) / "trace.jsonl" + trace.write_text( + "⚠ tirith security scanner enabled but not available\n" + "session_id: 20260901_110244_f07155\n" + "Blue\n", + encoding="utf-8", + ) + observed = evaluator.get_harness_adapter("hermes").parse_trace( + trace, trace, skill_name="target-skill" + ) + self.assertEqual(observed["response"], "Blue") + def test_unsupported_harness_rejected_with_supported_list(self) -> None: with tempfile.TemporaryDirectory() as temporary: @@ -505,12 +581,102 @@ def test_all_supported_harness_adapters_build_commands(self) -> None: self.assertIsNotNone(spec.loader) evaluator = importlib.util.module_from_spec(spec) spec.loader.exec_module(evaluator) - - expected_harnesses = ["antigravity", "claude", "codex", "cursor-agent", "hermes", "muse", "pi", "script"] + + expected_harnesses = [ + "antigravity", + "claude", + "codex", + "cursor-agent", + "hermes", + "muse", + "pi", + "script", + ] self.assertEqual(sorted(evaluator.SUPPORTED_HARNESSES), expected_harnesses) - + with tempfile.TemporaryDirectory() as temporary: ws = Path(temporary) + expected_commands = { + "antigravity": [ + "/bin/antigravity", + "--model", + "test-model", + "--print", + "Hello world", + ], + "claude": [ + "/bin/claude", + "--print", + "--output-format", + "json", + "--model", + "test-model", + "Hello world", + ], + "codex": [ + "/bin/codex", + "exec", + "--json", + "--ephemeral", + "--skip-git-repo-check", + "--ignore-user-config", + "--ignore-rules", + "--sandbox", + "read-only", + "--model", + "test-model", + "Hello world", + ], + "cursor-agent": [ + "/bin/cursor-agent", + "--print", + "--force", + "--trust", + "--output-format", + "json", + "--model", + "test-model", + "Hello world", + ], + "hermes": [ + "/bin/hermes", + "chat", + "-Q", + "-q", + "Hello world", + "--model", + "test-model", + ], + "muse": [ + "/bin/muse", + "exec", + "--json", + "--workspace", + str(ws), + "--trust-workspace", + "--disable-approval", + "--model", + "test-model", + "Hello world", + ], + "pi": [ + "/bin/pi", + "--print", + "--model", + "test-model", + "--", + "Hello world", + ], + "script": [ + "/bin/script", + "--model", + "test-model", + "--role", + "treatment", + "--prompt", + "Hello world", + ], + } for harness_name in expected_harnesses: adapter = evaluator.get_harness_adapter(harness_name) cmd = adapter.build_command( @@ -522,9 +688,7 @@ def test_all_supported_harness_adapters_build_commands(self) -> None: timeout_seconds=30, skill_name="test-skill", ) - self.assertIsInstance(cmd, list) - self.assertTrue(len(cmd) >= 2) - self.assertIn("test-model", cmd) + self.assertEqual(cmd, expected_commands[harness_name]) def test_script_harness_live_execution(self) -> None: @@ -574,4 +738,149 @@ def test_script_harness_live_execution(self) -> None: treatment_response = (output / "task-t1" / "trial-001" / "treatment" / "response.md").read_text(encoding="utf-8") self.assertEqual(treatment_response, "Hello from custom script runner!") self.assertTrue(pair_report["activation"]["trace_skill_read"]) + self.assertFalse((output / "codex-home").exists()) + + def _write_muse_catalog(self, root: Path) -> None: + catalog = root / "user-home" / ".local" / "share" / "muse" / "model-catalog" + catalog.mkdir(parents=True) + (catalog / "meta.json").write_text( + json.dumps( + { + "schema_version": 1, + "rows": [ + {"model_id": "muse-spark-1.2"}, + {"model_id": "muse-spark-1.2-contributor"}, + ], + } + ) + + "\n", + encoding="utf-8", + ) + + def test_models_lists_muse_catalog_from_the_isolated_home(self) -> None: + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + self._write_muse_catalog(root) + result = subprocess.run( + [ + "python3", + str(EVALUATOR), + "models", + "--harness", + "muse", + "--harness-bin", + str(FAKE_MUSE), + ], + cwd=ROOT, + text=True, + capture_output=True, + check=False, + env=self.isolated_env(root), + ) + self.assertEqual(result.returncode, 0, result.stderr) + payload = json.loads(result.stdout) + self.assertTrue(payload["enumerable"]) + self.assertEqual(payload["source"], "local_catalog") + self.assertEqual( + payload["models"], + ["muse-spark-1.2", "muse-spark-1.2-contributor"], + ) + + def test_dry_run_rejects_a_muse_model_missing_from_the_catalog(self) -> None: + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + self._write_muse_catalog(root) + skill = self.make_skill(root) + tasks = root / "tasks.jsonl" + tasks.write_text( + '{"id":"choice","prompt":"Choose Blue.","graders":[{"type":"regex","pattern":"Blue"}]}\n', + encoding="utf-8", + ) + result = subprocess.run( + [ + "python3", + str(EVALUATOR), + "run", + "--skill", + str(skill), + "--tasks", + str(tasks), + "--output", + str(root / "run"), + "--harness", + "muse", + "--harness-bin", + str(FAKE_MUSE), + "--model", + "gpt-5.6-sol-medium", + "--dry-run", + ], + cwd=ROOT, + text=True, + capture_output=True, + check=False, + env=self.isolated_env(root), + ) + self.assertEqual(result.returncode, 1, result.stderr) + self.assertIn("gpt-5.6-sol-medium", result.stderr) + self.assertIn("muse-spark-1.2", result.stderr) + + def test_models_lists_cursor_agent_cli_ids(self) -> None: + result = self.run_cli( + "models", + "--harness", + "cursor-agent", + "--harness-bin", + str(FAKE_CURSOR_AGENT), + ) + self.assertEqual(result.returncode, 0, result.stderr) + payload = json.loads(result.stdout) + self.assertEqual(payload["source"], "cli") + self.assertEqual(payload["models"], ["claude-sonnet-5-medium", "gpt-5.6-sol-medium"]) + + def test_models_lists_pi_cli_ids(self) -> None: + result = self.run_cli( + "models", + "--harness", + "pi", + "--harness-bin", + str(FAKE_PI), + ) + self.assertEqual(result.returncode, 0, result.stderr) + payload = json.loads(result.stdout) + self.assertEqual(payload["source"], "cli") + self.assertEqual( + payload["models"], + ["google/gemini-2.5-flash", "openai-codex/gpt-5.6-sol"], + ) + + def test_models_lists_antigravity_cli_ids(self) -> None: + result = self.run_cli( + "models", + "--harness", + "antigravity", + "--harness-bin", + str(FAKE_AGY), + ) + self.assertEqual(result.returncode, 0, result.stderr) + payload = json.loads(result.stdout) + self.assertEqual(payload["source"], "cli") + self.assertEqual( + payload["models"], + ["claude-sonnet-4-6", "gemini-3.7-flash-high"], + ) + + def test_models_empty_listing_when_the_cli_cannot_enumerate(self) -> None: + result = self.run_cli( + "models", + "--harness", + "claude", + "--harness-bin", + str(FAKE_CODEX), + ) + self.assertEqual(result.returncode, 0, result.stderr) + payload = json.loads(result.stdout) + self.assertFalse(payload["enumerable"]) + self.assertEqual(payload["source"], "unavailable") + self.assertEqual(payload["models"], []) From a267a6f2f62d1f340a9e0adfa9d1b5d35e0866db Mon Sep 17 00:00:00 2001 From: jon-devlapaz Date: Tue, 1 Sep 2026 12:34:50 -0500 Subject: [PATCH 2/4] feat: ease operator paths, harness discovery, and live progress Relative and home paths now resolve before recording, binaries are found in standard locations without --harness-bin, --force replaces a prior output directory, and invocations emit 15s heartbeats under a 300s default timeout. Co-authored-by: Cursor --- .gitignore | 1 + README.md | 21 ++- docs/minimum-eval-contract.md | 9 +- ...ux-ergonomics-and-harness-friction-spec.md | 177 ++++++++++++++++++ skills/skill-eval-loop/SKILL.md | 11 +- skills/skill-eval-loop/scripts/core/util.py | 23 ++- .../skill-eval-loop/scripts/harnesses/base.py | 45 ++++- .../scripts/harnesses/script.py | 11 +- .../scripts/skill_eval_loop.py | 107 ++++++----- tests/fixtures/simple-fake-codex | 7 + tests/test_calibration.py | 4 +- tests/test_harnesses.py | 127 ++++++++++++- tests/test_tasks.py | 51 ++++- 13 files changed, 510 insertions(+), 84 deletions(-) create mode 100644 docs/ux-ergonomics-and-harness-friction-spec.md diff --git a/.gitignore b/.gitignore index c7f2628..86b0e28 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ +.DS_Store .agents/ .pytest_cache/ .ruff_cache/ diff --git a/README.md b/README.md index 54eb896..599bafc 100644 --- a/README.md +++ b/README.md @@ -25,14 +25,18 @@ The public launcher requires Python 3 and no package installation: ```bash EVALUATOR="$PWD/.agents/skills/skill-eval-loop/scripts/skill-eval-loop" "$EVALUATOR" healthcheck -"$EVALUATOR" models --harness pi --harness-bin /absolute/path/to/pi +"$EVALUATOR" models --harness pi ``` Copy `--model` and `--judge-model` from that harness's listing. If the listing is non-empty, `run` and `calibrate` reject ids that are not on it. An empty -listing does not reject. For a quality-complete rubric run, use the same -`--harness` for student and judge: a different `--judge-harness` can mark a -run independent, but that calibration cannot bind. +listing does not reject. Omit `--harness-bin` when the CLI is on `PATH` or in a +standard location such as `~/.local/bin`. Relative, `~/`, and symlink paths are +resolved to canonical absolute paths before validation and recording. Re-use an +output directory with `--force`. The default timeout is 300 seconds. For a +quality-complete rubric run, use the same `--harness` for student and judge: a +different `--judge-harness` can mark a run independent, but that calibration +cannot bind. ## Run an evaluation @@ -47,14 +51,12 @@ Run a side-effect-free plan before a live invocation: ```bash "$EVALUATOR" run \ - --skill /absolute/path/to/target-skill \ - --tasks /absolute/path/to/tasks.jsonl \ + --skill ./skills/target-skill \ + --tasks ./tasks.jsonl \ --output "$PWD/.eval-output/fresh-run" \ --harness pi \ - --harness-bin /absolute/path/to/pi \ --model exact-model-id \ --trials 1 \ - --timeout-seconds 300 \ --dry-run ``` @@ -90,7 +92,7 @@ not a quality unknown. Omitting `--calibration` is allowed, but a rubric run then remains quality-incomplete and cannot exit `0`. The runner invokes the configured harness sequentially in read-only mode, -emitting invocation progress to stderr. Odd trials run control first; even +emitting invocation start, 15-second heartbeats, and finish lines to stderr. Odd trials run control first; even trials run treatment first. The evaluator injects the exact `SKILL.md` text itself, so treatment exposure does not depend on model-side discovery. Target, judge, and calibration invocations share one lifecycle that uses cleaned @@ -123,7 +125,6 @@ python3 skills/skill-eval-loop/scripts/skill_eval_loop.py calibrate \ --fixtures /absolute/path/to/calibration/v1.json \ --output "$PWD/.eval-output/fresh-calibration" \ --harness pi \ - --harness-bin /absolute/path/to/pi \ --model exact-model-id \ --judge-model exact-judge-model-id \ --dry-run diff --git a/docs/minimum-eval-contract.md b/docs/minimum-eval-contract.md index 0110860..a9bf355 100644 --- a/docs/minimum-eval-contract.md +++ b/docs/minimum-eval-contract.md @@ -22,15 +22,16 @@ Changing any component creates a different evaluation. A run requires: -- an absolute skill directory; -- either an absolute newline-delimited JSON task file or the target-owned +- a skill directory (CLI paths may be relative or `~/`; retained evidence stores + the canonical absolute path); +- either a newline-delimited JSON task file or the target-owned `evals/tasks.jsonl` file; - a supported harness and its resolved executable; - an exact target model identifier; - a different exact judge model identifier when a task uses a rubric; - a positive trial count; -- a positive timeout; -- an absolute output directory; +- a positive timeout (CLI default 300 seconds); +- an output directory (same path-resolution rule as the skill path); - an exact judge model when any task uses a rubric grader. The retained configuration also records the harness version, hashes of the diff --git a/docs/ux-ergonomics-and-harness-friction-spec.md b/docs/ux-ergonomics-and-harness-friction-spec.md new file mode 100644 index 0000000..fa959cc --- /dev/null +++ b/docs/ux-ergonomics-and-harness-friction-spec.md @@ -0,0 +1,177 @@ +# RFC: Evaluator UX Ergonomics & Multi-Harness Operational Polish + +**Status**: Implemented (2026-09-01) +**Date**: 2026-08-31 +**Author**: Antigravity Agent +**Context**: Post-Phase 2 dogfooding findings against live agent CLIs (`cursor-agent`, `muse`, `codex`) + +Implemented against the 2026-09-01 harness-agnostic runner. Deviations from the +original draft: + +- `--output` is allowed to not exist yet. The draft's `must_exist=True` for every + path would reject a fresh output directory. +- Timeout sends `SIGTERM` then `SIGKILL` to the harness process, not a process + group. Current adapters do not daemonize children that need group kill. +- Credential isolation is adapter-owned for every harness, not a Codex special + case. Temp workspaces already used `ignore_cleanup_errors=True`. + +--- + +## 1. Executive Summary & Problem Statement + +During live dogfooding of `skill-eval-loop` with real autonomous coding agents (`cursor-agent` using `claude-sonnet-5-medium` and `gpt-5.6-sol-medium` judge), five primary operational friction points were identified: + +1. **Path Pedantry**: The CLI rejected relative paths (`--skill .agents/skills/karpathy-guidelines`) with `ERROR: skill path must be absolute`, violating standard CLI ergonomics. +2. **Binary Discovery Blindspots**: Harnesses installed in standard user binary paths (such as `~/.local/bin/cursor-agent` or `~/.local/bin/muse`) were not detected unless passed via explicit `--harness-bin `. +3. **Output Directory Collisions**: Re-running iterative evaluations or calibrations aborted with `ERROR: output directory already exists`, forcing repetitive manual `rm -rf` cleanup. +4. **Agent Execution Latency Under-estimation**: The default `120s` timeout expired on complex agent tasks where autonomous coding agents inspect repositories, execute sub-commands, and run test suites. +5. **Opaque Multi-Turn Progress**: Running long agent trials (60–180s) provided zero streaming feedback between `PROGRESS: starting target ...` and completion, preventing operators from distinguishing active tool use from deadlocks. + +This RFC defines the formal specification to eliminate these friction points while preserving strict deterministic reproducibility and host credential isolation. + +--- + +## 2. Detailed Technical Specification + +### Seam 1: Intelligent Path Normalization + +#### Requirement: +All path arguments (`--skill`, `--tasks`, `--output`, `--fixtures`, `--calibration`, `--harness-bin`, `--judge-harness-bin`) MUST accept relative paths, tilde expansions (`~/...`), and symlinks, resolving them deterministically to canonical absolute paths prior to validation and snapshot recording. + +#### Implementation: +```python +def normalize_user_path(path_str: str | Path | None, label: str, must_exist: bool = True) -> Path: + if path_str is None: + raise ValueError(f"{label} is required") + expanded = Path(path_str).expanduser() + resolved = expanded.resolve() + if must_exist and not resolved.exists(): + raise ValueError(f"{label} path does not exist: {path_str}") + return resolved +``` + +#### Behavior & Verification: +- `--skill ./skills/my-skill` resolves to `$(pwd)/skills/my-skill`. +- `run.json` and `calibration.json` continue to record the fully resolved canonical path for immutable provenance. + +--- + +### Seam 2: Standard Harness Binary Auto-Discovery + +#### Requirement: +When `--harness ` is provided without an explicit `--harness-bin`, the evaluator MUST probe standard user and system binary directories before raising a missing executable error. + +#### Discovery Search Order: +1. Explicit `--harness-bin` argument (if supplied). +2. Ambient `shutil.which(name)`. +3. Candidate search directories in priority order: + - `~/.local/bin/` + - `~/.cargo/bin/` + - `~/.bun/bin/` + - `~/.npm-global/bin/` + - `/opt/homebrew/bin/` + - `/opt/homebrew/sbin/` + - `/usr/local/bin/` + - `/usr/bin/` + - `/bin/` + +#### Code Contract: +```python +def discover_executable(executable_name: str) -> str: + # 1. Standard PATH lookup + found = shutil.which(executable_name) + if found: + return found + + # 2. Known standard user/system bin directories + candidates = [ + Path.home() / ".local" / "bin", + Path.home() / ".cargo" / "bin", + Path.home() / ".bun" / "bin", + Path.home() / ".npm-global" / "bin", + Path("/opt/homebrew/bin"), + Path("/opt/homebrew/sbin"), + Path("/usr/local/bin"), + Path("/usr/bin"), + Path("/bin"), + ] + for directory in candidates: + target = directory / executable_name + if target.is_file() and os.access(target, os.X_OK): + return str(target) + + raise ValueError( + f"executable {executable_name!r} not found in PATH or standard binary locations (~/.local/bin, /opt/homebrew/bin, etc.). " + f"Please supply --harness-bin /path/to/{executable_name}" + ) +``` + +--- + +### Seam 3: Idempotent Output & Overwrite Control + +#### Requirement: +To accommodate rapid development cycles without sacrificing safety against accidental overwrites: +1. Running into an existing output directory MUST default to exiting with code 1 and a helpful error message suggesting `--force`. +2. Supplying `--force` / `-f` MUST safely purge and recreate the target output directory before execution begins. + +#### CLI Contract: +```bash +# Refuses to clobber existing directory: +python3 skill_eval_loop.py run --skill ... --output ./out +# ERROR: output directory already exists: /path/to/out. Use --force to overwrite. + +# Safely recreates and proceeds: +python3 skill_eval_loop.py run --skill ... --output ./out --force +``` + +--- + +### Seam 4: Realistic Execution Latency Budgets + +#### Requirement: +Agent harnesses executing multi-turn tool calling (e.g. running Vitest, creating files, linting) routinely exceed simple prompt-completion timeouts. + +#### Standard Timeout Configuration: +- Default `--timeout-seconds`: `300` (5 minutes). +- CLI flag preserves full operator override: `--timeout-seconds `. +- Graceful termination: Send `SIGTERM` to the process group, wait up to 5s, followed by `SIGKILL` on timeout. + +--- + +### Seam 5: Heartbeat & Progress Telemetry + +#### Requirement: +Invocations exceeding 15 seconds MUST print periodic heartbeats (e.g. every 15s) indicating active elapsed time and running role (`control`, `treatment`, `judge`, `pairwise`), ensuring the operator has live confirmation that the subprocess is healthy. + +#### Terminal Output Format: +```text +PROGRESS: starting target ambiguous-auth-scope control +PROGRESS: [ambiguous-auth-scope control] still running... (elapsed: 15s) +PROGRESS: [ambiguous-auth-scope control] still running... (elapsed: 30s) +PROGRESS: finished target ambiguous-auth-scope control: completed in 41200 ms +``` + +--- + +## 3. Security & Invariant Protections + +1. **Credential isolation**: Each adapter owns auth copying or ambient login. + Tokens are not written into public trial reports. +2. **Workspace tempfile lifecycle**: Temporary workspace directories use + `ignore_cleanup_errors=True` to guarantee robust cleanup even when child + processes generate locked files or deep `node_modules` trees. +3. **No Network in CI**: All automated unit and regression tests MUST use fake + mock harnesses without network access or live API dependencies. + +--- + +## 4. Verification Plan + +| Capability | Test Scenario | Acceptance Criteria | +|---|---|---| +| Relative Path Resolution | Run with `--skill ../karpathy-guidelines` | Canonical path recorded in `run.json`; command executes without error. | +| Harness Auto-Discovery | Place mock executable in `~/.local/bin/` | `--harness ` locates executable without `--harness-bin`. | +| Overwrite Flag | Run twice with `--output ` and `--force` | First run creates directory; second run with `--force` successfully purges and re-evaluates. | +| Timeout Headroom | Execute long task taking 180s | Successfully finishes under 300s default without premature timeout abort. | +| Progress Heartbeat | Task running > 30s | Periodic stderr progress messages emitted every 15s. | diff --git a/skills/skill-eval-loop/SKILL.md b/skills/skill-eval-loop/SKILL.md index 5beb0e9..44ed469 100644 --- a/skills/skill-eval-loop/SKILL.md +++ b/skills/skill-eval-loop/SKILL.md @@ -79,21 +79,22 @@ missing-suite error is the precondition for this coordinator workflow. ## Plan the exact run -Pass absolute paths and a fresh output directory. Dry-run validates consumed -inputs, resolves the harness executable, hashes the skill and tasks, and creates -neither run artifacts nor provider calls. +Pass skill, task, and output paths (relative and `~/` paths resolve to +canonical absolute paths in the retained plan). Re-use an output directory with +`--force`. Dry-run validates consumed inputs, resolves the harness executable, +hashes the skill and tasks, and creates neither run artifacts nor provider +calls. Omit `--harness-bin` when the CLI is on `PATH` or in `~/.local/bin`. +The default timeout is 300 seconds. ```bash "$EVALUATOR" run \ --skill /absolute/path/to/target-skill \ --output /absolute/path/to/.eval-output/fresh-run \ --harness pi \ - --harness-bin /absolute/path/to/pi \ --model exact-model-id \ --judge-model exact-judge-model-id \ --calibration /absolute/path/to/fresh-calibration/calibration.json \ --trials 1 \ - --timeout-seconds 300 \ --dry-run ``` diff --git a/skills/skill-eval-loop/scripts/core/util.py b/skills/skill-eval-loop/scripts/core/util.py index c0d2ee5..7c08c5e 100644 --- a/skills/skill-eval-loop/scripts/core/util.py +++ b/skills/skill-eval-loop/scripts/core/util.py @@ -5,16 +5,29 @@ import hashlib import json from pathlib import Path, PurePath +import shutil import sys import unicodedata from typing import Any -def absolute_path(value: str, label: str) -> Path: - path = Path(value) - if not path.is_absolute(): - raise ValueError(f"{label} path must be absolute") - return path +def user_path(value: str | None, label: str, *, must_exist: bool = False) -> Path: + if value is None or not str(value).strip(): + raise ValueError(f"{label} is required") + resolved = Path(value).expanduser().resolve() + if must_exist and not resolved.exists(): + raise ValueError(f"{label} path does not exist: {value}") + return resolved + + +def prepare_output_directory(output: Path, force: bool) -> None: + if not output.exists(): + return + if not force: + raise ValueError( + f"output directory already exists: {output}. Use --force to overwrite." + ) + shutil.rmtree(output) def required_string(value: Any, label: str) -> str: diff --git a/skills/skill-eval-loop/scripts/harnesses/base.py b/skills/skill-eval-loop/scripts/harnesses/base.py index ce7418b..dee881a 100644 --- a/skills/skill-eval-loop/scripts/harnesses/base.py +++ b/skills/skill-eval-loop/scripts/harnesses/base.py @@ -3,6 +3,7 @@ from __future__ import annotations from dataclasses import asdict, dataclass +import os from pathlib import Path import shutil import subprocess @@ -58,6 +59,38 @@ def is_infrastructure_failure(message: str) -> bool: return any(marker in lowered for marker in INFRASTRUCTURE_FAILURE_MARKERS) +def standard_bin_dirs() -> list[Path]: + return [ + Path.home() / ".local" / "bin", + Path.home() / ".cargo" / "bin", + Path.home() / ".bun" / "bin", + Path.home() / ".npm-global" / "bin", + Path("/opt/homebrew/bin"), + Path("/opt/homebrew/sbin"), + Path("/usr/local/bin"), + Path("/usr/bin"), + Path("/bin"), + ] + + +def discover_executable(executable_name: str) -> str | None: + expanded = Path(executable_name).expanduser() + looks_like_path = expanded.is_absolute() or os.sep in executable_name or executable_name.startswith("~") + if looks_like_path: + resolved = expanded.resolve() + if resolved.is_file() and os.access(resolved, os.X_OK): + return str(resolved) + return shutil.which(executable_name) + found = shutil.which(executable_name) + if found: + return found + for directory in standard_bin_dirs(): + target = directory / executable_name + if target.is_file() and os.access(target, os.X_OK): + return str(target) + return None + + class BaseHarnessAdapter: name: str = "" default_executable: str = "" @@ -66,13 +99,13 @@ def resolve(self, executable: str | None) -> tuple[str, str]: target = executable or self.default_executable if not target: raise ValueError(f"executable is required for harness {self.name!r}") - resolved = shutil.which(target) + resolved = discover_executable(target) if resolved is None: - path = Path(target).resolve() - if path.is_file(): - resolved = str(path) - else: - raise ValueError(f"{self.name} executable not found: {target}") + raise ValueError( + f"{self.name} executable not found: {target}. " + f"Searched PATH and standard binary locations (~/.local/bin, /opt/homebrew/bin). " + f"Supply --harness-bin /path/to/{target}" + ) try: version = subprocess.run( [resolved, "--version"], text=True, capture_output=True, check=True diff --git a/skills/skill-eval-loop/scripts/harnesses/script.py b/skills/skill-eval-loop/scripts/harnesses/script.py index ca74cfd..396f2cd 100644 --- a/skills/skill-eval-loop/scripts/harnesses/script.py +++ b/skills/skill-eval-loop/scripts/harnesses/script.py @@ -4,10 +4,9 @@ import json from pathlib import Path -import shutil from typing import Any -from harnesses.base import BaseHarnessAdapter, TraceResult +from harnesses.base import BaseHarnessAdapter, TraceResult, discover_executable class ScriptAdapter(BaseHarnessAdapter): @@ -17,13 +16,9 @@ class ScriptAdapter(BaseHarnessAdapter): def resolve(self, executable: str | None) -> tuple[str, str]: if not executable: raise ValueError("harness-bin is required for script harness") - resolved = shutil.which(executable) + resolved = discover_executable(executable) if resolved is None: - path = Path(executable).resolve() - if path.is_file(): - resolved = str(path) - else: - raise ValueError(f"script executable not found: {executable}") + raise ValueError(f"script executable not found: {executable}") return resolved, "custom-script 1.0" def build_command( diff --git a/skills/skill-eval-loop/scripts/skill_eval_loop.py b/skills/skill-eval-loop/scripts/skill_eval_loop.py index 67a1620..f9a5cc6 100644 --- a/skills/skill-eval-loop/scripts/skill_eval_loop.py +++ b/skills/skill-eval-loop/scripts/skill_eval_loop.py @@ -44,10 +44,11 @@ ) from core.review import Agreement, ReviewItem, ReviewPacket, ReviewerLabels # noqa: E402 from core.util import ( # noqa: E402 - absolute_path, hash_file, load_json_object, normalized_id, + prepare_output_directory, + user_path, print_json, relative_workspace_path, required_string, @@ -95,6 +96,10 @@ def error(message: str) -> None: print(f"ERROR: {message}", file=sys.stderr) +HEARTBEAT_SECONDS = 15 +TERMINATE_GRACE_SECONDS = 5 + + def progress(message: str) -> None: print(f"PROGRESS: {message}", file=sys.stderr, flush=True) @@ -386,7 +391,7 @@ def hash_skill(root: Path) -> str: def resolve_tasks_path(skill: Path, value: str | None) -> Path: if value is not None: - return absolute_path(value, "tasks") + return user_path(value, "tasks", must_exist=True) owned_suite = skill / "evals" / "tasks.jsonl" if not owned_suite.is_file(): raise ValueError( @@ -421,8 +426,8 @@ def build_plan(arguments: argparse.Namespace) -> dict[str, Any]: raise ValueError("promotion runs require an explicit independently controlled tasks path") if arguments.promotion and arguments.trials < 3: raise ValueError("promotion runs require at least 3 trials") - skill = absolute_path(arguments.skill, "skill") - output = absolute_path(arguments.output, "output") + skill = user_path(arguments.skill, "skill", must_exist=True) + output = user_path(arguments.output, "output") if not (skill / "SKILL.md").is_file(): raise ValueError("skill path must contain SKILL.md") tasks_path = resolve_tasks_path(skill, arguments.tasks) @@ -438,7 +443,7 @@ def build_plan(arguments: argparse.Namespace) -> dict[str, Any]: calibration: dict[str, Any] | None = None if arguments.calibration is not None: try: - calibration_path = absolute_path(arguments.calibration, "calibration") + calibration_path = user_path(arguments.calibration, "calibration", must_exist=True) except ValueError as exc: raise CalibrationBindingError(str(exc)) from exc calibration = load_calibration_binding(calibration_path, arguments.model, arguments.judge_model).as_dict() @@ -667,25 +672,41 @@ def _invoke( ) started = time.monotonic() timed_out = False + exit_code = -1 + timeout_seconds = int(self.configuration["timeout_seconds"]) + deadline = started + timeout_seconds progress(f"starting {display_name}") - try: - with trace_path.open("w", encoding="utf-8") as trace, stderr_path.open( - "w", encoding="utf-8" - ) as stderr: - completed = subprocess.run( - arguments, - cwd=workspace, - env=environment, - stdin=subprocess.DEVNULL, - stdout=trace, - stderr=stderr, - timeout=self.configuration["timeout_seconds"], - check=False, - ) - exit_code = completed.returncode - except subprocess.TimeoutExpired: - timed_out = True - exit_code = -1 + with trace_path.open("w", encoding="utf-8") as trace, stderr_path.open( + "w", encoding="utf-8" + ) as stderr: + process = subprocess.Popen( + arguments, + cwd=workspace, + env=environment, + stdin=subprocess.DEVNULL, + stdout=trace, + stderr=stderr, + ) + while True: + remaining = deadline - time.monotonic() + if remaining <= 0: + process.terminate() + try: + process.wait(timeout=TERMINATE_GRACE_SECONDS) + except subprocess.TimeoutExpired: + process.kill() + process.wait() + timed_out = True + exit_code = -1 + break + try: + exit_code = process.wait(timeout=min(HEARTBEAT_SECONDS, remaining)) + break + except subprocess.TimeoutExpired: + elapsed = max(1, round(time.monotonic() - started)) + progress( + f"[{display_name}] still running... (elapsed: {elapsed}s)" + ) duration_ms = round((time.monotonic() - started) * 1000) observed = adapter.parse_trace( trace_path=trace_path, @@ -1160,8 +1181,7 @@ def run_live(plan: dict[str, Any]) -> dict[str, Any]: raise CalibrationBindingError("calibration changed after dry-run planning") if binding.fixtures_sha256 != configuration.get("fixtures_sha256"): raise CalibrationBindingError("calibration fixture changed after dry-run planning") - if output.exists(): - raise ValueError(f"output directory already exists: {output}") + prepare_output_directory(output, bool(plan.get("force"))) skill_name = skill.name output.mkdir(parents=True) runtime = HarnessRuntime(output, configuration) @@ -1294,8 +1314,8 @@ def build_calibration_plan(arguments: argparse.Namespace) -> dict[str, Any]: raise ValueError("model, judge-model, and positive timeout-seconds are required") if arguments.judge_model == arguments.model: raise ValueError("judge-model must differ from model") - fixtures = absolute_path(arguments.fixtures, "fixtures") - output = absolute_path(arguments.output, "output") + fixtures = user_path(arguments.fixtures, "fixtures", must_exist=True) + output = user_path(arguments.output, "output") suite = load_calibration(fixtures).as_dict() judge_harness = getattr(arguments, "judge_harness", None) or target_harness executable, version = resolve_harness(target_harness, arguments.harness_bin) @@ -1396,8 +1416,7 @@ def run_calibrate(plan: dict[str, Any]) -> dict[str, Any]: suite = load_calibration(fixtures).as_dict() if suite["sha256"] != configuration["fixtures_sha256"]: raise ValueError("calibration fixtures changed after dry-run planning") - if output.exists(): - raise ValueError(f"output directory already exists: {output}") + prepare_output_directory(output, bool(plan.get("force"))) output.mkdir(parents=True) runtime = HarnessRuntime(output, configuration) try: @@ -1519,6 +1538,7 @@ def run(arguments: argparse.Namespace) -> int: if arguments.dry_run: print_json(plan) return 0 + plan["force"] = bool(getattr(arguments, "force", False)) result = run_live(plan) print_json(result) return live_exit_code(result["valid"], result["quality_status"]) @@ -1529,6 +1549,7 @@ def calibrate(arguments: argparse.Namespace) -> int: if arguments.dry_run: print_json(plan) return 0 + plan["force"] = bool(getattr(arguments, "force", False)) result = run_calibrate(plan) print_json(result) return calibration_exit_code(result) @@ -1551,11 +1572,10 @@ def load_reviewable_run(run_dir: Path) -> tuple[dict[str, Any], dict[str, Any], def prepare_review(arguments: argparse.Namespace) -> int: - run_dir = absolute_path(arguments.run_dir, "run-dir") - output = absolute_path(arguments.output, "output") + run_dir = user_path(arguments.run_dir, "run-dir", must_exist=True) + output = user_path(arguments.output, "output") run, configuration, run_path = load_reviewable_run(run_dir) - if output.exists(): - raise ValueError(f"output directory already exists: {output}") + prepare_output_directory(output, arguments.force) items: list[ReviewItem] = [] copied: list[tuple[Path, Path]] = [] @@ -1698,11 +1718,10 @@ def count_agreement(left: str, right: str) -> int: def finalize_review(arguments: argparse.Namespace) -> int: - run_dir = absolute_path(arguments.run_dir, "run-dir") - manifest_path = absolute_path(arguments.manifest, "manifest") - output = absolute_path(arguments.output, "output") - if output.exists(): - raise ValueError(f"output directory already exists: {output}") + run_dir = user_path(arguments.run_dir, "run-dir", must_exist=True) + manifest_path = user_path(arguments.manifest, "manifest", must_exist=True) + output = user_path(arguments.output, "output") + prepare_output_directory(output, arguments.force) if not math.isfinite(arguments.cost_usd) or arguments.cost_usd < 0: raise ValueError("cost-usd must be a finite non-negative number") cost_note = required_string(arguments.cost_note, "cost-note") @@ -1714,7 +1733,7 @@ def finalize_review(arguments: argparse.Namespace) -> int: raise ValueError("manifest: run hash does not match the retained promotion run") if manifest.get("tasks_sha256") != configuration.get("tasks_sha256"): raise ValueError("manifest: tasks hash does not match the retained promotion run") - attestation_path = absolute_path(arguments.holdout_attestation, "holdout-attestation") + attestation_path = user_path(arguments.holdout_attestation, "holdout-attestation", must_exist=True) attestation = load_json_object(attestation_path, "holdout attestation") if attestation.get("version") != 1: raise ValueError("holdout attestation field version: must be 1") @@ -1759,7 +1778,7 @@ def finalize_review(arguments: argparse.Namespace) -> int: if len(arguments.labels) != 2: raise ValueError("finalize-review requires exactly two independent label files") manifest_hash = hash_file(manifest_path) - label_paths = [absolute_path(value, "labels") for value in arguments.labels] + label_paths = [user_path(value, "labels", must_exist=True) for value in arguments.labels] reviews = [ load_reviewer_labels(path, manifest_hash, items) for path in label_paths ] @@ -2021,7 +2040,7 @@ def parser() -> argparse.ArgumentParser: run_parser.add_argument("--judge-harness-bin") run_parser.add_argument("--model", required=True) run_parser.add_argument("--trials", type=int, default=1) - run_parser.add_argument("--timeout-seconds", type=int, default=120) + run_parser.add_argument("--timeout-seconds", type=int, default=300) run_parser.add_argument("--judge-model", default="") run_parser.add_argument("--calibration") run_parser.add_argument( @@ -2029,6 +2048,7 @@ def parser() -> argparse.ArgumentParser: action="store_true", help="require an explicit task set, accepted rubric calibration, and at least 3 trials", ) + run_parser.add_argument("--force", "-f", action="store_true") run_parser.add_argument("--dry-run", action="store_true") run_parser.set_defaults(handler=run) calibrate_parser = commands.add_parser( @@ -2042,7 +2062,8 @@ def parser() -> argparse.ArgumentParser: calibrate_parser.add_argument("--judge-harness-bin") calibrate_parser.add_argument("--model", required=True) calibrate_parser.add_argument("--judge-model", required=True) - calibrate_parser.add_argument("--timeout-seconds", type=int, default=120) + calibrate_parser.add_argument("--timeout-seconds", type=int, default=300) + calibrate_parser.add_argument("--force", "-f", action="store_true") calibrate_parser.add_argument("--dry-run", action="store_true") calibrate_parser.set_defaults(handler=calibrate) review_parser = commands.add_parser( @@ -2050,6 +2071,7 @@ def parser() -> argparse.ArgumentParser: ) review_parser.add_argument("--run-dir", required=True) review_parser.add_argument("--output", required=True) + review_parser.add_argument("--force", "-f", action="store_true") review_parser.set_defaults(handler=prepare_review) finalize_parser = commands.add_parser( "finalize-review", help="measure two human reviews against retained promotion evidence" @@ -2061,6 +2083,7 @@ def parser() -> argparse.ArgumentParser: finalize_parser.add_argument("--cost-usd", type=float, required=True) finalize_parser.add_argument("--cost-note", required=True) finalize_parser.add_argument("--output", required=True) + finalize_parser.add_argument("--force", "-f", action="store_true") finalize_parser.set_defaults(handler=finalize_review) return result diff --git a/tests/fixtures/simple-fake-codex b/tests/fixtures/simple-fake-codex index f6c4a3b..98da4ca 100755 --- a/tests/fixtures/simple-fake-codex +++ b/tests/fixtures/simple-fake-codex @@ -42,6 +42,13 @@ if [ -n "${SIMPLE_FAKE_AUTH_LOG:-}" ]; then fi fi +if [ "$judge_like" -eq 0 ] && [ -n "${SIMPLE_FAKE_SLEEP_SECONDS:-}" ]; then + skill_name=${SKILL_EVAL_SKILL_NAME:-skill} + if [ ! -f "$PWD/.agents/skills/$skill_name/SKILL.md" ]; then + sleep "$SIMPLE_FAKE_SLEEP_SECONDS" + fi +fi + if [ -n "${SIMPLE_FAKE_INFRA_FAILURE:-}" ]; then printf '%s\n' '{"type":"turn.failed","error":{"message":"error sending request: failed to lookup address information"}}' printf '%s\n' 'failed to lookup address information' >&2 diff --git a/tests/test_calibration.py b/tests/test_calibration.py index 2b3184e..e7897bd 100644 --- a/tests/test_calibration.py +++ b/tests/test_calibration.py @@ -186,7 +186,7 @@ def test_relative_calibration_path_exits_two(self) -> None: ) self.assertEqual(result.returncode, 2, result.stderr) - self.assertIn("calibration path must be absolute", result.stderr) + self.assertIn("calibration path does not exist", result.stderr) def test_empty_calibration_path_exits_two(self) -> None: @@ -194,7 +194,7 @@ def test_empty_calibration_path_exits_two(self) -> None: result, _, _ = self.run_live_rubric(Path(temporary), calibration="") self.assertEqual(result.returncode, 2, result.stderr) - self.assertIn("calibration path must be absolute", result.stderr) + self.assertIn("calibration is required", result.stderr) def test_post_plan_calibration_or_fixture_drift_exits_two(self) -> None: diff --git a/tests/test_harnesses.py b/tests/test_harnesses.py index 5a52578..d3fe223 100644 --- a/tests/test_harnesses.py +++ b/tests/test_harnesses.py @@ -234,7 +234,7 @@ def test_live_run_discards_auth_when_initialization_fails(self) -> None: original_copyfile = evaluator.shutil.copyfile def fail_task_copy(source: Path, destination: Path) -> None: - if Path(destination) == output / "tasks.jsonl": + if Path(destination).resolve() == (output / "tasks.jsonl").resolve(): raise OSError("task copy failed") original_copyfile(source, destination) @@ -884,3 +884,128 @@ def test_models_empty_listing_when_the_cli_cannot_enumerate(self) -> None: self.assertEqual(payload["source"], "unavailable") self.assertEqual(payload["models"], []) + + def test_discovers_harness_in_user_local_bin(self) -> None: + spec = importlib.util.spec_from_file_location("evaluator_mod", EVALUATOR) + self.assertIsNotNone(spec) + self.assertIsNotNone(spec.loader) + evaluator = importlib.util.module_from_spec(spec) + spec.loader.exec_module(evaluator) + with tempfile.TemporaryDirectory() as temporary: + home = Path(temporary) / "home" + local_bin = home / ".local" / "bin" + local_bin.mkdir(parents=True) + exe = local_bin / "cursor-agent" + exe.write_text("#!/bin/sh\nprintf 'mock-cursor 1.0\\n'\n", encoding="utf-8") + exe.chmod(0o755) + with patch.dict(os.environ, {"HOME": str(home), "PATH": "/usr/bin:/bin"}): + resolved, version = evaluator.get_harness_adapter("cursor-agent").resolve(None) + self.assertEqual(Path(resolved).resolve(), exe.resolve()) + self.assertEqual(version, "mock-cursor 1.0") + + + def test_force_overwrites_an_existing_output_directory(self) -> None: + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + skill = self.make_skill(root) + tasks = root / "tasks.jsonl" + tasks.write_text( + '{"id":"choice","prompt":"Choose Blue.","graders":[{"type":"regex","pattern":"Blue"}]}\n', + encoding="utf-8", + ) + output = root / "run" + first = self.run_cli( + "run", + "--skill", + str(skill), + "--tasks", + str(tasks), + "--output", + str(output), + "--harness", + "codex", + "--harness-bin", + str(FAKE_CODEX), + "--model", + "test-model", + ) + self.assertIn(first.returncode, {0, 1}, first.stderr) + self.assertTrue((output / "run.json").is_file()) + blocked = self.run_cli( + "run", + "--skill", + str(skill), + "--tasks", + str(tasks), + "--output", + str(output), + "--harness", + "codex", + "--harness-bin", + str(FAKE_CODEX), + "--model", + "test-model", + ) + self.assertEqual(blocked.returncode, 1, blocked.stderr) + self.assertIn("already exists", blocked.stderr) + self.assertIn("--force", blocked.stderr) + forced = self.run_cli( + "run", + "--skill", + str(skill), + "--tasks", + str(tasks), + "--output", + str(output), + "--harness", + "codex", + "--harness-bin", + str(FAKE_CODEX), + "--model", + "test-model", + "--force", + ) + self.assertIn(forced.returncode, {0, 1}, forced.stderr) + self.assertTrue((output / "run.json").is_file()) + + + def test_long_invocation_emits_stderr_heartbeats(self) -> None: + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + skill = self.make_skill(root) + tasks = root / "tasks.jsonl" + tasks.write_text( + '{"id":"choice","prompt":"Choose Blue.","graders":[{"type":"regex","pattern":"Blue"}]}\n', + encoding="utf-8", + ) + output = root / "run" + result = subprocess.run( + [ + "python3", + str(EVALUATOR), + "run", + "--skill", + str(skill), + "--tasks", + str(tasks), + "--output", + str(output), + "--harness", + "codex", + "--harness-bin", + str(FAKE_CODEX), + "--model", + "test-model", + "--timeout-seconds", + "60", + ], + cwd=ROOT, + text=True, + capture_output=True, + check=False, + env=self.isolated_env(root, {"SIMPLE_FAKE_SLEEP_SECONDS": "16"}), + ) + self.assertIn(result.returncode, {0, 1}, result.stderr) + self.assertIn("still running... (elapsed:", result.stderr) + self.assertRegex(result.stderr, r"elapsed: 1[5-9]s") + diff --git a/tests/test_tasks.py b/tests/test_tasks.py index e5f5f15..df386bf 100644 --- a/tests/test_tasks.py +++ b/tests/test_tasks.py @@ -3,6 +3,7 @@ import importlib.util import json from pathlib import Path +import subprocess import tempfile from helpers import ( @@ -78,6 +79,7 @@ def test_dry_run_validates_inputs_without_creating_output(self) -> None: self.assertFalse(plan["created_artifacts"]) self.assertEqual(plan["configuration"]["intervention"], "injected_skill_instructions") self.assertEqual(plan["counts"]["total_invocations"], 15) + self.assertEqual(plan["configuration"]["timeout_seconds"], 300) self.assertEqual( plan["task_snapshot"][0]["graders"][1]["dimensions"][0]["name"], "safe choice", @@ -227,7 +229,10 @@ def test_dry_run_uses_target_owned_tasks_when_tasks_are_omitted(self) -> None: ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(json.loads(result.stdout)["configuration"]["tasks_path"], str(tasks)) + self.assertEqual( + json.loads(result.stdout)["configuration"]["tasks_path"], + str(tasks.resolve()), + ) def test_promotion_requires_explicit_tasks_and_repeated_trials(self) -> None: @@ -475,3 +480,47 @@ def test_evals_and_tests_are_not_payload_hash_or_treatment_copy(self) -> None: self.assertFalse((destination / "evals").exists()) self.assertFalse((destination / "tests").exists()) + + def test_relative_and_home_paths_resolve_in_the_plan(self) -> None: + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + skill = self.make_skill(root) + tasks = root / "tasks.jsonl" + tasks.write_text( + '{"id":"choice","prompt":"Choose Blue.","graders":[{"type":"regex","pattern":"Blue"}]}\n', + encoding="utf-8", + ) + result = subprocess.run( + [ + "python3", + str(EVALUATOR), + "run", + "--skill", + "~/target-skill", + "--tasks", + "tasks.jsonl", + "--output", + "./fresh-run", + "--harness", + "codex", + "--harness-bin", + str(FAKE_CODEX), + "--model", + "test-model", + "--dry-run", + ], + cwd=root, + text=True, + capture_output=True, + check=False, + env={**self.isolated_env(root), "HOME": str(root)}, + ) + self.assertEqual(result.returncode, 0, result.stderr) + plan = json.loads(result.stdout) + self.assertEqual(plan["configuration"]["skill_path"], str(skill.resolve())) + self.assertEqual(plan["configuration"]["tasks_path"], str(tasks.resolve())) + self.assertEqual( + plan["configuration"]["output_dir"], + str((root / "fresh-run").resolve()), + ) + From dae3431f1eb77e24bd3378ef913037af97873409 Mon Sep 17 00:00:00 2001 From: jon-devlapaz Date: Tue, 1 Sep 2026 12:35:01 -0500 Subject: [PATCH 3/4] docs: keep the 2026-08-31 maintainability review Retain the dated structural audit as a local reference so the working tree is clean for the quality run. Co-authored-by: Cursor --- docs/thermo-nuclear-review-2026-08-31.md | 193 +++++++++++++++++++++++ 1 file changed, 193 insertions(+) create mode 100644 docs/thermo-nuclear-review-2026-08-31.md diff --git a/docs/thermo-nuclear-review-2026-08-31.md b/docs/thermo-nuclear-review-2026-08-31.md new file mode 100644 index 0000000..e85d6ff --- /dev/null +++ b/docs/thermo-nuclear-review-2026-08-31.md @@ -0,0 +1,193 @@ +# Thermo-Nuclear Code Quality Review — 2026-08-31 + +**Subject:** `skill-eval-loop` — current branch `ed737d9` (merge `business-ready-promotion-review`) +**Scope:** `skills/skill-eval-loop/scripts/skill_eval_loop.py` (3012 lines), `tests/test_skill_eval_loop.py` (2096 lines), `AGENTS.md`, `ZEN.md`, `tasks/plan.md`, `.github/workflows/validate.yml` +**Bar:** ZEN maintainability + thermo-nuclear rules 0–7 (1k-line ceiling, no spaghetti, boring code wins, code-judo) +**Method:** Static inspection + `git diff --stat HEAD~1` / `git log --oneline` / `wc -l` / `grep -c "if \|elif"` / `ruff check` / `python3 -m unittest discover -s tests -v` +**Verdict:** **PASS with severe structural debt** — behavior is correct and CI is honest; architecture has 3× outgrown its container by instruction and must split next. + +--- + +## 0. Evidence Snapshot (reproducible) + +| Signal | Value | Command | +|---|---|---| +| Evaluator size | **3012 lines** | `wc -l skills/skill-eval-loop/scripts/skill_eval_loop.py` | +| Test size | **2096 lines** (single file) | `wc -l tests/test_skill_eval_loop.py` | +| Combined | 5108 lines in 2 files | | +| Last diff | **+1294 lines** in `skill_eval_loop.py`, 8 files +1828/-93 | `git diff --stat HEAD~1` | +| Branching | **319 `if`/`elif`** | `grep -n "if \|elif " skill_eval_loop.py \| wc -l` | +| Lint | **1 fixable `F841`** unused `exc` at `skill_eval_loop.py:376` | `ruff check` | +| Tests | **55/55 OK** in ~24s | `python3 -m unittest discover -s tests -v` | +| CI jobs | 3: `validate` + `standalone-package` (4 platforms) + `tink-package` (pinned `tink 1.0.0`, Rust 1.95) | `.github/workflows/validate.yml` | +| Debt markers | 0 `TODO/FIXME/HACK/XXX` | `grep -n TODO skill_eval_loop.py` | + +> Re-run: `python3 -m unittest discover -s tests -v && ruff check skills/skill-eval-loop/scripts/skill_eval_loop.py && wc -l skills/skill-eval-loop/scripts/skill_eval_loop.py tests/test_skill_eval_loop.py` + +--- + +## 1. Code-Judo Proposal — Delete Categories of Complexity + +**Current:** One file owns CLI + plan building + 7 harness adapters + `HarnessRuntime` (process/workspace/env/token lifecycle) + grading + rubric/pairwise judging + calibration + review packet + report rendering. `dict[str,Any]` everywhere. + +**Inevitable shape (same behavior, ~40% fewer branches):** +``` +skill_eval_loop.py # CLI only (~200 lines) +harnesses/ + base.py # BaseHarnessAdapter + TraceResult dataclass + codex.py / claude.py / cursor_agent.py / pi.py / hermes.py / muse.py / antigravity.py / script.py +core/ + tasks.py # load_tasks, parse_rubric_dimensions, parse_grader + grading.py # grade, grade_one, same_json, workspace_target + judging.py # judge_prompt, pairwise_prompt, run_rubric_judge, run_pairwise_judge, judge_conditions + calibration.py # load_calibration, _load_calibration_binding, run_calibration_case + review.py # prepare_review, finalize_review, load_reviewer_labels + report.py # write_pair_report, dimension_line, quality rollups + runtime.py # HarnessRuntime / CodexRuntime +``` + +**What disappears:** +- 7 duplicated `parse_trace` result dicts (`response`, `actual_model`, `session_id`, `skill_accessed`, `failure_message`, `input_tokens`, …) at `skill_eval_loop.py:402`, `509`, `591`, `697`, `800` — unified `TraceResult`. +- `mark_provisional` identity wrapper at `skill_eval_loop.py:1662` (pure alias of `mark_judgment_status` at `1647`). +- `BaseHarnessAdapter.is_infrastructure_failure` (420) vs free function `is_infrastructure_failure` (1113) duplication. +- Per-adapter `prepare_environment` copy-paste; `CodexAdapter:440` is the only one with real semantics. + +--- + +## 2. Findings (severity → file:line → remedy) + +### BLOCKER — File Sprawl Past 1k Ceiling (Rule 1) + +| # | Location | Observation | Remedy | +|---|---|---|---| +| F1 | `skill_eval_loop.py:1` — 3012 lines, 3× ceiling | Grew +1294 in one PR (`git diff --stat HEAD~1`). Rule 1 treats >1k as strong smell. | Next PR must split `harnesses/` first. Waiver no longer justified: `tasks/todo.md` shows Phase 2 hill-climb is check-listed done; `AGENTS.md:8` “Do not start by splitting” has expired. | +| F2 | `tests/test_skill_eval_loop.py:1` — 2096 lines, 55 tests in one class | Single `SkillEvalLoopCliTests` hides domains. | Split to `test_tasks.py` / `test_harnesses.py` / `test_judging.py` / `test_calibration.py` / `test_review.py`. | + +### HIGH — Spaghetti / Scattered Conditionals (Rule 2) + +| # | Location | Observation | Remedy | +|---|---|---|---| +| F3 | `HarnessRuntime._invoke:1239–1361` | 120+ line method mixing PATH scrubbing (`pyenv`, `homebrew`, `/usr/bin` at 1272), `CODEX_HOME` injection (1230), `SKILL_EVAL_SKILL_NAME` vs `SKILL_EVAL_ROLE` (1288), timeout vs infra failure (1330), model identity (1328). 5 responsibilities, sequential side-effects. | Extract `EnvironmentPolicy` + `ModelIdentityCheck` + `ArtifactLayout`. Adapter owns `env`/`command`/`trace schema`; runtime owns only `subprocess.run` + lay-down. | +| F4 | `grade_one:1151`, `judge_conditions:1728` | `if configuration["judge_model"] == configuration["model"] → same_model` then `elif not runner_is_valid → runner_gate_failed` then `elif deterministic_status != pass → deterministic_gate_failed` — branching where a `JudgeGate` policy object should exist. | Replace with `JudgeGate.evaluate(conditions, isolation) -> GateResult` (typed). | +| F5 | `prepare_review:2445` + `finalize_review:2640` | 500+ lines of manual JSON IO + path math + `relative_to` + hash checks inside free functions. No `ReviewPacket`/`HoldoutAttestation` model. | Introduce `ReviewPacket`, `ReviewerLabels`, `Agreement` dataclasses; IO at boundary only. | + +### HIGH — Duplication / Thin Abstractions (Rules 3, 4, 6) + +| # | Location | Observation | Remedy | +|---|---|---|---| +| F6 | `mark_provisional:1662` | `def mark_provisional(result): return mark_judgment_status(result)` — identity wrapper, zero clarity. Callers at `1663` only. | Delete; call `mark_judgment_status` directly or inline `provisional_non_independent` assignment. | +| F7 | `BaseHarnessAdapter:357–434` + `CodexAdapter:436`, `ClaudeAdapter:479`, `CursorAgentAdapter:546`, `PiAdapter:628`, `HermesAdapter:646`, `MuseAdapter:669`, `AntigravityAdapter:740`, `ScriptAdapter:763` | 7 adapters duplicate `parse_trace` 7-field dict + `build_command` boilerplate + `prepare_environment` returning `({}, None)` or `({HOME: str(home)}, home)`. `CursorAgentAdapter:550` silently copies `~/.config/cursor/cli-config.json` with `except OSError: pass` — untested host leakage. | `base.py: TraceResult dataclass` + `NoopEnv` vs `IsolatedHome(name)` helpers. `CursorAgent` adapter should require explicit opt-in or be deleted. `ScriptAdapter:763` is the only legitimate distinct `resolve` (no default executable). | +| F8 | `is_infrastructure_failure:420` (method) vs `is_infrastructure_failure:1113` (free fn) | Two copies of same `casefold` + 7-marker check. | Keep one canonical (free fn), have method delegate. | +| F9 | `SUPPORTED_HARNESSES:836` dict is correct, but `get_harness_adapter:848` + `resolve_harness:856` re-resolve string → adapter → executable in two places | Architectural drift — harness identity resolved twice. | Single `HarnessRegistry.resolve(name, executable) -> (Adapter, executable, version)`. | + +### MEDIUM — Type / Boundary Cleanliness (Rule 5) + +| # | Location | Observation | Remedy | +|---|---|---|---| +| F10 | `load_tasks:130`, `load_calibration:177`, `judge_prompt:1520`, `parse_judge_dimensions:1599` | `dict[str,Any]` throughout; rubric invariant (“needs `response_not_empty` preflight + ≥1 rubric with ≥2 levels”) validated deep inside `parse_rubric_dimensions:69`, not at boundary. Callers re-check `isinstance`. | Typed `Task`, `Rubric`, `Dimension`, `Level`, `CalibrationBinding` dataclasses. `load_tasks` returns `list[Task]` or raises `CalibrationBindingError` once. | +| F11 | `unknown_judgment:1626` hard-codes 7-field `execution` dict; `unknown_pairwise:1643` aliases it | Same shape repeated via dict literal, no contract. | Reuse `TraceResult.unknown(judge_model)` factory. | +| F12 | `pairwise_mapping:1552` uses `random.Random(trial).randrange(2)`; `calibration_mapping:1558` uses `seed % 2` | Two different determinism idioms for same blind-label flip. | One `blind_flip(seed) -> Mapping` function. | + +### MEDIUM — Orchestration / Atomicity (Rule 7) + +| # | Location | Observation | Remedy | +|---|---|---|---| +| F13 | `run_live:2066` sequential `control → treatment → judge × N → pairwise × N` | Control/treatment per trial are independent but serialized. No correctness need; measurable wall-time cost for `trials > 1`. | After modularization, experiment with `concurrent.futures` for control/treatment pair; keep artifact dirs isolated (already are per `pair_dir/control` vs `pair_dir/treatment`). Additive, not blocking. | +| F14 | `prepare_run_codex_home:1027` + `discard_runtime_home:1041` manual `shutil.rmtree` vs `TemporaryDirectory` in `run_condition:1374` | Two lifecycles for same concern; `prepare_run_codex_home` copies only `auth.json`, but `HarnessRuntime.__init__:1212` re-injects `CODEX_HOME` at 1230. | Unify: `HarnessRuntime` owns one `TemporaryDirectory` for codex-home; `prepare_run_codex_home` becomes `CodexHome.create(output_dir) -> Path` context manager. | + +### LOW — Nits That Still Matter + +| # | Location | Observation | Remedy | +|---|---|---|---| +| F15 | `skill_eval_loop.py:376` `except (...) as exc:` unused | `F841` flagged by `ruff`. | `except (CalledProcessError, OSError):` — delete binding. | +| F16 | `BaseHarnessAdapter.resolve:373` runs `[resolved, "--version"]` synchronously without timeout | Could hang on broken binary. | Add `timeout=5` or delete version probing (unused beyond display). | +| F17 | `docs/minimum-eval-contract.md` 17k lines vs `docs/thermo-nuclear-review-2026-08-31.md` (this file) — contract is thorough but not linked from `README.md` run section | Discoverability. | Already fixed in `d982330 docs: link promotion review evidence` — verify link stays. | + +--- + +## 3. What’s Good — Do Not Regress + +- Deterministic gate precedes judging; `unknown` on malformed/timeout/mismatch; zero judge calls on deterministic failure — `judge_conditions:1728`, validated by 55 tests. +- Blinded pairwise mapping preserved outside prompt (`pairwise_mapping:1552`, `calibration_mapping:1558`). +- Calibration binding hard-fails on hash drift (`_load_calibration_binding:237`, `load_calibration_binding:321`). +- CI honesty: `.github/workflows/validate.yml:12` — `unittest` + `healthcheck.sh` + whitespace + tracked-artifact rejection + `standalone-package` (4 platforms, `env -i PATH=/usr/bin:/bin`) + `tink-package` (pinned `tink 1.0.0`, Rust 1.95). No live calls, no credentials. +- Zero `TODO/FIXME/HACK` — clean working tree beyond this doc. + +--- + +## 4. Validation Checklist for Next Agent + +Run these exactly; all must be green before merge: + +```bash +python3 -m unittest discover -s tests -v +ruff check skills/skill-eval-loop/scripts/skill_eval_loop.py +ruff check tests/ +skills/skill-eval-loop/scripts/healthcheck.sh +wc -l skills/skill-eval-loop/scripts/skill_eval_loop.py # target: <1000 after split +grep -c "if \|elif " skills/skill-eval-loop/scripts/skill_eval_loop.py # expect ↓ from 319 +git diff --stat HEAD # no file should grow >1k in one PR +``` + +**Behavioral invariants to assert (add as tests if splitting):** +- [ ] `control` never sees skill payload; `treatment` hash matches source (`run_condition:1387`). +- [ ] Same-model judge → `unknown` `same_model` (no pass) (`judge_conditions:1742`). +- [ ] Deterministic failure → 0 judge invocations. +- [ ] `provisional_non_independent` vs `independent` labeled correctly (`mark_judgment_status:1647` — same harness vs cross-harness). +- [ ] Calibration binding rejects fixture/hash drift (`run_calibrate:2310`, `load_calibration_binding:321`). +- [ ] `prepare_review` packet is blinded; `finalize_review` measures agreement without exposing mapping prematurely. + +--- + +## 5. Recommended Next 3 PRs (smallest complete changes, in order) + +1. **PR A — Extract `harnesses/`** (~400 lines deleted): Move 7 adapters to `harnesses/*.py`, introduce `TraceResult` dataclass, `NoopEnv`/`IsolatedHome` helpers, delete `mark_provisional` wrapper, fix `F841`. Tests unchanged. +2. **PR B — Extract `core/judging.py`**: Unify `unknown_judgment`/`unknown_pairwise`, `is_infrastructure_failure`, `JudgeGate` policy; collapse `if blocked_reason` chain in `judge_conditions:1748`. +3. **PR C — Typed boundaries**: `Task`/`Rubric`/`CalibrationBinding` dataclasses at `load_tasks`/`load_calibration`; `prepare_review`/`finalize_review` → `ReviewPacket` model; split `tests/test_skill_eval_loop.py` by domain. + +Each PR: `python3 -m unittest discover -s tests -v` green, `ruff` clean, file stays <1k delta. + +--- + +## 6. Reviewer Sign-off + +| Reviewer | Date | Result | Notes | +|---|---|---|---| +| (agent 1) | 2026-08-31 | — | Authored findings | +| (agent 2) | | | Validate checklist above, confirm metrics, challenge judo proposal | +| Human | | | Approve split sequencing; lift `AGENTS.md:8` “Do not split” constraint | + +--- + +## Appendix — Raw Counts + +``` +git log --oneline -10: +ed737d9 Merge pull request #3 from jon-devlapaz/codex/business-ready-promotion-review +d982330 docs: link promotion review evidence +cc43c96 feat: report trial outcome variance +ca5b9df fix: retain complete promotion review inputs +a355612 docs: define the promotion evidence handoff +a387521 feat: finalize human-grounded promotion evidence +0c7165d feat: prepare blinded promotion reviews +57bcba0 Merge pull request #2 from jon-devlapaz/codex/phase-2-evaluator-hardening +f425a7e docs: define promotion evidence workflow +b32889e test: add public React skill benchmark + +git diff --stat HEAD~1: + README.md | 19 +- + docs/minimum-eval-contract.md | 18 +- + skills/skill-eval-loop/SKILL.md | 14 +- + .../references/promotion-workflow.md | 106 ++ + skills/skill-eval-loop/scripts/skill_eval_loop.py | 1294 ++++++++++++++++++-- + tasks/plan.md | 30 +- + tasks/todo.md | 8 +- + tests/test_skill_eval_loop.py | 432 ++++++- + 8 files changed, 1828 insertions(+), 93 deletions(-) + +grep inventory (abbrev): + def / class count: 48 definitions in skill_eval_loop.py + if / elif count: 319 +``` + +*Generated for independent validation. File: `docs/thermo-nuclear-review-2026-08-31.md`.* From c5587942866bb6672876d052ffd6e40a70a353d1 Mon Sep 17 00:00:00 2001 From: jon-devlapaz Date: Tue, 1 Sep 2026 23:57:12 -0500 Subject: [PATCH 4/4] feat: bind rubric runs only when calibration covers task dimensions Color-choice fixtures cannot score the React review suite. Reject mismatched calibration at plan time and ship a shared-dimension v2 dataset. Co-authored-by: Cursor --- README.md | 41 +++++++- skills/skill-eval-loop/SKILL.md | 3 +- .../scripts/skill_eval_loop.py | 23 +++++ tasks/react-best-practices-v2.jsonl | 8 ++ .../fixtures/calibration/react-review-v1.json | 69 +++++++++++++ tests/test_calibration.py | 97 +++++++++++++++++++ tests/test_tasks.py | 38 ++++++++ 7 files changed, 274 insertions(+), 5 deletions(-) create mode 100644 tasks/react-best-practices-v2.jsonl create mode 100644 tests/fixtures/calibration/react-review-v1.json diff --git a/README.md b/README.md index 599bafc..177e836 100644 --- a/README.md +++ b/README.md @@ -80,12 +80,45 @@ The checked-in development benchmark evaluates Vercel's Fetch that exact revision into a controlled local directory and pass the absolute skill subpath plus the checked-in task file to `run --dry-run`. Reject the plan if the revision or payload hash differs. The public task file is -development evidence, not a secret client holdout. +development evidence, not a secret client holdout. The 2026-09-01 Cursor run +showed v1 is saturated for `claude-sonnet-5-medium`: both conditions already +met the rubric. + +### Quality dataset (v2) + +Use `tasks/react-best-practices-v2.jsonl` with +`tests/fixtures/calibration/react-review-v1.json`. Eight review-in-prompt +examples share binary dimensions `primary_diagnosis`, `actionable_fix`, and +`grounded_claims`. Bound calibration must cover every task rubric dimension +name. Run at least 3 trials on the same harness pair. This remains a +development experiment until humans review a sample of transcripts. + +- expected v2 task SHA-256: + `9c3a558691d2507ccb332d8f20d25422f52f4ab9602b479c0e2e0c3a498e959b` +- expected react-review calibration fixture SHA-256: + `5b093a444351abe683dbe3177f0c1ef93f161e02c466a94b9b0376de599cfd06` + +The 2026-09-01 same-harness Cursor v2 run +(`.eval-output/react-best-practices-v2-cursor/`) used the previous task SHA +and is retained as provisional evidence. `memo-default-callback` was then +rewritten so the gold label matches React memo semantics: Header's inline +default is passed into a memoized child. + +The 2026-09-02 bound independent run +(`.eval-output/react-best-practices-v2-cursor-agy-judge/`) used that SHA, +`claude-sonnet-5-medium` on `cursor-agent`, and `gemini-3.6-flash-high` on +`antigravity` after a 3/3 accepted same-harness calibration of that judge id. +It exited 0 with `quality_status: independent`. Across 24 paired trials, +`quality_outcome` was control 15, tie 4, treatment 2, inconsistent 3. +Per-output binary scores were at ceiling for both conditions +(`primary_diagnosis` and `grounded_claims` 24/24; `actionable_fix` control +23/24, treatment 24/24). Pairwise leftover scoring is not a skill win. This +is not a promotion claim. For rubric tasks, also pass `--judge-model` with a different exact model -identifier and `--calibration /absolute/path/to/calibration.json` from an -accepted calibrate run on that same harness pair. The runner judges each -condition only after deterministic gates pass. A valid same-harness judgment +identifier and `--calibration` from an accepted calibrate run on that same +harness pair. The runner judges each condition only after deterministic gates +pass. Color-choice fixtures cannot bind v2. A valid same-harness judgment is `provisional_non_independent`; a timeout, failed gate, malformed response, or identity mismatch is `unknown`. A missing trace-reported model is unattested, not a quality unknown. Omitting `--calibration` is allowed, but a rubric run diff --git a/skills/skill-eval-loop/SKILL.md b/skills/skill-eval-loop/SKILL.md index 44ed469..163cb65 100644 --- a/skills/skill-eval-loop/SKILL.md +++ b/skills/skill-eval-loop/SKILL.md @@ -175,7 +175,8 @@ below threshold, and `2` if a judgment is invalid. The operator-controlled `calibration.json` and its original absolute fixture path are the binding trust root. The runner validates their internal consistency, models, labels, agreement threshold, assignment -orientations, and fixture hash. It does not authenticate the origin of the raw +orientations, fixture hash, and coverage of every task rubric dimension +name. It does not authenticate the origin of the raw judge artifacts. Keep the calibration directory and fixture under controlled local custody; moving the fixture invalidates the binding even if its content is unchanged. diff --git a/skills/skill-eval-loop/scripts/skill_eval_loop.py b/skills/skill-eval-loop/scripts/skill_eval_loop.py index f9a5cc6..286ae98 100644 --- a/skills/skill-eval-loop/scripts/skill_eval_loop.py +++ b/skills/skill-eval-loop/scripts/skill_eval_loop.py @@ -213,6 +213,28 @@ def load_tasks(path: Path) -> list[Task]: INTERVENTION = "injected_skill_instructions" +def rubric_dimension_names(tasks: list[dict[str, Any]]) -> set[str]: + names: set[str] = set() + for task in tasks: + for grader in task["graders"]: + if grader["type"] != "rubric": + continue + for dimension in grader["dimensions"]: + names.add(dimension["name"]) + return names + + +def require_calibration_covers_dimensions(fixtures_path: str, tasks: list[dict[str, Any]]) -> None: + suite = load_calibration(Path(fixtures_path)) + covered = {dimension.name for dimension in suite.dimensions} + missing = sorted(rubric_dimension_names(tasks) - covered) + if missing: + raise CalibrationBindingError( + "calibration fixtures omit rubric dimensions required by the tasks: " + + ", ".join(missing) + ) + + def load_calibration(path: Path) -> CalibrationSuite: try: raw = json.loads(path.read_text(encoding="utf-8")) @@ -447,6 +469,7 @@ def build_plan(arguments: argparse.Namespace) -> dict[str, Any]: except ValueError as exc: raise CalibrationBindingError(str(exc)) from exc calibration = load_calibration_binding(calibration_path, arguments.model, arguments.judge_model).as_dict() + require_calibration_covers_dimensions(calibration["fixtures_path"], tasks) judge_harness = getattr(arguments, "judge_harness", None) or target_harness executable, version = resolve_harness(target_harness, arguments.harness_bin) judge_executable, judge_version = resolve_harness( diff --git a/tasks/react-best-practices-v2.jsonl b/tasks/react-best-practices-v2.jsonl new file mode 100644 index 0000000..42f8f8f --- /dev/null +++ b/tasks/react-best-practices-v2.jsonl @@ -0,0 +1,8 @@ +{"id": "barrel-lucide-imports", "split": "core", "category": "bundle", "source": {"urls": ["https://vercel.com/blog/how-we-optimized-package-imports-in-next-js"]}, "prompt": "Review this Next.js 15 App Router client module. Return a concise performance review. Do not edit files.\n\n```tsx\n'use client'\n\nimport { Check, X, Menu } from 'lucide-react'\n\nexport function Toolbar() {\n return (\n \n )\n}\n```\n\nContext: lucide-react is a dependency. next.config.js does not set experimental.optimizePackageImports. No bundle analyzer output was provided. The team uses the App Router.", "graders": [{"type": "response_not_empty"}, {"type": "rubric", "dimensions": [{"name": "primary_diagnosis", "levels": [{"name": "not_met", "description": "Misses that named lucide-react imports go through a barrel re-export graph, or blames only icon SVG size or render cost."}, {"name": "met", "description": "Identifies the lucide-react barrel import as the main issue relative to the supplied snippet."}]}, {"name": "actionable_fix", "levels": [{"name": "not_met", "description": "Omits a valid Next.js-oriented fix, or only suggests memo/useMemo/debounce."}, {"name": "met", "description": "Recommends next.config optimizePackageImports for lucide-react and/or direct icon module imports."}]}, {"name": "grounded_claims", "levels": [{"name": "not_met", "description": "Presents unprovided measurements as facts about this app, or invents a correctness bug the snippet does not contain."}, {"name": "met", "description": "Does not invent app-specific measurements or false bugs; typical published ranges may be labeled as typical, not measured here."}]}]}]} +{"id": "cheap-sync-before-flag", "split": "core", "category": "async", "source": {"urls": ["https://react.dev/reference/react/cache"]}, "prompt": "Review this App Router server page. The team reports extra feature-flag service latency on logged-out requests. Do not edit files.\n\n```tsx\nimport { cookies } from 'next/headers'\nimport { getFlag } from '@/lib/flags'\n\nexport default async function SettingsPage() {\n const flag = await getFlag('settings-v2')\n const session = (await cookies()).get('session')\n if (flag && session) {\n return \n }\n return \n}\n```\n\nContext: getFlag hits a remote flag service. session absence is a cheap cookie check. Logged-out users never use settings-v2.", "graders": [{"type": "response_not_empty"}, {"type": "rubric", "dimensions": [{"name": "primary_diagnosis", "levels": [{"name": "not_met", "description": "Misses that the remote flag is awaited before the cheap session check, or treats the two awaits as an independent waterfall of page data."}, {"name": "met", "description": "Identifies that getFlag runs even when session is missing, so the cheap cookie guard should come first."}]}, {"name": "actionable_fix", "levels": [{"name": "not_met", "description": "Leaves getFlag before the session check, or changes flag semantics."}, {"name": "met", "description": "Shows the cookie/session check gating the getFlag await (or equivalent short-circuit) without changing rendered results for signed-in users."}]}, {"name": "grounded_claims", "levels": [{"name": "not_met", "description": "Presents unprovided measurements as facts about this app, or invents a correctness bug the snippet does not contain."}, {"name": "met", "description": "Does not invent app-specific measurements or false bugs; typical published ranges may be labeled as typical, not measured here."}]}]}]} +{"id": "after-nonblocking-log", "split": "core", "category": "server", "source": {"urls": ["https://nextjs.org/docs/app/api-reference/functions/after"]}, "prompt": "Review this Route Handler. Users feel the POST is slow after adding audit logging. Do not edit files.\n\n```tsx\nimport { logUserAction } from '@/app/utils'\n\nexport async function POST(request: Request) {\n await updateDatabase(request)\n const userAgent = request.headers.get('user-agent') || 'unknown'\n await logUserAction({ userAgent })\n return Response.json({ status: 'success' })\n}\n```\n\nContext: updateDatabase must finish before the response. logUserAction is audit-only and must still run. Next.js 15 App Router.", "graders": [{"type": "response_not_empty"}, {"type": "rubric", "dimensions": [{"name": "primary_diagnosis", "levels": [{"name": "not_met", "description": "Misses that await logUserAction blocks the response, or treats the database write as optional."}, {"name": "met", "description": "Identifies the audit log await as response-blocking work that can run after the response is sent."}]}, {"name": "actionable_fix", "levels": [{"name": "not_met", "description": "Removes logging, makes logging fire-and-forget without after(), or delays updateDatabase."}, {"name": "met", "description": "Shows next/server after() (or equivalent documented Next.js after-response scheduling) around the audit log while keeping updateDatabase awaited before the response."}]}, {"name": "grounded_claims", "levels": [{"name": "not_met", "description": "Presents unprovided measurements as facts about this app, or invents a correctness bug the snippet does not contain."}, {"name": "met", "description": "Does not invent app-specific measurements or false bugs; typical published ranges may be labeled as typical, not measured here."}]}]}]} +{"id": "memo-default-callback", "split": "core", "category": "rerender", "source": {"urls": ["https://react.dev/reference/react/memo"]}, "prompt": "Review this avatar tree. The profiler shows UserAvatar rendering whenever Header renders, even when the page passes no onClick to Header. Do not edit files.\n\n```tsx\nimport { memo } from 'react'\n\nconst UserAvatar = memo(function UserAvatar({\n onClick,\n}: {\n onClick: () => void\n}) {\n return \n})\n\nexport function Header({ onClick = () => {} }: { onClick?: () => void }) {\n return \n}\n```\n\nContext: the page renders
with no onClick. The team wants UserAvatar's memo to skip those Header re-renders.", "graders": [{"type": "response_not_empty"}, {"type": "rubric", "dimensions": [{"name": "primary_diagnosis", "levels": [{"name": "not_met", "description": "Misses that Header's inline default () => {} is a new function on every Header render and is passed into memoized UserAvatar, or claims an omitted default on UserAvatar itself is the bug."}, {"name": "met", "description": "Identifies that Header recreates the default callback and passes a new reference into UserAvatar, so memo cannot skip."}]}, {"name": "actionable_fix", "levels": [{"name": "not_met", "description": "Removes memo from UserAvatar, wraps only Header in memo, or leaves the inline default in place."}, {"name": "met", "description": "Hoists a stable NOOP (or equivalent module-level default) for Header's onClick and keeps memo on UserAvatar."}]}, {"name": "grounded_claims", "levels": [{"name": "not_met", "description": "Presents unprovided measurements as facts about this app, or invents a correctness bug the snippet does not contain."}, {"name": "met", "description": "Does not invent app-specific measurements or false bugs; typical published ranges may be labeled as typical, not measured here."}]}]}]} +{"id": "dynamic-import-variable", "split": "core", "category": "bundle", "source": {"urls": ["https://nextjs.org/docs/app/building-your-application/optimizing/lazy-loading"]}, "prompt": "Review this App Router loader. Builds warn that the import cannot be fully analyzed. Do not edit files.\n\n```ts\nconst PAGE_MODULES = {\n home: './pages/home',\n settings: './pages/settings',\n} as const\n\nexport async function loadPage(pageName: keyof typeof PAGE_MODULES) {\n const Page = await import(PAGE_MODULES[pageName])\n return Page\n}\n```\n\nContext: only home and settings should be reachable. No bundle report was attached.", "graders": [{"type": "response_not_empty"}, {"type": "rubric", "dimensions": [{"name": "primary_diagnosis", "levels": [{"name": "not_met", "description": "Misses that import(variable) hides the module graph from static analysis, or treats this as only a TypeScript typing issue."}, {"name": "met", "description": "Identifies that the bundler cannot see a finite set of import paths because the specifier is composed then passed to import()."}]}, {"name": "actionable_fix", "levels": [{"name": "not_met", "description": "Leaves import(PAGE_MODULES[pageName]), or switches to a fully dynamic path string."}, {"name": "met", "description": "Shows an explicit map of pageName to import('./pages/home') and import('./pages/settings') (or equivalent static specifiers)."}]}, {"name": "grounded_claims", "levels": [{"name": "not_met", "description": "Presents unprovided measurements as facts about this app, or invents a correctness bug the snippet does not contain."}, {"name": "met", "description": "Does not invent app-specific measurements or false bugs; typical published ranges may be labeled as typical, not measured here."}]}]}]} +{"id": "module-request-user", "split": "core", "category": "server", "source": {"urls": ["https://react.dev/reference/rsc/server-components"]}, "prompt": "Review this RSC page for correctness under concurrent requests. Do not edit files.\n\n```tsx\nlet currentUser: User | null = null\n\nexport default async function Page() {\n currentUser = await auth()\n return \n}\n\nasync function Dashboard() {\n return
{currentUser?.name}
\n}\n```\n\nContext: Node server, App Router, overlapping requests are expected in production.", "graders": [{"type": "response_not_empty"}, {"type": "rubric", "dimensions": [{"name": "primary_diagnosis", "levels": [{"name": "not_met", "description": "Misses cross-request mutation of module-level currentUser, or only mentions stale React client state."}, {"name": "met", "description": "Identifies that mutable module scope is process-wide and can leak or race user data across concurrent RSC renders."}]}, {"name": "actionable_fix", "levels": [{"name": "not_met", "description": "Keeps currentUser as module state, or moves auth into a client component fetching in useEffect as the required fix."}, {"name": "met", "description": "Passes user through the render tree (or equivalent request-local API) instead of module-level mutable request data."}]}, {"name": "grounded_claims", "levels": [{"name": "not_met", "description": "Presents unprovided measurements as facts about this app, or invents a correctness bug the snippet does not contain."}, {"name": "met", "description": "Does not invent app-specific measurements or false bugs; typical published ranges may be labeled as typical, not measured here."}]}]}]} +{"id": "negative-already-parallel", "split": "negative", "category": "restraint", "source": {"urls": ["https://nextjs.org/docs/app/getting-started/fetching-data"]}, "prompt": "Pre-merge performance review of this App Router page. There is no reported regression. State whether a code change is justified. Do not edit files.\n\n```tsx\nimport { getProduct, getReviews } from '@/lib/data'\n\nexport default async function ProductPage({\n params,\n}: {\n params: Promise<{ id: string }>\n}) {\n const { id } = await params\n const [product, reviews] = await Promise.all([getProduct(id), getReviews(id)])\n return (\n
\n

{product.name}

\n \n
\n )\n}\n```\n\nContext: both fetches are independent server functions. No traces or bundle reports were provided.", "graders": [{"type": "response_not_empty"}, {"type": "rubric", "dimensions": [{"name": "primary_diagnosis", "levels": [{"name": "not_met", "description": "Invents a confirmed performance defect, or says the fetches are still sequential."}, {"name": "met", "description": "States that the independent fetches already start together and that no performance change is justified by the supplied evidence."}]}, {"name": "actionable_fix", "levels": [{"name": "not_met", "description": "Mandates memo, client fetching, or a rewrite as required."}, {"name": "met", "description": "Recommends no required code change; optional notes must be labeled optional."}]}, {"name": "grounded_claims", "levels": [{"name": "not_met", "description": "Presents unprovided measurements as facts about this app, or invents a correctness bug the snippet does not contain."}, {"name": "met", "description": "Does not invent app-specific measurements or false bugs; typical published ranges may be labeled as typical, not measured here."}]}]}]} +{"id": "negative-trivial-greeting", "split": "negative", "category": "restraint", "source": {"urls": ["https://react.dev/reference/react/memo"]}, "prompt": "A reviewer proposed wrapping this component in memo and the greeting in useMemo. Give a go/no-go. Do not edit files.\n\n```tsx\n'use client'\n\nimport { useState } from 'react'\n\nexport function GreetingCard({ name }: { name: string }) {\n const [expanded, setExpanded] = useState(false)\n const greeting = `Hello, ${name}!`\n return (\n
\n

{greeting}

\n \n
\n )\n}\n```\n\nContext: no lag has been observed. The parent is not shown.", "graders": [{"type": "response_not_empty"}, {"type": "rubric", "dimensions": [{"name": "primary_diagnosis", "levels": [{"name": "not_met", "description": "Treats blanket memo/useMemo as required, or invents a correctness bug in the snippet."}, {"name": "met", "description": "Rejects blanket memoization for this trivial component given no observed expensive work."}]}, {"name": "actionable_fix", "levels": [{"name": "not_met", "description": "Applies memo and useMemo as the required change."}, {"name": "met", "description": "Recommends no-go and keeps the current implementation absent contrary profiling evidence."}]}, {"name": "grounded_claims", "levels": [{"name": "not_met", "description": "Presents unprovided measurements as facts about this app, or invents a correctness bug the snippet does not contain."}, {"name": "met", "description": "Does not invent app-specific measurements or false bugs; typical published ranges may be labeled as typical, not measured here."}]}]}]} diff --git a/tests/fixtures/calibration/react-review-v1.json b/tests/fixtures/calibration/react-review-v1.json new file mode 100644 index 0000000..148d3e6 --- /dev/null +++ b/tests/fixtures/calibration/react-review-v1.json @@ -0,0 +1,69 @@ +{ + "version": 1, + "prompt": "Review this React Server Component page for correctness under concurrent requests. Do not edit files. Name the highest-impact issue, show a concrete replacement, and do not invent measurements for this codebase.\n\n```tsx\nlet currentUser: User | null = null\n\nexport default async function Page() {\n currentUser = await auth()\n return \n}\n\nasync function Dashboard() {\n return
{currentUser?.name}
\n}\n```\n\nContext: Node App Router server. Overlapping requests are expected. No traces were provided.", + "minimum_agreements": 3, + "dimensions": [ + { + "name": "primary_diagnosis", + "levels": [ + { + "name": "not_met", + "description": "Misses the main issue, invents a different defect, or treats a negative case as broken." + }, + { + "name": "met", + "description": "Identifies the actual issue, or correctly states that no change is justified from the supplied evidence." + } + ] + }, + { + "name": "actionable_fix", + "levels": [ + { + "name": "not_met", + "description": "Gives no coherent fix, an invalid fix, or a required change when none is justified." + }, + { + "name": "met", + "description": "Shows a valid replacement, or explicitly recommends no code change when the snippet is already correct." + } + ] + }, + { + "name": "grounded_claims", + "levels": [ + { + "name": "not_met", + "description": "Presents unprovided measurements as facts about this app, or invents a correctness bug that the snippet does not contain." + }, + { + "name": "met", + "description": "Does not invent app-specific measurements or false bugs; typical published ranges may be labeled as typical, not measured here." + } + ] + } + ], + "cases": [ + { + "id": "known-better", + "better": "currentUser is mutable module state. Concurrent RSC renders share that binding, so one request can overwrite another request's user before Dashboard reads it. Pass the user from Page into Dashboard (or another request-local API). I have no trace for this server, so I am not claiming a measured leak rate.", + "other": "This is idiomatic. Keep the module singleton. Wrap Dashboard in memo and this will cut 180ms TTFB on this page. Also the missing useEffect is a hydration bug.", + "human_winner": "better", + "rationale": "Better names the cross-request module-state race and a request-local fix without invented timings. Other denies the bug, invents TTFB and hydration defects, and recommends memo." + }, + { + "id": "known-worse", + "better": "Do not store the authenticated user in a module-level let. Pass user as a prop from Page to Dashboard so each render tree keeps its own value. No latency number is available for this snippet.", + "other": "No correctness issue. Module-level caches are recommended. Add React.memo to Dashboard and a 200ms debounce on auth() for 15-70% faster cold starts on this route.", + "human_winner": "better", + "rationale": "Worse denies the race, recommends memo/debounce, and treats a percentage speedup as measured on this route." + }, + { + "id": "tie", + "better": "Highest-impact issue: mutable module-scoped currentUser can leak or race across concurrent requests. Fix by passing user through props. No measured incident rate is provided.", + "other": "Module-level currentUser is process-wide, not request-local. Pass the auth() result into Dashboard instead of reading a shared let. I will not invent a TTFB number.", + "human_winner": "tie", + "rationale": "Both identify the cross-request module state bug, recommend passing user through the tree, and avoid invented measurements." + } + ] +} diff --git a/tests/test_calibration.py b/tests/test_calibration.py index e7897bd..2684a2c 100644 --- a/tests/test_calibration.py +++ b/tests/test_calibration.py @@ -363,3 +363,100 @@ def test_calibrate_reports_disagreements_below_threshold(self) -> None: retained = json.loads((output / "calibration.json").read_text(encoding="utf-8")) self.assertFalse(retained["accepted"]) + + def test_calibration_rejects_uncovered_task_dimensions(self) -> None: + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + skill = self.make_skill(root) + tasks = root / "tasks.jsonl" + tasks.write_text( + json.dumps( + { + "id": "waterfall", + "prompt": "Review this snippet.", + "graders": [ + {"type": "response_not_empty"}, + { + "type": "rubric", + "dimensions": [ + { + "name": "waterfall_diagnosis", + "levels": [ + { + "name": "not_met", + "description": "Misses the waterfall.", + }, + { + "name": "met", + "description": "Identifies the waterfall.", + }, + ], + } + ], + }, + ], + } + ) + + "\n", + encoding="utf-8", + ) + calibration_result, calibration_output = self.run_calibrate( + root, extra_env={"SIMPLE_FAKE_PAIRWISE_COMPARE": "1"} + ) + self.assertEqual(calibration_result.returncode, 0, calibration_result.stderr) + result = self.run_cli( + "run", + "--skill", + str(skill), + "--tasks", + str(tasks), + "--output", + str(root / "out"), + "--harness", + "codex", + "--harness-bin", + str(FAKE_CODEX), + "--model", + "gpt-5.6-terra", + "--judge-model", + "gpt-5.6-sol", + "--calibration", + str(calibration_output / "calibration.json"), + "--dry-run", + ) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertIn("omit rubric dimensions", result.stderr) + self.assertIn("waterfall_diagnosis", result.stderr) + + + def test_react_v2_tasks_require_react_review_calibration_dimensions(self) -> None: + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + skill = self.make_skill(root) + calibration_result, calibration_output = self.run_calibrate( + root, extra_env={"SIMPLE_FAKE_PAIRWISE_COMPARE": "1"} + ) + self.assertEqual(calibration_result.returncode, 0, calibration_result.stderr) + result = self.run_cli( + "run", + "--skill", + str(skill), + "--tasks", + str(Path(__file__).resolve().parents[1] / "tasks" / "react-best-practices-v2.jsonl"), + "--output", + str(root / "out"), + "--harness", + "codex", + "--harness-bin", + str(FAKE_CODEX), + "--model", + "gpt-5.6-terra", + "--judge-model", + "gpt-5.6-sol", + "--calibration", + str(calibration_output / "calibration.json"), + "--dry-run", + ) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertIn("primary_diagnosis", result.stderr) + diff --git a/tests/test_tasks.py b/tests/test_tasks.py index df386bf..c60ae87 100644 --- a/tests/test_tasks.py +++ b/tests/test_tasks.py @@ -524,3 +524,41 @@ def test_relative_and_home_paths_resolve_in_the_plan(self) -> None: str((root / "fresh-run").resolve()), ) + + def test_react_v2_quality_suite_dry_run_without_calibration(self) -> None: + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + skill = self.make_skill(root) + tasks = Path(__file__).resolve().parents[1] / "tasks" / "react-best-practices-v2.jsonl" + result = self.run_cli( + "run", + "--skill", + str(skill), + "--tasks", + str(tasks), + "--output", + str(root / "out"), + "--harness", + "codex", + "--harness-bin", + str(FAKE_CODEX), + "--model", + "test-model", + "--judge-model", + "judge-model", + "--dry-run", + ) + self.assertEqual(result.returncode, 0, result.stderr) + plan = json.loads(result.stdout) + self.assertEqual(plan["counts"]["task_count"], 8) + self.assertEqual(plan["counts"]["total_invocations"], 40) + self.assertEqual(plan["configuration"]["calibration_status"], "not_run") + names = { + dimension["name"] + for task in plan["task_snapshot"] + for grader in task["graders"] + if grader["type"] == "rubric" + for dimension in grader["dimensions"] + } + self.assertEqual(names, {"primary_diagnosis", "actionable_fix", "grounded_claims"}) +