From 293e54cc887fd2798ec26fd6db1079a8e6200756 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Wed, 28 Oct 2026 15:56:41 -0700 Subject: [PATCH 1/6] Add Smallest complete change rule to /go, with eval cases /go's single pass follows the rule, and the stage loop passes it to each stage implementer alongside the judgment-call rule. review-code --fix agents still get neither. ai/evals/smallest-change is an eval-only plugin for `claude plugin eval`. The with arm loads a SessionStart hook that prints rule.md, a copy of the rule that ai/tests/test-eval-rule-sync.sh keeps in sync. The without arm is the tool's no-plugin baseline, so the rule is the only difference. Three scaffolded Python cases grade with hidden tests, a production-line limit, a dependency check, and (in two cases) a check that the change uses the stdlib module or existing helper. Claude-Session: https://claude.ai/code/session_01AovXvYri48wFhqGV3epaMi --- .github/workflows/test.yml | 1 + ai/README.md | 5 ++ .../.claude-plugin/plugin.json | 5 ++ ai/evals/smallest-change/README.md | 47 +++++++++++ ai/evals/smallest-change/evals/.gitignore | 1 + .../evals/csv-export/case.yaml | 33 ++++++++ .../evals/csv-export/grade.json | 6 ++ .../evals/csv-export/hidden/test_hidden.py | 41 +++++++++ .../evals/csv-export/repo/app/__init__.py | 0 .../evals/csv-export/repo/app/report.py | 12 +++ .../evals/csv-export/repo/pyproject.toml | 5 ++ .../evals/csv-export/repo/tests/__init__.py | 0 .../csv-export/repo/tests/test_report.py | 15 ++++ .../evals/csv-export/scaffold.sh | 3 + ai/evals/smallest-change/evals/grade.py | 84 +++++++++++++++++++ .../evals/permalink-reuse/case.yaml | 33 ++++++++ .../evals/permalink-reuse/grade.json | 6 ++ .../permalink-reuse/hidden/test_hidden.py | 26 ++++++ .../permalink-reuse/repo/blog/__init__.py | 0 .../evals/permalink-reuse/repo/blog/models.py | 15 ++++ .../evals/permalink-reuse/repo/blog/text.py | 15 ++++ .../evals/permalink-reuse/repo/pyproject.toml | 5 ++ .../permalink-reuse/repo/tests/__init__.py | 0 .../permalink-reuse/repo/tests/test_models.py | 15 ++++ .../permalink-reuse/repo/tests/test_text.py | 17 ++++ .../evals/permalink-reuse/scaffold.sh | 3 + ai/evals/smallest-change/evals/scaffold.sh | 44 ++++++++++ .../evals/shared-bug-fix/case.yaml | 28 +++++++ .../evals/shared-bug-fix/grade.json | 3 + .../shared-bug-fix/hidden/test_hidden.py | 29 +++++++ .../evals/shared-bug-fix/repo/pyproject.toml | 5 ++ .../shared-bug-fix/repo/shop/__init__.py | 0 .../evals/shared-bug-fix/repo/shop/cart.py | 15 ++++ .../evals/shared-bug-fix/repo/shop/money.py | 10 +++ .../evals/shared-bug-fix/repo/shop/refunds.py | 9 ++ .../shared-bug-fix/repo/tests/__init__.py | 0 .../shared-bug-fix/repo/tests/test_shop.py | 24 ++++++ .../evals/shared-bug-fix/scaffold.sh | 3 + ai/evals/smallest-change/hooks/hooks.json | 14 ++++ ai/evals/smallest-change/rule.md | 15 ++++ ai/skills/go/SKILL.md | 16 ++++ ai/tests/test-eval-rule-sync.sh | 24 ++++++ 42 files changed, 632 insertions(+) create mode 100644 ai/evals/smallest-change/.claude-plugin/plugin.json create mode 100644 ai/evals/smallest-change/README.md create mode 100644 ai/evals/smallest-change/evals/.gitignore create mode 100644 ai/evals/smallest-change/evals/csv-export/case.yaml create mode 100644 ai/evals/smallest-change/evals/csv-export/grade.json create mode 100644 ai/evals/smallest-change/evals/csv-export/hidden/test_hidden.py create mode 100644 ai/evals/smallest-change/evals/csv-export/repo/app/__init__.py create mode 100644 ai/evals/smallest-change/evals/csv-export/repo/app/report.py create mode 100644 ai/evals/smallest-change/evals/csv-export/repo/pyproject.toml create mode 100644 ai/evals/smallest-change/evals/csv-export/repo/tests/__init__.py create mode 100644 ai/evals/smallest-change/evals/csv-export/repo/tests/test_report.py create mode 100755 ai/evals/smallest-change/evals/csv-export/scaffold.sh create mode 100644 ai/evals/smallest-change/evals/grade.py create mode 100644 ai/evals/smallest-change/evals/permalink-reuse/case.yaml create mode 100644 ai/evals/smallest-change/evals/permalink-reuse/grade.json create mode 100644 ai/evals/smallest-change/evals/permalink-reuse/hidden/test_hidden.py create mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/blog/__init__.py create mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/blog/models.py create mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/blog/text.py create mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/pyproject.toml create mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/tests/__init__.py create mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_models.py create mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_text.py create mode 100755 ai/evals/smallest-change/evals/permalink-reuse/scaffold.sh create mode 100755 ai/evals/smallest-change/evals/scaffold.sh create mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/case.yaml create mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/grade.json create mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/hidden/test_hidden.py create mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/pyproject.toml create mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/__init__.py create mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/cart.py create mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/money.py create mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/refunds.py create mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/__init__.py create mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/test_shop.py create mode 100755 ai/evals/smallest-change/evals/shared-bug-fix/scaffold.sh create mode 100644 ai/evals/smallest-change/hooks/hooks.json create mode 100644 ai/evals/smallest-change/rule.md create mode 100755 ai/tests/test-eval-rule-sync.sh diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f4ff95d6..b24c77ca 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -23,6 +23,7 @@ jobs: ai/skills/wait-for-pr-reviews/scripts/tests/test-portable-skill.sh ai/skills/address-pr-reviews/scripts/tests/test-portable-skill.sh ai/tests/test-plain-writing-contract.sh + ai/tests/test-eval-rule-sync.sh ai/tests/test-ai-installers.sh ai/tests/test-install-claude-automation.sh ai/tests/test-lint-edited-file.sh diff --git a/ai/README.md b/ai/README.md index a4c61038..9ca95bd3 100644 --- a/ai/README.md +++ b/ai/README.md @@ -50,6 +50,10 @@ CI enforces all three rules: the same script with `--check` rejects a copy that When `implementation-planner` marks a plan with two or more stages `**Execution**: staged`, `go` implements it one stage at a time. The planner marks a plan staged only when one session would run out of context before it finished the work, and `go --staged` or `--single-pass` overrides its choice. A fresh agent writes each stage, a second fresh agent checks it against the stage's success criteria and `Check` command, and the parent commits it, so an interrupted run resumes in the stage it stopped in. `implementation-planner` writes the `Check` command and a `## Decisions` section that the stage agents follow. +## Evals + +`ai/evals/` holds `claude plugin eval` suites. [`ai/evals/smallest-change/`](evals/smallest-change/README.md) compares implementation with and without the Smallest complete change rule that `go` gives its implementers. + ## Fresh review processes `go` supports Claude Code and Codex. It passes its current harness explicitly to `skills/go/scripts/run-review.py`, which starts a fresh review in the same checkout. Claude uses print mode; Codex uses `exec`. Both save the review result under the checkout's `.notes/` directory so the parent can resume at validation after a context clear. The runner requires Python 3.11 or later and the selected CLI on `PATH`; it never substitutes the other harness. The installed `review-code` skill must support `REVIEW_CODE_REVIEW_DIR`; update it from its source repository with `bin/setup` if the runner reports an older installation. Review sessions, reports, temporary checkouts, and hook files stay under each attempt in `.notes/go-reviews/`. The Codex child receives no write grants for shared review caches. After success, the parent copies the report into the shared review history, where it survives removal of the implementation worktree. A Claude review runs on the accounts that docket's `config.toml` names in `claude_config_dir` and `claude_fallback_config_dirs`, in the order docket's rotation picks them. The runner skips each account that a usage limit in docket's `limits.jsonl` still applies to. A limit applies to every config directory signed in to the login (one account in one organization) that docket recorded it with. `run-review.py accounts` prints that rotation, the limits docket recorded, and the order a review would try the accounts in now, as JSON, and `usage-all` shows it below each account's usage. Without a docket config, the review runs on the current account. The runner refuses an account that is not signed in or has no `review-code` skill. When an account hits its usage limit before the review edits any files, the runner reviews again on the next account that is signed in to a different login, and then on the current account if the list leaves it out. A run with `--since-run ` copies that earlier run's report into its own attempt and asks `review-code` for an `--append` review of the changes since that report. @@ -83,6 +87,7 @@ ai/skills/wait-for-pr-reviews/scripts/tests/test-pending-reviews.sh ai/skills/wait-for-pr-reviews/scripts/tests/test-portable-skill.sh ai/skills/address-pr-reviews/scripts/tests/test-portable-skill.sh ai/tests/test-plain-writing-contract.sh +ai/tests/test-eval-rule-sync.sh python3 ai/skills/plain-writing/scripts/tests/test_plain_writing_lint.py ai/tests/test-ai-installers.sh ai/tests/test-command-log.sh diff --git a/ai/evals/smallest-change/.claude-plugin/plugin.json b/ai/evals/smallest-change/.claude-plugin/plugin.json new file mode 100644 index 00000000..be50ce30 --- /dev/null +++ b/ai/evals/smallest-change/.claude-plugin/plugin.json @@ -0,0 +1,5 @@ +{ + "name": "smallest-change-eval", + "version": "0.1.0", + "description": "Eval-only plugin that adds /go's Smallest complete change rule to the session context, so claude plugin eval can compare implementation with and without it." +} diff --git a/ai/evals/smallest-change/README.md b/ai/evals/smallest-change/README.md new file mode 100644 index 00000000..425becb1 --- /dev/null +++ b/ai/evals/smallest-change/README.md @@ -0,0 +1,47 @@ +# Smallest complete change eval + +An eval-only plugin that measures what the Smallest complete change rule in `ai/skills/go/SKILL.md` does to a plain implementation task. It never runs /go, so a run opens no PR and requests no review. + +## What each arm loads + +- **With:** this plugin. Its only content is a `SessionStart` hook that prints `rule.md` into the session context. The plugin has no skill, so the rule is in context on every run, and the result does not depend on whether the model chooses to invoke a skill. /go gives its implementers this rule together with the Judgment calls rule, but this arm gets this rule alone, so the "easy-to-reverse call" that item 1 mentions has no definition here. +- **Without:** the baseline arm that `--ablation with-without` adds. It runs the same prompt in the same scaffolded workspace with no plugin loaded, so the rule is the only difference between the arms. + +`rule.md` is the rule from `ai/skills/go/SKILL.md` without its first sentence, which tells /go which agents get the rule. `ai/tests/test-eval-rule-sync.sh` fails when the two copies differ. + +## Cases + +Each case is a small Python repo where an over-built answer is tempting. + +| Case | Task | What over-building looks like | +| --- | --- | --- | +| `csv-export` | Add `write_csv(rows, path)` | Hand-rolled quoting, or delimiter and encoding options nobody asked for | +| `permalink-reuse` | Add `Post.permalink` | A new slug function when `blog/text.py` already has `slugify` | +| `shared-bug-fix` | Fix a thousands-separator bug reported against the cart | Patching only the cart, when the refund path calls the same parser | + +`shared-bug-fix` guards the other direction: the rule must shrink the solution, not the scope, so a caller-only patch fails its hidden tests. + +## How grading works + +`claude plugin eval` has no grader that runs a command, so the grading runs inside the workspace. `evals/scaffold.sh` copies the case's `repo/` into the workspace and commits it. It also puts the hidden tests and `grade.py` in `.eval/` and writes a `Stop` hook to `.claude/settings.json`. When the session ends, the hook runs `grade.py`, which writes `eval-result.txt`. The case's regex graders read that file: + +- `hidden_tests: pass` when the hidden tests and the repo's own tests pass. This grader has weight 2. +- `size: ok` when the production lines added or removed stay within the case's `max_lines` in `grade.json`. Lines under `tests/` and in `test_*` files do not count. +- `dependencies: unchanged` when no dependency file (`pyproject.toml`, `requirements*.txt`, `setup.py`, `setup.cfg`, `Pipfile`) changed. +- `expected_code: ok` (`csv-export` and `permalink-reuse` only) when the change adds a line that uses the `csv` module or `slugify`. + +`eval-result.txt` also records the exact line count and the changed files, which `--keep-temp` leaves on disk for a closer look. + +## Running it + +Each run is a full Claude session, so the full suite (3 cases × 3 runs × 2 arms) is 18 sessions. Start with one case and one run: + +```bash +claude plugin eval ai/evals/smallest-change --scaffold --allow-tools Edit Write Bash --case csv-export --runs 1 --max-cost-usd 1 --no-publish +``` + +`--scaffold` is required, because without it the workspace is empty. `--allow-tools` grants the tools the cases list. `--max-cost-usd 0` loads and plans every case without starting a run, which checks that the cases parse. + +The hidden tests sit in the workspace's `.eval/` folder. Git ignores that folder, but the agent can still find and read it in either arm. + +The graders depend on the eval run loading the workspace's `.claude/settings.json`, which the eval docs do not cover. If every grader fails in both arms and `--keep-temp` shows no `eval-result.txt`, the `Stop` hook did not run. diff --git a/ai/evals/smallest-change/evals/.gitignore b/ai/evals/smallest-change/evals/.gitignore new file mode 100644 index 00000000..fbca2253 --- /dev/null +++ b/ai/evals/smallest-change/evals/.gitignore @@ -0,0 +1 @@ +results/ diff --git a/ai/evals/smallest-change/evals/csv-export/case.yaml b/ai/evals/smallest-change/evals/csv-export/case.yaml new file mode 100644 index 00000000..e8f0d840 --- /dev/null +++ b/ai/evals/smallest-change/evals/csv-export/case.yaml @@ -0,0 +1,33 @@ +schema_version: "1.1" +name: csv-export +description: Writing a CSV export, where the standard library's csv module beats hand-rolled quoting and unrequested options. +context: + scaffold_script: scaffold.sh +execution: + max_turns: 25 + timeout_seconds: 300 + allowed_tools: [Read, Glob, Grep, Edit, Write, Bash] + prompt: | + Add a `write_csv(rows, path)` function to `app/report.py`. It writes the rows that `build_rows` returns to a CSV file at `path`: a header row with the column names, then one line per row. Customer names come from a signup form, so they can contain any text. +graders: + - name: hidden-tests + type: regex + target: {source: file, path: eval-result.txt} + pattern: "^hidden_tests: pass$" + flags: m + weight: 2 + - name: diff-size + type: regex + target: {source: file, path: eval-result.txt} + pattern: "^size: ok$" + flags: m + - name: no-new-dependency + type: regex + target: {source: file, path: eval-result.txt} + pattern: "^dependencies: unchanged$" + flags: m + - name: uses-csv-module + type: regex + target: {source: file, path: eval-result.txt} + pattern: "^expected_code: ok$" + flags: m diff --git a/ai/evals/smallest-change/evals/csv-export/grade.json b/ai/evals/smallest-change/evals/csv-export/grade.json new file mode 100644 index 00000000..be557a14 --- /dev/null +++ b/ai/evals/smallest-change/evals/csv-export/grade.json @@ -0,0 +1,6 @@ +{ + "max_lines": 15, + "require_added": { + "app/report.py": "\\bcsv\\." + } +} diff --git a/ai/evals/smallest-change/evals/csv-export/hidden/test_hidden.py b/ai/evals/smallest-change/evals/csv-export/hidden/test_hidden.py new file mode 100644 index 00000000..be8af88a --- /dev/null +++ b/ai/evals/smallest-change/evals/csv-export/hidden/test_hidden.py @@ -0,0 +1,41 @@ +import csv +import os +import tempfile +import unittest +from decimal import Decimal + +from app.report import write_csv + + +def read_back(path): + with open(path, newline="") as f: + return list(csv.reader(f)) + + +class WriteCsvTest(unittest.TestCase): + def setUp(self): + handle, self.path = tempfile.mkstemp(suffix=".csv") + os.close(handle) + self.addCleanup(os.remove, self.path) + + def test_writes_header_then_rows(self): + write_csv([{"order_id": 1, "customer": "Ada", "total": Decimal("6.00")}], self.path) + + self.assertEqual(read_back(self.path), [["order_id", "customer", "total"], ["1", "Ada", "6.00"]]) + + def test_quotes_commas_quotes_and_newlines_in_customer_names(self): + names = ['Smith, Jo', 'The "Best" Shop', "Line one\nLine two"] + rows = [{"order_id": i, "customer": name, "total": Decimal("1.00")} for i, name in enumerate(names)] + + write_csv(rows, self.path) + + self.assertEqual([row[1] for row in read_back(self.path)[1:]], names) + + def test_writes_only_the_header_when_there_are_no_rows(self): + write_csv([], self.path) + + self.assertEqual(read_back(self.path), [["order_id", "customer", "total"]]) + + +if __name__ == "__main__": + unittest.main() diff --git a/ai/evals/smallest-change/evals/csv-export/repo/app/__init__.py b/ai/evals/smallest-change/evals/csv-export/repo/app/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/ai/evals/smallest-change/evals/csv-export/repo/app/report.py b/ai/evals/smallest-change/evals/csv-export/repo/app/report.py new file mode 100644 index 00000000..4e46e676 --- /dev/null +++ b/ai/evals/smallest-change/evals/csv-export/repo/app/report.py @@ -0,0 +1,12 @@ +from decimal import Decimal + +COLUMNS = ("order_id", "customer", "total") + + +def build_rows(orders): + """Returns one row per order, in the column order of COLUMNS.""" + rows = [] + for order in orders: + total = sum((Decimal(item["price"]) * item["quantity"] for item in order["items"]), Decimal("0")) + rows.append({"order_id": order["id"], "customer": order["customer"], "total": total}) + return rows diff --git a/ai/evals/smallest-change/evals/csv-export/repo/pyproject.toml b/ai/evals/smallest-change/evals/csv-export/repo/pyproject.toml new file mode 100644 index 00000000..2f5ab3a9 --- /dev/null +++ b/ai/evals/smallest-change/evals/csv-export/repo/pyproject.toml @@ -0,0 +1,5 @@ +[project] +name = "orders" +version = "0.1.0" +requires-python = ">=3.9" +dependencies = [] diff --git a/ai/evals/smallest-change/evals/csv-export/repo/tests/__init__.py b/ai/evals/smallest-change/evals/csv-export/repo/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/ai/evals/smallest-change/evals/csv-export/repo/tests/test_report.py b/ai/evals/smallest-change/evals/csv-export/repo/tests/test_report.py new file mode 100644 index 00000000..190a24a3 --- /dev/null +++ b/ai/evals/smallest-change/evals/csv-export/repo/tests/test_report.py @@ -0,0 +1,15 @@ +import unittest +from decimal import Decimal + +from app.report import build_rows + + +class BuildRowsTest(unittest.TestCase): + def test_totals_each_order(self): + orders = [{"id": 7, "customer": "Ada", "items": [{"price": "2.50", "quantity": 2}, {"price": "1.00", "quantity": 1}]}] + + self.assertEqual(build_rows(orders), [{"order_id": 7, "customer": "Ada", "total": Decimal("6.00")}]) + + +if __name__ == "__main__": + unittest.main() diff --git a/ai/evals/smallest-change/evals/csv-export/scaffold.sh b/ai/evals/smallest-change/evals/csv-export/scaffold.sh new file mode 100755 index 00000000..1aace498 --- /dev/null +++ b/ai/evals/smallest-change/evals/csv-export/scaffold.sh @@ -0,0 +1,3 @@ +#!/usr/bin/env bash +case_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +exec bash "${case_dir}/../scaffold.sh" "${case_dir}" diff --git a/ai/evals/smallest-change/evals/grade.py b/ai/evals/smallest-change/evals/grade.py new file mode 100644 index 00000000..a7e21412 --- /dev/null +++ b/ai/evals/smallest-change/evals/grade.py @@ -0,0 +1,84 @@ +"""Grades an eval workspace after the agent stops and writes eval-result.txt for the case's regex graders. + +Usage: grade.py + +grade.json in the same folder sets `max_lines`, the most production lines the change may add or remove, and optionally `require_added`, a map from a file path to a regex that some added line in that file must match. +""" + +import json +import os +import re +import subprocess +import sys +from pathlib import Path + +RESULT = "eval-result.txt" +DEPENDENCY_FILES = re.compile(r"(^|/)(pyproject\.toml|setup\.py|setup\.cfg|Pipfile|requirements[^/]*\.txt)$") + + +def git(root, *args): + return subprocess.run(["git", *args], cwd=root, capture_output=True, text=True, check=True).stdout + + +def is_test(path): + return path.startswith("tests/") or Path(path).name.startswith("test_") + + +def run_tests(root, start_dir, top_dir): + env = {**os.environ, "PYTHONPATH": str(root), "PYTHONDONTWRITEBYTECODE": "1"} + result = subprocess.run( + [sys.executable, "-m", "unittest", "discover", "-s", start_dir, "-t", top_dir], + cwd=root, + env=env, + capture_output=True, + text=True, + timeout=120, + ) + return result.returncode == 0, result.stderr[-2000:] + + +def main(): + root = Path(sys.argv[1]) + config = json.loads((Path(__file__).parent / "grade.json").read_text()) + base = (Path(__file__).parent / "base").read_text().strip() + + hidden_ok, hidden_log = run_tests(root, ".eval/hidden", ".eval/hidden") + repo_ok, repo_log = run_tests(root, "tests", ".") + + # Intent-to-add makes new files show up in `git diff` without staging their contents. + git(root, "add", "-N", "--", ".", f":!{RESULT}") + changed = [] + for line in git(root, "diff", "--numstat", base, "--", ".", f":!{RESULT}").splitlines(): + added, removed, path = line.split("\t", 2) + changed.append((path, int(added) if added != "-" else 0, int(removed) if removed != "-" else 0)) + + production_lines = sum(a + r for path, a, r in changed if not is_test(path)) + dependency_files = [path for path, _, _ in changed if DEPENDENCY_FILES.search(path)] + + lines = [ + f"hidden_tests: {'pass' if hidden_ok and repo_ok else 'fail'}", + f"size: {'ok' if production_lines <= config['max_lines'] else 'over'}", + f"production_lines_changed: {production_lines} (limit {config['max_lines']})", + f"dependencies: {'changed' if dependency_files else 'unchanged'}", + ] + if config.get("require_added"): + missing = [] + for path, pattern in config["require_added"].items(): + added_lines = [ + l[1:] for l in git(root, "diff", "-U0", base, "--", path).splitlines() + if l.startswith("+") and not l.startswith("+++") + ] + if not any(re.search(pattern, l) for l in added_lines): + missing.append(path) + lines.append(f"expected_code: {'missing in ' + ', '.join(missing) if missing else 'ok'}") + lines.append(f"changed_files: {', '.join(path for path, _, _ in changed) or 'none'}") + if not hidden_ok: + lines += ["", "hidden test output:", hidden_log] + if not repo_ok: + lines += ["", "repo test output:", repo_log] + + (root / RESULT).write_text("\n".join(lines) + "\n") + + +if __name__ == "__main__": + main() diff --git a/ai/evals/smallest-change/evals/permalink-reuse/case.yaml b/ai/evals/smallest-change/evals/permalink-reuse/case.yaml new file mode 100644 index 00000000..23423e67 --- /dev/null +++ b/ai/evals/smallest-change/evals/permalink-reuse/case.yaml @@ -0,0 +1,33 @@ +schema_version: "1.1" +name: permalink-reuse +description: Adding a URL path to a model, where the codebase already has a slugify helper that handles accents and punctuation. +context: + scaffold_script: scaffold.sh +execution: + max_turns: 25 + timeout_seconds: 300 + allowed_tools: [Read, Glob, Grep, Edit, Write, Bash] + prompt: | + Add a `permalink` property to `Post` in `blog/models.py`. It returns the post's URL path in the form `/blog////`, where the slug comes from the title. Titles can contain accented letters and punctuation. +graders: + - name: hidden-tests + type: regex + target: {source: file, path: eval-result.txt} + pattern: "^hidden_tests: pass$" + flags: m + weight: 2 + - name: diff-size + type: regex + target: {source: file, path: eval-result.txt} + pattern: "^size: ok$" + flags: m + - name: no-new-dependency + type: regex + target: {source: file, path: eval-result.txt} + pattern: "^dependencies: unchanged$" + flags: m + - name: reuses-slugify + type: regex + target: {source: file, path: eval-result.txt} + pattern: "^expected_code: ok$" + flags: m diff --git a/ai/evals/smallest-change/evals/permalink-reuse/grade.json b/ai/evals/smallest-change/evals/permalink-reuse/grade.json new file mode 100644 index 00000000..f4429e4b --- /dev/null +++ b/ai/evals/smallest-change/evals/permalink-reuse/grade.json @@ -0,0 +1,6 @@ +{ + "max_lines": 10, + "require_added": { + "blog/models.py": "\\bslugify\\b" + } +} diff --git a/ai/evals/smallest-change/evals/permalink-reuse/hidden/test_hidden.py b/ai/evals/smallest-change/evals/permalink-reuse/hidden/test_hidden.py new file mode 100644 index 00000000..c1d7076b --- /dev/null +++ b/ai/evals/smallest-change/evals/permalink-reuse/hidden/test_hidden.py @@ -0,0 +1,26 @@ +import unittest +from datetime import date + +from blog.models import Post + + +def permalink(title, published=date(2026, 3, 9)): + return Post(title=title, published=published, body="").permalink + + +class PermalinkTest(unittest.TestCase): + def test_builds_year_month_slug_path(self): + self.assertEqual(permalink("Hello World"), "/blog/2026/03/hello-world/") + + def test_drops_accents_and_punctuation(self): + self.assertEqual(permalink("Café déjà vu!"), "/blog/2026/03/cafe-deja-vu/") + + def test_collapses_repeated_separators(self): + self.assertEqual(permalink(" Rust -- & Go "), "/blog/2026/03/rust-go/") + + def test_pads_single_digit_months(self): + self.assertEqual(permalink("New Year", date(2027, 1, 1)), "/blog/2027/01/new-year/") + + +if __name__ == "__main__": + unittest.main() diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/__init__.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/models.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/models.py new file mode 100644 index 00000000..2e1d0186 --- /dev/null +++ b/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/models.py @@ -0,0 +1,15 @@ +from dataclasses import dataclass +from datetime import date + +from blog.text import truncate + + +@dataclass +class Post: + title: str + published: date + body: str + + @property + def excerpt(self): + return truncate(self.body, 140) diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/text.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/text.py new file mode 100644 index 00000000..1e289642 --- /dev/null +++ b/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/text.py @@ -0,0 +1,15 @@ +import re +import unicodedata + + +def slugify(value): + """Lowercases `value`, drops accents and punctuation, and joins the words with hyphens.""" + ascii_text = unicodedata.normalize("NFKD", value).encode("ascii", "ignore").decode("ascii") + return re.sub(r"[^a-z0-9]+", "-", ascii_text.lower()).strip("-") + + +def truncate(value, length): + """Cuts `value` to at most `length` characters at a word boundary and adds an ellipsis when it cuts.""" + if len(value) <= length: + return value + return value[: length - 1].rsplit(" ", 1)[0] + "…" diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/pyproject.toml b/ai/evals/smallest-change/evals/permalink-reuse/repo/pyproject.toml new file mode 100644 index 00000000..bb7f52a8 --- /dev/null +++ b/ai/evals/smallest-change/evals/permalink-reuse/repo/pyproject.toml @@ -0,0 +1,5 @@ +[project] +name = "blog" +version = "0.1.0" +requires-python = ">=3.9" +dependencies = [] diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/__init__.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_models.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_models.py new file mode 100644 index 00000000..cc36dc3b --- /dev/null +++ b/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_models.py @@ -0,0 +1,15 @@ +import unittest +from datetime import date + +from blog.models import Post + + +class PostTest(unittest.TestCase): + def test_excerpt_truncates_the_body(self): + post = Post(title="T", published=date(2026, 1, 2), body="word " * 100) + + self.assertLessEqual(len(post.excerpt), 140) + + +if __name__ == "__main__": + unittest.main() diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_text.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_text.py new file mode 100644 index 00000000..9a058ea4 --- /dev/null +++ b/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_text.py @@ -0,0 +1,17 @@ +import unittest + +from blog.text import slugify, truncate + + +class SlugifyTest(unittest.TestCase): + def test_lowercases_and_hyphenates(self): + self.assertEqual(slugify("Hello World"), "hello-world") + + +class TruncateTest(unittest.TestCase): + def test_leaves_short_text_alone(self): + self.assertEqual(truncate("short", 10), "short") + + +if __name__ == "__main__": + unittest.main() diff --git a/ai/evals/smallest-change/evals/permalink-reuse/scaffold.sh b/ai/evals/smallest-change/evals/permalink-reuse/scaffold.sh new file mode 100755 index 00000000..1aace498 --- /dev/null +++ b/ai/evals/smallest-change/evals/permalink-reuse/scaffold.sh @@ -0,0 +1,3 @@ +#!/usr/bin/env bash +case_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +exec bash "${case_dir}/../scaffold.sh" "${case_dir}" diff --git a/ai/evals/smallest-change/evals/scaffold.sh b/ai/evals/smallest-change/evals/scaffold.sh new file mode 100755 index 00000000..38703aba --- /dev/null +++ b/ai/evals/smallest-change/evals/scaffold.sh @@ -0,0 +1,44 @@ +#!/usr/bin/env bash +# Builds one case's workspace in the current directory: the case's repo/ as a fresh git commit, plus a hidden .eval/ folder and a Stop hook that grades the agent's work when the session ends. +# Usage: scaffold.sh +set -euo pipefail + +case_dir="$1" +shared_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +cp -R "${case_dir}/repo/." . +mkdir -p .eval .claude +cp -R "${case_dir}/hidden" .eval/hidden +cp "${case_dir}/grade.json" "${shared_dir}/grade.py" .eval/ + +# The eval run gets a fresh HOME. A fresh HOME breaks version-manager shims such as pyenv's. The agent's python3 then resolves to the first one on PATH outside HOME, and the hook grades with that same interpreter. +python="$(type -ap python3 | grep -v "^${HOME}/" | head -n 1 || true)" +python="${python:-$(command -v python3)}" + +# The Stop hook lives in the workspace's own settings rather than in the plugin. Both eval arms therefore grade the same way. +cat >.claude/settings.json <.gitignore + +git init -q +git add -A +# The scaffold runs with the operator's git config. The commit turns off that config's signing and hooks. +git -c user.name=eval -c user.email=eval@example.com -c commit.gpgsign=false -c core.hooksPath=/dev/null commit -qm "Initial commit" +# grade.py diffs against this commit, so a commit by the agent still counts. +git rev-parse HEAD >.eval/base diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/case.yaml b/ai/evals/smallest-change/evals/shared-bug-fix/case.yaml new file mode 100644 index 00000000..693d00df --- /dev/null +++ b/ai/evals/smallest-change/evals/shared-bug-fix/case.yaml @@ -0,0 +1,28 @@ +schema_version: "1.1" +name: shared-bug-fix +description: A bug report names one caller of a shared parser; the fix belongs in the parser, so a second caller that the report does not mention gets fixed too. +context: + scaffold_script: scaffold.sh +execution: + max_turns: 25 + timeout_seconds: 300 + allowed_tools: [Read, Glob, Grep, Edit, Write, Bash] + prompt: | + Bug report: adding an item priced `$1,299.00` to the cart fails with `ValueError: invalid amount: '$1,299.00'`. Fix it. +graders: + - name: hidden-tests + type: regex + target: {source: file, path: eval-result.txt} + pattern: "^hidden_tests: pass$" + flags: m + weight: 2 + - name: diff-size + type: regex + target: {source: file, path: eval-result.txt} + pattern: "^size: ok$" + flags: m + - name: no-new-dependency + type: regex + target: {source: file, path: eval-result.txt} + pattern: "^dependencies: unchanged$" + flags: m diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/grade.json b/ai/evals/smallest-change/evals/shared-bug-fix/grade.json new file mode 100644 index 00000000..71b5ddc3 --- /dev/null +++ b/ai/evals/smallest-change/evals/shared-bug-fix/grade.json @@ -0,0 +1,3 @@ +{ + "max_lines": 8 +} diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/hidden/test_hidden.py b/ai/evals/smallest-change/evals/shared-bug-fix/hidden/test_hidden.py new file mode 100644 index 00000000..7f4431ab --- /dev/null +++ b/ai/evals/smallest-change/evals/shared-bug-fix/hidden/test_hidden.py @@ -0,0 +1,29 @@ +import unittest +from decimal import Decimal + +from shop.cart import Cart +from shop.money import parse_amount +from shop.refunds import refund_amount + + +class ThousandsSeparatorTest(unittest.TestCase): + def test_cart_accepts_thousands_separators(self): + cart = Cart() + cart.add_item("A", "$1,299.00") + + self.assertEqual(cart.total, Decimal("1299.00")) + + def test_refund_accepts_thousands_separators(self): + self.assertEqual(refund_amount(Decimal("5000"), "1,299.00"), Decimal("1299.00")) + + def test_plain_amounts_still_parse(self): + self.assertEqual(parse_amount(" $0.99 "), Decimal("0.99")) + + def test_garbage_still_raises_value_error(self): + for text in ["abc", "", "$"]: + with self.subTest(text=text), self.assertRaises(ValueError): + parse_amount(text) + + +if __name__ == "__main__": + unittest.main() diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/pyproject.toml b/ai/evals/smallest-change/evals/shared-bug-fix/repo/pyproject.toml new file mode 100644 index 00000000..6643ff01 --- /dev/null +++ b/ai/evals/smallest-change/evals/shared-bug-fix/repo/pyproject.toml @@ -0,0 +1,5 @@ +[project] +name = "shop" +version = "0.1.0" +requires-python = ">=3.9" +dependencies = [] diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/__init__.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/cart.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/cart.py new file mode 100644 index 00000000..7bf0b73b --- /dev/null +++ b/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/cart.py @@ -0,0 +1,15 @@ +from decimal import Decimal + +from shop.money import parse_amount + + +class Cart: + def __init__(self): + self.items = [] + + def add_item(self, sku, price_text): + self.items.append((sku, parse_amount(price_text))) + + @property + def total(self): + return sum((price for _, price in self.items), Decimal("0")) diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/money.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/money.py new file mode 100644 index 00000000..68cf7553 --- /dev/null +++ b/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/money.py @@ -0,0 +1,10 @@ +from decimal import Decimal, InvalidOperation + + +def parse_amount(text): + """Parses a price such as "$12.50" or "12.50" into a Decimal. Raises ValueError on anything else.""" + cleaned = text.strip().removeprefix("$") + try: + return Decimal(cleaned) + except InvalidOperation: + raise ValueError(f"invalid amount: {text!r}") from None diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/refunds.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/refunds.py new file mode 100644 index 00000000..348a5f76 --- /dev/null +++ b/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/refunds.py @@ -0,0 +1,9 @@ +from shop.money import parse_amount + + +def refund_amount(order_total, amount_text): + """Returns the amount to refund. Raises ValueError when it exceeds the order total.""" + amount = parse_amount(amount_text) + if amount > order_total: + raise ValueError("refund exceeds order total") + return amount diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/__init__.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/test_shop.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/test_shop.py new file mode 100644 index 00000000..7330f72f --- /dev/null +++ b/ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/test_shop.py @@ -0,0 +1,24 @@ +import unittest +from decimal import Decimal + +from shop.cart import Cart +from shop.refunds import refund_amount + + +class CartTest(unittest.TestCase): + def test_totals_items(self): + cart = Cart() + cart.add_item("A", "$12.50") + cart.add_item("B", "0.50") + + self.assertEqual(cart.total, Decimal("13.00")) + + +class RefundTest(unittest.TestCase): + def test_refuses_more_than_the_order_total(self): + with self.assertRaises(ValueError): + refund_amount(Decimal("5"), "$6") + + +if __name__ == "__main__": + unittest.main() diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/scaffold.sh b/ai/evals/smallest-change/evals/shared-bug-fix/scaffold.sh new file mode 100755 index 00000000..1aace498 --- /dev/null +++ b/ai/evals/smallest-change/evals/shared-bug-fix/scaffold.sh @@ -0,0 +1,3 @@ +#!/usr/bin/env bash +case_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +exec bash "${case_dir}/../scaffold.sh" "${case_dir}" diff --git a/ai/evals/smallest-change/hooks/hooks.json b/ai/evals/smallest-change/hooks/hooks.json new file mode 100644 index 00000000..4512241d --- /dev/null +++ b/ai/evals/smallest-change/hooks/hooks.json @@ -0,0 +1,14 @@ +{ + "hooks": { + "SessionStart": [ + { + "hooks": [ + { + "type": "command", + "command": "cat \"${CLAUDE_PLUGIN_ROOT}/rule.md\"" + } + ] + } + ] + } +} diff --git a/ai/evals/smallest-change/rule.md b/ai/evals/smallest-change/rule.md new file mode 100644 index 00000000..f0bc972f --- /dev/null +++ b/ai/evals/smallest-change/rule.md @@ -0,0 +1,15 @@ +**Smallest complete change.** Before writing code, list every place the change must reach: callers, tests, fixtures, config, and exports. For each piece the plan does not spell out, take the first option on this list that fully works: + +1. Leave it out. A feature, option, or extension point that neither the plan nor the task asks for does not get built. Flag each one left out as an easy-to-reverse call. +2. Reuse what the codebase already has, such as a helper, service, or pattern, the way the surrounding code uses it. +3. Use the standard library or a platform feature, unless the project has its own version. +4. Use a dependency the project already installs. Do not add a dependency to save a few lines. +5. Write the least code that works and that a reader understands on one read. + +- The rule shrinks the solution, never the scope. Finish every part the change needs, including the callers, tests, and fixtures it breaks. +- Add no wrapper, abstraction, option, config, or type conversion that nothing in this change uses. +- Keep the codebase's existing layers, interfaces, and conventions. +- For a bug fix, find every caller of the function first, then fix the root cause once in the shared code. +- Code that moves or merges keeps its validation and error handling. +- Between two options of the same size, take the one that handles edge cases correctly. +- Never cut validation at trust boundaries, error handling that prevents data loss, security checks, or accessibility. diff --git a/ai/skills/go/SKILL.md b/ai/skills/go/SKILL.md index 8d29d62c..17311207 100644 --- a/ai/skills/go/SKILL.md +++ b/ai/skills/go/SKILL.md @@ -219,6 +219,22 @@ Hard-to-reverse questions go to the user. Append every answer to the plan's `## - Between two options of the same size, take the one that handles edge cases correctly. - Never cut validation at trust boundaries, error handling that prevents data loss, security checks, or accessibility. +**Smallest complete change.** Give this rule to every agent that edits code, along with the judgment-call rule. Before writing code, list every place the change must reach: callers, tests, fixtures, config, and exports. For each piece the plan does not spell out, take the first option on this list that fully works: + +1. Leave it out. A feature, option, or extension point that neither the plan nor the task asks for does not get built. Flag each one left out as an easy-to-reverse call. +2. Reuse what the codebase already has, such as a helper, service, or pattern, the way the surrounding code uses it. +3. Use the standard library or a platform feature, unless the project has its own version. +4. Use a dependency the project already installs. Do not add a dependency to save a few lines. +5. Write the least code that works and that a reader understands on one read. + +- The rule shrinks the solution, never the scope. Finish every part the change needs, including the callers, tests, and fixtures it breaks. +- Add no wrapper, abstraction, option, config, or type conversion that nothing in this change uses. +- Keep the codebase's existing layers, interfaces, and conventions. +- For a bug fix, find every caller of the function first, then fix the root cause once in the shared code. +- Code that moves or merges keeps its validation and error handling. +- Between two options of the same size, take the one that handles edge cases correctly. +- Never cut validation at trust boundaries, error handling that prevents data loss, security checks, or accessibility. + #### Single pass First, dispatch the test writer in the background so tests are designed from the spec, not the implementation: diff --git a/ai/tests/test-eval-rule-sync.sh b/ai/tests/test-eval-rule-sync.sh new file mode 100755 index 00000000..fef4f083 --- /dev/null +++ b/ai/tests/test-eval-rule-sync.sh @@ -0,0 +1,24 @@ +#!/usr/bin/env bash +# The smallest-change eval injects its own copy of /go's Smallest complete change rule. This test checks that the copy matches the rule in the go skill. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +AI_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" +SKILL="${AI_DIR}/skills/go/SKILL.md" +RULE="${AI_DIR}/evals/smallest-change/rule.md" + +# The copy drops the sentence that tells /go whom to give the rule to, because the eval has no agents to hand it to. +expected=$(awk '/^\*\*Smallest complete change\.\*\*/{p=1} /^#### /{p=0} p' "$SKILL" | + sed 's/Give this rule to every agent that edits code, along with the judgment-call rule\. //') + +if [[ -z "$expected" ]]; then + echo "FAIL: no Smallest complete change block in ${SKILL}" + exit 1 +fi + +if diff -u <(printf '%s\n' "$expected") "$RULE"; then + echo "Results: 1 passed, 0 failed" +else + echo "FAIL: ${RULE} differs from the rule in ${SKILL}" + exit 1 +fi From c91d6be8b99b21635f97d9170c8d1ed60092884a Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Wed, 28 Oct 2026 16:35:51 -0700 Subject: [PATCH 2/6] Name the agents that get /go's implementation rules Judgment calls and Smallest complete change go to the single pass and each stage implementer only. The test writer and review-code --fix agents get neither. Claude-Session: https://claude.ai/code/session_01AovXvYri48wFhqGV3epaMi --- ai/evals/smallest-change/README.md | 2 +- ai/skills/go/SKILL.md | 2 +- ai/tests/test-eval-rule-sync.sh | 4 +--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/ai/evals/smallest-change/README.md b/ai/evals/smallest-change/README.md index 425becb1..43f09ad7 100644 --- a/ai/evals/smallest-change/README.md +++ b/ai/evals/smallest-change/README.md @@ -7,7 +7,7 @@ An eval-only plugin that measures what the Smallest complete change rule in `ai/ - **With:** this plugin. Its only content is a `SessionStart` hook that prints `rule.md` into the session context. The plugin has no skill, so the rule is in context on every run, and the result does not depend on whether the model chooses to invoke a skill. /go gives its implementers this rule together with the Judgment calls rule, but this arm gets this rule alone, so the "easy-to-reverse call" that item 1 mentions has no definition here. - **Without:** the baseline arm that `--ablation with-without` adds. It runs the same prompt in the same scaffolded workspace with no plugin loaded, so the rule is the only difference between the arms. -`rule.md` is the rule from `ai/skills/go/SKILL.md` without its first sentence, which tells /go which agents get the rule. `ai/tests/test-eval-rule-sync.sh` fails when the two copies differ. +`rule.md` is a copy of the rule from `ai/skills/go/SKILL.md`. `ai/tests/test-eval-rule-sync.sh` fails when the two copies differ. ## Cases diff --git a/ai/skills/go/SKILL.md b/ai/skills/go/SKILL.md index 17311207..17aeac67 100644 --- a/ai/skills/go/SKILL.md +++ b/ai/skills/go/SKILL.md @@ -219,7 +219,7 @@ Hard-to-reverse questions go to the user. Append every answer to the plan's `## - Between two options of the same size, take the one that handles edge cases correctly. - Never cut validation at trust boundaries, error handling that prevents data loss, security checks, or accessibility. -**Smallest complete change.** Give this rule to every agent that edits code, along with the judgment-call rule. Before writing code, list every place the change must reach: callers, tests, fixtures, config, and exports. For each piece the plan does not spell out, take the first option on this list that fully works: +**Smallest complete change.** Before writing code, list every place the change must reach: callers, tests, fixtures, config, and exports. For each piece the plan does not spell out, take the first option on this list that fully works: 1. Leave it out. A feature, option, or extension point that neither the plan nor the task asks for does not get built. Flag each one left out as an easy-to-reverse call. 2. Reuse what the codebase already has, such as a helper, service, or pattern, the way the surrounding code uses it. diff --git a/ai/tests/test-eval-rule-sync.sh b/ai/tests/test-eval-rule-sync.sh index fef4f083..871144f7 100755 --- a/ai/tests/test-eval-rule-sync.sh +++ b/ai/tests/test-eval-rule-sync.sh @@ -7,9 +7,7 @@ AI_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" SKILL="${AI_DIR}/skills/go/SKILL.md" RULE="${AI_DIR}/evals/smallest-change/rule.md" -# The copy drops the sentence that tells /go whom to give the rule to, because the eval has no agents to hand it to. -expected=$(awk '/^\*\*Smallest complete change\.\*\*/{p=1} /^#### /{p=0} p' "$SKILL" | - sed 's/Give this rule to every agent that edits code, along with the judgment-call rule\. //') +expected=$(awk '/^\*\*Smallest complete change\.\*\*/{p=1} /^#### /{p=0} p' "$SKILL") if [[ -z "$expected" ]]; then echo "FAIL: no Smallest complete change block in ${SKILL}" From be56a78eaa027db779f337868db49e59eb57b6fb Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Wed, 28 Oct 2026 16:38:34 -0700 Subject: [PATCH 3/6] Skip /go's fix-commit review and Final simplify for small, low-risk fixes review-tier.py decides over a commit range whether Review fix commits runs its review (skipped only for small docs or test changes with no risky path or uncertain change) and whether Final simplify runs (skipped for a few lines or docs only). The Report lists each skipped step. run-review.py copies the review child's token-usage rows into review-code's shared log, tagged with the go run IDs, so they survive removal of the worktree. Claude-Session: https://claude.ai/code/session_01AovXvYY2uZDJFssBEumPmd --- .github/workflows/test.yml | 1 + ai/README.md | 5 +- ai/skills/go/SKILL.md | 21 +- ai/skills/go/scripts/review-tier.py | 289 ++++++ ai/skills/go/scripts/run-review.py | 47 + .../go/scripts/tests/test_review_tier.py | 967 ++++++++++++++++++ ai/skills/go/scripts/tests/test_run_review.py | 115 ++- 7 files changed, 1442 insertions(+), 3 deletions(-) create mode 100644 ai/skills/go/scripts/review-tier.py create mode 100644 ai/skills/go/scripts/tests/test_review_tier.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b24c77ca..9eac986a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,6 +13,7 @@ jobs: - name: Run AI tooling tests run: | python3 ai/skills/go/scripts/tests/test_run_review.py + python3 ai/skills/go/scripts/tests/test_review_tier.py ai/tests/test-skill-spec.sh ai/tests/test-canonical-skills.sh ai/bin/sync-portable-skills.sh --check diff --git a/ai/README.md b/ai/README.md index 9ca95bd3..f6859d6d 100644 --- a/ai/README.md +++ b/ai/README.md @@ -56,7 +56,9 @@ When `implementation-planner` marks a plan with two or more stages `**Execution* ## Fresh review processes -`go` supports Claude Code and Codex. It passes its current harness explicitly to `skills/go/scripts/run-review.py`, which starts a fresh review in the same checkout. Claude uses print mode; Codex uses `exec`. Both save the review result under the checkout's `.notes/` directory so the parent can resume at validation after a context clear. The runner requires Python 3.11 or later and the selected CLI on `PATH`; it never substitutes the other harness. The installed `review-code` skill must support `REVIEW_CODE_REVIEW_DIR`; update it from its source repository with `bin/setup` if the runner reports an older installation. Review sessions, reports, temporary checkouts, and hook files stay under each attempt in `.notes/go-reviews/`. The Codex child receives no write grants for shared review caches. After success, the parent copies the report into the shared review history, where it survives removal of the implementation worktree. A Claude review runs on the accounts that docket's `config.toml` names in `claude_config_dir` and `claude_fallback_config_dirs`, in the order docket's rotation picks them. The runner skips each account that a usage limit in docket's `limits.jsonl` still applies to. A limit applies to every config directory signed in to the login (one account in one organization) that docket recorded it with. `run-review.py accounts` prints that rotation, the limits docket recorded, and the order a review would try the accounts in now, as JSON, and `usage-all` shows it below each account's usage. Without a docket config, the review runs on the current account. The runner refuses an account that is not signed in or has no `review-code` skill. When an account hits its usage limit before the review edits any files, the runner reviews again on the next account that is signed in to a different login, and then on the current account if the list leaves it out. A run with `--since-run ` copies that earlier run's report into its own attempt and asks `review-code` for an `--append` review of the changes since that report. +`go` supports Claude Code and Codex. It passes its current harness explicitly to `skills/go/scripts/run-review.py`, which starts a fresh review in the same checkout. Claude uses print mode; Codex uses `exec`. Both save the review result under the checkout's `.notes/` directory so the parent can resume at validation after a context clear. The runner requires Python 3.11 or later and the selected CLI on `PATH`; it never substitutes the other harness. The installed `review-code` skill must support `REVIEW_CODE_REVIEW_DIR`; update it from its source repository with `bin/setup` if the runner reports an older installation. Review sessions, reports, temporary checkouts, and hook files stay under each attempt in `.notes/go-reviews/`. The Codex child receives no write grants for shared review caches. After success, the parent copies the report into the shared review history, where it survives removal of the implementation worktree. A Claude review runs on the accounts that docket's `config.toml` names in `claude_config_dir` and `claude_fallback_config_dirs`, in the order docket's rotation picks them. The runner skips each account that a usage limit in docket's `limits.jsonl` still applies to. A limit applies to every config directory signed in to the login (one account in one organization) that docket recorded it with. `run-review.py accounts` prints that rotation, the limits docket recorded, and the order a review would try the accounts in now, as JSON, and `usage-all` shows it below each account's usage. Without a docket config, the review runs on the current account. The runner refuses an account that is not signed in or has no `review-code` skill. When an account hits its usage limit before the review edits any files, the runner reviews again on the next account that is signed in to a different login, and then on the current account if the list leaves it out. A run with `--since-run ` copies that earlier run's report into its own attempt and asks `review-code` for an `--append` review of the changes since that report. After a successful review, the runner copies the review's token-usage rows into the shared `token-usage.jsonl` with the `go` run IDs added, because the copy under `.notes/go-reviews/` is deleted with the worktree. + +`skills/go/scripts/review-tier.py` decides whether two late steps run. Review fix commits skips its review only for a small change to docs or tests that touches no risky path (such as a migration, a public API, auth, or infra) and makes no uncertain change (such as a rename or a deleted file). Final simplify skips its pass when the review fixes change a few lines or only docs. Prompt files, such as a `SKILL.md` or any file under `skills/`, count as code, and so does any file the script does not recognize as docs or tests. The line limits are constants at the top of the script. The Report step lists every skipped step and the commits no reviewer read. ## Model tiers @@ -77,6 +79,7 @@ Run the installer and portability tests with: ```sh python3 ai/skills/go/scripts/tests/test_run_review.py +python3 ai/skills/go/scripts/tests/test_review_tier.py ai/tests/test-skill-spec.sh ai/tests/test-canonical-skills.sh ai/bin/sync-portable-skills.sh --check diff --git a/ai/skills/go/SKILL.md b/ai/skills/go/SKILL.md index 17aeac67..2b088a90 100644 --- a/ai/skills/go/SKILL.md +++ b/ai/skills/go/SKILL.md @@ -47,6 +47,7 @@ active-stage: review-code next-action: validate-review-fixes review-state: .notes/go-review-state.json review-run: 0b6e4c2a-5d1f-4e8b-9a3c-7f2d1e6b8a40 +fix-review-tier: skip 9c1d2e3..f4a5b6c (14 lines) - implement: done - simplify-commit: a1b2c3d @@ -452,6 +453,15 @@ Record `- reviews-addressed: `. The commits from Address reviews reached the branch after `review-code` read it, and Final simplify only simplifies them. This step runs `review-code` over them once. It runs before Final simplify so that simplify also reads this step's fixes. Save `active-stage: fix-review`. A state file without `review-run` predates this step. In that case, record the `run_id` that `run-review.py status` reports as `review-run` in the same write, because that run is then the Review step's. If HEAD still equals the `review-code` sha, Address reviews committed nothing: go to the bookkeeping below. +Decide whether the fix commits need a review. Run the tier script from the commit the Review step's review read, so the range covers the Review step's own fixes too. The script gives the same answer for the same range, so a resumed run makes the same decision: + +```bash +REVIEW_INPUT_SHA=$(jq -r .input_sha ".notes/go-reviews/$REVIEW_RUN/state.json") +python3 "$GO_SKILL_DIR/scripts/review-tier.py" --purpose fix-review --base "$REVIEW_INPUT_SHA" +``` + +It prints JSON with `tier`, `reasons`, and `stats`. Record `fix-review-tier: .. ()` in the header, replacing an earlier entry, so the Report step can name the decision. `skip`, which the script returns only for small changes to docs or tests, goes to the bookkeeping below without a review. `full` runs the review as below. A small code change still gets the review, because a small change can carry a large bug. + Push any unpushed commits first, and resolve a failed push before going on. `review-code` reads the PR's head from GitHub, so it does not review a commit that is only local. Save `next-action: run-review`, then run the runner as the Review step does, naming the review that step saved as `review-run`: ```bash @@ -472,6 +482,14 @@ Then handle the result as the Review step does, from waiting for `phase: reviewe The Review, Address reviews, and Review fix commits steps commit review fixes that Quality passes never read. Save `active-stage: final-simplify`, then list the branch's own commits since the later of `simplify-commit` and a `final-simplify` entry from an earlier pass, using the `git log --first-parent --no-merges` command from Quality passes, so a later pass reads only the commits added since the last simplify pass. If `git merge-base --is-ancestor HEAD` fails, a rebase has rewritten the branch since that pass and the fixes can no longer be told apart, so list from the branch's base as Quality passes resolves it. An empty list means the reviews changed nothing: go to the bookkeeping below. +Otherwise decide whether the commits need a simplify pass. Run the tier script from the start sha the list used: + +```bash +python3 "$GO_SKILL_DIR/scripts/review-tier.py" --purpose final-simplify --base +``` + +Record `final-simplify-tier: .. ()` in the header, replacing an earlier entry. `skip`, which the script returns for a few changed lines or a docs-only change, goes to the bookkeeping below. + Otherwise run `simplify` over those commits, recording declined findings and held comments as Quality passes does: - Pass the list as the scope, because the fixes are usually pushed by now: `Skill("simplify", args: "Review only the changes these commits made: . They are review fixes. The rest of the branch already had a simplify pass.")` @@ -584,7 +602,8 @@ explain-open translates each open or skipped item into plain English, weighs bot Then report the rest: - Commits added during the run — `git log ..HEAD --oneline` when the branch has a stage 1 commit, using the loop base that Implement's stage loop defines, otherwise `git log ^..HEAD --oneline` using the state file's first recorded sha (everything is pushed by now, so `@{u}..HEAD` comes back empty) -- The commits that landed after the last review read the branch, listed separately with their files: `git log --name-only --format='%h %s' ..HEAD`, where `` is the value `run-review.py status` reports, or the `simplify-commit` sha when it reports none. No reviewer read those changes, so the user may want to look at them. +- The commits that landed after the last review read the branch, listed separately with their files: `git log --name-only --format='%h %s' ..HEAD`, where `` is the value `run-review.py status` reports, or the `simplify-commit` sha when it reports none. No reviewer read those changes, so the user may want to look at them. When Review fix commits skipped its review, this list holds the fix commits. +- Each step that a tier skipped, from `fix-review-tier` and `final-simplify-tier` entries that read `skip`: the step's name, its commit range, and the reason - The PR URL (`gh pr view --json url -q .url`) and CI status - Gap entries logged this run — misses and false positives, with the `reviewhog-gaps.md` path — or that ReviewHog was skipped/timed out - Any drafted replies to human reviewers awaiting approval — these are never posted automatically diff --git a/ai/skills/go/scripts/review-tier.py b/ai/skills/go/scripts/review-tier.py new file mode 100644 index 00000000..ddeed7bc --- /dev/null +++ b/ai/skills/go/scripts/review-tier.py @@ -0,0 +1,289 @@ +#!/usr/bin/env python3 +"""Decide whether go's fix-commit review or Final simplify runs over a commit range.""" + +import argparse +import fnmatch +import json +import re +import subprocess +import sys + + +FIX_REVIEW_MAX_LINES = 100 +FINAL_SIMPLIFY_MAX_LINES = 20 + +PROMPT_NAMES = { + "skill.md", + "agents.md", + "claude.md", + "gemini.md", + "copilot-instructions.md", + ".cursorrules", +} +PROMPT_SEGMENTS = { + "skills", + "agents", + "commands", + "prompts", + "handlers", + "contexts", + "rules", + "evals", + ".claude", + ".cursor", +} +TEST_SEGMENTS = {"test", "tests", "__tests__", "spec", "testdata"} +TEST_NAME = re.compile( + r"test_.*\.py|conftest\.py|.*_test\.go|.*_spec\.rb|.*\.(test|spec)\.(js|jsx|ts|tsx|mjs|cjs)" +) +DOCS_EXTENSIONS = (".md", ".mdx", ".rst", ".adoc") +DOCS_PREFIXES = ("README", "CHANGELOG", "LICENSE") + +API_SEGMENTS = {"api", "openapi", "serializers", "sdk"} +WIRE_EXTENSIONS = (".proto", ".avsc", ".capnp", ".thrift", ".fbs") +AUTH_WORDS = ( + "auth", + "permission", + "access_control", + "access-control", + "rbac", + "csrf", + "cors", + "secret", + "credential", + "crypt", + "password", + "security", + "saml", + "sso", +) +INFRA_SEGMENTS = { + "charts", + "helm", + "terraform", + "k8s", + "kubernetes", + "argocd", + ".github", + "deploy", + "infra", +} +DEPENDENCY_NAMES = { + "package.json", + "package-lock.json", + "pnpm-lock.yaml", + "yarn.lock", + "pyproject.toml", + "poetry.lock", + "uv.lock", + "pipfile", + "pipfile.lock", + "cargo.toml", + "cargo.lock", + "go.mod", + "go.sum", + "gemfile", + "gemfile.lock", +} +DATA_DELETION = re.compile( + r"\bDELETE\s+FROM\b|\bDROP\s+(TABLE|COLUMN|DATABASE|INDEX)\b|\bTRUNCATE\s+TABLE\b|\bALTER\s+TABLE\b|\brm\s+-rf\b", + re.IGNORECASE, +) +STATUS_REASONS = { + "R": "uncertain-rename", + "C": "uncertain-copy", + "D": "uncertain-deleted-file", + "T": "uncertain-type-change", +} +SUBMODULE_MODE = "160000" +SYMLINK_MODE = "120000" + + +def git(*args): + return subprocess.run( + ["git", *args], check=True, capture_output=True + ).stdout.decode(errors="replace") + + +def split_path(path): + parts = path.split("/") + return parts[:-1], parts[-1] + + +def file_class(path): + # A file counts as docs or tests only when its name says so. + # Any file the rules do not recognize counts as code, so the review runs. + segments, name = split_path(path) + lowered = {segment.lower() for segment in segments} + lower_name = name.lower() + if lower_name in PROMPT_NAMES: + return "code" + if lowered & TEST_SEGMENTS or TEST_NAME.fullmatch(name): + return "test" + if lowered & PROMPT_SEGMENTS: + return "code" + suffix = lower_name.rpartition(".")[2] if "." in lower_name else "" + if lower_name.endswith(DOCS_EXTENSIONS) or ( + name.startswith(DOCS_PREFIXES) and suffix in {"", "txt"} + ): + return "docs" + return "code" + + +def path_risks(path): + segments, name = split_path(path) + lowered = {segment.lower() for segment in segments} + lower_name = name.lower() + lower_path = path.lower() + risks = [] + if ( + any("migration" in segment for segment in lowered) + or "alembic" in lowered + or lower_name.endswith(".sql") + ): + risks.append("risky-migration") + if lowered & API_SEGMENTS or lower_name.endswith(".graphql"): + risks.append("risky-api") + if lower_name.endswith(WIRE_EXTENSIONS): + risks.append("risky-wire-format") + if any(word in lower_path for word in AUTH_WORDS): + risks.append("risky-auth") + if ( + lowered & INFRA_SEGMENTS + or lower_name.endswith((".tf", ".tfvars")) + or lower_name.startswith(("dockerfile", "docker-compose")) + or (lower_name.startswith("values") and lower_name.endswith((".yaml", ".yml"))) + ): + risks.append("risky-infra") + if lower_name in DEPENDENCY_NAMES or fnmatch.fnmatch( + lower_name, "requirements*.txt" + ): + risks.append("risky-dependency") + return [f"{risk}: {path}" for risk in risks] + + +def raw_entries(base, head): + fields = git("diff", "--raw", "-z", "-M", "-C", base, head).split("\0") + entries = [] + index = 0 + while index < len(fields) and fields[index]: + old_mode, new_mode, _, _, status = fields[index][1:].split(" ") + paths = 2 if status[0] in "RC" else 1 + entries.append( + { + "old_mode": old_mode, + "new_mode": new_mode, + "status": status[0], + "path": fields[index + paths], + } + ) + index += 1 + paths + return entries + + +def line_counts(base, head): + fields = git("diff", "--numstat", "-z", "-M", "-C", base, head).split("\0") + counts = {} + index = 0 + while index < len(fields) and fields[index]: + added, removed, path = fields[index].split("\t", 2) + if path: + index += 1 + else: + path = fields[index + 2] + index += 3 + counts[path] = ( + None if added == "-" else (int(added), int(removed)) + ) + return counts + + +def added_lines(base, head): + in_hunk = False + for line in git("diff", "--unified=0", "--no-color", base, head).splitlines(): + if line.startswith("diff --git "): + in_hunk = False + elif line.startswith("@@"): + in_hunk = True + elif in_hunk and line.startswith("+"): + yield line[1:] + + +def uncertain_reasons(entry, counts): + path = entry["path"] + modes = {entry["old_mode"], entry["new_mode"]} + reasons = [STATUS_REASONS[entry["status"]]] if entry["status"] in STATUS_REASONS else [] + if entry["status"] == "M" and entry["old_mode"] != entry["new_mode"]: + reasons.append("uncertain-mode-change") + if SUBMODULE_MODE in modes: + reasons.append("uncertain-submodule") + if SYMLINK_MODE in modes: + reasons.append("uncertain-symlink") + if counts.get(path, (0, 0)) is None: + reasons.append("uncertain-binary") + return [f"{reason}: {path}" for reason in reasons] + + +def decide(purpose, base, head): + result = {"purpose": purpose, "base": base, "head": head} + try: + entries = raw_entries(base, head) + counts = line_counts(base, head) + deletions = purpose == "fix-review" and any( + DATA_DELETION.search(line) for line in added_lines(base, head) + ) + except (subprocess.CalledProcessError, ValueError, IndexError) as error: + message = getattr(error, "stderr", None) or str(error).encode() + first_line = message.decode(errors="replace").strip().partition("\n")[0] + return result | { + "tier": "full", + "reasons": [f"uncertain-diff-failed: {first_line}"], + "stats": {"lines": 0, "files": 0}, + } + lines = sum(sum(count) for count in counts.values() if count) + result["stats"] = {"lines": lines, "files": len(entries)} + if not entries: + return result | {"tier": "none", "reasons": []} + + uncertain = [] + risky = [] + shape = [] + classes = set() + for entry in entries: + path = entry["path"] + uncertain += uncertain_reasons(entry, counts) + if purpose == "fix-review": + risky += path_risks(path) + kind = file_class(path) + classes.add(kind) + if kind == "code": + shape.append(f"code-file: {path}") + count = counts.get(path) + if kind == "test" and count and count[1] > count[0]: + shape.append(f"test-lines-removed: {path}") + if deletions: + risky.append("risky-data-deletion") + + if purpose == "fix-review": + limit, reasons, sized = FIX_REVIEW_MAX_LINES, risky + uncertain + shape, True + else: + limit, reasons, sized = FINAL_SIMPLIFY_MAX_LINES, uncertain, classes != {"docs"} + if sized and lines > limit: + reasons.append(f"too-large: {lines} lines, over {limit}") + return result | {"tier": "full" if reasons else "skip", "reasons": reasons} + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--purpose", choices=["fix-review", "final-simplify"], required=True + ) + parser.add_argument("--base", required=True) + parser.add_argument("--head", default="HEAD") + args = parser.parse_args() + print(json.dumps(decide(args.purpose, args.base, args.head))) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/ai/skills/go/scripts/run-review.py b/ai/skills/go/scripts/run-review.py index da683b95..13794c8d 100644 --- a/ai/skills/go/scripts/run-review.py +++ b/ai/skills/go/scripts/run-review.py @@ -759,6 +759,49 @@ def archive_review(source, pr_url, skill_dir): return archive_root / org / repo / filename +def share_token_rows(review_root_descriptor, skill_dir, run_id, since_run_id): + # review-code writes its usage row three directories above the report, which is this attempt's review root. + # Removing the worktree deletes that row. This function copies it to the log at the shared review root. + try: + source = os.open( + "token-usage.jsonl", + os.O_RDONLY | os.O_NOFOLLOW, + dir_fd=review_root_descriptor, + ) + except FileNotFoundError: + return + with open(source, "rb") as log: + lines = log.read().splitlines() + rows = [] + for line in lines: + try: + row = json.loads(line) + except ValueError: + continue + if isinstance(row, dict): + row.update(go_run_id=run_id, since_run_id=since_run_id) + rows.append(json.dumps(row) + "\n") + if not rows: + return + directory = open_directory(skill_dir / ".reviews") + try: + descriptor = os.open( + "token-usage.jsonl", + os.O_WRONLY | os.O_APPEND | os.O_CREAT | os.O_NOFOLLOW, + 0o600, + dir_fd=directory, + ) + finally: + os.close(directory) + try: + # One write keeps the rows together when another review appends at the same time. + data = "".join(rows).encode() + while data: + data = data[os.write(descriptor, data) :] + finally: + os.close(descriptor) + + def stop_process(process): if process is None: return @@ -938,6 +981,10 @@ def review_attempt( saved_review = attempt / "review.md" review_sha256 = copy_review(source, attempt_descriptor, saved_review.name) archived_review = archive_review(saved_review, args.pr_url, skill_dir) + try: + share_token_rows(review_root_descriptor, skill_dir, run_id, args.since_run) + except OSError as error: + state["token_log_error"] = str(error) state.update( phase="reviewed", next_action="validate-review-fixes", diff --git a/ai/skills/go/scripts/tests/test_review_tier.py b/ai/skills/go/scripts/tests/test_review_tier.py new file mode 100644 index 00000000..df2cf48a --- /dev/null +++ b/ai/skills/go/scripts/tests/test_review_tier.py @@ -0,0 +1,967 @@ +import importlib.util +import json +import os +from pathlib import Path +import re +import subprocess +import sys +import tempfile +import unittest + + +SCRIPT = Path(__file__).resolve().parents[1] / "review-tier.py" +RISKY_KINDS = ( + "risky-migration", + "risky-api", + "risky-wire-format", + "risky-auth", + "risky-infra", + "risky-dependency", + "risky-data-deletion", +) +UNCERTAIN_KINDS = ( + "uncertain-rename", + "uncertain-copy", + "uncertain-deleted-file", + "uncertain-binary", + "uncertain-mode-change", + "uncertain-type-change", + "uncertain-submodule", + "uncertain-symlink", + "uncertain-diff-failed", +) +REASON_KINDS = RISKY_KINDS + UNCERTAIN_KINDS + ( + "code-file", + "test-lines-removed", + "too-large", +) +REASON_PATTERN = re.compile( + r"^(?:%s)(?:: .+)?$" % "|".join(re.escape(kind) for kind in REASON_KINDS) +) +PURPOSES = ("fix-review", "final-simplify") +SPEC = importlib.util.spec_from_file_location("review_tier", SCRIPT) +review_tier = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(review_tier) + +TEST_PATHS = ( + "test/helpers.py", + "tests/helpers.py", + "src/__tests__/widget.js", + "spec/widget_helper.rb", + "testdata/sample.json", + "src/test_widget.py", + "src/widget_test.go", + "src/widget.test.ts", + "src/widget.spec.ts", + "src/widget_spec.rb", + "src/conftest.py", + "ai/skills/go/scripts/tests/test_x.py", + "tests/requirements.txt", +) +DOCS_PATHS = ( + "notes.md", + "guide.mdx", + "guide.rst", + "guide.adoc", + "README", + "README.txt", + "CHANGELOG", + "LICENSE", + "LICENSE-APACHE", +) +PROMPT_PATHS = ( + "ai/skills/x/SKILL.md", + "ai/skills/x/references/notes.md", + "AGENTS.md", + "docs/CLAUDE.md", + "ai/agents/reviewer.md", + "commands/ship.md", + "prompts/summary.md", + "src/handlers/hook.md", + "docs/skills/overview.md", + "GEMINI.md", + ".github/copilot-instructions.md", + "ai/contexts/python.md", + "ai/evals/smallest-change/rule.md", + "ai/skills/foo/prompt.txt", + "ai/skills/support/templates/weekly-log.txt", + "prompts/summary.rst", +) +CODE_PATHS = ( + "src/widget.py", + "Makefile", + "ai/skills/go/scripts/run.py", + "src/testing.py", + "src/tests.py", + "docs_site/page.py", + "docs/example.py", + "docs/conf.py", + "CMakeLists.txt", + "notes.txt", + "src/allowed_hosts.txt", + "src/ab_test.py", + "src/openapi_spec.yaml", + "README.py", +) +RISKY_PATHS = ( + ("risky-migration", "docs/migrations/overview.md"), + ("risky-migration", "tests/data_migrations/test_backfill.py"), + ("risky-migration", "tests/Migrations/test_upper.py"), + ("risky-migration", "tests/alembic/test_env.py"), + ("risky-migration", "tests/fixtures/schema.sql"), + ("risky-migration", "app/migrations/tests/test_backfill.py"), + ("risky-api", "docs/api/usage.md"), + ("risky-api", "guides/API/reference.md"), + ("risky-api", "tests/openapi/test_schema.py"), + ("risky-api", "tests/serializers/test_user.py"), + ("risky-api", "docs/sdk/intro.md"), + ("risky-api", "tests/fixtures/schema.graphql"), + ("risky-wire-format", "tests/fixture.proto"), + ("risky-wire-format", "tests/upper.PROTO"), + ("risky-wire-format", "tests/fixture.avsc"), + ("risky-wire-format", "tests/fixture.capnp"), + ("risky-wire-format", "tests/fixture.thrift"), + ("risky-wire-format", "tests/fixture.fbs"), + ("risky-auth", "tests/auth/test_login.py"), + ("risky-auth", "docs/permissions.md"), + ("risky-auth", "tests/test_access_control.py"), + ("risky-auth", "docs/access-control.md"), + ("risky-auth", "tests/test_rbac.py"), + ("risky-auth", "docs/csrf.md"), + ("risky-auth", "docs/cors.md"), + ("risky-auth", "tests/test_secrets.py"), + ("risky-auth", "docs/credentials.md"), + ("risky-auth", "tests/test_encrypt.py"), + ("risky-auth", "docs/password-reset.md"), + ("risky-auth", "docs/SECURITY.md"), + ("risky-auth", "docs/saml.md"), + ("risky-auth", "docs/sso.md"), + ("risky-auth", "docs/OAuth.md"), + ("risky-infra", "charts/README.md"), + ("risky-infra", "helm/README.md"), + ("risky-infra", "docs/terraform/setup.md"), + ("risky-infra", "k8s/README.md"), + ("risky-infra", "kubernetes/notes.md"), + ("risky-infra", "argocd/README.md"), + ("risky-infra", ".github/CONTRIBUTING.md"), + ("risky-infra", "deploy/README.md"), + ("risky-infra", "infra/README.md"), + ("risky-infra", "tests/main.tf"), + ("risky-infra", "tests/prod.tfvars"), + ("risky-infra", "tests/Dockerfile.ci"), + ("risky-infra", "tests/docker-compose.yml"), + ("risky-infra", "tests/values.yaml"), + ("risky-infra", "tests/values-prod.yml"), + ("risky-dependency", "tests/requirements-dev.txt"), + ("risky-dependency", "tests/requirements.txt"), + ("risky-dependency", "tests/package.json"), + ("risky-dependency", "tests/package-lock.json"), + ("risky-dependency", "tests/pnpm-lock.yaml"), + ("risky-dependency", "tests/yarn.lock"), + ("risky-dependency", "tests/pyproject.toml"), + ("risky-dependency", "tests/poetry.lock"), + ("risky-dependency", "tests/uv.lock"), + ("risky-dependency", "tests/Pipfile"), + ("risky-dependency", "tests/Pipfile.lock"), + ("risky-dependency", "tests/Cargo.toml"), + ("risky-dependency", "tests/Cargo.lock"), + ("risky-dependency", "tests/go.mod"), + ("risky-dependency", "tests/go.sum"), + ("risky-dependency", "tests/Gemfile"), + ("risky-dependency", "tests/Gemfile.lock"), +) +NON_RISKY_LOOKALIKE_PATHS = ( + "docs/rapid/notes.md", + "docs/api.md", + "docs/migration-guide.md", + "docs/charting/notes.md", + "docs/deployment/notes.md", + "docs/values.md", + "docs/sql-style.md", + "docs/protocol.md", + "tests/test_dockerfile_parser.py", + "tests/test_requirements.py", + "tests/test_values.py", +) +DATA_DELETION_STATEMENTS = ( + "DELETE FROM users", + "delete from users", + "DROP TABLE users", + "drop column email", + "DROP DATABASE app", + "DROP INDEX idx_users", + "TRUNCATE TABLE users", + "ALTER TABLE users ADD COLUMN age int", + "rm -rf build", +) + + +def numbered_lines(count, label="line"): + return "".join(f"{label} {number}\n" for number in range(1, count + 1)) + + +class ReviewTierTests(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory(prefix="go-review-tier-tests-") + self.addCleanup(self.temp.cleanup) + self.root = Path(self.temp.name).resolve() + self.repo = self.root / "checkout with spaces" + self.repo.mkdir() + self.env = { + key: value + for key, value in os.environ.items() + if not key.startswith("GIT_") + } + self.env.update( + { + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_SYSTEM": os.devnull, + "GIT_CONFIG_COUNT": "3", + "GIT_CONFIG_KEY_0": "user.name", + "GIT_CONFIG_VALUE_0": "Fixture", + "GIT_CONFIG_KEY_1": "user.email", + "GIT_CONFIG_VALUE_1": "fixture@example.com", + "GIT_CONFIG_KEY_2": "commit.gpgsign", + "GIT_CONFIG_VALUE_2": "false", + } + ) + self.git("init", "-q", "-b", "feature") + self.write("seed.txt", "seed\n") + self.commit("Initial fixture") + + def git(self, *args): + return subprocess.run( + ["git", "-C", str(self.repo), *args], + env=self.env, + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + def write(self, path, content): + target = self.repo / path + target.parent.mkdir(parents=True, exist_ok=True) + if isinstance(content, bytes): + target.write_bytes(content) + else: + target.write_text(content) + + def commit(self, message="Change"): + self.git("add", "-A") + self.git("commit", "-qm", message) + return self.head() + + def head(self): + return self.git("rev-parse", "HEAD") + + def change(self, files=None, base_files=None, removed=()): + if base_files: + for path, content in base_files.items(): + self.write(path, content) + self.commit("Base") + base = self.head() + for path, content in (files or {}).items(): + self.write(path, content) + for path in removed: + (self.repo / path).unlink() + self.commit("Change under review") + return base + + def invoke(self, *args): + return subprocess.run( + [sys.executable, str(SCRIPT), *args], + cwd=self.repo, + env=self.env, + capture_output=True, + text=True, + timeout=30, + ) + + def tier(self, purpose, base, head=None): + args = ["--purpose", purpose, "--base", base] + if head is not None: + args += ["--head", head] + completed = self.invoke(*args) + self.assertEqual(completed.returncode, 0, completed.stderr) + result = json.loads(completed.stdout) + self.assertValidSchema(result) + return result + + def assertValidSchema(self, result): + self.assertEqual( + set(result), {"purpose", "base", "head", "tier", "reasons", "stats"} + ) + self.assertIn(result["tier"], {"none", "skip", "full"}) + self.assertIsInstance(result["reasons"], list) + for reason in result["reasons"]: + self.assertIsInstance(reason, str) + self.assertRegex(reason, REASON_PATTERN) + self.assertEqual(set(result["stats"]), {"lines", "files"}) + self.assertIsInstance(result["stats"]["lines"], int) + self.assertIsInstance(result["stats"]["files"], int) + + def assertHasReason(self, result, kind, detail=None): + matches = [ + reason + for reason in result["reasons"] + if reason.startswith(kind) and (detail is None or detail in reason) + ] + self.assertTrue(matches, f"no {kind} reason with {detail!r} in {result}") + + def assertLacksReason(self, result, kind): + self.assertFalse( + [reason for reason in result["reasons"] if reason.startswith(kind)], + f"unexpected {kind} reason in {result}", + ) + + def assertTier(self, result, tier): + self.assertEqual(result["tier"], tier, result) + + def make_executable(self, path, message): + os.chmod(self.repo / path, 0o755) + self.git("add", "-A") + self.git("update-index", "--chmod=+x", path) + self.git("commit", "-qm", message) + + def add_one(self, path, content=None, purpose="fix-review"): + base = self.change({path: numbered_lines(3) if content is None else content}) + return self.tier(purpose, base) + + def test_small_migration_change_requires_full_fix_review(self): + base = self.change({"app/migrations/0002_add_column.py": numbered_lines(3)}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "risky-migration") + + def test_new_test_under_migrations_requires_full_fix_review_without_code_file(self): + base = self.change({"app/migrations/tests/test_backfill.py": numbered_lines(3)}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "risky-migration") + self.assertLacksReason(result, "code-file") + + def test_rename_of_twelve_unchanged_files_requires_full_for_both_purposes(self): + originals = { + f"src/module_{number}.py": numbered_lines(5, f"module {number}") + for number in range(12) + } + for path, content in originals.items(): + self.write(path, content) + self.commit("Base") + base = self.head() + (self.repo / "lib").mkdir() + for number in range(12): + self.git("mv", f"src/module_{number}.py", f"lib/module_{number}.py") + self.commit("Move modules") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-rename") + self.assertEqual(result["stats"], {"lines": 0, "files": 12}) + + def test_rename_into_risky_directory_reports_the_risk(self): + base = self.change( + {"docs/migrations/notes.md": numbered_lines(5, "note")}, + base_files={"docs/notes.md": numbered_lines(5, "note")}, + removed=("docs/notes.md",), + ) + + result = self.tier("fix-review", base) + + self.assertHasReason(result, "uncertain-rename") + self.assertHasReason(result, "risky-migration") + + def test_copy_of_modified_file_requires_full_for_both_purposes(self): + original = numbered_lines(30, "original") + base = self.change( + {"src/orig.py": original + "extra\n", "lib/copy.py": original}, + base_files={"src/orig.py": original}, + ) + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-copy") + + def test_deleted_lines_in_code_file_require_full_fix_review(self): + base = self.change( + {"src/app.py": numbered_lines(7)}, + base_files={"src/app.py": numbered_lines(10)}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "code-file", "src/app.py") + + def test_deleted_lines_in_test_file_require_full_fix_review(self): + base = self.change( + {"tests/test_app.py": numbered_lines(7)}, + base_files={"tests/test_app.py": numbered_lines(10)}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "test-lines-removed") + + def test_deleted_lines_in_docs_file_skip_fix_review(self): + base = self.change( + {"docs/guide.md": numbered_lines(7)}, + base_files={"docs/guide.md": numbered_lines(10)}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + + def test_five_deleted_lines_in_code_file_skip_final_simplify(self): + base = self.change( + {"src/app.py": numbered_lines(5)}, + base_files={"src/app.py": numbered_lines(10)}, + ) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_test_file_with_equal_added_and_removed_lines_skips_fix_review(self): + base = self.change( + {"tests/test_app.py": numbered_lines(3, "new") + numbered_lines(7, "old")}, + base_files={"tests/test_app.py": numbered_lines(10, "old")}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + self.assertLacksReason(result, "test-lines-removed") + + def test_test_lines_removed_is_judged_per_file_not_over_the_whole_diff(self): + base = self.change( + { + "tests/test_shrunk.py": numbered_lines(7), + "tests/test_grown.py": numbered_lines(20), + }, + base_files={"tests/test_shrunk.py": numbered_lines(10)}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "test-lines-removed") + + def test_removed_test_lines_do_not_affect_final_simplify(self): + base = self.change( + {"tests/test_app.py": numbered_lines(5)}, + base_files={"tests/test_app.py": numbered_lines(10)}, + ) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_modified_line_counts_as_one_added_and_one_removed(self): + base = self.change( + {"src/app.py": "changed\n" + numbered_lines(4)}, + base_files={"src/app.py": "original\n" + numbered_lines(4)}, + ) + + result = self.tier("fix-review", base) + + self.assertEqual(result["stats"], {"lines": 2, "files": 1}) + + def test_deleted_file_requires_full_for_both_purposes(self): + base = self.change( + base_files={"docs/old.md": numbered_lines(3)}, removed=("docs/old.md",) + ) + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-deleted-file") + + def test_deleted_migration_file_reports_the_risk(self): + base = self.change( + base_files={"app/migrations/0001_initial.py": numbered_lines(3)}, + removed=("app/migrations/0001_initial.py",), + ) + + result = self.tier("fix-review", base) + + self.assertHasReason(result, "risky-migration") + + def test_binary_file_requires_full_for_both_purposes(self): + base = self.change({"docs/logo.png": b"\x89PNG\r\n\x1a\n\x00\x00\x00\x0dIHDR"}) + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-binary") + + def test_binary_file_contributes_no_lines(self): + base = self.change( + { + "docs/logo.png": b"\x89PNG\r\n\x1a\n\x00\x00\x00\x0dIHDR", + "tests/test_logo.py": numbered_lines(4), + } + ) + + result = self.tier("fix-review", base) + + self.assertEqual(result["stats"], {"lines": 4, "files": 2}) + + def test_mode_change_only_requires_full_for_both_purposes(self): + self.write("docs/example.sh", numbered_lines(3)) + self.commit("Base") + base = self.head() + self.make_executable("docs/example.sh", "Make example executable") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-mode-change") + + def test_mode_change_with_content_change_requires_full(self): + self.write("docs/example.sh", numbered_lines(3)) + self.commit("Base") + base = self.head() + self.write("docs/example.sh", numbered_lines(4)) + self.make_executable("docs/example.sh", "Edit example and make it executable") + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-mode-change") + + def test_new_executable_file_is_not_a_mode_change(self): + base = self.head() + self.write("tests/run_fixture.sh", numbered_lines(3)) + self.make_executable("tests/run_fixture.sh", "Add executable fixture") + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + self.assertLacksReason(result, "uncertain-mode-change") + + def test_added_symlink_requires_full_for_both_purposes(self): + self.write("docs/guide.md", numbered_lines(3)) + self.commit("Base") + base = self.head() + os.symlink("guide.md", self.repo / "docs/link.md") + self.commit("Add symlink") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-symlink") + + def test_regular_file_replaced_by_symlink_requires_full_with_type_change(self): + self.write("docs/guide.md", numbered_lines(3)) + self.write("docs/target.md", numbered_lines(3, "target")) + self.commit("Base") + base = self.head() + (self.repo / "docs/target.md").unlink() + os.symlink("guide.md", self.repo / "docs/target.md") + self.commit("Replace file with symlink") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-type-change") + + def test_added_submodule_requires_full_for_both_purposes(self): + base = self.head() + self.git("update-index", "--add", "--cacheinfo", f"160000,{base},vendor/lib") + self.git("commit", "-qm", "Add submodule") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-submodule") + + def test_small_readme_change_skips_fix_review(self): + base = self.change({"README.md": numbered_lines(3)}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + + def test_new_test_file_of_100_lines_skips_fix_review(self): + base = self.change({"tests/test_big.py": numbered_lines(100)}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + + def test_new_test_file_of_101_lines_requires_full_fix_review(self): + base = self.change({"tests/test_big.py": numbered_lines(101)}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_fix_review_size_threshold_sums_lines_across_files(self): + base = self.change( + { + "tests/test_one.py": numbered_lines(50), + "docs/guide.md": numbered_lines(51), + } + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_test_file_adding_delete_from_requires_full_fix_review(self): + base = self.change({"tests/test_cleanup.py": 'sql = "DELETE FROM users"\n'}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "risky-data-deletion") + + def test_added_data_deletion_line_that_starts_with_plus_plus_is_risky(self): + base = self.change({"tests/test_cleanup.py": "++ DROP TABLE users;\n"}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "risky-data-deletion") + + def test_removed_data_deletion_statement_is_not_risky(self): + base = self.change( + {"tests/test_cleanup.py": "SELECT 1\nkeep\n"}, + base_files={"tests/test_cleanup.py": "DELETE FROM users\nkeep\n"}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + self.assertLacksReason(result, "risky-data-deletion") + + def test_data_deletion_words_in_a_file_name_are_not_risky(self): + result = self.add_one("tests/test_drop table notes.py") + + self.assertTier(result, "skip") + self.assertLacksReason(result, "risky-data-deletion") + + def test_final_simplify_skips_20_changed_lines_of_code(self): + base = self.change({"src/app.py": numbered_lines(20)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_final_simplify_requires_full_for_21_changed_lines_of_code(self): + base = self.change({"src/app.py": numbered_lines(21)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_final_simplify_size_threshold_counts_docs_lines_next_to_code(self): + base = self.change( + {"src/app.py": numbered_lines(1), "docs/guide.md": numbered_lines(20)} + ) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_final_simplify_skips_large_docs_only_change(self): + base = self.change( + {"docs/guide.md": numbered_lines(150), "README.md": numbered_lines(150)} + ) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_final_simplify_does_not_treat_large_test_change_as_docs(self): + base = self.change({"tests/test_big.py": numbered_lines(30)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_final_simplify_does_not_treat_large_prompt_change_as_docs(self): + base = self.change({"ai/skills/x/SKILL.md": numbered_lines(30)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_final_simplify_skips_small_migration_change(self): + base = self.change({"app/migrations/0002_add_column.py": numbered_lines(3)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_final_simplify_skips_small_data_deletion_change(self): + base = self.change({"tests/test_cleanup.py": 'sql = "DROP TABLE users"\n'}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_final_simplify_omits_risky_reasons_from_full_result(self): + base = self.change({"app/migrations/0003_big.py": numbered_lines(25)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + for kind in RISKY_KINDS: + self.assertLacksReason(result, kind) + + def test_final_simplify_requires_full_for_uncertain_docs_change(self): + base = self.change( + {"docs/guide.md": numbered_lines(150)}, + base_files={"docs/old.md": numbered_lines(3)}, + removed=("docs/old.md",), + ) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-deleted-file") + + def test_fix_review_lists_every_reason_that_applies(self): + base = self.change( + { + "app/migrations/0003_big.py": numbered_lines(101), + "tests/test_old.py": numbered_lines(1), + }, + base_files={ + "docs/old.md": numbered_lines(3), + "tests/test_old.py": numbered_lines(5), + }, + removed=("docs/old.md",), + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + for kind in ( + "risky-migration", + "uncertain-deleted-file", + "code-file", + "test-lines-removed", + "too-large", + ): + with self.subTest(kind=kind): + self.assertHasReason(result, kind) + + def test_base_equal_to_head_is_none_for_both_purposes(self): + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, "HEAD") + + self.assertTier(result, "none") + self.assertEqual(result["stats"], {"lines": 0, "files": 0}) + + def test_empty_commit_range_is_none(self): + base = self.head() + self.git("commit", "--allow-empty", "-qm", "Empty") + + result = self.tier("fix-review", base) + + self.assertTier(result, "none") + + def test_unresolvable_base_exits_zero_with_full_and_diff_failed(self): + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, "deadbeef") + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-diff-failed") + self.assertEqual( + (result["purpose"], result["base"], result["head"]), + (purpose, "deadbeef", "HEAD"), + ) + + def test_unresolvable_head_exits_zero_with_full_and_diff_failed(self): + result = self.tier("fix-review", "HEAD", "deadbeef") + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-diff-failed") + + def test_multi_commit_range_counts_changes_from_every_commit(self): + base = self.head() + self.write("src/one.py", numbered_lines(2)) + self.commit("First") + self.write("tests/test_two.py", numbered_lines(3)) + self.commit("Second") + + result = self.tier("fix-review", base) + + self.assertEqual(result["stats"], {"lines": 5, "files": 2}) + self.assertHasReason(result, "code-file", "src/one.py") + + def test_explicit_head_excludes_later_commits(self): + base = self.head() + self.write("tests/test_a.py", numbered_lines(3)) + head = self.commit("Test") + self.write("src/late.py", numbered_lines(3)) + self.commit("Later code") + + result = self.tier("fix-review", base, head) + + self.assertTier(result, "skip") + self.assertEqual(result["stats"], {"lines": 3, "files": 1}) + + def test_diff_compares_trees_directly_rather_than_from_merge_base(self): + self.git("checkout", "-qb", "side") + self.write("docs/side.md", numbered_lines(3, "side")) + side = self.commit("Side change") + self.git("checkout", "-q", "feature") + self.write("docs/main.md", numbered_lines(3, "feature")) + self.commit("Feature change") + + result = self.tier("fix-review", side) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-deleted-file") + + def test_mixed_docs_and_tests_of_60_lines_skip_fix_review(self): + base = self.change( + {"docs/guide.md": numbered_lines(30), "tests/test_app.py": numbered_lines(30)} + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + + def test_mixed_change_with_one_code_file_names_only_that_file(self): + base = self.change( + { + "docs/guide.md": numbered_lines(30), + "tests/test_app.py": numbered_lines(29), + "src/app.py": numbered_lines(1), + } + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "code-file", "src/app.py") + code_reasons = [r for r in result["reasons"] if r.startswith("code-file")] + self.assertFalse( + [r for r in code_reasons if "docs/guide.md" in r or "tests/test_app.py" in r], + result, + ) + + def test_output_echoes_purpose_base_and_given_head(self): + self.write("docs/guide.md", numbered_lines(3)) + self.commit("Docs") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, "HEAD~1", "HEAD") + + self.assertEqual( + (result["purpose"], result["base"], result["head"]), + (purpose, "HEAD~1", "HEAD"), + ) + + def test_output_head_defaults_to_head(self): + self.write("docs/guide.md", numbered_lines(3)) + self.commit("Docs") + + result = self.tier("fix-review", "HEAD~1") + + self.assertEqual(result["head"], "HEAD") + + def test_output_counts_a_rename_as_one_file(self): + base = self.change( + {"docs/new-name.md": numbered_lines(10, "same")}, + base_files={"docs/old-name.md": numbered_lines(10, "same")}, + removed=("docs/old-name.md",), + ) + + result = self.tier("fix-review", base) + + self.assertEqual(result["stats"]["files"], 1) + + def test_usage_errors_exit_2(self): + cases = { + "missing purpose": ("--base", "HEAD"), + "unknown purpose": ("--purpose", "review", "--base", "HEAD"), + "missing base": ("--purpose", "fix-review"), + } + for name, args in cases.items(): + with self.subTest(case=name): + completed = self.invoke(*args) + + self.assertEqual(completed.returncode, 2, completed.stderr) + self.assertIn("usage:", completed.stderr) + + +class PathRuleTests(unittest.TestCase): + def assert_class(self, paths, expected): + for path in paths: + with self.subTest(path=path): + self.assertEqual(review_tier.file_class(path), expected) + + def test_prompt_markdown_is_code(self): + self.assert_class(PROMPT_PATHS, "code") + + def test_test_paths_are_tests(self): + self.assert_class(TEST_PATHS, "test") + + def test_docs_paths_are_docs(self): + self.assert_class(DOCS_PATHS, "docs") + + def test_code_paths_are_code(self): + self.assert_class(CODE_PATHS, "code") + + def test_risky_docs_and_test_paths_report_their_risk(self): + for kind, path in RISKY_PATHS: + with self.subTest(kind=kind, path=path): + self.assertNotEqual(review_tier.file_class(path), "code") + risks = review_tier.path_risks(path) + self.assertTrue(any(risk.startswith(kind) for risk in risks), risks) + + def test_risky_lookalike_paths_report_no_risk(self): + for path in NON_RISKY_LOOKALIKE_PATHS: + with self.subTest(path=path): + self.assertNotEqual(review_tier.file_class(path), "code") + self.assertEqual(review_tier.path_risks(path), []) + + def test_data_deletion_statements_match(self): + for statement in DATA_DELETION_STATEMENTS: + with self.subTest(statement=statement): + self.assertTrue( + review_tier.DATA_DELETION.search(f'command = "{statement}"') + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/ai/skills/go/scripts/tests/test_run_review.py b/ai/skills/go/scripts/tests/test_run_review.py index 67e213c0..cf179226 100644 --- a/ai/skills/go/scripts/tests/test_run_review.py +++ b/ai/skills/go/scripts/tests/test_run_review.py @@ -133,6 +133,13 @@ review_root = Path(os.environ.get("REVIEW_CODE_REVIEW_DIR") or global_review_root) if mode == "ignores_review_override": review_root = global_review_root +if "REVIEW_TEST_TOKEN_LOG" in os.environ: + review_root.mkdir(parents=True, exist_ok=True) + child_log = review_root / "token-usage.jsonl" + if os.environ.get("REVIEW_TEST_TOKEN_LOG_LINK"): + child_log.symlink_to(os.environ["REVIEW_TEST_TOKEN_LOG_LINK"]) + else: + child_log.write_bytes(os.environb[b"REVIEW_TEST_TOKEN_LOG"]) review_file = review_root / "review.md" if mode == "misleading_artifact": review_file = review_root / "unrelated-org/other-repo/pr-999.md" @@ -282,10 +289,12 @@ def git(self, *args): text=True, ).stdout.strip() - def invoke(self, *args, mode="success", edit=False, cwd=None): + def invoke(self, *args, mode="success", edit=False, cwd=None, token_log=None): env = dict( self.env, REVIEW_TEST_MODE=mode, REVIEW_TEST_EDIT="yes" if edit else "no" ) + if token_log is not None: + env["REVIEW_TEST_TOKEN_LOG"] = token_log return subprocess.run( [sys.executable, str(RUNNER), *args], cwd=cwd or self.repo, @@ -1689,6 +1698,110 @@ def test_since_run_appends_to_the_earlier_review_and_archives_both(self): self.assertEqual(state["archive_review_file"], str(self.archive_file)) self.assertEqual(self.archive_file.read_text(), expected) + def shared_token_log(self): + return self.review_root / "token-usage.jsonl" + + def shared_token_rows(self): + return [ + json.loads(line) + for line in self.shared_token_log().read_text().splitlines() + ] + + def test_review_appends_its_token_rows_to_the_shared_log(self): + self.shared_token_log().write_text('{"identifier": "earlier"}\n') + rows = '{"identifier": "123", "total_tokens": 5}\n{"identifier": "123", "total_tokens": 7}\n' + + state = self.assert_success(self.run_review(token_log=rows)) + + self.assertEqual( + self.shared_token_rows(), + [ + {"identifier": "earlier"}, + { + "identifier": "123", + "total_tokens": 5, + "go_run_id": state["run_id"], + "since_run_id": None, + }, + { + "identifier": "123", + "total_tokens": 7, + "go_run_id": state["run_id"], + "since_run_id": None, + }, + ], + ) + self.assertNotIn("token_log_error", state) + + def test_since_run_token_rows_name_the_earlier_run(self): + earlier, result = self.review_since_earlier_run( + mode="append_seed", token_log='{"review_mode": "delta"}\n' + ) + state = self.assert_success(result) + + self.assertEqual( + self.shared_token_rows()[-1], + { + "review_mode": "delta", + "go_run_id": state["run_id"], + "since_run_id": earlier["run_id"], + }, + ) + + def test_review_without_a_child_token_log_leaves_the_shared_log_alone(self): + self.assert_success(self.run_review()) + + self.assertFalse(self.shared_token_log().exists()) + + def test_reused_review_does_not_append_its_rows_again(self): + self.assert_success(self.run_review(token_log='{"total_tokens": 1}\n')) + self.assert_success(self.run_review(token_log='{"total_tokens": 1}\n')) + + self.assertEqual(len(self.shared_token_rows()), 1) + + def test_failed_review_does_not_append_its_token_rows(self): + for mode in ("blocked", "change_head"): + with self.subTest(mode=mode): + result = self.run_review(mode=mode, token_log='{"total_tokens": 1}\n') + self.assertNotEqual(result.returncode, 0, result.stdout) + self.assertFalse(self.shared_token_log().exists()) + + def test_token_rows_that_are_not_json_objects_are_dropped(self): + rows = '{"total_tokens": 1}\nnot json\n[1, 2]\n\n{"total_tokens": 2}\n' + + self.assert_success(self.run_review(token_log=rows)) + + self.assertEqual( + [row["total_tokens"] for row in self.shared_token_rows()], [1, 2] + ) + + def test_undecodable_token_rows_are_dropped_and_the_review_still_succeeds(self): + rows = '{"total_tokens": 1}\n\udcff\udcfe{"total_tokens": 2}\n' + + state = self.assert_success(self.run_review(token_log=rows)) + + self.assertEqual( + [row["total_tokens"] for row in self.shared_token_rows()], [1] + ) + self.assertNotIn("token_log_error", state) + + def test_shared_log_symlink_is_not_followed_and_the_review_still_succeeds(self): + self.shared_token_log().symlink_to(self.protected) + + state = self.assert_success(self.run_review(token_log='{"total_tokens": 1}\n')) + + self.assertEqual(self.protected.read_text(), "Preserve this content.\n") + self.assertIn("token_log_error", state) + self.assertEqual(self.state()["phase"], "reviewed") + + def test_child_token_log_symlink_is_not_read(self): + self.env["REVIEW_TEST_TOKEN_LOG_LINK"] = str(self.protected) + + state = self.assert_success(self.run_review(token_log="")) + + self.assertFalse(self.shared_token_log().exists()) + self.assertIn("token_log_error", state) + def test_since_run_child_returning_the_untouched_seed_fails(self): earlier, result = self.review_since_earlier_run(mode="untouched_seed") self.assertNotEqual(result.returncode, 0, result.stdout) From 81b3cfc156f1f4604d50dc70b54c63b193d5b025 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Thu, 29 Oct 2026 09:14:43 -0700 Subject: [PATCH 4/6] Drop the copies of #267's commits that a rebase re-applied Claude-Session: https://claude.ai/code/session_01AovXvYY2uZDJFssBEumPmd --- .github/workflows/test.yml | 1 - ai/README.md | 5 -- .../.claude-plugin/plugin.json | 5 -- ai/evals/smallest-change/README.md | 47 ----------- ai/evals/smallest-change/evals/.gitignore | 1 - .../evals/csv-export/case.yaml | 33 -------- .../evals/csv-export/grade.json | 6 -- .../evals/csv-export/hidden/test_hidden.py | 41 --------- .../evals/csv-export/repo/app/__init__.py | 0 .../evals/csv-export/repo/app/report.py | 12 --- .../evals/csv-export/repo/pyproject.toml | 5 -- .../evals/csv-export/repo/tests/__init__.py | 0 .../csv-export/repo/tests/test_report.py | 15 ---- .../evals/csv-export/scaffold.sh | 3 - ai/evals/smallest-change/evals/grade.py | 84 ------------------- .../evals/permalink-reuse/case.yaml | 33 -------- .../evals/permalink-reuse/grade.json | 6 -- .../permalink-reuse/hidden/test_hidden.py | 26 ------ .../permalink-reuse/repo/blog/__init__.py | 0 .../evals/permalink-reuse/repo/blog/models.py | 15 ---- .../evals/permalink-reuse/repo/blog/text.py | 15 ---- .../evals/permalink-reuse/repo/pyproject.toml | 5 -- .../permalink-reuse/repo/tests/__init__.py | 0 .../permalink-reuse/repo/tests/test_models.py | 15 ---- .../permalink-reuse/repo/tests/test_text.py | 17 ---- .../evals/permalink-reuse/scaffold.sh | 3 - ai/evals/smallest-change/evals/scaffold.sh | 44 ---------- .../evals/shared-bug-fix/case.yaml | 28 ------- .../evals/shared-bug-fix/grade.json | 3 - .../shared-bug-fix/hidden/test_hidden.py | 29 ------- .../evals/shared-bug-fix/repo/pyproject.toml | 5 -- .../shared-bug-fix/repo/shop/__init__.py | 0 .../evals/shared-bug-fix/repo/shop/cart.py | 15 ---- .../evals/shared-bug-fix/repo/shop/money.py | 10 --- .../evals/shared-bug-fix/repo/shop/refunds.py | 9 -- .../shared-bug-fix/repo/tests/__init__.py | 0 .../shared-bug-fix/repo/tests/test_shop.py | 24 ------ .../evals/shared-bug-fix/scaffold.sh | 3 - ai/evals/smallest-change/hooks/hooks.json | 14 ---- ai/evals/smallest-change/rule.md | 15 ---- ai/skills/go/SKILL.md | 16 ---- ai/tests/test-eval-rule-sync.sh | 22 ----- 42 files changed, 630 deletions(-) delete mode 100644 ai/evals/smallest-change/.claude-plugin/plugin.json delete mode 100644 ai/evals/smallest-change/README.md delete mode 100644 ai/evals/smallest-change/evals/.gitignore delete mode 100644 ai/evals/smallest-change/evals/csv-export/case.yaml delete mode 100644 ai/evals/smallest-change/evals/csv-export/grade.json delete mode 100644 ai/evals/smallest-change/evals/csv-export/hidden/test_hidden.py delete mode 100644 ai/evals/smallest-change/evals/csv-export/repo/app/__init__.py delete mode 100644 ai/evals/smallest-change/evals/csv-export/repo/app/report.py delete mode 100644 ai/evals/smallest-change/evals/csv-export/repo/pyproject.toml delete mode 100644 ai/evals/smallest-change/evals/csv-export/repo/tests/__init__.py delete mode 100644 ai/evals/smallest-change/evals/csv-export/repo/tests/test_report.py delete mode 100755 ai/evals/smallest-change/evals/csv-export/scaffold.sh delete mode 100644 ai/evals/smallest-change/evals/grade.py delete mode 100644 ai/evals/smallest-change/evals/permalink-reuse/case.yaml delete mode 100644 ai/evals/smallest-change/evals/permalink-reuse/grade.json delete mode 100644 ai/evals/smallest-change/evals/permalink-reuse/hidden/test_hidden.py delete mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/blog/__init__.py delete mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/blog/models.py delete mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/blog/text.py delete mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/pyproject.toml delete mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/tests/__init__.py delete mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_models.py delete mode 100644 ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_text.py delete mode 100755 ai/evals/smallest-change/evals/permalink-reuse/scaffold.sh delete mode 100755 ai/evals/smallest-change/evals/scaffold.sh delete mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/case.yaml delete mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/grade.json delete mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/hidden/test_hidden.py delete mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/pyproject.toml delete mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/__init__.py delete mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/cart.py delete mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/money.py delete mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/refunds.py delete mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/__init__.py delete mode 100644 ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/test_shop.py delete mode 100755 ai/evals/smallest-change/evals/shared-bug-fix/scaffold.sh delete mode 100644 ai/evals/smallest-change/hooks/hooks.json delete mode 100644 ai/evals/smallest-change/rule.md delete mode 100755 ai/tests/test-eval-rule-sync.sh diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9eac986a..32558f2c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -24,7 +24,6 @@ jobs: ai/skills/wait-for-pr-reviews/scripts/tests/test-portable-skill.sh ai/skills/address-pr-reviews/scripts/tests/test-portable-skill.sh ai/tests/test-plain-writing-contract.sh - ai/tests/test-eval-rule-sync.sh ai/tests/test-ai-installers.sh ai/tests/test-install-claude-automation.sh ai/tests/test-lint-edited-file.sh diff --git a/ai/README.md b/ai/README.md index f6859d6d..26b0d88c 100644 --- a/ai/README.md +++ b/ai/README.md @@ -50,10 +50,6 @@ CI enforces all three rules: the same script with `--check` rejects a copy that When `implementation-planner` marks a plan with two or more stages `**Execution**: staged`, `go` implements it one stage at a time. The planner marks a plan staged only when one session would run out of context before it finished the work, and `go --staged` or `--single-pass` overrides its choice. A fresh agent writes each stage, a second fresh agent checks it against the stage's success criteria and `Check` command, and the parent commits it, so an interrupted run resumes in the stage it stopped in. `implementation-planner` writes the `Check` command and a `## Decisions` section that the stage agents follow. -## Evals - -`ai/evals/` holds `claude plugin eval` suites. [`ai/evals/smallest-change/`](evals/smallest-change/README.md) compares implementation with and without the Smallest complete change rule that `go` gives its implementers. - ## Fresh review processes `go` supports Claude Code and Codex. It passes its current harness explicitly to `skills/go/scripts/run-review.py`, which starts a fresh review in the same checkout. Claude uses print mode; Codex uses `exec`. Both save the review result under the checkout's `.notes/` directory so the parent can resume at validation after a context clear. The runner requires Python 3.11 or later and the selected CLI on `PATH`; it never substitutes the other harness. The installed `review-code` skill must support `REVIEW_CODE_REVIEW_DIR`; update it from its source repository with `bin/setup` if the runner reports an older installation. Review sessions, reports, temporary checkouts, and hook files stay under each attempt in `.notes/go-reviews/`. The Codex child receives no write grants for shared review caches. After success, the parent copies the report into the shared review history, where it survives removal of the implementation worktree. A Claude review runs on the accounts that docket's `config.toml` names in `claude_config_dir` and `claude_fallback_config_dirs`, in the order docket's rotation picks them. The runner skips each account that a usage limit in docket's `limits.jsonl` still applies to. A limit applies to every config directory signed in to the login (one account in one organization) that docket recorded it with. `run-review.py accounts` prints that rotation, the limits docket recorded, and the order a review would try the accounts in now, as JSON, and `usage-all` shows it below each account's usage. Without a docket config, the review runs on the current account. The runner refuses an account that is not signed in or has no `review-code` skill. When an account hits its usage limit before the review edits any files, the runner reviews again on the next account that is signed in to a different login, and then on the current account if the list leaves it out. A run with `--since-run ` copies that earlier run's report into its own attempt and asks `review-code` for an `--append` review of the changes since that report. After a successful review, the runner copies the review's token-usage rows into the shared `token-usage.jsonl` with the `go` run IDs added, because the copy under `.notes/go-reviews/` is deleted with the worktree. @@ -90,7 +86,6 @@ ai/skills/wait-for-pr-reviews/scripts/tests/test-pending-reviews.sh ai/skills/wait-for-pr-reviews/scripts/tests/test-portable-skill.sh ai/skills/address-pr-reviews/scripts/tests/test-portable-skill.sh ai/tests/test-plain-writing-contract.sh -ai/tests/test-eval-rule-sync.sh python3 ai/skills/plain-writing/scripts/tests/test_plain_writing_lint.py ai/tests/test-ai-installers.sh ai/tests/test-command-log.sh diff --git a/ai/evals/smallest-change/.claude-plugin/plugin.json b/ai/evals/smallest-change/.claude-plugin/plugin.json deleted file mode 100644 index be50ce30..00000000 --- a/ai/evals/smallest-change/.claude-plugin/plugin.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "name": "smallest-change-eval", - "version": "0.1.0", - "description": "Eval-only plugin that adds /go's Smallest complete change rule to the session context, so claude plugin eval can compare implementation with and without it." -} diff --git a/ai/evals/smallest-change/README.md b/ai/evals/smallest-change/README.md deleted file mode 100644 index 43f09ad7..00000000 --- a/ai/evals/smallest-change/README.md +++ /dev/null @@ -1,47 +0,0 @@ -# Smallest complete change eval - -An eval-only plugin that measures what the Smallest complete change rule in `ai/skills/go/SKILL.md` does to a plain implementation task. It never runs /go, so a run opens no PR and requests no review. - -## What each arm loads - -- **With:** this plugin. Its only content is a `SessionStart` hook that prints `rule.md` into the session context. The plugin has no skill, so the rule is in context on every run, and the result does not depend on whether the model chooses to invoke a skill. /go gives its implementers this rule together with the Judgment calls rule, but this arm gets this rule alone, so the "easy-to-reverse call" that item 1 mentions has no definition here. -- **Without:** the baseline arm that `--ablation with-without` adds. It runs the same prompt in the same scaffolded workspace with no plugin loaded, so the rule is the only difference between the arms. - -`rule.md` is a copy of the rule from `ai/skills/go/SKILL.md`. `ai/tests/test-eval-rule-sync.sh` fails when the two copies differ. - -## Cases - -Each case is a small Python repo where an over-built answer is tempting. - -| Case | Task | What over-building looks like | -| --- | --- | --- | -| `csv-export` | Add `write_csv(rows, path)` | Hand-rolled quoting, or delimiter and encoding options nobody asked for | -| `permalink-reuse` | Add `Post.permalink` | A new slug function when `blog/text.py` already has `slugify` | -| `shared-bug-fix` | Fix a thousands-separator bug reported against the cart | Patching only the cart, when the refund path calls the same parser | - -`shared-bug-fix` guards the other direction: the rule must shrink the solution, not the scope, so a caller-only patch fails its hidden tests. - -## How grading works - -`claude plugin eval` has no grader that runs a command, so the grading runs inside the workspace. `evals/scaffold.sh` copies the case's `repo/` into the workspace and commits it. It also puts the hidden tests and `grade.py` in `.eval/` and writes a `Stop` hook to `.claude/settings.json`. When the session ends, the hook runs `grade.py`, which writes `eval-result.txt`. The case's regex graders read that file: - -- `hidden_tests: pass` when the hidden tests and the repo's own tests pass. This grader has weight 2. -- `size: ok` when the production lines added or removed stay within the case's `max_lines` in `grade.json`. Lines under `tests/` and in `test_*` files do not count. -- `dependencies: unchanged` when no dependency file (`pyproject.toml`, `requirements*.txt`, `setup.py`, `setup.cfg`, `Pipfile`) changed. -- `expected_code: ok` (`csv-export` and `permalink-reuse` only) when the change adds a line that uses the `csv` module or `slugify`. - -`eval-result.txt` also records the exact line count and the changed files, which `--keep-temp` leaves on disk for a closer look. - -## Running it - -Each run is a full Claude session, so the full suite (3 cases × 3 runs × 2 arms) is 18 sessions. Start with one case and one run: - -```bash -claude plugin eval ai/evals/smallest-change --scaffold --allow-tools Edit Write Bash --case csv-export --runs 1 --max-cost-usd 1 --no-publish -``` - -`--scaffold` is required, because without it the workspace is empty. `--allow-tools` grants the tools the cases list. `--max-cost-usd 0` loads and plans every case without starting a run, which checks that the cases parse. - -The hidden tests sit in the workspace's `.eval/` folder. Git ignores that folder, but the agent can still find and read it in either arm. - -The graders depend on the eval run loading the workspace's `.claude/settings.json`, which the eval docs do not cover. If every grader fails in both arms and `--keep-temp` shows no `eval-result.txt`, the `Stop` hook did not run. diff --git a/ai/evals/smallest-change/evals/.gitignore b/ai/evals/smallest-change/evals/.gitignore deleted file mode 100644 index fbca2253..00000000 --- a/ai/evals/smallest-change/evals/.gitignore +++ /dev/null @@ -1 +0,0 @@ -results/ diff --git a/ai/evals/smallest-change/evals/csv-export/case.yaml b/ai/evals/smallest-change/evals/csv-export/case.yaml deleted file mode 100644 index e8f0d840..00000000 --- a/ai/evals/smallest-change/evals/csv-export/case.yaml +++ /dev/null @@ -1,33 +0,0 @@ -schema_version: "1.1" -name: csv-export -description: Writing a CSV export, where the standard library's csv module beats hand-rolled quoting and unrequested options. -context: - scaffold_script: scaffold.sh -execution: - max_turns: 25 - timeout_seconds: 300 - allowed_tools: [Read, Glob, Grep, Edit, Write, Bash] - prompt: | - Add a `write_csv(rows, path)` function to `app/report.py`. It writes the rows that `build_rows` returns to a CSV file at `path`: a header row with the column names, then one line per row. Customer names come from a signup form, so they can contain any text. -graders: - - name: hidden-tests - type: regex - target: {source: file, path: eval-result.txt} - pattern: "^hidden_tests: pass$" - flags: m - weight: 2 - - name: diff-size - type: regex - target: {source: file, path: eval-result.txt} - pattern: "^size: ok$" - flags: m - - name: no-new-dependency - type: regex - target: {source: file, path: eval-result.txt} - pattern: "^dependencies: unchanged$" - flags: m - - name: uses-csv-module - type: regex - target: {source: file, path: eval-result.txt} - pattern: "^expected_code: ok$" - flags: m diff --git a/ai/evals/smallest-change/evals/csv-export/grade.json b/ai/evals/smallest-change/evals/csv-export/grade.json deleted file mode 100644 index be557a14..00000000 --- a/ai/evals/smallest-change/evals/csv-export/grade.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "max_lines": 15, - "require_added": { - "app/report.py": "\\bcsv\\." - } -} diff --git a/ai/evals/smallest-change/evals/csv-export/hidden/test_hidden.py b/ai/evals/smallest-change/evals/csv-export/hidden/test_hidden.py deleted file mode 100644 index be8af88a..00000000 --- a/ai/evals/smallest-change/evals/csv-export/hidden/test_hidden.py +++ /dev/null @@ -1,41 +0,0 @@ -import csv -import os -import tempfile -import unittest -from decimal import Decimal - -from app.report import write_csv - - -def read_back(path): - with open(path, newline="") as f: - return list(csv.reader(f)) - - -class WriteCsvTest(unittest.TestCase): - def setUp(self): - handle, self.path = tempfile.mkstemp(suffix=".csv") - os.close(handle) - self.addCleanup(os.remove, self.path) - - def test_writes_header_then_rows(self): - write_csv([{"order_id": 1, "customer": "Ada", "total": Decimal("6.00")}], self.path) - - self.assertEqual(read_back(self.path), [["order_id", "customer", "total"], ["1", "Ada", "6.00"]]) - - def test_quotes_commas_quotes_and_newlines_in_customer_names(self): - names = ['Smith, Jo', 'The "Best" Shop', "Line one\nLine two"] - rows = [{"order_id": i, "customer": name, "total": Decimal("1.00")} for i, name in enumerate(names)] - - write_csv(rows, self.path) - - self.assertEqual([row[1] for row in read_back(self.path)[1:]], names) - - def test_writes_only_the_header_when_there_are_no_rows(self): - write_csv([], self.path) - - self.assertEqual(read_back(self.path), [["order_id", "customer", "total"]]) - - -if __name__ == "__main__": - unittest.main() diff --git a/ai/evals/smallest-change/evals/csv-export/repo/app/__init__.py b/ai/evals/smallest-change/evals/csv-export/repo/app/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/ai/evals/smallest-change/evals/csv-export/repo/app/report.py b/ai/evals/smallest-change/evals/csv-export/repo/app/report.py deleted file mode 100644 index 4e46e676..00000000 --- a/ai/evals/smallest-change/evals/csv-export/repo/app/report.py +++ /dev/null @@ -1,12 +0,0 @@ -from decimal import Decimal - -COLUMNS = ("order_id", "customer", "total") - - -def build_rows(orders): - """Returns one row per order, in the column order of COLUMNS.""" - rows = [] - for order in orders: - total = sum((Decimal(item["price"]) * item["quantity"] for item in order["items"]), Decimal("0")) - rows.append({"order_id": order["id"], "customer": order["customer"], "total": total}) - return rows diff --git a/ai/evals/smallest-change/evals/csv-export/repo/pyproject.toml b/ai/evals/smallest-change/evals/csv-export/repo/pyproject.toml deleted file mode 100644 index 2f5ab3a9..00000000 --- a/ai/evals/smallest-change/evals/csv-export/repo/pyproject.toml +++ /dev/null @@ -1,5 +0,0 @@ -[project] -name = "orders" -version = "0.1.0" -requires-python = ">=3.9" -dependencies = [] diff --git a/ai/evals/smallest-change/evals/csv-export/repo/tests/__init__.py b/ai/evals/smallest-change/evals/csv-export/repo/tests/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/ai/evals/smallest-change/evals/csv-export/repo/tests/test_report.py b/ai/evals/smallest-change/evals/csv-export/repo/tests/test_report.py deleted file mode 100644 index 190a24a3..00000000 --- a/ai/evals/smallest-change/evals/csv-export/repo/tests/test_report.py +++ /dev/null @@ -1,15 +0,0 @@ -import unittest -from decimal import Decimal - -from app.report import build_rows - - -class BuildRowsTest(unittest.TestCase): - def test_totals_each_order(self): - orders = [{"id": 7, "customer": "Ada", "items": [{"price": "2.50", "quantity": 2}, {"price": "1.00", "quantity": 1}]}] - - self.assertEqual(build_rows(orders), [{"order_id": 7, "customer": "Ada", "total": Decimal("6.00")}]) - - -if __name__ == "__main__": - unittest.main() diff --git a/ai/evals/smallest-change/evals/csv-export/scaffold.sh b/ai/evals/smallest-change/evals/csv-export/scaffold.sh deleted file mode 100755 index 1aace498..00000000 --- a/ai/evals/smallest-change/evals/csv-export/scaffold.sh +++ /dev/null @@ -1,3 +0,0 @@ -#!/usr/bin/env bash -case_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -exec bash "${case_dir}/../scaffold.sh" "${case_dir}" diff --git a/ai/evals/smallest-change/evals/grade.py b/ai/evals/smallest-change/evals/grade.py deleted file mode 100644 index a7e21412..00000000 --- a/ai/evals/smallest-change/evals/grade.py +++ /dev/null @@ -1,84 +0,0 @@ -"""Grades an eval workspace after the agent stops and writes eval-result.txt for the case's regex graders. - -Usage: grade.py - -grade.json in the same folder sets `max_lines`, the most production lines the change may add or remove, and optionally `require_added`, a map from a file path to a regex that some added line in that file must match. -""" - -import json -import os -import re -import subprocess -import sys -from pathlib import Path - -RESULT = "eval-result.txt" -DEPENDENCY_FILES = re.compile(r"(^|/)(pyproject\.toml|setup\.py|setup\.cfg|Pipfile|requirements[^/]*\.txt)$") - - -def git(root, *args): - return subprocess.run(["git", *args], cwd=root, capture_output=True, text=True, check=True).stdout - - -def is_test(path): - return path.startswith("tests/") or Path(path).name.startswith("test_") - - -def run_tests(root, start_dir, top_dir): - env = {**os.environ, "PYTHONPATH": str(root), "PYTHONDONTWRITEBYTECODE": "1"} - result = subprocess.run( - [sys.executable, "-m", "unittest", "discover", "-s", start_dir, "-t", top_dir], - cwd=root, - env=env, - capture_output=True, - text=True, - timeout=120, - ) - return result.returncode == 0, result.stderr[-2000:] - - -def main(): - root = Path(sys.argv[1]) - config = json.loads((Path(__file__).parent / "grade.json").read_text()) - base = (Path(__file__).parent / "base").read_text().strip() - - hidden_ok, hidden_log = run_tests(root, ".eval/hidden", ".eval/hidden") - repo_ok, repo_log = run_tests(root, "tests", ".") - - # Intent-to-add makes new files show up in `git diff` without staging their contents. - git(root, "add", "-N", "--", ".", f":!{RESULT}") - changed = [] - for line in git(root, "diff", "--numstat", base, "--", ".", f":!{RESULT}").splitlines(): - added, removed, path = line.split("\t", 2) - changed.append((path, int(added) if added != "-" else 0, int(removed) if removed != "-" else 0)) - - production_lines = sum(a + r for path, a, r in changed if not is_test(path)) - dependency_files = [path for path, _, _ in changed if DEPENDENCY_FILES.search(path)] - - lines = [ - f"hidden_tests: {'pass' if hidden_ok and repo_ok else 'fail'}", - f"size: {'ok' if production_lines <= config['max_lines'] else 'over'}", - f"production_lines_changed: {production_lines} (limit {config['max_lines']})", - f"dependencies: {'changed' if dependency_files else 'unchanged'}", - ] - if config.get("require_added"): - missing = [] - for path, pattern in config["require_added"].items(): - added_lines = [ - l[1:] for l in git(root, "diff", "-U0", base, "--", path).splitlines() - if l.startswith("+") and not l.startswith("+++") - ] - if not any(re.search(pattern, l) for l in added_lines): - missing.append(path) - lines.append(f"expected_code: {'missing in ' + ', '.join(missing) if missing else 'ok'}") - lines.append(f"changed_files: {', '.join(path for path, _, _ in changed) or 'none'}") - if not hidden_ok: - lines += ["", "hidden test output:", hidden_log] - if not repo_ok: - lines += ["", "repo test output:", repo_log] - - (root / RESULT).write_text("\n".join(lines) + "\n") - - -if __name__ == "__main__": - main() diff --git a/ai/evals/smallest-change/evals/permalink-reuse/case.yaml b/ai/evals/smallest-change/evals/permalink-reuse/case.yaml deleted file mode 100644 index 23423e67..00000000 --- a/ai/evals/smallest-change/evals/permalink-reuse/case.yaml +++ /dev/null @@ -1,33 +0,0 @@ -schema_version: "1.1" -name: permalink-reuse -description: Adding a URL path to a model, where the codebase already has a slugify helper that handles accents and punctuation. -context: - scaffold_script: scaffold.sh -execution: - max_turns: 25 - timeout_seconds: 300 - allowed_tools: [Read, Glob, Grep, Edit, Write, Bash] - prompt: | - Add a `permalink` property to `Post` in `blog/models.py`. It returns the post's URL path in the form `/blog////`, where the slug comes from the title. Titles can contain accented letters and punctuation. -graders: - - name: hidden-tests - type: regex - target: {source: file, path: eval-result.txt} - pattern: "^hidden_tests: pass$" - flags: m - weight: 2 - - name: diff-size - type: regex - target: {source: file, path: eval-result.txt} - pattern: "^size: ok$" - flags: m - - name: no-new-dependency - type: regex - target: {source: file, path: eval-result.txt} - pattern: "^dependencies: unchanged$" - flags: m - - name: reuses-slugify - type: regex - target: {source: file, path: eval-result.txt} - pattern: "^expected_code: ok$" - flags: m diff --git a/ai/evals/smallest-change/evals/permalink-reuse/grade.json b/ai/evals/smallest-change/evals/permalink-reuse/grade.json deleted file mode 100644 index f4429e4b..00000000 --- a/ai/evals/smallest-change/evals/permalink-reuse/grade.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "max_lines": 10, - "require_added": { - "blog/models.py": "\\bslugify\\b" - } -} diff --git a/ai/evals/smallest-change/evals/permalink-reuse/hidden/test_hidden.py b/ai/evals/smallest-change/evals/permalink-reuse/hidden/test_hidden.py deleted file mode 100644 index c1d7076b..00000000 --- a/ai/evals/smallest-change/evals/permalink-reuse/hidden/test_hidden.py +++ /dev/null @@ -1,26 +0,0 @@ -import unittest -from datetime import date - -from blog.models import Post - - -def permalink(title, published=date(2026, 3, 9)): - return Post(title=title, published=published, body="").permalink - - -class PermalinkTest(unittest.TestCase): - def test_builds_year_month_slug_path(self): - self.assertEqual(permalink("Hello World"), "/blog/2026/03/hello-world/") - - def test_drops_accents_and_punctuation(self): - self.assertEqual(permalink("Café déjà vu!"), "/blog/2026/03/cafe-deja-vu/") - - def test_collapses_repeated_separators(self): - self.assertEqual(permalink(" Rust -- & Go "), "/blog/2026/03/rust-go/") - - def test_pads_single_digit_months(self): - self.assertEqual(permalink("New Year", date(2027, 1, 1)), "/blog/2027/01/new-year/") - - -if __name__ == "__main__": - unittest.main() diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/__init__.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/models.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/models.py deleted file mode 100644 index 2e1d0186..00000000 --- a/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/models.py +++ /dev/null @@ -1,15 +0,0 @@ -from dataclasses import dataclass -from datetime import date - -from blog.text import truncate - - -@dataclass -class Post: - title: str - published: date - body: str - - @property - def excerpt(self): - return truncate(self.body, 140) diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/text.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/text.py deleted file mode 100644 index 1e289642..00000000 --- a/ai/evals/smallest-change/evals/permalink-reuse/repo/blog/text.py +++ /dev/null @@ -1,15 +0,0 @@ -import re -import unicodedata - - -def slugify(value): - """Lowercases `value`, drops accents and punctuation, and joins the words with hyphens.""" - ascii_text = unicodedata.normalize("NFKD", value).encode("ascii", "ignore").decode("ascii") - return re.sub(r"[^a-z0-9]+", "-", ascii_text.lower()).strip("-") - - -def truncate(value, length): - """Cuts `value` to at most `length` characters at a word boundary and adds an ellipsis when it cuts.""" - if len(value) <= length: - return value - return value[: length - 1].rsplit(" ", 1)[0] + "…" diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/pyproject.toml b/ai/evals/smallest-change/evals/permalink-reuse/repo/pyproject.toml deleted file mode 100644 index bb7f52a8..00000000 --- a/ai/evals/smallest-change/evals/permalink-reuse/repo/pyproject.toml +++ /dev/null @@ -1,5 +0,0 @@ -[project] -name = "blog" -version = "0.1.0" -requires-python = ">=3.9" -dependencies = [] diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/__init__.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_models.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_models.py deleted file mode 100644 index cc36dc3b..00000000 --- a/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_models.py +++ /dev/null @@ -1,15 +0,0 @@ -import unittest -from datetime import date - -from blog.models import Post - - -class PostTest(unittest.TestCase): - def test_excerpt_truncates_the_body(self): - post = Post(title="T", published=date(2026, 1, 2), body="word " * 100) - - self.assertLessEqual(len(post.excerpt), 140) - - -if __name__ == "__main__": - unittest.main() diff --git a/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_text.py b/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_text.py deleted file mode 100644 index 9a058ea4..00000000 --- a/ai/evals/smallest-change/evals/permalink-reuse/repo/tests/test_text.py +++ /dev/null @@ -1,17 +0,0 @@ -import unittest - -from blog.text import slugify, truncate - - -class SlugifyTest(unittest.TestCase): - def test_lowercases_and_hyphenates(self): - self.assertEqual(slugify("Hello World"), "hello-world") - - -class TruncateTest(unittest.TestCase): - def test_leaves_short_text_alone(self): - self.assertEqual(truncate("short", 10), "short") - - -if __name__ == "__main__": - unittest.main() diff --git a/ai/evals/smallest-change/evals/permalink-reuse/scaffold.sh b/ai/evals/smallest-change/evals/permalink-reuse/scaffold.sh deleted file mode 100755 index 1aace498..00000000 --- a/ai/evals/smallest-change/evals/permalink-reuse/scaffold.sh +++ /dev/null @@ -1,3 +0,0 @@ -#!/usr/bin/env bash -case_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -exec bash "${case_dir}/../scaffold.sh" "${case_dir}" diff --git a/ai/evals/smallest-change/evals/scaffold.sh b/ai/evals/smallest-change/evals/scaffold.sh deleted file mode 100755 index 38703aba..00000000 --- a/ai/evals/smallest-change/evals/scaffold.sh +++ /dev/null @@ -1,44 +0,0 @@ -#!/usr/bin/env bash -# Builds one case's workspace in the current directory: the case's repo/ as a fresh git commit, plus a hidden .eval/ folder and a Stop hook that grades the agent's work when the session ends. -# Usage: scaffold.sh -set -euo pipefail - -case_dir="$1" -shared_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" - -cp -R "${case_dir}/repo/." . -mkdir -p .eval .claude -cp -R "${case_dir}/hidden" .eval/hidden -cp "${case_dir}/grade.json" "${shared_dir}/grade.py" .eval/ - -# The eval run gets a fresh HOME. A fresh HOME breaks version-manager shims such as pyenv's. The agent's python3 then resolves to the first one on PATH outside HOME, and the hook grades with that same interpreter. -python="$(type -ap python3 | grep -v "^${HOME}/" | head -n 1 || true)" -python="${python:-$(command -v python3)}" - -# The Stop hook lives in the workspace's own settings rather than in the plugin. Both eval arms therefore grade the same way. -cat >.claude/settings.json <.gitignore - -git init -q -git add -A -# The scaffold runs with the operator's git config. The commit turns off that config's signing and hooks. -git -c user.name=eval -c user.email=eval@example.com -c commit.gpgsign=false -c core.hooksPath=/dev/null commit -qm "Initial commit" -# grade.py diffs against this commit, so a commit by the agent still counts. -git rev-parse HEAD >.eval/base diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/case.yaml b/ai/evals/smallest-change/evals/shared-bug-fix/case.yaml deleted file mode 100644 index 693d00df..00000000 --- a/ai/evals/smallest-change/evals/shared-bug-fix/case.yaml +++ /dev/null @@ -1,28 +0,0 @@ -schema_version: "1.1" -name: shared-bug-fix -description: A bug report names one caller of a shared parser; the fix belongs in the parser, so a second caller that the report does not mention gets fixed too. -context: - scaffold_script: scaffold.sh -execution: - max_turns: 25 - timeout_seconds: 300 - allowed_tools: [Read, Glob, Grep, Edit, Write, Bash] - prompt: | - Bug report: adding an item priced `$1,299.00` to the cart fails with `ValueError: invalid amount: '$1,299.00'`. Fix it. -graders: - - name: hidden-tests - type: regex - target: {source: file, path: eval-result.txt} - pattern: "^hidden_tests: pass$" - flags: m - weight: 2 - - name: diff-size - type: regex - target: {source: file, path: eval-result.txt} - pattern: "^size: ok$" - flags: m - - name: no-new-dependency - type: regex - target: {source: file, path: eval-result.txt} - pattern: "^dependencies: unchanged$" - flags: m diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/grade.json b/ai/evals/smallest-change/evals/shared-bug-fix/grade.json deleted file mode 100644 index 71b5ddc3..00000000 --- a/ai/evals/smallest-change/evals/shared-bug-fix/grade.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "max_lines": 8 -} diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/hidden/test_hidden.py b/ai/evals/smallest-change/evals/shared-bug-fix/hidden/test_hidden.py deleted file mode 100644 index 7f4431ab..00000000 --- a/ai/evals/smallest-change/evals/shared-bug-fix/hidden/test_hidden.py +++ /dev/null @@ -1,29 +0,0 @@ -import unittest -from decimal import Decimal - -from shop.cart import Cart -from shop.money import parse_amount -from shop.refunds import refund_amount - - -class ThousandsSeparatorTest(unittest.TestCase): - def test_cart_accepts_thousands_separators(self): - cart = Cart() - cart.add_item("A", "$1,299.00") - - self.assertEqual(cart.total, Decimal("1299.00")) - - def test_refund_accepts_thousands_separators(self): - self.assertEqual(refund_amount(Decimal("5000"), "1,299.00"), Decimal("1299.00")) - - def test_plain_amounts_still_parse(self): - self.assertEqual(parse_amount(" $0.99 "), Decimal("0.99")) - - def test_garbage_still_raises_value_error(self): - for text in ["abc", "", "$"]: - with self.subTest(text=text), self.assertRaises(ValueError): - parse_amount(text) - - -if __name__ == "__main__": - unittest.main() diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/pyproject.toml b/ai/evals/smallest-change/evals/shared-bug-fix/repo/pyproject.toml deleted file mode 100644 index 6643ff01..00000000 --- a/ai/evals/smallest-change/evals/shared-bug-fix/repo/pyproject.toml +++ /dev/null @@ -1,5 +0,0 @@ -[project] -name = "shop" -version = "0.1.0" -requires-python = ">=3.9" -dependencies = [] diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/__init__.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/cart.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/cart.py deleted file mode 100644 index 7bf0b73b..00000000 --- a/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/cart.py +++ /dev/null @@ -1,15 +0,0 @@ -from decimal import Decimal - -from shop.money import parse_amount - - -class Cart: - def __init__(self): - self.items = [] - - def add_item(self, sku, price_text): - self.items.append((sku, parse_amount(price_text))) - - @property - def total(self): - return sum((price for _, price in self.items), Decimal("0")) diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/money.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/money.py deleted file mode 100644 index 68cf7553..00000000 --- a/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/money.py +++ /dev/null @@ -1,10 +0,0 @@ -from decimal import Decimal, InvalidOperation - - -def parse_amount(text): - """Parses a price such as "$12.50" or "12.50" into a Decimal. Raises ValueError on anything else.""" - cleaned = text.strip().removeprefix("$") - try: - return Decimal(cleaned) - except InvalidOperation: - raise ValueError(f"invalid amount: {text!r}") from None diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/refunds.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/refunds.py deleted file mode 100644 index 348a5f76..00000000 --- a/ai/evals/smallest-change/evals/shared-bug-fix/repo/shop/refunds.py +++ /dev/null @@ -1,9 +0,0 @@ -from shop.money import parse_amount - - -def refund_amount(order_total, amount_text): - """Returns the amount to refund. Raises ValueError when it exceeds the order total.""" - amount = parse_amount(amount_text) - if amount > order_total: - raise ValueError("refund exceeds order total") - return amount diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/__init__.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/test_shop.py b/ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/test_shop.py deleted file mode 100644 index 7330f72f..00000000 --- a/ai/evals/smallest-change/evals/shared-bug-fix/repo/tests/test_shop.py +++ /dev/null @@ -1,24 +0,0 @@ -import unittest -from decimal import Decimal - -from shop.cart import Cart -from shop.refunds import refund_amount - - -class CartTest(unittest.TestCase): - def test_totals_items(self): - cart = Cart() - cart.add_item("A", "$12.50") - cart.add_item("B", "0.50") - - self.assertEqual(cart.total, Decimal("13.00")) - - -class RefundTest(unittest.TestCase): - def test_refuses_more_than_the_order_total(self): - with self.assertRaises(ValueError): - refund_amount(Decimal("5"), "$6") - - -if __name__ == "__main__": - unittest.main() diff --git a/ai/evals/smallest-change/evals/shared-bug-fix/scaffold.sh b/ai/evals/smallest-change/evals/shared-bug-fix/scaffold.sh deleted file mode 100755 index 1aace498..00000000 --- a/ai/evals/smallest-change/evals/shared-bug-fix/scaffold.sh +++ /dev/null @@ -1,3 +0,0 @@ -#!/usr/bin/env bash -case_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -exec bash "${case_dir}/../scaffold.sh" "${case_dir}" diff --git a/ai/evals/smallest-change/hooks/hooks.json b/ai/evals/smallest-change/hooks/hooks.json deleted file mode 100644 index 4512241d..00000000 --- a/ai/evals/smallest-change/hooks/hooks.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "hooks": { - "SessionStart": [ - { - "hooks": [ - { - "type": "command", - "command": "cat \"${CLAUDE_PLUGIN_ROOT}/rule.md\"" - } - ] - } - ] - } -} diff --git a/ai/evals/smallest-change/rule.md b/ai/evals/smallest-change/rule.md deleted file mode 100644 index f0bc972f..00000000 --- a/ai/evals/smallest-change/rule.md +++ /dev/null @@ -1,15 +0,0 @@ -**Smallest complete change.** Before writing code, list every place the change must reach: callers, tests, fixtures, config, and exports. For each piece the plan does not spell out, take the first option on this list that fully works: - -1. Leave it out. A feature, option, or extension point that neither the plan nor the task asks for does not get built. Flag each one left out as an easy-to-reverse call. -2. Reuse what the codebase already has, such as a helper, service, or pattern, the way the surrounding code uses it. -3. Use the standard library or a platform feature, unless the project has its own version. -4. Use a dependency the project already installs. Do not add a dependency to save a few lines. -5. Write the least code that works and that a reader understands on one read. - -- The rule shrinks the solution, never the scope. Finish every part the change needs, including the callers, tests, and fixtures it breaks. -- Add no wrapper, abstraction, option, config, or type conversion that nothing in this change uses. -- Keep the codebase's existing layers, interfaces, and conventions. -- For a bug fix, find every caller of the function first, then fix the root cause once in the shared code. -- Code that moves or merges keeps its validation and error handling. -- Between two options of the same size, take the one that handles edge cases correctly. -- Never cut validation at trust boundaries, error handling that prevents data loss, security checks, or accessibility. diff --git a/ai/skills/go/SKILL.md b/ai/skills/go/SKILL.md index 2b088a90..4f87d7e6 100644 --- a/ai/skills/go/SKILL.md +++ b/ai/skills/go/SKILL.md @@ -220,22 +220,6 @@ Hard-to-reverse questions go to the user. Append every answer to the plan's `## - Between two options of the same size, take the one that handles edge cases correctly. - Never cut validation at trust boundaries, error handling that prevents data loss, security checks, or accessibility. -**Smallest complete change.** Before writing code, list every place the change must reach: callers, tests, fixtures, config, and exports. For each piece the plan does not spell out, take the first option on this list that fully works: - -1. Leave it out. A feature, option, or extension point that neither the plan nor the task asks for does not get built. Flag each one left out as an easy-to-reverse call. -2. Reuse what the codebase already has, such as a helper, service, or pattern, the way the surrounding code uses it. -3. Use the standard library or a platform feature, unless the project has its own version. -4. Use a dependency the project already installs. Do not add a dependency to save a few lines. -5. Write the least code that works and that a reader understands on one read. - -- The rule shrinks the solution, never the scope. Finish every part the change needs, including the callers, tests, and fixtures it breaks. -- Add no wrapper, abstraction, option, config, or type conversion that nothing in this change uses. -- Keep the codebase's existing layers, interfaces, and conventions. -- For a bug fix, find every caller of the function first, then fix the root cause once in the shared code. -- Code that moves or merges keeps its validation and error handling. -- Between two options of the same size, take the one that handles edge cases correctly. -- Never cut validation at trust boundaries, error handling that prevents data loss, security checks, or accessibility. - #### Single pass First, dispatch the test writer in the background so tests are designed from the spec, not the implementation: diff --git a/ai/tests/test-eval-rule-sync.sh b/ai/tests/test-eval-rule-sync.sh deleted file mode 100755 index 871144f7..00000000 --- a/ai/tests/test-eval-rule-sync.sh +++ /dev/null @@ -1,22 +0,0 @@ -#!/usr/bin/env bash -# The smallest-change eval injects its own copy of /go's Smallest complete change rule. This test checks that the copy matches the rule in the go skill. -set -uo pipefail - -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -AI_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" -SKILL="${AI_DIR}/skills/go/SKILL.md" -RULE="${AI_DIR}/evals/smallest-change/rule.md" - -expected=$(awk '/^\*\*Smallest complete change\.\*\*/{p=1} /^#### /{p=0} p' "$SKILL") - -if [[ -z "$expected" ]]; then - echo "FAIL: no Smallest complete change block in ${SKILL}" - exit 1 -fi - -if diff -u <(printf '%s\n' "$expected") "$RULE"; then - echo "Results: 1 passed, 0 failed" -else - echo "FAIL: ${RULE} differs from the rule in ${SKILL}" - exit 1 -fi From 8db3f13ab268b0f9db89e60f0971b3818623e11f Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Thu, 29 Oct 2026 09:30:44 -0700 Subject: [PATCH 5/6] Address review findings Claude-Session: https://claude.ai/code/session_01AovXvYY2uZDJFssBEumPmd --- ai/skills/go/scripts/review-tier.py | 24 ++++++++++++------- ai/skills/go/scripts/run-review.py | 3 ++- .../go/scripts/tests/test_review_tier.py | 22 +++++++++++++---- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/ai/skills/go/scripts/review-tier.py b/ai/skills/go/scripts/review-tier.py index ddeed7bc..68dbd58a 100644 --- a/ai/skills/go/scripts/review-tier.py +++ b/ai/skills/go/scripts/review-tier.py @@ -163,7 +163,9 @@ def path_risks(path): def raw_entries(base, head): - fields = git("diff", "--raw", "-z", "-M", "-C", base, head).split("\0") + fields = git( + "diff", "--no-ext-diff", "--no-textconv", "--raw", "-z", "-M", "-C", base, head + ).split("\0") entries = [] index = 0 while index < len(fields) and fields[index]: @@ -182,7 +184,9 @@ def raw_entries(base, head): def line_counts(base, head): - fields = git("diff", "--numstat", "-z", "-M", "-C", base, head).split("\0") + fields = git( + "diff", "--no-ext-diff", "--no-textconv", "--numstat", "-z", "-M", "-C", base, head + ).split("\0") counts = {} index = 0 while index < len(fields) and fields[index]: @@ -200,7 +204,10 @@ def line_counts(base, head): def added_lines(base, head): in_hunk = False - for line in git("diff", "--unified=0", "--no-color", base, head).splitlines(): + patch = git( + "diff", "--no-ext-diff", "--no-textconv", "--unified=0", "--no-color", base, head + ) + for line in patch.splitlines(): if line.startswith("diff --git "): in_hunk = False elif line.startswith("@@"): @@ -242,8 +249,6 @@ def decide(purpose, base, head): } lines = sum(sum(count) for count in counts.values() if count) result["stats"] = {"lines": lines, "files": len(entries)} - if not entries: - return result | {"tier": "none", "reasons": []} uncertain = [] risky = [] @@ -265,10 +270,13 @@ def decide(purpose, base, head): risky.append("risky-data-deletion") if purpose == "fix-review": - limit, reasons, sized = FIX_REVIEW_MAX_LINES, risky + uncertain + shape, True + reasons = risky + uncertain + shape + limit = FIX_REVIEW_MAX_LINES else: - limit, reasons, sized = FINAL_SIMPLIFY_MAX_LINES, uncertain, classes != {"docs"} - if sized and lines > limit: + reasons = uncertain + # A docs-only change skips Final simplify at any size. + limit = None if classes == {"docs"} else FINAL_SIMPLIFY_MAX_LINES + if limit is not None and lines > limit: reasons.append(f"too-large: {lines} lines, over {limit}") return result | {"tier": "full" if reasons else "skip", "reasons": reasons} diff --git a/ai/skills/go/scripts/run-review.py b/ai/skills/go/scripts/run-review.py index 13794c8d..62d85402 100644 --- a/ai/skills/go/scripts/run-review.py +++ b/ai/skills/go/scripts/run-review.py @@ -794,7 +794,8 @@ def share_token_rows(review_root_descriptor, skill_dir, run_id, since_run_id): finally: os.close(directory) try: - # One write keeps the rows together when another review appends at the same time. + # The rows go out in one os.write call, so O_APPEND keeps them together when another review appends at the same time. + # The loop only finishes a short write. data = "".join(rows).encode() while data: data = data[os.write(descriptor, data) :] diff --git a/ai/skills/go/scripts/tests/test_review_tier.py b/ai/skills/go/scripts/tests/test_review_tier.py index df2cf48a..477984ab 100644 --- a/ai/skills/go/scripts/tests/test_review_tier.py +++ b/ai/skills/go/scripts/tests/test_review_tier.py @@ -86,6 +86,8 @@ "ai/skills/foo/prompt.txt", "ai/skills/support/templates/weekly-log.txt", "prompts/summary.rst", + "ai/skills/x/tests/SKILL.md", + "tests/AGENTS.md", ) CODE_PATHS = ( "src/widget.py", @@ -291,7 +293,7 @@ def assertValidSchema(self, result): self.assertEqual( set(result), {"purpose", "base", "head", "tier", "reasons", "stats"} ) - self.assertIn(result["tier"], {"none", "skip", "full"}) + self.assertIn(result["tier"], {"skip", "full"}) self.assertIsInstance(result["reasons"], list) for reason in result["reasons"]: self.assertIsInstance(reason, str) @@ -651,6 +653,15 @@ def test_added_data_deletion_line_that_starts_with_plus_plus_is_risky(self): self.assertTier(result, "full") self.assertHasReason(result, "risky-data-deletion") + def test_external_diff_driver_does_not_hide_added_data_deletion(self): + base = self.change({"tests/test_cleanup.py": 'sql = "DELETE FROM users"\n'}) + self.env["GIT_EXTERNAL_DIFF"] = "true" + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "risky-data-deletion") + def test_removed_data_deletion_statement_is_not_risky(self): base = self.change( {"tests/test_cleanup.py": "SELECT 1\nkeep\n"}, @@ -779,21 +790,22 @@ def test_fix_review_lists_every_reason_that_applies(self): with self.subTest(kind=kind): self.assertHasReason(result, kind) - def test_base_equal_to_head_is_none_for_both_purposes(self): + def test_base_equal_to_head_skips_both_purposes(self): for purpose in PURPOSES: with self.subTest(purpose=purpose): result = self.tier(purpose, "HEAD") - self.assertTier(result, "none") + self.assertTier(result, "skip") + self.assertEqual(result["reasons"], []) self.assertEqual(result["stats"], {"lines": 0, "files": 0}) - def test_empty_commit_range_is_none(self): + def test_empty_commit_range_skips(self): base = self.head() self.git("commit", "--allow-empty", "-qm", "Empty") result = self.tier("fix-review", base) - self.assertTier(result, "none") + self.assertTier(result, "skip") def test_unresolvable_base_exits_zero_with_full_and_diff_failed(self): for purpose in PURPOSES: From ba883f8b1c5405e7927cb9f6f5e7570ce9a671a2 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Thu, 29 Oct 2026 09:33:04 -0700 Subject: [PATCH 6/6] Simplify review fixes Claude-Session: https://claude.ai/code/session_01AovXvYY2uZDJFssBEumPmd --- ai/skills/go/scripts/review-tier.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/ai/skills/go/scripts/review-tier.py b/ai/skills/go/scripts/review-tier.py index 68dbd58a..db5ee1e7 100644 --- a/ai/skills/go/scripts/review-tier.py +++ b/ai/skills/go/scripts/review-tier.py @@ -105,6 +105,11 @@ def git(*args): ).stdout.decode(errors="replace") +def git_diff(*args): + # A user's external diff driver or textconv filter would change the output this script parses. + return git("diff", "--no-ext-diff", "--no-textconv", *args) + + def split_path(path): parts = path.split("/") return parts[:-1], parts[-1] @@ -163,9 +168,7 @@ def path_risks(path): def raw_entries(base, head): - fields = git( - "diff", "--no-ext-diff", "--no-textconv", "--raw", "-z", "-M", "-C", base, head - ).split("\0") + fields = git_diff("--raw", "-z", "-M", "-C", base, head).split("\0") entries = [] index = 0 while index < len(fields) and fields[index]: @@ -184,9 +187,7 @@ def raw_entries(base, head): def line_counts(base, head): - fields = git( - "diff", "--no-ext-diff", "--no-textconv", "--numstat", "-z", "-M", "-C", base, head - ).split("\0") + fields = git_diff("--numstat", "-z", "-M", "-C", base, head).split("\0") counts = {} index = 0 while index < len(fields) and fields[index]: @@ -204,10 +205,7 @@ def line_counts(base, head): def added_lines(base, head): in_hunk = False - patch = git( - "diff", "--no-ext-diff", "--no-textconv", "--unified=0", "--no-color", base, head - ) - for line in patch.splitlines(): + for line in git_diff("--unified=0", "--no-color", base, head).splitlines(): if line.startswith("diff --git "): in_hunk = False elif line.startswith("@@"):