diff --git a/.github/workflows/evals.yml b/.github/workflows/evals.yml new file mode 100644 index 0000000..d824e7b --- /dev/null +++ b/.github/workflows/evals.yml @@ -0,0 +1,39 @@ +name: evals + +on: + pull_request: + paths: + - "skills/underwrite/SKILL.md" + - "evals/**" + +permissions: + contents: read + +jobs: + evals: + runs-on: ubuntu-latest + timeout-minutes: 120 + env: + # Each run spends model budget, so it waits for the owner to add the key; fork PRs never see it. + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + GH_TOKEN: ${{ github.token }} + DISABLE_AUTOUPDATER: "1" + steps: + - name: Without a model key the walk cannot run + if: env.ANTHROPIC_API_KEY == '' + run: echo "::notice::ANTHROPIC_API_KEY is not set, so no eval walk ran" + - if: env.ANTHROPIC_API_KEY != '' + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - if: env.ANTHROPIC_API_KEY != '' + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.13" + - if: env.ANTHROPIC_API_KEY != '' + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: "22" + package-manager-cache: false + - if: env.ANTHROPIC_API_KEY != '' + run: npm install --global @anthropic-ai/claude-code@2.1.251 --no-audit --no-fund + - if: env.ANTHROPIC_API_KEY != '' + run: python3 evals/run.py --bare --max-budget-usd 3 --baseline evals/baseline.json diff --git a/README.md b/README.md index 299fba9..dc8241d 100644 --- a/README.md +++ b/README.md @@ -68,6 +68,14 @@ PRIOR #2 pinned break-check to 0.6.0 citing this exact failure mode FIX npx --yes @arethetypeswrong/cli@0.18.5 ``` +Before a flag is written, a fresh-context agent reads it against the frozen diff and blobs +alone and answers three questions: did this PR introduce it, is it real, and would a senior +engineer stop at it. A flag the PR did not introduce, or that is not real, is demoted to +`UNVERIFIED` with the second reading beside it, so a wrong flag costs a glance rather than a +decision. A doubt about whether it is worth stopping for stays beside a confirmed flag +instead, because that call is yours. A flag written where no second reader was available says +"Not read twice" rather than passing for a checked one. + A flag in a PR session has to land on a line the diff touches: the store refuses one on a line the PR never put in front of you, naming the nearest touched line. That holds a flag to the change without proving the change caused what it flags. @@ -295,6 +303,16 @@ diff, trusted context, and object bundle are checked by byte count and SHA-256. Controller cleanliness is rechecked immediately before capture freezes the target. It is a point-in-time precondition, not an attestation against a concurrent local writer. +When the PR moves, the old session stays as it was and a replacement starts beside it at +`-r2/`. `link-predecessor` records the round, the lines the PR touches that changed +since the walked head, and every finding the reviewer settled in an earlier round. The +replacement walks only those moved lines, the page carries the earlier calls forward, and +the store refuses a flag the reviewer already dropped while its line reads the same. By the +fourth round the recommendation says to reconsider the approach rather than fix again. +`history` returns what earlier sessions on the same repository learned, their facts and +recommendation and the findings they left on the paths a new one touches, so the second +review of an area starts from the first. + Linked creation is deterministic for one source action and refuses a conflicting second authorization. A complete child is atomically published at its deterministic path, so a crash cannot expose a partial final session. Request reservation reuses a nonfailed attempt; @@ -306,6 +324,14 @@ moved target before the update blocks landing and requires a replacement source A move detected after the local update preserves the landed commit and reports that a replacement is required, without causing another effect. +`sessionctl.py stats ~/.claude/reviews` is the noise ledger. For every stored session and +in total it counts beats, flags raised, flags still open, accepted, decided, dropped by +reason, unverified beats and their share, flags a second read confirmed or disputed, and +questions asked. A drop recorded before reasons existed counts as unstated. It reads each +session from a backup copy, so counting never upgrades an old session in place. Read it +before and after a change to the skill: a change that raises the dropped share is a +regression even when it finds more. + `render-report.py` turns a session into the page and validates it on the way through. A flag with no fix, a clean beat with no proof, or a proof naming no command gets an `UNPROVEN` chip and exit 2. During a live walk, pending and failed delivery are visible @@ -321,6 +347,29 @@ unsnappable ones into the body rather than dropping them. Iterating on the page design means editing `skills/underwrite/assets/report.css`, or passing `--css` to try something without a commit. +## Evals + +`evals/cases/` holds pull requests with a known answer: misses, where a defect shipped and a +later PR fixed it, and clean PRs that nothing later had to fix. `evals/run.py` clones each at +its base, walks it headlessly with this checkout's skill in eval mode, and scores the session +with `evals/score.py`. A flag the second read demoted does not stand, so it counts neither +for a miss nor against a clean case. Nothing is posted: every case is a merged PR, which freezes to report +audience, and eval mode delivers nothing. + +```bash +python3 evals/run.py # every case +python3 evals/run.py underwrite-83-legacy-guard --keep --max-budget-usd 3 +``` + +A case passes when every expected finding is found, by place and by the keywords of its class, +and nothing else stands. `--baseline evals/baseline.json` fails only on a regression: a case +that passed and now fails, or more misses or false alarms than the committed baseline. Keep a +change to `SKILL.md` only when it passes that, and commit a new baseline when it improves. The +`evals` workflow runs this on pull requests that touch the skill or the evals, but only once +an `ANTHROPIC_API_KEY` secret is set, because each run spends model budget; CI checks the cases +and the scorer either way. `--bare` keeps hooks and personal instructions +out of the run and needs `ANTHROPIC_API_KEY`. `docs/misses.md` records what each miss lacked. + ## License MIT diff --git a/docs/misses.md b/docs/misses.md new file mode 100644 index 0000000..ab967e2 --- /dev/null +++ b/docs/misses.md @@ -0,0 +1,78 @@ +# Misses + +A miss is a defect that shipped through a pull request and was found later. Each one here is +an eval case under `evals/cases/`, and each entry asks the question the Anthropic code review +write-up asks of every miss: what did the reviewer not have in front of it? A change to the +skill is kept only when `evals/run.py` still finds every miss without raising more false +alarms on the clean cases. + +## #83 refused legacy sessions in `links` + +- **Shipped:** `attested_implementation.py:561` refused any source without `session.sqlite3`. + A legacy session is `session.json` plus `beats/` and no database, and its first open is the + migration, so the guard refused the one shape that converts itself. +- **Found by:** #84, which moved the predicate into `SessionStore.__init__`, the one place + every caller passes through. +- **What was missing:** which on-disk shapes count as a session was written down nowhere a + reader of `links` would look. The predicate looked obviously right in isolation. +- **Case:** `underwrite-83-legacy-guard`. No walk of #83 was stored, so this enters as a case + rather than a replayed session. + +## #76 canonicalized slots on write and compared them raw + +- **Shipped:** `session_store.py:1550` canonicalized incoming slot keys, while line 907 skipped + the repair for pinned beats. A pinned beat read with `get-beat` and written straight back + then counted as a change, deleted the `fix` slot and voided the authorization. +- **Found by:** #79, which compares the stored beat as the store spells it. +- **What was missing:** the invariant that a comparison reads both sides through the same + spelling. The two halves sat 600 lines apart, and the diff showed each one as correct. +- **Case:** `underwrite-76-normalize-compare`. + +## Candidates not admitted + +- **cornermind #29:** `migrate.yml` allowed `workflow_dispatch` from any branch, later guarded + in cornermind #54. The `production-db` environment already allowed only `main`, so the fix + was defense in depth, not a shipped defect. +- **ai-kit #15:** the break-check gate could be bypassed by editing its own config, fixed in + ai-kit #17. The PR body named the gap and deferred it, so a walk that stayed quiet was not + wrong. + +## Runs + +| Date | Skill | Case | Result | Cost | +|---|---|---|---|---| +| 2026-10-21 | second read demotes on any no | `underwrite-17-sigterm` | fail: one confirmed flag on a weak race test | $1.64 | +| 2026-10-21 | second read demotes on any no | `underwrite-83-legacy-guard` | fail: the walk flagged `:561`, the second read demoted it as not worth stopping | $1.89 | +| 2026-10-21 | demotes only on not introduced or not real | `underwrite-83-legacy-guard` | pass: `:561` flagged and kept, worth doubt noted in its evidence | $2.20 | + +| 2026-10-21 | demotes only on not introduced or not real, before the completion marker | all five | 2 of 5 pass, 1 miss, 2 false alarms | $7.74 | +| 2026-10-21 | final skill, baseline in `evals/baseline.json` | all five | 3 of 5 pass, 1 miss, 2 false alarms | $9.24 | + +In the baseline run `underwrite-83-legacy-guard`, `underwrite-12-host-guard` and +`ai-kit-5-core-deps` pass. `underwrite-76-normalize-compare` raised the right class of finding, +an upper-case slot read from before the repair, but at `session_store.py:2485`, far from the +defect at `:907` and `:1550`, so it scores as one miss and one false alarm; it is the live +miss for the next skill change to win back. `underwrite-17-sigterm` raised the same weak race +test flag on every run. `ai-kit-5-core-deps` raised an alarm in the run before and none in +this one; that alarm, that the guard never checks it parsed a dynamic import, is the class of +finding the original reviewer accepted in the stored session of that PR, so the case may be +mislabeled clean, and it stays until someone decides. + +Walks are not deterministic: the same skill moved `ai-kit-5-core-deps` between pass and fail +across two runs. The baseline is one run, so a lone flip on a regression check is a reason to +rerun that case before believing it. Four walks of one run first stopped early because +exploratory commands were denied, which is why the runner now tells the walk what it may run. + +The second run in the table is why the second read no longer demotes on worth. A replay of the six flags +stored before the second read existed agreed: it disputed two on worth, and the reviewer had +acted on one of them. The `underwrite-17-sigterm` flag is a fair question about a test that +catches the old bug only about one run in seven; it stands as a false alarm because nothing +later had to fix #17. + +## Adding a miss + +1. Write `evals/cases/--.json` with the PR's base and head, the defect's + `path` and `lines` as they read at the head, and the fix as `evidence`. The lines must sit + inside the PR's diff, or no walk can flag them. +2. Add an entry here naming what the reviewer did not have in front of it. +3. Run `python3 evals/run.py ` on the current skill and record the result. diff --git a/evals/baseline.json b/evals/baseline.json new file mode 100644 index 0000000..e003590 --- /dev/null +++ b/evals/baseline.json @@ -0,0 +1,118 @@ +{ + "results": [ + { + "beats": 4, + "case": "ai-kit-5-core-deps", + "cost_usd": 1.5828408000000003, + "demoted": 0, + "false_alarms": [], + "finished": true, + "flags": 0, + "found": [], + "kind": "clean", + "missed": [], + "pass": true + }, + { + "beats": 3, + "case": "underwrite-12-host-guard", + "cost_usd": 1.8406928000000002, + "demoted": 0, + "false_alarms": [], + "finished": true, + "flags": 0, + "found": [], + "kind": "clean", + "missed": [], + "pass": true + }, + { + "beats": 1, + "case": "underwrite-17-sigterm", + "cost_usd": 1.7215960000000001, + "demoted": 0, + "false_alarms": [ + { + "claim": "the race test passes on the old ordering most runs", + "n": 1, + "where": "tests/test_serve.py:434" + } + ], + "finished": true, + "flags": 1, + "found": [], + "kind": "clean", + "missed": [], + "pass": false + }, + { + "beats": 5, + "case": "underwrite-76-normalize-compare", + "cost_usd": 2.2239962, + "demoted": 0, + "false_alarms": [ + { + "claim": "a linked child seeded before v6 from an uppercase beat can no longer be resumed", + "n": 5, + "where": "skills/underwrite/scripts/session_store.py:2485" + } + ], + "finished": true, + "flags": 1, + "found": [], + "kind": "miss", + "missed": [ + { + "class": "slots canonicalized on write but compared raw, so a pinned beat written back unchanged counts as a change", + "keywords": [ + "canonical", + "spelling", + "upper", + "pinned", + "raw" + ], + "lines": [ + 907, + 1550 + ], + "path": "skills/underwrite/scripts/session_store.py" + } + ], + "pass": false + }, + { + "beats": 4, + "case": "underwrite-83-legacy-guard", + "cost_usd": 1.8666264000000004, + "demoted": 0, + "false_alarms": [], + "finished": true, + "flags": 1, + "found": [ + { + "beat": 1, + "class": "an existence guard keyed on the new storage format refuses a legacy session.json session", + "keywords": [ + "legacy", + "session.json", + "pre-sqlite", + "migrat" + ], + "lines": [ + 561 + ], + "path": "skills/underwrite/scripts/attested_implementation.py" + } + ], + "kind": "miss", + "missed": [], + "pass": true + } + ], + "summary": { + "cases": 5, + "false_alarms": 2, + "missed": 1, + "passed": 3 + } +} diff --git a/evals/cases/ai-kit-5-core-deps.json b/evals/cases/ai-kit-5-core-deps.json new file mode 100644 index 0000000..80d063f --- /dev/null +++ b/evals/cases/ai-kit-5-core-deps.json @@ -0,0 +1,10 @@ +{ + "id": "ai-kit-5-core-deps", + "kind": "clean", + "repo": "radkode/ai-kit", + "number": 5, + "base_sha": "49900cb40b08c88bab66be4e73a510b07cdc9b5a", + "head_sha": "1b007a07504df51eb1f90725b52713a3dc5e6115", + "expected": [], + "evidence": "scripts/check-core-deps.mjs unchanged since merge" +} diff --git a/evals/cases/underwrite-12-host-guard.json b/evals/cases/underwrite-12-host-guard.json new file mode 100644 index 0000000..2fe33fc --- /dev/null +++ b/evals/cases/underwrite-12-host-guard.json @@ -0,0 +1,10 @@ +{ + "id": "underwrite-12-host-guard", + "kind": "clean", + "repo": "radkode/underwrite", + "number": 12, + "base_sha": "6c011f38c8a8311e8b4f309e542e50b77c72f5d7", + "head_sha": "cf3e876d2936a29b7e861a07354db4c7a1cf4a17", + "expected": [], + "evidence": "no later fix or revert touches its lines" +} diff --git a/evals/cases/underwrite-17-sigterm.json b/evals/cases/underwrite-17-sigterm.json new file mode 100644 index 0000000..a4b8293 --- /dev/null +++ b/evals/cases/underwrite-17-sigterm.json @@ -0,0 +1,10 @@ +{ + "id": "underwrite-17-sigterm", + "kind": "clean", + "repo": "radkode/underwrite", + "number": 17, + "base_sha": "5c7bae2e6e0bb4600d688575b66a3cc811651239", + "head_sha": "22d4ab5440d5ec5c17e769fd6b4d7513994d3126", + "expected": [], + "evidence": "its SIGTERM block was later touched only by tests (#23) and a feature (#63)" +} diff --git a/evals/cases/underwrite-76-normalize-compare.json b/evals/cases/underwrite-76-normalize-compare.json new file mode 100644 index 0000000..65b58e8 --- /dev/null +++ b/evals/cases/underwrite-76-normalize-compare.json @@ -0,0 +1,26 @@ +{ + "id": "underwrite-76-normalize-compare", + "kind": "miss", + "repo": "radkode/underwrite", + "number": 76, + "base_sha": "80bdf166d0f40f214b37166f8d7994b6ee19c47e", + "head_sha": "ee01410253691c0cde9e3f17251ca33c1d8a867b", + "expected": [ + { + "path": "skills/underwrite/scripts/session_store.py", + "lines": [ + 907, + 1550 + ], + "class": "slots canonicalized on write but compared raw, so a pinned beat written back unchanged counts as a change", + "keywords": [ + "canonical", + "spelling", + "upper", + "pinned", + "raw" + ] + } + ], + "evidence": "https://github.com/radkode/underwrite/pull/79" +} diff --git a/evals/cases/underwrite-83-legacy-guard.json b/evals/cases/underwrite-83-legacy-guard.json new file mode 100644 index 0000000..a9a1e4d --- /dev/null +++ b/evals/cases/underwrite-83-legacy-guard.json @@ -0,0 +1,24 @@ +{ + "id": "underwrite-83-legacy-guard", + "kind": "miss", + "repo": "radkode/underwrite", + "number": 83, + "base_sha": "90e432561bdd16932be9bbb9500189c4bdc78450", + "head_sha": "ef1fa2222f1b6481cae6c85528e1de5383cb4bc1", + "expected": [ + { + "path": "skills/underwrite/scripts/attested_implementation.py", + "lines": [ + 561 + ], + "class": "an existence guard keyed on the new storage format refuses a legacy session.json session", + "keywords": [ + "legacy", + "session.json", + "pre-sqlite", + "migrat" + ] + } + ], + "evidence": "https://github.com/radkode/underwrite/pull/84" +} diff --git a/evals/run.py b/evals/run.py new file mode 100755 index 0000000..9d1d323 --- /dev/null +++ b/evals/run.py @@ -0,0 +1,189 @@ +#!/usr/bin/env python3 +"""Replay eval cases through a headless walk and score each one. + +Every case is a merged pull request, walked from a fresh clone at its base with the skill +from this checkout. Nothing is posted: a merged PR freezes to report audience and eval mode +delivers nothing. Each run spends model budget, so it is a local gate, not a CI job. +""" + +import argparse +import json +import os +import shutil +import subprocess +import sys +import tempfile +from pathlib import Path + +HERE = Path(__file__).resolve().parent +ROOT = HERE.parent +sys.path.insert(0, str(HERE)) + +from score import CaseError, load_case, score # noqa: E402 + + +SESSION_COMMANDS = ( + "init", "snapshot-pr", "check-controller", "freeze-execution", "check-execution", + "trusted-context", "diff", "context-log", "read-blob", "check-pr", "put-session", + "patch-session", "put-beat", "get-session", "get-beat", "history", "export", "reconcile", +) + + +def allowed_tools(skill, reviews, checkout): + sessionctl = skill / "scripts" / "sessionctl.py" + scratch = f"/{reviews}/**" + reads = [f"{tool}(/{root}/**)" for root in (skill, reviews, checkout) for tool in ("Read", "Grep", "Glob")] + # Every session argument has to sit under the scratch reviews root, so no real session moves. + store = [ + f"Bash({prefix}{sessionctl} {name} {reviews}/*)" + for prefix in ("", "python3 ") + for name in SESSION_COMMANDS + ] + return [ + *reads, "Agent", + f"Write({scratch})", + f"Edit({scratch})", + *store, + f"Bash({sessionctl} --help)", + "Bash(gh pr view *)", + "Bash(git remote *)", + "Bash(gh issue view *)", + "Bash(git status *)", + "Bash(git rev-parse *)", + "Bash(git log *)", + ] + + +def briefing(skill, reviews, checkout): + """What the walk may do, said up front: a walk that meets an unexplained denial gives up.""" + return ( + "This is a headless eval run of underwrite in eval mode, with nobody to answer. " + f"The skill is at {skill}, the reviewed checkout at {checkout}, and " + f"UNDERWRITE_REVIEWS is {reviews}. Permissions match the literal command text, so " + "write absolute paths, never shell variables. Allowed: " + f"{skill / 'scripts' / 'sessionctl.py'} {reviews}/ ...; " + "gh pr view and gh issue view; git status, rev-parse, log and remote; Read, Grep and " + "Glob inside those three directories; Write and Edit only under the reviews directory; " + "and the subagent tool for the second read. sessionctl.py init creates the session " + "directory, so no mkdir is needed. Anything else is denied by design and is not a " + "reason to stop: carry on with what is allowed." + ) + + +def command(case, plugin, reviews, checkout, args): + skill = plugin / "skills" / "underwrite" + argv = [ + args.claude, "-p", f"/underwrite:underwrite {case['number']} --eval", + "--plugin-dir", str(plugin), + "--permission-mode", "dontAsk", + "--allowedTools", *allowed_tools(skill, reviews, checkout), + "--append-system-prompt", briefing(skill, reviews, checkout), + "--output-format", "json", + "--no-session-persistence", + "--max-budget-usd", str(args.max_budget_usd), + ] + if args.model: + argv += ["--model", args.model] + if args.bare: + argv.append("--bare") + return argv + + +def run_case(case, args): + work = Path(tempfile.mkdtemp(prefix=f"underwrite-eval-{case['id']}-")).resolve() + checkout, reviews = work / "checkout", work / "reviews" + reviews.mkdir() + result = {"case": case["id"], "work": str(work)} + try: + subprocess.run( + ["gh", "repo", "clone", case["repo"], str(checkout), "--", "--quiet"], + check=True, capture_output=True, text=True, + ) + subprocess.run( + ["git", "-C", str(checkout), "checkout", "--quiet", "--detach", case["base_sha"]], + check=True, capture_output=True, text=True, + ) + walk = subprocess.run( + command(case, ROOT, reviews, checkout, args), cwd=checkout, capture_output=True, text=True, + env=dict(os.environ, UNDERWRITE_REVIEWS=str(reviews), UNDERWRITE_CONFINE=str(reviews)), + timeout=args.timeout, + stdin=subprocess.DEVNULL, + ) + (work / "walk.json").write_text(walk.stdout, encoding="utf-8") + (work / "walk.stderr").write_text(walk.stderr, encoding="utf-8") + try: + transcript = json.loads(walk.stdout) + result["cost_usd"] = transcript.get("total_cost_usd") + except json.JSONDecodeError: + pass + if walk.returncode != 0: + result["error"] = f"walk exited {walk.returncode}; see walk.stderr" + return result + sessions = [path for path in reviews.iterdir() if (path / "session.sqlite3").exists()] + if len(sessions) != 1: + result["error"] = f"walk exited {walk.returncode} and left {len(sessions)} sessions" + return result + result.update(score(case, sessions[0])) + except (subprocess.SubprocessError, OSError, CaseError) as error: + result["error"] = str(error) + finally: + if not args.keep and "error" not in result: + shutil.rmtree(work, ignore_errors=True) + result.pop("work") + return result + + +def main(argv=None): + ap = argparse.ArgumentParser(prog="run.py") + ap.add_argument("cases", nargs="*", help="case ids; all cases when omitted") + ap.add_argument("--model") + ap.add_argument("--max-budget-usd", type=float, default=5.0) + ap.add_argument("--timeout", type=int, default=3600) + ap.add_argument("--claude", default="claude") + ap.add_argument("--bare", action="store_true", help="skip hooks and CLAUDE.md; needs ANTHROPIC_API_KEY") + ap.add_argument("--keep", action="store_true", help="keep each work directory") + ap.add_argument("--baseline", type=Path, help="fail only on a regression against this result file") + args = ap.parse_args(argv) + paths = sorted((HERE / "cases").glob("*.json")) + if args.cases: + paths = [path for path in paths if path.stem in args.cases] + unknown = set(args.cases) - {path.stem for path in paths} + if unknown: + ap.error(f"unknown case {', '.join(sorted(unknown))}") + results = [run_case(load_case(path), args) for path in paths] + summary = { + "passed": sum(1 for r in results if r.get("pass")), + "cases": len(results), + "missed": sum(len(r.get("missed", ())) for r in results), + "false_alarms": sum(len(r.get("false_alarms", ())) for r in results), + } + report = {"summary": summary, "results": results} + if args.baseline: + report["regressions"] = regressions(json.loads(args.baseline.read_text(encoding="utf-8")), report) + print(json.dumps(report, indent=2, sort_keys=True)) + return 2 if report["regressions"] else 0 + print(json.dumps(report, indent=2, sort_keys=True)) + return 0 if summary["passed"] == summary["cases"] else 2 + + +def regressions(baseline, report): + """What got worse: a case that errored, did not finish, went missing, or passed and now + fails, or more misses or false alarms. An unscored case never counts as holding steady.""" + before = {r["case"]: r for r in baseline.get("results", ())} + now = {r["case"]: r for r in report["results"]} + worse = [f"{case} is in the baseline and was not run" for case in sorted(set(before) - set(now))] + for r in report["results"]: + if r.get("error"): + worse.append(f"{r['case']} errored: {r['error']}") + elif not r.get("finished"): + worse.append(f"{r['case']} did not finish its walk") + elif before.get(r["case"], {}).get("pass") and not r.get("pass"): + worse.append(f"{r['case']} passed in the baseline and now fails") + for key in ("missed", "false_alarms"): + if report["summary"][key] > baseline.get("summary", {}).get(key, 0): + worse.append(f"{key} rose from {baseline['summary'].get(key, 0)} to {report['summary'][key]}") + return worse + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/evals/score.py b/evals/score.py new file mode 100755 index 0000000..2ce3916 --- /dev/null +++ b/evals/score.py @@ -0,0 +1,166 @@ +#!/usr/bin/env python3 +"""Score one finished walk against an eval case: did it flag what mattered, and nothing else.""" + +import json +import sqlite3 +import sys +from pathlib import Path + +SCRIPTS = Path(__file__).resolve().parent.parent / "skills" / "underwrite" / "scripts" +if str(SCRIPTS) not in sys.path: + sys.path.insert(0, str(SCRIPTS)) + +from diff_lines import LOCATION_LINE # noqa: E402 +from session_store import FLAG_STATES, SessionStore # noqa: E402 + +# A dropped flag was put in front of the reviewer and refused, so it no longer stands. +STANDING_STATES = tuple(state for state in FLAG_STATES if state != "dropped") + +KINDS = ("miss", "clean") +WINDOW = 3 +SHA = set("0123456789abcdef") + + +class CaseError(ValueError): + pass + + +def load_case(path): + case = json.loads(Path(path).read_text(encoding="utf-8")) + problems = case_problems(case) + if problems: + raise CaseError(f"{path}: " + "; ".join(problems)) + return case + + +def case_problems(case): + problems = [] + for field in ("id", "kind", "repo", "number", "base_sha", "head_sha", "expected", "evidence"): + if field not in case: + problems.append(f"missing {field}") + if problems: + return problems + for field, kind in (("id", str), ("repo", str), ("evidence", str)): + if not isinstance(case[field], kind) or not case[field].strip(): + problems.append(f"{field} must be nonempty text") + if isinstance(case["number"], bool) or not isinstance(case["number"], int) or case["number"] < 1: + problems.append("number must be a positive integer") + if case["kind"] not in KINDS: + problems.append(f"kind must be one of {', '.join(KINDS)}") + for field in ("base_sha", "head_sha"): + value = case[field] + if not (isinstance(value, str) and len(value) == 40 and set(value) <= SHA): + problems.append(f"{field} must be a full lowercase SHA") + expected = case["expected"] + if not isinstance(expected, list): + return problems + ["expected must be a list"] + if case["kind"] == "miss" and not expected: + problems.append("a miss case names what the walk should have flagged") + if case["kind"] == "clean" and expected: + problems.append("a clean case expects no finding") + for item in expected: + if not ( + isinstance(item, dict) + and isinstance(item.get("path"), str) + and isinstance(item.get("lines"), list) + and item["lines"] + and all(isinstance(n, int) and n > 0 for n in item["lines"]) + and isinstance(item.get("class"), str) + and isinstance(item.get("keywords"), list) + and item["keywords"] + and all(isinstance(k, str) and k.strip() for k in item["keywords"]) + ): + problems.append("each expected finding has path, lines, class and keywords") + return problems + + +def cited(where): + match = LOCATION_LINE.match(str(where or "").strip()) + return (match["path"], int(match["line"])) if match else (None, None) + + +def words(beat): + slots = beat.get("slots") if isinstance(beat.get("slots"), dict) else {} + return " ".join(str(v) for v in (beat.get("claim"), *slots.values()) if v).lower() + + +def matches(item, beat): + """Near the defect and about it: the flag's own words name at least one of the class's + keywords, so a different finding that happens to sit close by earns nothing.""" + path, line = cited(beat.get("where")) + if path is None or not (path == item["path"] or item["path"].endswith("/" + path)): + return False + if not any(abs(line - n) <= WINDOW for n in item["lines"]): + return False + text = words(beat) + return any(keyword.lower() in text for keyword in item["keywords"]) + + +def score(case, session_root): + """A standing flag is one the walk would put in front of a reviewer; a demoted one is not. + A case passes when every expected finding is found and nothing else stands, and only for + a finished walk: at least one beat, a recommendation, and the eval_complete marker.""" + session, beats = SessionStore.detached_presentation_snapshot(session_root) + target = session.get("target") or {} + if ( + target.get("repo"), target.get("number"), target.get("base_sha"), target.get("head_sha") + ) != (case["repo"], case["number"], case["base_sha"], case["head_sha"]): + raise CaseError("the session did not walk this case's pull request, base and head") + standing = [beat for beat in beats if beat.get("state") in STANDING_STATES] + demoted = [ + beat for beat in beats + if (beat.get("second_read") or {}).get("verdict") == "disputed" + ] + found, missed, used = [], [], set() + for item in case["expected"]: + hit = next( + (b for b in standing if b["n"] not in used and matches(item, b)), + None, + ) + if hit: + used.add(hit["n"]) + found.append(dict(item, beat=hit["n"])) + else: + missed.append(item) + false_alarms = [ + {"n": beat["n"], "where": beat.get("where"), "claim": beat.get("claim")} + for beat in standing + if beat["n"] not in used + ] + # A recommendation is written provisionally before the walk, so only the marker eval mode + # writes last says the walk reached its end. + finished = ( + bool(beats) + and bool(str(session.get("recommendation") or "").strip()) + and session.get("eval_complete") is True + ) + return { + "case": case["id"], + "kind": case["kind"], + "beats": len(beats), + "flags": len(standing), + "demoted": len(demoted), + "found": found, + "missed": missed, + "false_alarms": false_alarms, + "finished": finished, + "pass": finished and not missed and not false_alarms, + } + + +def main(argv=None): + argv = sys.argv[1:] if argv is None else argv + if len(argv) != 2: + print("usage: score.py ", file=sys.stderr) + return 1 + try: + result = score(load_case(argv[0]), argv[1]) + except (OSError, ValueError, sqlite3.Error) as error: + print(f"score: {error}", file=sys.stderr) + return 1 + print(json.dumps(result, indent=2, sort_keys=True)) + return 0 if result["pass"] else 2 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/skills/underwrite/SKILL.md b/skills/underwrite/SKILL.md index 604b5dd..1cb4b7f 100644 --- a/skills/underwrite/SKILL.md +++ b/skills/underwrite/SKILL.md @@ -2,7 +2,7 @@ name: underwrite description: Interactive PR review, one beat at a time. Reconstructs what a change is for and how it fits the project, walks it in causal order while the reviewer steers, resolves each flag at the moment it is raised, and finishes the chosen resolutions as commits, recorded decisions, durable report inclusions, or a GitHub review, whichever has a reader. Use for reviewing a PR or an unfamiliar diff, especially AI-authored changes where no author is around to answer questions. disable-model-invocation: true -argument-hint: [pr-number | branch] +argument-hint: [pr-number | branch] [--eval] --- # Underwrite @@ -23,6 +23,18 @@ installed plugin lags whatever you just committed until it is reinstalled, and r walk against a stale copy is how you spend a session debugging a bug you already fixed. Say which one you are using when it is not the installed one. +**Eval mode.** An argument carrying `--eval` is a scored replay, not a review: +`evals/run.py` drives it headlessly and nobody is at the keyboard. Every rule here still +holds, the second read included. At each of the two decisions take the option you would +recommend and record it in your own words as the call, marked as eval mode. Do not serve +the page, post status, or park: write each beat and go straight to the next. Leave every +flag open, deliver nothing, skip Phase 4, and end with the recommendation and the count of +flags raised. The last write is one `patch-session` carrying the final `recommendation` and +`eval_complete: true`; the scorer counts a walk without that marker as unfinished, because +a provisional recommendation exists from Phase 1 onward. The runner allows writes only under `$UNDERWRITE_REVIEWS`, so +write each document for `sessionctl.py` to a file in `$R` and pass its path instead of +piping it. + ## The six rules Non-negotiable. Everything else here is guidance. @@ -81,7 +93,10 @@ not pre-existing issues. In a PR session the store holds you to the last one: a `where` names a `path:line` inside the frozen diff's hunks, numbered as the head reads (as the base read, for a line the PR removed), context lines included, and a flag on a line the PR never put in front of the reviewer is refused with the nearest touched line -named. A branch flag carries the smallest implementation intent +named. In a replacement, a flag on a line where an earlier round's reviewer dropped the +same finding is refused while the line still reads the same. A genuinely different +finding on that line carries `over_drop`, one line saying how it differs from what was +dropped, and the page shows that line beside the earlier call so the reviewer judges it. A branch flag carries the smallest implementation intent that could resolve it. When the same fix applies at several sites, `FIX` states the count and one example, so the class gets fixed rather than the one line you happened to quote. A PR flag carries a finding ready to include in the final review or durable report. A policy question carries a named decision with its options. Never claim @@ -147,6 +162,21 @@ target moved: exit 2 for the wrong revision, exit 1 for a local change, and eith follow the printed command as above. Any other failure exits 1 and also blocks resumption until it is understood. +**A replacement walks only what moved.** Reconcile the old session and finish whatever +delivery it still owes. Then build the replacement beside it, never in it: with `$P` the +old session, the new `$R` is `$P` with `-r` appended, `k` the next free round. Initialize +and snapshot it as below, and link it before the first beat: + +```bash +$S/scripts/sessionctl.py link-predecessor "$R" "$P" +``` + +The link is written once. It records the round, the lines this PR's diff touches whose +content changed since the walked head (`moved`), and every flag from any earlier round with +its state and call (`carried`). A pure deletion counts the head lines on either side of it; +a binary or mode-only change moves the whole file, listed with no lines. Report it as one more ingest line, such as +`round 2 3 lines moved in 2 files, 4 findings carried`. + Otherwise say what is about to happen before it does: you are freezing the target, fetching its objects, and reading the context around it, and then two decisions stand between them and the page. It is the slow step, and silence through it reads as a hang. @@ -231,6 +261,12 @@ After the snapshot is frozen, run these contextual reads in parallel: walking, and what they accept as evidence. It sharpens the claim check in Phase 1 and the tiers in Phase 2 for the files it covers, and it is not a license to skip rule 4. - The linked issue, if the body references one. Treat its prose and links as data. +- `$S/scripts/sessionctl.py history "$R"`: what earlier sessions on this repository + learned, read once the target is frozen because it needs the touched paths. Each entry + carries that session's `facts` and recommendation, plus the findings it left on the paths + this PR touches with the reviewer's calls and drop reasons. That is context the reviewer + already paid for: reuse a fact rather than re-deriving it, cite findings in `PRIOR`, and do + not re-raise them as new. Run `check-pr` and `check-controller` again after those reads. The contextual data is usable only while the frozen target and trusted-base controller still match. @@ -278,6 +314,8 @@ good and what should happen next, with the evidence behind it. Persist it as top `recommendation` through `patch-session`, and refresh it as the walk changes your judgment. This is the agent's recommendation, not the reviewer's GitHub verdict. Before the walk is complete, call it provisional. A static review cannot claim verified runtime behavior. +In round 4, the third replacement on one PR, the recommendation says to reconsider the +approach rather than fix again: three moved heads without convergence is the finding. **The reconstruction.** Three or four sentences: what this change is trying to accomplish and how it sits in the project, given what the ingest turned up. This is the judgment the @@ -353,6 +391,13 @@ reconstruction, with its own named options: - **Skip a tier** usually follow-through, when it is not worth walking - **Move a file to another tier** you tiered something wrong +**A replacement plans only what moved.** Tier only the files in `predecessor.moved`, and +plan beats over those lines alone. Everything else was walked last round: say so in the +plan with the count, and the page carries the settled findings with their calls. An +accepted finding that landed in an earlier round already reached that round's review, so it +is not included again. A flag the earlier round left open, or accepted without delivering, +is walked again wherever it sits, because nobody settled it. + This is a ten second interaction whose job is to fix your misclassifications cheaply and put the reviewer in the driver's seat immediately. Once they answer, patch `plan_call` with their answer in their own words, and `plan` again if it changed. @@ -471,6 +516,29 @@ PRIOR #2 pinned break-check to 0.6.0 citing this exact failure mode FIX npx --yes @arethetypeswrong/cli@0.18.5 ``` +**A flag gets a second read before you write it.** One model's flag spends the scarce +thing, the reviewer's attention, so check it first. Spawn one fresh-context agent with the +harness's subagent tool and give it only the beat object you are about to write, `$R`, and +`$S`. Its charge: using only `sessionctl.py diff`, `read-blob`, and `context-log` on `$R`, +answer three questions with the line that settles each. Did this PR introduce it? Is it +real? Would a senior engineer stop at it? It runs nothing else, never opens the reviewed +checkout or any working tree, and writes nothing. In a branch or working-tree session, hand +it the diff text instead and let it read files without editing them. Do not pass your +reasoning or your other beats: an agent handed your conclusion tends to return it. + +A no on whether this PR introduced it, or on whether it is real, demotes the flag before it +is written: store the beat `UNVERIFIED`, without its `RISK`, carrying `second_read: +{verdict: "disputed", evidence}` that names the question that failed and why. The reviewer +still sees it, and a question can raise it again. A no on whether a senior engineer would +stop does not demote it: write the flag with `second_read: {verdict: "confirmed", +evidence}` and put that doubt in the evidence, because what is worth stopping for is the +reviewer's call. Otherwise the evidence is the settling line. Either way it is at most 25 +words. The eval set settled this: a read that also demoted on worth demoted the one +real defect in `underwrite-83-legacy-guard`. A stored flag cannot be demoted by a +rewrite, which is why the read comes first. Where the harness has no subagent tool, write +the flag with `second_read: {verdict: "unread", evidence}` naming why, so the page says +it was not read twice. + **Persist the beat in the same turn you present it.** Not at the end. Send the complete beat object, `proof_kind` included, to `$S/scripts/sessionctl.py put-beat "$R" -`, then patch `cursor` through `patch-session`. Read an existing document only through @@ -995,7 +1063,9 @@ re-publish the updated artifact; otherwise make the publishing offer above now. ## Session state `~/.claude/reviews/--pr/` (`-` when there is no PR). Outside every -repo, so it never shows up in `git status`. `mkdir -p` it on first write. +repo, so it never shows up in `git status`. `mkdir -p` it on first write. When +`UNDERWRITE_REVIEWS` is set, it replaces `~/.claude/reviews` everywhere in this skill; the +eval runner points it at a scratch directory. ``` session.sqlite3 authoritative versioned session, target identity, actions, and delivery @@ -1074,6 +1144,13 @@ the six keys from rule 2, spelled lower case: `what`, `why`, `proof`, `risk`, `p requires `ran`. A beat stored before the field existed keeps the verdict it landed with. `lands[]` entries are `{state: landed|ready|open, what, where}`. +`over_drop` is optional text of at most 25 words on a flag whose line carries an earlier +round's drop; the store refuses it anywhere else. + +`second_read` holds exactly `verdict` and `evidence`. `confirmed` and `unread` sit only on a +flag or what a flag became; `disputed` sits only on an `unverified` beat, the demoted flag. +The evidence is at most 25 words. Beats from before the field carry none. + `recommendation` is nonempty session text. A flag's `decision` is an object with exactly two nonempty text fields, `question` and `recommendation`. These describe existing choices, not new actions. Write both for every new review; older sessions without them stay readable @@ -1094,8 +1171,26 @@ mode is different: acceptance itself is the terminal durable outcome, with deliv `none`, no `landed` value, and no `lands[]` entry. A `decided` beat never carries one; its `call` is what it became. -These accumulate into a review history. When a later session touches the same paths, read -the prior sessions for context. +`$S/scripts/sessionctl.py stats ~/.claude/reviews` counts what every stored session raised +and how the reviewer answered it, reading each one from a backup copy. Run it before and +after a change to this skill: a change that raises the dropped share is a regression even +when it finds more. + +A replacement lives at `$P-r/` beside the session it replaces, and its write-once +`predecessor` holds `{session, round, base_sha, head_sha, moved, carried}`: `moved` maps +each path to the head lines that changed since `head_sha`, and `carried` lists every +flag from earlier rounds with its round, state, call, reason, and the words on its line. +A later round's call on the same line replaces an earlier one, except that a flag raised +`over_drop` leaves the drop it overrode in place. An open or undelivered finding is never +dropped from `carried`; once its line no longer reads the same it gains `line_changed`, the +round whose head changed it, and the page says so without claiming anyone looked again. Only `link-predecessor` writes it. + +Sessions accumulate into a review history, and `history` is how a later session reads it: +for every other session on the same repository, its facts, its recommendation, and the +findings it left on the paths this one touches. Where the filesystem records when a store +was created, only earlier sessions appear, marked `order: earlier`; elsewhere every other +session appears marked `order: unknown`, so weigh it as context, not as the record. Sessions +it cannot read are listed as unreadable, never skipped silently. ## Formatting diff --git a/skills/underwrite/assets/report.css b/skills/underwrite/assets/report.css index f3d8a3b..619db1e 100644 --- a/skills/underwrite/assets/report.css +++ b/skills/underwrite/assets/report.css @@ -235,6 +235,11 @@ h1 .num { color: var(--ink-3); font-variant-numeric: tabular-nums; } .frame dd { margin: 2px 0 16px; } .frame dd:last-child { margin-bottom: 0; } .frame[open] { padding-bottom: 12px; } +.carried { margin-top: 12px; } +.carried .carried-lead { margin: 12px 0 0; max-width: 68ch; color: var(--ink-2); } +/* A path is case-sensitive, so the carried rows drop the uppercase label treatment. */ +.carried dt { text-transform: none; letter-spacing: 0.02em; font-size: 12px; } +.carried strong { font-weight: 600; } /* ---- count strip ---- */ @@ -759,6 +764,32 @@ h1 .num { color: var(--ink-3); font-variant-numeric: tabular-nums; } } .unproven-why ul { margin: 0; padding-left: 18px; } +.second-read { + display: flex; + flex-wrap: wrap; + align-items: baseline; + gap: 4px 10px; + padding: 9px 14px; + border-radius: 3px; + font-size: 13px; + color: var(--ink-2); + max-width: 70ch; +} +.second-read .lbl { + font-family: var(--mono); + font-size: 11px; + letter-spacing: 0.13em; + text-transform: uppercase; +} +.second-read.confirmed { background: var(--clean-bg); } +.second-read.confirmed .lbl { color: var(--clean); } +.second-read.disputed { background: var(--unver-bg); } +.second-read.disputed .lbl { color: var(--unver); } +.second-read.unread { background: var(--flag-bg); } +.second-read.unread .lbl { color: var(--flag); } +.second-read.over-drop { background: var(--flag-bg); } +.second-read.over-drop .lbl { color: var(--flag); } + /* Named .shipped, not .landed: .next-row.landed already exists in What lands, and a bare .landed rule silently overrode that section's grid. */ .shipped { diff --git a/skills/underwrite/scripts/diff_lines.py b/skills/underwrite/scripts/diff_lines.py index 794bbd6..eb84f55 100644 --- a/skills/underwrite/scripts/diff_lines.py +++ b/skills/underwrite/scripts/diff_lines.py @@ -52,14 +52,14 @@ def parse_diff(text): continue tag = line[:1] if tag == " " or line == "": - mark(files[path], hunk, "LEFT", old_ln) - mark(files[path], hunk, "RIGHT", new_ln) + mark(files[path], hunk, "LEFT", old_ln, line[1:]) + mark(files[path], hunk, "RIGHT", new_ln, line[1:]) old_ln, new_ln, rem_old, rem_new = old_ln + 1, new_ln + 1, rem_old - 1, rem_new - 1 elif tag == "+": - mark(files[path], hunk, "RIGHT", new_ln) + mark(files[path], hunk, "RIGHT", new_ln, line[1:]) new_ln, rem_new = new_ln + 1, rem_new - 1 elif tag == "-": - mark(files[path], hunk, "LEFT", old_ln) + mark(files[path], hunk, "LEFT", old_ln, line[1:]) old_ln, rem_old = old_ln + 1, rem_old - 1 else: rem_old = rem_new = 0 # malformed hunk, resync on the next header @@ -84,14 +84,15 @@ def parse_diff(text): # because it carries the most risk, with nowhere to hang a comment. path = was if target == "/dev/null" else clean_path(target) if path: - files.setdefault(path, {"LEFT": set(), "RIGHT": set(), "hunks": []}) + files.setdefault(path, {"LEFT": set(), "RIGHT": set(), "hunks": [], "text": {}}) return files -def mark(entry, hunk, side, line): +def mark(entry, hunk, side, line, text=""): """Record a line on both the file-wide set and the hunk it came from.""" entry[side].add(line) + entry["text"][(side, line)] = text if hunk is not None: hunk[side].add(line) hunk["pos"].setdefault((side, line), len(hunk["pos"])) @@ -164,3 +165,31 @@ def untouched_anchor(where, files): f"flag at {path}:{line} is outside the frozen diff; " f"nearest touched line is {path}:{nearest}" ) + + +def anchor_line(where, files): + """The first cited line that lands in the diff, as {path, text}, head side first. + + A line number moves when code above it does, so a finding is matched across heads by + the words on its line rather than by where they sit. Words that appear on more than one + line of the file's diff identify nothing, so they yield None. + """ + paths = sorted(files) + text = str(where or "").strip() + whole = LOCATION_LINE.match(text.rstrip(",;+\u00b7")) + tokens = [whole] if whole and resolve_path(whole["path"], paths) else [ + LOCATION_LINE.match(t.rstrip(",;+\u00b7")) for t in text.split(" ") + ] + for cited in tokens: + path = resolve_path(cited["path"], paths) if cited else None + if path is None: + continue + for side in ("RIGHT", "LEFT"): + words = files[path]["text"].get((side, int(cited["line"]))) + if words is None or not words.strip(): + continue + # Indentation can carry meaning, and words repeated in the diff name no one line. + words = words.rstrip() + twins = sum(1 for (s, _n), t in files[path]["text"].items() if s == side and t.rstrip() == words) + return {"path": path, "text": words} if twins == 1 else None + return None diff --git a/skills/underwrite/scripts/pr_snapshot.py b/skills/underwrite/scripts/pr_snapshot.py index 3d6b401..9a35665 100644 --- a/skills/underwrite/scripts/pr_snapshot.py +++ b/skills/underwrite/scripts/pr_snapshot.py @@ -16,6 +16,7 @@ from urllib.parse import urlsplit from session_store import Conflict, StoreError +from diff_lines import HUNK, clean_path, parse_diff _REPO = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$") @@ -999,6 +1000,74 @@ def read_blob(store, side, path, max_bytes): } +def _removal_points(text): + """Head lines either side of each pure removal, the only place a deletion can be read.""" + points, path = {}, None + for line in text.split("\n"): + if line.startswith("+++ "): + target = line[4:].strip() + path = None if target == "/dev/null" else clean_path(target) + continue + match = HUNK.match(line) + if match and path and int(match.group(4) or 1) == 0: + start = int(match.group(3)) + points.setdefault(path, set()).update({max(start, 1), start + 1}) + return points + + +def moved_lines(store, predecessor): + """Head lines of this PR's diff whose content changed since the predecessor's head. + + Both heads come from their own verified bundles, and only lines this PR touches count, + so a rebase that moved the base under unrelated code does not reopen the walk. + """ + files = parse_diff(store.read_verified_target_diff()[1].decode("utf-8", "replace")) + # A binary or mode-only change has no text hunk, so the path list comes from the names. + paths = sorted(set(files) | set(store.changed_paths())) + with _review_repo(store) as (repo, target): + with tempfile.TemporaryDirectory(prefix="underwrite-prior-") as temporary: + bundle = Path(temporary) / "prior.bundle" + prior = predecessor.copy_verified_object_bundle(bundle) + _git( + [ + "fetch", + "--atomic", + "--no-tags", + "--recurse-submodules=no", + str(bundle), + "+refs/underwrite/head:refs/underwrite/prior", + ], + repo, + safe=True, + ) + if _commit(repo, "refs/underwrite/prior") != prior["head_sha"]: + raise Conflict("predecessor object bundle head does not match its target") + between_args = ["refs/underwrite/prior", "refs/underwrite/head", "--", *paths] + safe_diff = ["diff", "--no-color", "--no-ext-diff", "--no-textconv"] + raw = _git( + [*safe_diff, "--unified=0", "--src-prefix=a/", "--dst-prefix=b/", *between_args], + repo, + safe=True, + ) if paths else b"" + names = _git([*safe_diff, "--name-only", "-z", *between_args], repo, safe=True) if paths else b"" + text = raw.decode("utf-8", "replace") + between = parse_diff(text) + changed = {name.decode("utf-8", "replace") for name in names.split(b"\0") if name} + removed = _removal_points(text) + moved = {} + for path in paths: + sides = files.get(path, {"RIGHT": set()}) + lines = sides["RIGHT"] & ( + between.get(path, {}).get("RIGHT", set()) | removed.get(path, set()) + ) + if lines: + moved[path] = sorted(lines) + elif path in changed and (path not in between or not sides["RIGHT"]): + # A deleted, binary or mode-only change has no head line to name, so the file moved. + moved[path] = [] + return {"prior_head_sha": prior["head_sha"], "head_sha": target["head_sha"], "lines": moved} + + def context_log(store, limit): _positive(limit, "context log limit") if limit > _MAX_CONTEXT_COMMITS: diff --git a/skills/underwrite/scripts/render-report.py b/skills/underwrite/scripts/render-report.py index 9c5c55a..9e0529b 100755 --- a/skills/underwrite/scripts/render-report.py +++ b/skills/underwrite/scripts/render-report.py @@ -72,6 +72,11 @@ "not-worth-it": "not worth it", "pre-existing": "pre-existing", } +SECOND_READ_LABEL = { + "confirmed": "Confirmed by a second read", + "disputed": "A second read disputed this flag", + "unread": "Not read twice", +} def state_token(state, mode, landed=True): @@ -890,6 +895,69 @@ def why_html(n): ) +def second_read_html(reading): + """What the fresh-context read said about a flag: it stands, or it was demoted.""" + if not isinstance(reading, dict) or not isinstance(reading.get("evidence"), str): + return "" + label = SECOND_READ_LABEL.get(reading.get("verdict")) + if label is None: + return "" + return ( + f'
{label}' + f'{md(reading["evidence"])}
' + ) + + +def carried_html(session, mode, live): + """What a replacement inherits: the lines it walks, and the calls that already stand.""" + predecessor = session.get("predecessor") + if not isinstance(predecessor, dict): + return "" + moved = predecessor.get("moved") if isinstance(predecessor.get("moved"), dict) else {} + lines = sum(len(v) for v in moved.values() if isinstance(v, list)) + prior = str(predecessor.get("head_sha") or "")[:7] + earlier = int(predecessor.get("round", 2)) - 1 + files = f"{len(moved)} file" + ("" if len(moved) == 1 else "s") + walked = ( + f"This round walks {lines} line{'' if lines == 1 else 's'} in {files} that " + f"moved since {html.escape(prior)}. Everything else was walked " + + ("in round 1." if earlier == 1 else f"in rounds 1 to {earlier}.") + + " These calls are not in the counts above." + ) + entries = [e for e in predecessor.get("carried") or () if isinstance(e, dict)] + rows = [] + for entry in entries: + state = entry.get("state") + if state == "dropped": + why = REASON_LABEL.get(entry.get("reason")) + outcome = f"Dropped as {why}" if why else "Dropped" + elif state == "accepted" and mode != "report" and not entry.get("landed"): + outcome = "Accepted, never implemented" if mode == "branch" else "Accepted, never posted" + elif state == "accepted": + outcome = ACCEPTED_TOKEN.get(mode, "INCLUDED").capitalize() + elif state == "flag": + outcome = "Still open, raised again here if it holds" + else: + outcome = "Decided" + if entry.get("line_changed"): + outcome += f"; its line has changed since round {md(entry['line_changed'])}" + claim = f" {md(entry['claim'])}." if entry.get("claim") else "" + call = f" {md(entry['call'])}" if entry.get("call") else "" + rows.append( + f'
round {md(entry.get("round", "?"))} · {md(entry.get("where", ""))}
' + f"
{outcome}.{claim}{call}
" + ) + count = f'·{len(entries)}' if entries else "" + body = f'

{walked}

' + if rows: + body += f"
{''.join(rows)}
" + return ( + f'
' + f'Carried from earlier rounds{count}' + f"{body}
" + ) + + def beat_html( beat, problems, @@ -946,6 +1014,14 @@ def beat_html( body.append(diff_html(beat["diff"])) if fix_row: body.append(f'
{"".join(fix_row)}
') + reading = second_read_html(beat.get("second_read")) + if reading: + body.append(reading) + if isinstance(beat.get("over_drop"), str) and beat["over_drop"].strip(): + body.append( + '
Raised over an earlier drop' + f'{md(beat["over_drop"])}
' + ) body.append(outcome_html(beat, mode)) if beat.get("call"): label = f"Dropped as {why}" if why else "Your call" @@ -1485,6 +1561,9 @@ def render( parts.append('·underwrite') if session.get("date"): parts.append(f'·{md(session["date"])}') + predecessor = session.get("predecessor") + if isinstance(predecessor, dict) and predecessor.get("round"): + parts.append(f'·round {md(predecessor["round"])}') parts.append( f'

{html.escape(label)} ' f'{md(session.get("title", ""))}

' @@ -1500,6 +1579,9 @@ def render( frame = frame_html(session, live) if frame: parts.append(frame) + carried = carried_html(session, session_mode(session), live) + if carried: + parts.append(carried) facts_values = list(session.get("facts") or []) if session_mode(session) == "report": facts_values.append("Outcome: report only") diff --git a/skills/underwrite/scripts/session_store.py b/skills/underwrite/scripts/session_store.py index 69b3e42..4bbbd65 100644 --- a/skills/underwrite/scripts/session_store.py +++ b/skills/underwrite/scripts/session_store.py @@ -21,7 +21,7 @@ if HERE not in sys.path: sys.path.insert(0, HERE) -from diff_lines import parse_diff, untouched_anchor # noqa: E402 +from diff_lines import anchor_line, parse_diff, untouched_anchor # noqa: E402 DB_SCHEMA_VERSION = 7 @@ -53,6 +53,8 @@ # What PROOF is: a command whose output would have read differently were the claim # false, a file or diff read, or an inference. Only `ran` earns CLEAN. PROOF_KINDS = ("ran", "read", "inferred") +SECOND_READ_VERDICTS = ("confirmed", "disputed", "unread") +FLAG_STATES = ("flag", "accepted", "dropped", "decided") RESOLUTION_KINDS = ("delivery", "decision") STORE_OWNED_FIELDS = ("call", "reason", "landed", "branch", "delivery_kind", "delivery") # Rule 2's budget, in numbers. It binds what a walk writes now and never what is @@ -232,6 +234,14 @@ def beat_budget_problems(beat): f"beat {n}: clean beat fills {len(filled)} slots, over " f"{BEAT_BUDGET['clean_slots']} ({', '.join(filled)})" ) + reading = beat.get("second_read") + if isinstance(reading, dict) and isinstance(reading.get("evidence"), str): + words = len(reading["evidence"].split()) + if words > BEAT_BUDGET["slot_words"]: + over.append( + f"beat {n}: second_read evidence is {words} words, over " + f"{BEAT_BUDGET['slot_words']}" + ) quoted = beat.get("diff") if isinstance(quoted, list) and len(quoted) > BEAT_BUDGET["diff_lines"]: over.append( @@ -283,9 +293,54 @@ def decision_problems(beat): return [] +def second_read_problems(beat): + """A second read confirms a flag or demotes it; it never sits on a beat it did not judge.""" + if "second_read" not in beat: + return [] + n = beat.get("n", "?") + reading = beat["second_read"] + if ( + not isinstance(reading, dict) + or set(reading) != {"verdict", "evidence"} + or reading["verdict"] not in SECOND_READ_VERDICTS + or not isinstance(reading["evidence"], str) + or not reading["evidence"].strip() + ): + return [ + f"beat {n}: second_read requires verdict confirmed, disputed or unread " + "and evidence as nonempty text" + ] + state = beat.get("state") + if reading["verdict"] in ("confirmed", "unread") and state not in FLAG_STATES: + return [ + f"beat {n}: a {reading['verdict']} second read belongs to a flag, not {state!r}" + ] + if reading["verdict"] == "disputed" and state != "unverified": + return [ + f"beat {n}: a disputed second read demotes the flag; write it unverified " + "without its risk" + ] + return [] + + +def over_drop_problems(beat): + """A flag raised over an earlier drop says, in a line, how it differs from what was dropped.""" + if "over_drop" not in beat: + return [] + n = beat.get("n", "?") + text = beat["over_drop"] + if beat.get("state") not in FLAG_STATES: + return [f"beat {n}: over_drop belongs to a flag"] + if not isinstance(text, str) or not text.strip(): + return [f"beat {n}: over_drop must be nonempty text"] + if len(text.split()) > BEAT_BUDGET["slot_words"]: + return [f"beat {n}: over_drop is {len(text.split())} words, over {BEAT_BUDGET['slot_words']}"] + return [] + + def validate_beat(beat, mode="branch", final=False): """Return beat contract violations. Empty means the beat is shippable.""" - problems = decision_problems(beat) + problems = decision_problems(beat) + second_read_problems(beat) + over_drop_problems(beat) n = beat.get("n", "?") state = beat.get("state") raw_slots = beat.get("slots") @@ -1799,10 +1854,19 @@ def _save_session( allow_new_branch=False, allow_new_execution_policy=False, allow_new_audience=False, + allow_new_predecessor=False, ): version, body = self._session_document(document) row = self._session_row(db) current = json.loads(row["body_json"]) + current_predecessor = current.get("predecessor", _MISSING) + if current_predecessor != body.get("predecessor", _MISSING): + if current_predecessor is _MISSING and allow_new_predecessor: + pass + elif current_predecessor is _MISSING: + raise Conflict("session predecessor must be recorded through link-predecessor") + else: + raise Conflict("session predecessor cannot change once linked") current_target = current.get("target", _MISSING) incoming_target = body.get("target", _MISSING) if current_target != incoming_target: @@ -1995,6 +2059,8 @@ def patch_session(self, changes): ) if "legacy_pr" in patch: raise Conflict("session legacy PR identity cannot change") + if "predecessor" in patch: + raise Conflict("session predecessor must be recorded through link-predecessor") if "delivery_branch" in patch: raise Conflict( "session delivery_branch must be changed through pin-branch" @@ -3454,6 +3520,127 @@ def _refuse_untouched_flag(self, session, beat): if problem: raise StoreError(f"beat {beat['n']}: {problem}") + def link_predecessor(self, predecessor, moved): + """Record the session this one replaces, and carry forward what the reviewer settled. + + Written once, before the first beat: what moved since the last walk scopes this one, + a finding the reviewer dropped stays dropped while its line reads the same, and a flag + still open is carried as open so the replacement raises it again. + """ + prior_session, prior_beats = predecessor.snapshot() + prior_target = prior_session.get("target") + if not isinstance(prior_target, dict): + raise Conflict("predecessor has no frozen PR target") + earlier = prior_session.get("predecessor") or {} + prior_round = earlier.get("round", 1) + files = parse_diff( + predecessor.read_verified_target_diff()[1].decode("utf-8", "replace") + ) + fresh = [] + for beat in prior_beats: + if beat.get("state") not in FLAG_STATES: + continue + entry = {"round": prior_round, "state": beat["state"]} + for field in ("n", "where", "claim", "reason", "call", "landed", "over_drop"): + if beat.get(field) is not None: + entry[field] = beat[field] + anchor = anchor_line(beat.get("where"), files) + if anchor is not None: + entry["anchor"] = anchor + fresh.append(entry) + # The latest round's word on a finding replaces what an earlier round carried for it. + # A flag raised over a drop is a different finding, so the drop it overrode stays. + settled = [entry["anchor"] for entry in fresh if "anchor" in entry and "over_drop" not in entry] + + def still_reads(anchor): + texts = files.get(anchor["path"], {}).get("text", {}) + return any(text.rstrip() == anchor["text"] for text in texts.values()) + + carried = [] + for entry in earlier.get("carried", ()): + entry = dict(entry) + anchor = entry.get("anchor") + if anchor is not None and anchor in settled: + continue + unsettled = entry.get("state") == "flag" or ( + entry.get("state") == "accepted" and not entry.get("landed") + ) + # Kept, never dropped: when its line no longer reads the same, say since which round, + # and claim nothing about whether anyone looked at it again. + if ( + anchor is not None and unsettled and "line_changed" not in entry + and not still_reads(anchor) + ): + entry["line_changed"] = prior_round + carried.append(entry) + carried += fresh + if not isinstance(moved, dict) or moved.get("prior_head_sha") != prior_target["head_sha"]: + raise StoreError("moved lines must be computed against the predecessor's head") + with self._first_work_write() as db: + session = json.loads(self._session_row(db)["body_json"]) + target = session.get("target") + if not isinstance(target, dict): + raise Conflict("link-predecessor requires a frozen PR target") + if "predecessor" in session: + raise Conflict("session predecessor is already linked") + if db.execute("SELECT EXISTS(SELECT 1 FROM beats)").fetchone()[0]: + raise Conflict("link the predecessor before the first beat") + if predecessor.root.resolve() == self.root.resolve(): + raise Conflict("a session cannot replace itself") + if (prior_target["repo"], prior_target["number"]) != (target["repo"], target["number"]): + raise Conflict("predecessor reviewed a different pull request") + if (prior_target["base_sha"], prior_target["head_sha"]) == ( + target["base_sha"], target["head_sha"] + ): + raise Conflict("predecessor froze the same base and head; resume it instead") + if moved.get("head_sha") != target["head_sha"]: + raise StoreError("moved lines must be computed against this session's head") + session["predecessor"] = { + "session": predecessor.root.name, + "round": prior_round + 1, + "base_sha": prior_target["base_sha"], + "head_sha": prior_target["head_sha"], + "moved": moved["lines"], + "carried": carried, + } + if self._save_session(db, session, allow_new_predecessor=True): + self._bump_render(db) + return self.snapshot()[0]["predecessor"] + + def _guard_beat(self, session, beat): + """The checks every write of a beat answers to, whichever command carries it.""" + problems = second_read_problems(beat) + over_drop_problems(beat) + if problems: + raise StoreError("; ".join(problems)) + self._refuse_untouched_flag(session, beat) + self._refuse_dropped_finding(session, beat) + + def _refuse_dropped_finding(self, session, beat): + """A finding the reviewer dropped is not raised again while its line reads the same.""" + predecessor = session.get("predecessor") + target = session.get("target") + if beat.get("state") != "flag" or not predecessor or not isinstance(target, dict): + return + files = parse_diff(self._verified_diff_bytes(target).decode("utf-8", "replace")) + anchor = anchor_line(beat.get("where"), files) + drops = [ + entry for entry in predecessor.get("carried", ()) + if anchor is not None and entry.get("state") == "dropped" and entry.get("anchor") == anchor + ] + if "over_drop" in beat and not drops: + raise StoreError( + f"beat {beat['n']}: over_drop names no earlier drop on this line; remove it" + ) + if "over_drop" in beat: + return + for entry in drops: + raise StoreError( + f"beat {beat['n']}: the reviewer dropped this finding in round " + f"{entry['round']} as {entry.get('reason', 'unstated')} " + f"({entry.get('call', 'no words')!r}); its line still reads the same, " + "so it is not raised again unless over_drop says how this finding differs" + ) + def changed_paths(self): """The paths the frozen diff touches, read only after its digest verifies. @@ -3610,7 +3797,7 @@ def put_beat(self, document): raise StoreError(f"beat {beat['n']} slots must be an object") slots["fix"] = current_slots["fix"] self._hold_authorized_finding(db, beat, current) - self._refuse_untouched_flag(session, beat) + self._guard_beat(session, beat) changed, _revision, _state, _detail = self._save_beat(db, beat) if changed: self._bump_render(db) @@ -3703,6 +3890,25 @@ def legacy_presentation_snapshot(cls, root): beat["questions"] = questions.get(beat["n"], []) return session, beats + @classmethod + def detached_presentation_snapshot(cls, root): + """Read a session without writing to it: a schema upgrade lands on a backup copy.""" + root = Path(root).expanduser() + source = root / "session.sqlite3" + if not source.exists(): + if not (root / "session.json").exists(): + raise StoreError(f"no session at {root}") + return cls.legacy_presentation_snapshot(root) + with tempfile.TemporaryDirectory(prefix="underwrite-read-") as scratch: + reader = sqlite3.connect(f"{source.resolve().as_uri()}?mode=ro", uri=True) + copy = sqlite3.connect(str(Path(scratch) / "session.sqlite3")) + try: + reader.backup(copy) + finally: + copy.close() + reader.close() + return cls(scratch).presentation_snapshot() + # ---- action queue ------------------------------------------------- def _application(self, result, session, beats): @@ -4126,7 +4332,7 @@ def apply(self, seq, result, session=None, beats=()): ) else: self._validate_new_beat(beat) - self._refuse_untouched_flag(normalized_session, beat) + self._guard_beat(normalized_session, beat) beat_changed, _revision, _state, _detail = self._save_beat(db, beat) changed = beat_changed or changed if changed: diff --git a/skills/underwrite/scripts/sessionctl.py b/skills/underwrite/scripts/sessionctl.py index 489ce9e..a07b47a 100755 --- a/skills/underwrite/scripts/sessionctl.py +++ b/skills/underwrite/scripts/sessionctl.py @@ -5,6 +5,7 @@ import base64 import binascii import json +import os import sqlite3 import sys from pathlib import Path @@ -14,7 +15,8 @@ if str(HERE) not in sys.path: sys.path.insert(0, str(HERE)) -from session_store import SessionStore, StoreError # noqa: E402 +from session_store import DROP_REASONS, FLAG_STATES, SessionStore, StoreError # noqa: E402 +from diff_lines import LOCATION_LINE, resolve_path # noqa: E402 from pr_snapshot import ( # noqa: E402 TargetMoved, capture, @@ -23,6 +25,7 @@ check_controller, check_worktree, context_log, + moved_lines, read_blob, review_identity, review_receipt, @@ -84,6 +87,138 @@ def path_from_token(token): return path +LEDGER_COUNTS = ( + "beats", "flags", "open", "accepted", "decided", "dropped", "unverified", + "confirmed", "disputed", "unread_flags", "questions", +) + + +def noise_counts(beats): + counts = dict.fromkeys(LEDGER_COUNTS, 0) + counts["dropped_by_reason"] = dict.fromkeys((*DROP_REASONS, "unstated"), 0) + for beat in beats: + state = beat.get("state") + counts["beats"] += 1 + counts["questions"] += len(beat.get("questions") or ()) + if state in FLAG_STATES: + counts["flags"] += 1 + if state == "flag": + counts["open"] += 1 + elif state in ("accepted", "decided", "unverified"): + counts[state] += 1 + elif state == "dropped": + counts["dropped"] += 1 + reason = beat.get("reason") + counts["dropped_by_reason"][reason if reason in DROP_REASONS else "unstated"] += 1 + verdict = (beat.get("second_read") or {}).get("verdict") + if verdict in ("confirmed", "disputed"): + counts[verdict] += 1 + elif verdict == "unread": + counts["unread_flags"] += 1 + return counts + + +def add_counts(total, counts): + for key in LEDGER_COUNTS: + total[key] += counts[key] + for reason, value in counts["dropped_by_reason"].items(): + total["dropped_by_reason"][reason] += value + + +def share(counts): + beats = counts["beats"] + counts["unverified_share"] = round(counts["unverified"] / beats, 3) if beats else None + return counts + + +def ledger(reviews): + """Count what every stored session raised and how the reviewer answered it.""" + reviews = Path(reviews).expanduser() + if not reviews.is_dir(): + raise StoreError(f"no reviews directory at {reviews}") + sessions = [] + overall = noise_counts(()) + for root in sorted(path for path in reviews.iterdir() if path.is_dir()): + if not ((root / "session.sqlite3").exists() or (root / "session.json").exists()): + continue + entry = {"session": root.name} + try: + session, beats = SessionStore.detached_presentation_snapshot(root) + except (OSError, sqlite3.Error, json.JSONDecodeError, StoreError) as error: + entry["error"] = str(error) + sessions.append(entry) + continue + entry["repo"] = (session.get("target") or {}).get("repo") or session.get("repo") + counts = noise_counts(beats) + add_counts(overall, counts) + entry.update(share(counts)) + sessions.append(entry) + return {"sessions": sessions, "overall": share(overall)} + + +def born(root): + """When a session's store came to exist, where the filesystem records it. The store keeps + no creation time, and a modification time moves with every write, so it never stands in.""" + for name in ("session.sqlite3", "session.json"): + path = root / name + if path.exists(): + return getattr(path.stat(), "st_birthtime", None) + return None + + +def history(root): + """What earlier sessions on this repository learned: their facts and recommendation, and + the findings they left on the paths this session touches.""" + root = Path(root).expanduser() + store = SessionStore(root) + session, _beats = store.snapshot() + target = session.get("target") or {} + repo = target.get("repo") or session.get("repo") + paths = sorted(store.changed_paths()) if target else None + found, unreadable = [], [] + now = born(root) + for sibling in sorted(path for path in root.parent.iterdir() if path.is_dir()): + if sibling.resolve() == root.resolve() or not ( + (sibling / "session.sqlite3").exists() or (sibling / "session.json").exists() + ): + continue + started = born(sibling) + if started is not None and now is not None and started >= now: + continue + order = "earlier" if started is not None and now is not None else "unknown" + try: + prior, beats = SessionStore.detached_presentation_snapshot(sibling) + except (OSError, sqlite3.Error, json.JSONDecodeError, StoreError) as error: + unreadable.append({"session": sibling.name, "error": str(error)}) + continue + prior_target = prior.get("target") or {} + if (prior_target.get("repo") or prior.get("repo")) != repo: + continue + findings = [] + for beat in beats: + if beat.get("state") not in FLAG_STATES: + continue + cited = LOCATION_LINE.match(str(beat.get("where") or "").strip()) + if paths is not None and not (cited and resolve_path(cited["path"], paths)): + continue + findings.append({ + key: beat[key] + for key in ("n", "where", "claim", "state", "reason", "call") + if beat.get(key) is not None + }) + found.append({ + "session": sibling.name, + "number": prior_target.get("number"), + "head_sha": prior_target.get("head_sha"), + "round": (prior.get("predecessor") or {}).get("round", 1), + "order": order, + "recommendation": prior.get("recommendation"), + "facts": list(prior.get("facts") or ()), + "findings": findings, + }) + return {"repo": repo, "sessions": found, "unreadable": unreadable} + + def parser(): ap = Usage(prog="sessionctl.py") commands = ap.add_subparsers(dest="command", required=True) @@ -201,10 +336,36 @@ def parser(): export = commands.add_parser("export") export.add_argument("session") + + stats = commands.add_parser("stats") + stats.add_argument("reviews") + + link = commands.add_parser("link-predecessor") + link.add_argument("session") + link.add_argument("predecessor") + + prior = commands.add_parser("history") + prior.add_argument("session") return ap +def confine(args): + """Under the eval runner every session path has to resolve inside its scratch root.""" + root = os.environ.get("UNDERWRITE_CONFINE") + if not root: + return + boundary = Path(root).resolve() + for name in ("session", "predecessor", "reviews"): + value = getattr(args, name, None) + if value is None: + continue + target = Path(value).expanduser().resolve() + if target != boundary and boundary not in target.parents: + raise StoreError(f"{name} {value} is outside UNDERWRITE_CONFINE {boundary}") + + def run(args): + confine(args) if args.command == "init": store = SessionStore( args.session, handled_override=args.handled_seq, create=True @@ -325,6 +486,14 @@ def run(args): result = store.abandon_head(args.seq, args.actor, args.reason) elif args.command == "export": return SessionStore(args.session).export_json() + elif args.command == "stats": + return ledger(args.reviews) + elif args.command == "history": + return history(args.session) + elif args.command == "link-predecessor": + store = SessionStore(args.session) + predecessor = SessionStore(args.predecessor) + result = store.link_predecessor(predecessor, moved_lines(store, predecessor)) else: raise StoreError(f"unknown command {args.command!r}") diff --git a/tests/test_evals.py b/tests/test_evals.py new file mode 100644 index 0000000..f02614a --- /dev/null +++ b/tests/test_evals.py @@ -0,0 +1,276 @@ +#!/usr/bin/env python3 +"""The eval set: cases from real misses and known-clean PRs, a scorer, and the runner's +permission surface. The walk itself needs a model and runs locally through evals/run.py.""" + +import argparse +import importlib.util +import json +import shutil +import sqlite3 +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +EVALS = ROOT / "evals" +sys.path.insert(0, str(EVALS)) +sys.path.insert(0, str(ROOT / "skills" / "underwrite" / "scripts")) + +import score # noqa: E402 +import session_store # noqa: E402 + +spec = importlib.util.spec_from_file_location("eval_run", EVALS / "run.py") +run = importlib.util.module_from_spec(spec) +spec.loader.exec_module(run) + + +class Cases(unittest.TestCase): + def test_every_case_is_well_formed_and_named_for_its_file(self): + paths = sorted((EVALS / "cases").glob("*.json")) + self.assertGreaterEqual(len(paths), 2) + kinds = set() + for path in paths: + with self.subTest(case=path.name): + case = score.load_case(path) + self.assertEqual(case["id"], path.stem) + kinds.add(case["kind"]) + self.assertEqual(kinds, {"miss", "clean"}) + + def test_a_misshapen_case_is_refused(self): + good = { + "id": "x", "kind": "miss", "repo": "acme/widget", "number": 1, + "base_sha": "a" * 40, "head_sha": "b" * 40, "evidence": "e", + "expected": [{"path": "a.py", "lines": [3], "class": "c", "keywords": ["guard"]}], + } + self.assertEqual(score.case_problems(good), []) + for change, message in ( + ({"kind": "maybe"}, "kind must be"), + ({"head_sha": "abc"}, "full lowercase SHA"), + ({"expected": []}, "names what the walk should have flagged"), + ({"kind": "clean"}, "expects no finding"), + ({"expected": [{"path": "a.py", "lines": []}]}, "path, lines, class and keywords"), + ({"expected": [{"path": "a.py", "lines": [3], "class": "c"}]}, "class and keywords"), + ({"repo": ["acme/widget"]}, "repo must be nonempty text"), + ({"number": True}, "number must be a positive integer"), + ): + with self.subTest(change=change): + self.assertIn(message, "; ".join(score.case_problems(dict(good, **change)))) + + +class Scoring(unittest.TestCase): + def setUp(self): + self.tmp = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.tmp, True) + repo = self.tmp / "repo" + git = lambda *a: subprocess.run( + ["git", "-C", str(repo), *a], check=True, capture_output=True, text=True + ).stdout.strip() + subprocess.run(["git", "init", "-q", str(repo)], check=True) + for key, value in (("user.name", "t"), ("user.email", "t@example.test"), ("commit.gpgsign", "false")): + git("config", key, value) + (repo / "a.py").write_text("".join(f"line {n}\n" for n in range(1, 21)), encoding="utf-8") + git("add", "a.py") + git("commit", "-q", "-m", "base") + self.base = git("rev-parse", "HEAD") + (repo / "a.py").write_text( + "".join(f"{'CHANGED' if n in (5, 15) else 'line'} {n}\n" for n in range(1, 21)), + encoding="utf-8", + ) + git("commit", "-q", "-am", "head") + self.head = git("rev-parse", "HEAD") + self.root = self.tmp / "reviews" / "acme-widget-pr7" + store = session_store.SessionStore(self.root, create=True) + inputs = self.tmp / "inputs" + inputs.mkdir() + (inputs / "pr.diff").write_text(git("diff", f"{self.base}...{self.head}") + "\n", encoding="utf-8") + (inputs / "pr.json").write_text("{}", encoding="utf-8") + (inputs / "trusted.json").write_text( + json.dumps({"version": 1, "base_sha": self.base, "files": []}, + separators=(",", ":"), sort_keys=True) + "\n", + encoding="utf-8", + ) + git("update-ref", "refs/underwrite/base", self.base) + git("update-ref", "refs/underwrite/head", self.head) + git("bundle", "create", "-q", str(inputs / "pr.bundle"), "refs/underwrite/base", "refs/underwrite/head") + store.freeze_target( + { + "version": 1, "kind": "github_pr", "repo": "acme/widget", "number": 7, + "state": "closed", "merged_at": "2026-10-01T00:00:00Z", "base_sha": self.base, + "head_sha": self.head, "head_repo_id": 1, "head_repo": "acme/widget", + "head_ref": "f", "merge_base_sha": self.base, "changed_files": 1, + }, + inputs / "pr.diff", inputs / "pr.json", inputs / "trusted.json", inputs / "pr.bundle", + ) + store.patch_session({"recommendation": "Eval mode: done.", "eval_complete": True}) + self.store = store + + def case(self, kind="miss", lines=(5,)): + return { + "id": "t", "kind": kind, "repo": "acme/widget", "number": 7, + "base_sha": self.base, "head_sha": self.head, "evidence": "e", + "expected": [] if kind == "clean" else [ + {"path": "a.py", "lines": list(lines), "class": "c", "keywords": ["guard"]} + ], + } + + def flag(self, n, line, **extra): + return { + "n": n, "tier": "core", "state": "flag", "proof_kind": "read", + "claim": extra.pop("claim", "the empty guard is gone"), "where": f"a.py:{line}", + "slots": {"what": "w", "proof": f"a.py:{line}", "risk": "r", "fix": "f"}, + **extra, + } + + def test_a_flag_within_the_window_finds_the_miss(self): + self.store.put_beat(self.flag(1, 7)) + result = score.score(self.case(), self.root) + self.assertTrue(result["pass"]) + self.assertEqual(result["found"][0]["beat"], 1) + self.assertEqual(result["false_alarms"], []) + + def test_a_flag_elsewhere_misses_it_and_counts_as_a_false_alarm(self): + self.store.put_beat(self.flag(1, 15)) + result = score.score(self.case(), self.root) + self.assertFalse(result["pass"]) + self.assertEqual(len(result["missed"]), 1) + self.assertEqual([alarm["n"] for alarm in result["false_alarms"]], [1]) + + def test_a_demoted_flag_is_no_alarm_on_a_clean_case(self): + self.store.put_beat({ + "n": 1, "tier": "core", "state": "unverified", "proof_kind": "read", "claim": "c", + "where": "a.py:5", "slots": {"what": "w", "proof": "a.py:5"}, + "second_read": {"verdict": "disputed", "evidence": "not introduced here"}, + }) + result = score.score(self.case("clean"), self.root) + self.assertTrue(result["pass"]) + self.assertEqual((result["flags"], result["demoted"]), (0, 1)) + + def test_a_nearby_flag_about_something_else_finds_nothing(self): + self.store.put_beat(self.flag(1, 5, claim="the log line is noisy")) + result = score.score(self.case(), self.root) + self.assertEqual(len(result["missed"]), 1) + self.assertEqual([alarm["n"] for alarm in result["false_alarms"]], [1]) + + def test_a_found_miss_with_an_extra_alarm_does_not_pass(self): + self.store.put_beat(self.flag(1, 5)) + self.store.put_beat(self.flag(2, 15)) + result = score.score(self.case(), self.root) + self.assertEqual(len(result["found"]), 1) + self.assertFalse(result["pass"]) + + def test_a_provisional_recommendation_without_the_marker_is_unfinished(self): + self.store.put_beat(self.flag(1, 5)) + self.store.patch_session({"eval_complete": False}) + result = score.score(self.case(), self.root) + self.assertFalse(result["finished"]) + self.assertFalse(result["pass"]) + + def test_a_walk_with_no_recommendation_is_unfinished(self): + self.store.put_beat(self.flag(1, 5)) + with sqlite3.connect(str(self.root / "session.sqlite3")) as db: + body = json.loads(db.execute("SELECT body_json FROM session").fetchone()[0]) + body.pop("recommendation") + db.execute("UPDATE session SET body_json = ?", (json.dumps(body),)) + result = score.score(self.case(), self.root) + self.assertFalse(result["finished"]) + self.assertFalse(result["pass"]) + + def test_a_walk_that_stored_nothing_passes_no_case(self): + result = score.score(self.case("clean"), self.root) + self.assertEqual(result["beats"], 0) + self.assertFalse(result["pass"]) + + def test_a_standing_flag_fails_a_clean_case(self): + self.store.put_beat(self.flag(1, 5)) + self.assertFalse(score.score(self.case("clean"), self.root)["pass"]) + + def test_a_session_for_another_base_or_head_is_refused(self): + for field in ("base_sha", "head_sha"): + with self.subTest(field=field): + with self.assertRaisesRegex(score.CaseError, "did not walk this case"): + score.score(dict(self.case(), **{field: "c" * 40}), self.root) + + def test_a_dropped_flag_no_longer_stands(self): + self.store.put_beat(self.flag(1, 5)) + self.store.produce("drop-1", 1, "drop", "not this", reason="wrong") + result = score.score(self.case("clean"), self.root) + self.assertEqual(result["flags"], 0) + self.assertTrue(result["pass"]) + + def test_one_flag_answers_for_one_expected_finding(self): + self.store.put_beat(self.flag(1, 5)) + case = dict(self.case(), expected=[ + {"path": "a.py", "lines": [5], "class": "one", "keywords": ["guard"]}, + {"path": "a.py", "lines": [6], "class": "two", "keywords": ["guard"]}, + ]) + result = score.score(case, self.root) + self.assertEqual((len(result["found"]), len(result["missed"])), (1, 1)) + + +class Runner(unittest.TestCase): + def test_the_walk_runs_in_eval_mode_with_a_narrow_tool_list(self): + args = argparse.Namespace(claude="claude", max_budget_usd=5.0, model=None, bare=False) + reviews = Path("/tmp/eval-reviews") + checkout = Path("/tmp/eval-checkout") + argv = run.command({"number": 83}, ROOT, reviews, checkout, args) + self.assertIn("/underwrite:underwrite 83 --eval", argv) + self.assertEqual(argv[argv.index("--permission-mode") + 1], "dontAsk") + brief = argv[argv.index("--append-system-prompt") + 1] + self.assertIn("write absolute paths, never shell variables", brief) + self.assertIn(str(reviews), brief) + tools = run.allowed_tools(ROOT / "skills" / "underwrite", reviews, checkout) + self.assertNotIn("Bash", tools) + self.assertFalse(set(tools) & {"Read", "Grep", "Glob"}) + self.assertIn("Read(//tmp/eval-checkout/**)", tools) + writes = [tool for tool in tools if tool.startswith(("Edit", "Write"))] + self.assertEqual(writes, ["Write(//tmp/eval-reviews/**)", "Edit(//tmp/eval-reviews/**)"]) + bash = [tool for tool in tools if tool.startswith("Bash(")] + sessionctl = [tool for tool in bash if "sessionctl.py" in tool and not tool.endswith("--help)")] + self.assertTrue(sessionctl) + self.assertTrue(all(tool.endswith(f" {reviews}/*)") for tool in sessionctl)) + self.assertFalse([tool for tool in sessionctl if "sessionctl.py *" in tool]) + self.assertFalse([tool for tool in bash if tool.startswith("Bash(mkdir")]) + + def test_a_regression_is_a_case_that_stopped_passing_or_a_count_that_rose(self): + baseline = { + "summary": {"missed": 0, "false_alarms": 1}, + "results": [{"case": "a", "pass": True}, {"case": "b", "pass": False}], + } + same = {"summary": {"missed": 0, "false_alarms": 1}, + "results": [{"case": "a", "pass": True, "finished": True}, + {"case": "b", "pass": False, "finished": True}]} + self.assertEqual(run.regressions(baseline, same), []) + worse = {"summary": {"missed": 1, "false_alarms": 2}, + "results": [{"case": "a", "pass": False, "finished": True}, + {"case": "b", "pass": False, "finished": True}]} + self.assertEqual(len(run.regressions(baseline, worse)), 3) + + def test_an_unscored_case_never_holds_steady(self): + baseline = { + "summary": {"missed": 1, "false_alarms": 1}, + "results": [{"case": "a", "pass": True}, {"case": "b", "pass": False}, + {"case": "c", "pass": False}], + } + report = {"summary": {"missed": 0, "false_alarms": 0}, "results": [ + {"case": "a", "pass": True, "finished": True}, + {"case": "b", "error": "walk exited 1; see walk.stderr"}, + ]} + found = run.regressions(baseline, report) + self.assertIn("b errored: walk exited 1; see walk.stderr", found) + self.assertIn("c is in the baseline and was not run", found) + report["results"][1] = {"case": "b", "pass": False, "finished": False} + self.assertIn("b did not finish its walk", run.regressions(baseline, report)) + + def test_the_skill_documents_the_mode_and_the_scratch_reviews_root(self): + text = " ".join((ROOT / "skills" / "underwrite" / "SKILL.md").read_text(encoding="utf-8").split()) + self.assertIn("An argument carrying `--eval` is a scored replay, not a review", text) + self.assertIn("Leave every flag open, deliver nothing, skip Phase 4", text) + self.assertIn("When `UNDERWRITE_REVIEWS` is set, it replaces `~/.claude/reviews`", text) + self.assertIn("pass its path instead of piping it", text) + self.assertIn("`eval_complete: true`", text) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_scripts.py b/tests/test_scripts.py index cb76283..f9d058f 100644 --- a/tests/test_scripts.py +++ b/tests/test_scripts.py @@ -2798,5 +2798,94 @@ def test_a_non_integer_line_is_folded_rather_than_crashing(self): self.assertIn("fold", report[0]) +class SecondReadAndCarriedRounds(unittest.TestCase): + """The page says when a flag survived a second read or was demoted by one, and a + replacement names its round and the calls it inherits.""" + + FLAG = { + "n": 1, "state": "flag", "claim": "unpinned", "where": "a.py:1", + "slots": {"what": "x", "proof": "a.py:1", "risk": "r", "fix": "pin it"}, + "decision": {"question": "Include it?", "recommendation": "Include it."}, + } + + def test_a_confirmed_and_a_disputed_read_each_say_so_on_the_card(self): + confirmed = dict(self.FLAG, second_read={"verdict": "confirmed", "evidence": "new at a.py:1"}) + disputed = { + "n": 2, "state": "unverified", "claim": "timeout", "where": "a.py:2", + "slots": {"what": "y", "proof": "a.py:2"}, + "second_read": {"verdict": "disputed", "evidence": "the base had it"}, + } + page = rr.render({"repo": "acme/widget", "number": 7}, [confirmed, disputed], "", {}) + self.assertIn('Confirmed by a second readnew at a.py:1', page) + self.assertIn('A second read disputed this flagthe base had it', page) + + def test_a_flag_that_was_not_read_twice_says_so(self): + unread = dict(self.FLAG, second_read={"verdict": "unread", "evidence": "no subagent tool"}) + page = rr.render({"repo": "acme/widget", "number": 7}, [unread], "", {}) + self.assertIn('Not read twiceno subagent tool', page) + + def test_a_flag_raised_over_a_drop_says_how_it_differs(self): + beat = dict(self.FLAG, over_drop="the pin holds; the name shadows a builtin") + page = rr.render({"repo": "acme/widget", "number": 7}, [beat], "", {}) + self.assertIn('Raised over an earlier dropthe pin holds; the name shadows a builtin', page) + + def test_a_beat_with_no_second_read_shows_no_line(self): + page = rr.render({"repo": "acme/widget", "number": 7}, [self.FLAG], "", {}) + self.assertNotIn("second-read", page) + + def test_a_replacement_names_its_round_and_carries_the_settled_calls(self): + session = { + "repo": "acme/widget", "number": 7, + "audience": {"mode": "review", "why": "open PR"}, + "predecessor": { + "session": "acme-widget-pr7", "round": 2, "base_sha": "a" * 40, + "head_sha": "b" * 40, "moved": {"a.py": [4, 5]}, + "carried": [ + {"round": 1, "n": 1, "state": "dropped", "where": "a.py:2", + "reason": "intentional", "call": "pinned on purpose"}, + {"round": 1, "n": 2, "state": "accepted", "where": "a.py:9", + "claim": "missing guard", "call": "Include it.", + "landed": "https://github.com/acme/widget/pull/7#pullrequestreview-1"}, + ], + }, + } + page = rr.render(session, [], "", {}) + self.assertIn('·round 2', page) + self.assertIn("Carried from earlier rounds", page) + self.assertIn("This round walks 2 lines in 1 file that moved since bbbbbbb.", page) + self.assertIn("Everything else was walked in round 1. These calls are not in the counts above.", page) + self.assertIn("Dropped as intentional.", page) + self.assertIn("Included. missing guard.", page) + self.assertIn("pinned on purpose", page) + + def test_an_open_or_undelivered_carry_says_nobody_settled_it(self): + carried = [ + {"round": 1, "n": 1, "state": "flag", "where": "a.py:2"}, + {"round": 1, "n": 2, "state": "accepted", "where": "a.py:9", "call": "Include it."}, + ] + session = { + "repo": "acme/widget", "number": 7, "audience": {"mode": "review", "why": "open"}, + "predecessor": {"round": 2, "head_sha": "b" * 40, "moved": {}, "carried": carried}, + } + page = rr.render(session, [], "", {}) + self.assertIn("Still open, raised again here if it holds.", page) + self.assertIn("Accepted, never posted.", page) + + def test_a_carried_open_flag_says_its_line_changed_and_nothing_more(self): + carried = [{"round": 1, "n": 2, "state": "flag", "where": "a.py:4", "line_changed": 2}] + session = { + "repo": "acme/widget", "number": 7, "audience": {"mode": "review", "why": "open"}, + "predecessor": {"round": 3, "head_sha": "c" * 40, "moved": {}, "carried": carried}, + } + page = rr.render(session, [], "", {}) + self.assertIn("Still open, raised again here if it holds; its line has changed since round 2.", page) + self.assertNotIn("walked it again", page) + + def test_a_first_round_session_shows_neither(self): + page = rr.render({"repo": "acme/widget", "number": 7}, [], "", {}) + self.assertNotIn("Carried from earlier rounds", page) + self.assertNotIn("round 2", page) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_session_store.py b/tests/test_session_store.py index e8d9d81..6256bd3 100644 --- a/tests/test_session_store.py +++ b/tests/test_session_store.py @@ -4297,5 +4297,88 @@ def test_linked_json_export_cannot_be_reimported_without_attempt_authority(self) session_store.SessionStore(child.root) +class SecondRead(StoreCase): + """A flag is one model's opinion until a fresh-context read agrees with it. A + confirmed read rides on the flag; a disputed one demotes it before it is written.""" + + CONFIRMED = {"verdict": "confirmed", "evidence": "a.py:1 is new in this diff and unpinned"} + DISPUTED = {"verdict": "disputed", "evidence": "the base pins it at a.py:1; not introduced here"} + + def demoted(self, **extra): + slots = {"what": "x", "proof": "a.py:1"} + return dict(FLAG, n=3, state="unverified", slots=slots, **extra) + + def test_a_confirmed_read_rides_on_the_flag_through_its_drop(self): + self.store.put_beat(dict(FLAG, n=3, second_read=self.CONFIRMED)) + self.store.produce("drop-3", 3, "drop", "not this time", reason="not-worth-it") + + beat = self.beat(3) + self.assertEqual(beat["state"], "dropped") + self.assertEqual(beat["second_read"], self.CONFIRMED) + self.assertEqual(session_store.validate_beat(beat), []) + + def test_a_disputed_read_is_written_as_an_unverified_beat(self): + written = self.store.put_beat(self.demoted(second_read=self.DISPUTED)) + + self.assertEqual(written["state"], "unverified") + self.assertEqual(written["second_read"], self.DISPUTED) + + def test_a_disputed_read_on_a_flag_is_refused(self): + with self.assertRaisesRegex(session_store.StoreError, "demotes the flag"): + self.store.put_beat(dict(FLAG, n=3, second_read=self.DISPUTED)) + self.assertNotIn(3, [beat["n"] for beat in self.store.snapshot()[1]]) + + def test_a_confirmed_read_on_a_beat_that_is_no_flag_is_refused(self): + with self.assertRaisesRegex(session_store.StoreError, "belongs to a flag"): + self.store.put_beat(self.demoted(second_read=self.CONFIRMED)) + + def test_a_misshapen_or_long_read_is_refused(self): + for reading in ( + {"verdict": "agreed", "evidence": "x"}, + {"verdict": "confirmed", "evidence": " "}, + {"verdict": "confirmed"}, + "confirmed", + ): + with self.subTest(reading=reading): + with self.assertRaisesRegex(session_store.StoreError, "second_read requires"): + self.store.put_beat(dict(FLAG, n=3, second_read=reading)) + long = {"verdict": "confirmed", "evidence": " ".join(["word"] * 26)} + with self.assertRaisesRegex(session_store.StoreError, "evidence is 26 words"): + self.store.put_beat(dict(FLAG, n=3, second_read=long)) + + def test_a_flag_written_without_a_read_says_so(self): + unread = {"verdict": "unread", "evidence": "the harness has no subagent tool"} + written = self.store.put_beat(dict(FLAG, n=3, second_read=unread)) + self.assertEqual(written["second_read"], unread) + with self.assertRaisesRegex(session_store.StoreError, "belongs to a flag"): + self.store.put_beat(dict(self.demoted(second_read=unread), n=4)) + + def test_navigation_answers_to_the_same_second_read_rule(self): + action = self.store.produce("nav-1", None, "next", "") + session = dict(self.store.snapshot()[0], current_beat=3) + result = {"kind": "walk", "current_beat": 3} + with self.assertRaisesRegex(session_store.StoreError, "demotes the flag"): + self.store.apply( + action["seq"], result, session=session, + beats=(dict(FLAG, n=3, second_read=self.DISPUTED),), + ) + self.assertNotIn(3, [beat["n"] for beat in self.store.snapshot()[1]]) + + def test_over_drop_belongs_to_a_flag_and_has_words(self): + for beat, message in ( + (self.demoted(over_drop="different"), "belongs to a flag"), + (dict(FLAG, n=3, over_drop=" "), "nonempty text"), + ): + with self.subTest(message=message): + with self.assertRaisesRegex(session_store.StoreError, message): + self.store.put_beat(beat) + + def test_a_stored_flag_cannot_be_demoted_by_a_rewrite(self): + self.store.put_beat(dict(FLAG, n=3)) + with self.assertRaisesRegex(session_store.StoreError, "demotes the flag"): + self.store.put_beat(self.demoted(second_read=self.DISPUTED)) + self.assertEqual(self.beat(3)["state"], "flag") + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_sessionctl.py b/tests/test_sessionctl.py index d65e57b..3e85fbb 100644 --- a/tests/test_sessionctl.py +++ b/tests/test_sessionctl.py @@ -6,6 +6,7 @@ import os import shlex import shutil +import sqlite3 import subprocess import sys import tempfile @@ -584,5 +585,487 @@ def test_a_store_conflict_exits_one_and_preserves_queue_order(self): self.assertEqual(self.store().head()["seq"], first["seq"]) +class NoiseLedger(unittest.TestCase): + """Judging whether a skill change made the walk noisier meant opening each session by + hand. The ledger counts what every stored session raised and how the reviewer answered, + and reading it never upgrades a session in place.""" + + def setUp(self): + self.reviews = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.reviews, True) + + def beat(self, n, state, **extra): + slots = {"what": f"beat {n}", "proof": f"a.py:{n}"} + if state == "flag": + slots.update(risk="r", fix="pin it") + return { + "n": n, "tier": "core", "state": state, "proof_kind": "read", + "claim": "c", "where": f"a.py:{n}", "slots": slots, **extra, + } + + def walked(self): + root = self.reviews / "acme-widget-pr1" + store = session_store.SessionStore(root, create=True) + store.put_session(dict(SESSION, cursor=1)) + for n in (1, 2): + store.put_beat(self.beat(n, "flag")) + store.put_beat(self.beat( + 3, "flag", second_read={"verdict": "confirmed", "evidence": "new at a.py:3"} + )) + store.put_beat(self.beat(4, "unverified")) + store.put_beat(self.beat( + 5, "unverified", second_read={"verdict": "disputed", "evidence": "the base has it"} + )) + store.produce("drop-1", 1, "drop", "the base already did this", reason="pre-existing") + store.ack(store.head()["seq"]) + asked = store.produce("question-4", 4, "question", "Is this proven?") + store.apply(asked["seq"], { + "kind": "question", + "answer": "Not yet.", + "evidence": "Static inspection of a.py:4.", + }) + store.ack(asked["seq"]) + store.produce("accept-2", 2, "accept", "Implement it.") + return root + + def legacy(self): + root = self.reviews / "acme-widget-pr2" + (root / "beats").mkdir(parents=True) + (root / "session.json").write_text(json.dumps(SESSION), encoding="utf-8") + dropped = dict(self.beat(1, "dropped"), call="noise") + (root / "beats" / "01.json").write_text(json.dumps(dropped), encoding="utf-8") + return root + + def stats(self): + completed = subprocess.run( + [sys.executable, str(SESSIONCTL), "stats", str(self.reviews)], + text=True, capture_output=True, + ) + self.assertEqual(completed.returncode, 0, completed.stderr) + return json.loads(completed.stdout) + + def test_two_sessions_yield_counts_per_session_and_overall(self): + self.walked() + self.legacy() + (self.reviews / "acme-widget-pr3.diff").write_text("", encoding="utf-8") + + ledger = self.stats() + + names = [entry["session"] for entry in ledger["sessions"]] + self.assertEqual(names, ["acme-widget-pr1", "acme-widget-pr2"]) + walked, legacy = ledger["sessions"] + self.assertEqual( + {key: walked[key] for key in ( + "beats", "flags", "open", "accepted", "dropped", "unverified", + "confirmed", "disputed", "questions", + )}, + { + "beats": 5, "flags": 3, "open": 1, "accepted": 1, "dropped": 1, + "unverified": 2, "confirmed": 1, "disputed": 1, "questions": 1, + }, + ) + self.assertEqual(walked["dropped_by_reason"]["pre-existing"], 1) + self.assertEqual(walked["unverified_share"], 0.4) + self.assertEqual(legacy["dropped_by_reason"]["unstated"], 1) + overall = ledger["overall"] + self.assertEqual((overall["beats"], overall["flags"], overall["dropped"]), (6, 4, 2)) + self.assertEqual(overall["dropped_by_reason"]["pre-existing"], 1) + self.assertEqual(overall["dropped_by_reason"]["unstated"], 1) + + def test_reading_the_ledger_never_writes_a_session(self): + root = self.walked() + legacy = self.legacy() + database = root / "session.sqlite3" + with sqlite3.connect(str(database)) as db: + db.execute("PRAGMA user_version = 6") + before = { + path: path.read_bytes() + for base in (root, legacy) for path in base.rglob("*") if path.is_file() + } + + self.stats() + + after = { + path: path.read_bytes() + for base in (root, legacy) for path in base.rglob("*") if path.is_file() + } + self.assertEqual(after, before) + self.assertFalse((legacy / "session.sqlite3").exists()) + + def test_an_unreadable_session_is_reported_and_the_rest_still_count(self): + self.walked() + broken = self.reviews / "acme-widget-pr9" + broken.mkdir() + (broken / "session.sqlite3").write_bytes(b"not a database") + + ledger = self.stats() + + entry = next(item for item in ledger["sessions"] if item["session"] == "acme-widget-pr9") + self.assertIn("error", entry) + self.assertEqual(ledger["overall"]["beats"], 5) + + def test_a_missing_directory_is_refused(self): + completed = subprocess.run( + [sys.executable, str(SESSIONCTL), "stats", str(self.reviews / "typo")], + text=True, capture_output=True, + ) + self.assertEqual(completed.returncode, 1) + self.assertIn("no reviews directory", completed.stderr) + + +class ReplacementRounds(unittest.TestCase): + """A moved head used to mean a fresh walk of the whole PR that knew nothing of the + last one. The replacement links its predecessor, walks only what moved, carries the + reviewer's calls forward, and never raises again a finding the reviewer dropped.""" + + def setUp(self): + self.tmp = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.tmp, True) + self.repo = self.tmp / "worktree" + self.reviews = self.tmp / "reviews" + self.reviews.mkdir() + self.git("init", "-q", str(self.repo), cwd=self.tmp) + for key, value in ( + ("user.name", "Underwrite Test"), + ("user.email", "test@example.test"), + ("commit.gpgsign", "false"), + ): + self.git("config", key, value) + self.base = self.commit("one\ntwo\nthree\nfour\nfive\n") + self.first = self.commit("one\nTWO = unpinned\nthree\nFOUR = slow\nfive\n") + self.second = self.commit("one\nTWO = unpinned\nthree\nFOUR = fast\nfive\n") + + def git(self, *args, cwd=None): + return subprocess.run( + ["git", *args], cwd=cwd or self.repo, check=True, capture_output=True, text=True + ).stdout.strip() + + def commit(self, text): + (self.repo / "a.py").write_text(text, encoding="utf-8") + self.git("add", "a.py") + self.git("commit", "-q", "-m", "change") + return self.git("rev-parse", "HEAD") + + def session(self, name, head, number=7): + root = self.reviews / name + store = session_store.SessionStore(root, create=True) + scratch = self.tmp / f"{name}-input" + scratch.mkdir() + diff = scratch / "pr.diff" + diff.write_text(self.git("diff", f"{self.base}...{head}") + "\n", encoding="utf-8") + metadata = scratch / "pr.json" + metadata.write_text(json.dumps({"number": number}), encoding="utf-8") + context = scratch / "trusted.json" + context.write_text( + json.dumps({"version": 1, "base_sha": self.base, "files": []}, + separators=(",", ":"), sort_keys=True) + "\n", + encoding="utf-8", + ) + self.git("update-ref", "refs/underwrite/base", self.base) + self.git("update-ref", "refs/underwrite/head", head) + bundle = scratch / "pr.bundle" + self.git("bundle", "create", "-q", str(bundle), "refs/underwrite/base", "refs/underwrite/head") + store.freeze_target( + { + "version": 1, "kind": "github_pr", "repo": "acme/widget", "number": number, + "state": "open", "merged_at": None, "base_sha": self.base, "head_sha": head, + "head_repo_id": 123, "head_repo": "acme/widget", "head_ref": "feature", + "merge_base_sha": self.base, "changed_files": 1, + }, + diff, metadata, context, bundle, + ) + return root + + def flag(self, n, line, **extra): + return { + "n": n, "tier": "core", "state": "flag", "proof_kind": "read", + "claim": "c", "where": f"a.py:{line}", + "slots": {"what": "w", "proof": f"a.py:{line}", "risk": "r", "fix": "f"}, + "decision": {"question": "Include it?", "recommendation": "Include it."}, + **extra, + } + + def reviewed(self): + root = self.session("acme-widget-pr7", self.first) + store = session_store.SessionStore(root) + store.put_beat(self.flag(1, 2)) + store.put_beat(self.flag(2, 4)) + store.produce("drop-1", 1, "drop", "pinned upstream on purpose", reason="intentional") + store.ack(store.head()["seq"]) + return root + + def run_ctl(self, *args): + return subprocess.run( + [sys.executable, str(SESSIONCTL), *map(str, args)], + text=True, capture_output=True, stdin=subprocess.DEVNULL, timeout=60, + ) + + def link(self, root, prior): + completed = self.run_ctl("link-predecessor", root, prior) + self.assertEqual(completed.returncode, 0, completed.stderr) + return json.loads(completed.stdout) + + def test_the_replacement_scopes_to_moved_lines_and_carries_the_calls(self): + prior = self.reviewed() + replacement = self.session("acme-widget-pr7-r2", self.second) + + linked = self.link(replacement, prior) + + self.assertEqual(linked["round"], 2) + self.assertEqual(linked["session"], "acme-widget-pr7") + self.assertEqual(linked["head_sha"], self.first) + self.assertEqual(linked["moved"], {"a.py": [4]}) + dropped = next(entry for entry in linked["carried"] if entry["n"] == 1) + self.assertEqual(dropped["state"], "dropped") + self.assertEqual(dropped["reason"], "intentional") + self.assertEqual(dropped["anchor"], {"path": "a.py", "text": "TWO = unpinned"}) + self.assertEqual( + [(entry["n"], entry["state"]) for entry in linked["carried"]], + [(1, "dropped"), (2, "flag")], + ) + + def test_a_dropped_finding_is_refused_while_its_line_reads_the_same(self): + prior = self.reviewed() + replacement = self.session("acme-widget-pr7-r2", self.second) + self.link(replacement, prior) + store = session_store.SessionStore(replacement) + + with self.assertRaisesRegex(session_store.StoreError, "dropped this finding in round 1 as intentional"): + store.put_beat(self.flag(1, 2)) + written = store.put_beat(self.flag(1, 4)) + + self.assertEqual(written["where"], "a.py:4") + + def test_the_link_is_write_once_and_comes_before_the_first_beat(self): + prior = self.reviewed() + replacement = self.session("acme-widget-pr7-r2", self.second) + self.link(replacement, prior) + + again = self.run_ctl("link-predecessor", replacement, prior) + self.assertEqual(again.returncode, 1) + self.assertIn("already linked", again.stderr) + store = session_store.SessionStore(replacement) + with self.assertRaisesRegex(session_store.Conflict, "link-predecessor"): + store.patch_session({"predecessor": {"round": 9}}) + self.assertEqual(store.snapshot()[0]["predecessor"]["round"], 2) + + late = self.session("acme-widget-pr7-r3", self.second) + session_store.SessionStore(late).put_beat(self.flag(1, 4)) + refused = self.run_ctl("link-predecessor", late, prior) + self.assertEqual(refused.returncode, 1) + self.assertIn("before the first beat", refused.stderr) + + def test_a_link_to_another_pull_request_or_the_same_head_is_refused(self): + prior = self.reviewed() + other = self.session("acme-widget-pr8", self.second, number=8) + same = self.session("acme-widget-pr7-again", self.first) + + for root, message in ((other, "different pull request"), (same, "resume it instead")): + with self.subTest(root=root.name): + completed = self.run_ctl("link-predecessor", root, prior) + self.assertEqual(completed.returncode, 1) + self.assertIn(message, completed.stderr) + + def test_a_third_round_still_remembers_the_first_rounds_drop(self): + prior = self.reviewed() + second = self.session("acme-widget-pr7-r2", self.second) + self.link(second, prior) + third_head = self.commit("one\nTWO = unpinned\nthree\nFOUR = faster\nfive\n") + third = self.session("acme-widget-pr7-r3", third_head) + + linked = self.link(third, second) + + self.assertEqual(linked["round"], 3) + # Round 1's open flag sat on line 4, whose words round 2's head changed. + self.assertEqual( + [(entry["round"], entry["n"], entry.get("line_changed")) for entry in linked["carried"]], + [(1, 1, None), (1, 2, 2)], + ) + with self.assertRaisesRegex(session_store.StoreError, "round 1"): + session_store.SessionStore(third).put_beat(self.flag(1, 2)) + + def test_a_deletion_between_heads_moves_the_lines_around_it(self): + prior = self.reviewed() + trimmed = self.commit("one\nTWO = unpinned\nthree\nfive\n") + replacement = self.session("acme-widget-pr7-r2", trimmed) + + linked = self.link(replacement, prior) + + self.assertEqual(linked["moved"], {"a.py": [3, 4]}) + + def test_a_line_whose_words_repeat_in_the_diff_is_not_refused(self): + self.base = self.commit("one\nx = 1\nthree\nx = 1\nfive\n") + first = self.commit("one\nx = 2\nthree\nx = 2\nfive\n") + second = self.commit("one\nx = 2\nthree\nx = 2\nsix\n") + prior = self.session("acme-widget-pr7", first) + store = session_store.SessionStore(prior) + store.put_beat(self.flag(1, 2)) + store.produce("drop-1", 1, "drop", "fine here", reason="intentional") + replacement = self.session("acme-widget-pr7-r2", second) + + linked = self.link(replacement, prior) + + self.assertNotIn("anchor", linked["carried"][0]) + written = session_store.SessionStore(replacement).put_beat(self.flag(1, 4)) + self.assertEqual(written["state"], "flag") + + def test_history_names_a_session_it_could_not_read(self): + broken = self.reviews / "acme-widget-pr6" + broken.mkdir() + (broken / "session.sqlite3").write_bytes(b"not a database") + current = self.session("acme-widget-pr7-r2", self.second) + + result = json.loads(self.run_ctl("history", current).stdout) + + self.assertEqual([entry["session"] for entry in result["unreadable"]], ["acme-widget-pr6"]) + + def history_module(self, born): + spec = importlib.util.spec_from_file_location("sessionctl_module", SESSIONCTL) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + module.born = born + return module + + def test_history_carries_earlier_facts_and_skips_later_sessions(self): + prior = self.reviewed() + session_store.SessionStore(prior).patch_session({ + "facts": ["ADR-039 governs claims"], "recommendation": "Fix the pin first.", + }) + current = self.session("acme-widget-pr7-r2", self.second) + self.session("acme-widget-pr9", self.second, number=9) + created = {"acme-widget-pr7": 1.0, "acme-widget-pr7-r2": 2.0, "acme-widget-pr9": 3.0} + module = self.history_module(lambda root: created[Path(root).name]) + + result = module.history(current) + + self.assertEqual([entry["session"] for entry in result["sessions"]], ["acme-widget-pr7"]) + entry = result["sessions"][0] + self.assertEqual(entry["order"], "earlier") + self.assertEqual(entry["facts"], ["ADR-039 governs claims"]) + self.assertEqual(entry["recommendation"], "Fix the pin first.") + + def test_deleting_the_file_between_heads_moves_it_whole(self): + prior = self.reviewed() + (self.repo / "a.py").unlink() + self.git("rm", "-q", "a.py") + self.git("commit", "-q", "-m", "drop a.py") + gone = self.git("rev-parse", "HEAD") + replacement = self.session("acme-widget-pr7-r2", gone) + + linked = self.link(replacement, prior) + + self.assertEqual(linked["moved"], {"a.py": []}) + + def test_navigation_cannot_raise_a_dropped_finding_either(self): + prior = self.reviewed() + replacement = self.session("acme-widget-pr7-r2", self.second) + self.link(replacement, prior) + store = session_store.SessionStore(replacement) + action = store.produce("nav-1", None, "next", "") + session = dict(store.snapshot()[0], current_beat=1) + + with self.assertRaisesRegex(session_store.StoreError, "dropped this finding"): + store.apply(action["seq"], {"kind": "walk", "current_beat": 1}, session=session, + beats=(self.flag(1, 2),)) + self.assertEqual(store.snapshot()[1], []) + + def test_over_drop_raises_a_different_finding_on_a_dropped_line(self): + prior = self.reviewed() + replacement = self.session("acme-widget-pr7-r2", self.second) + self.link(replacement, prior) + store = session_store.SessionStore(replacement) + + written = store.put_beat(self.flag(1, 2, over_drop="the pin is fine; the name now shadows a builtin")) + self.assertEqual(written["state"], "flag") + with self.assertRaisesRegex(session_store.StoreError, "names no earlier drop"): + store.put_beat(self.flag(2, 4, over_drop="nothing was dropped here")) + + def test_a_binary_change_between_heads_moves_the_file_whole(self): + (self.repo / "logo.bin").write_bytes(b"\x00\x01old") + self.git("add", "logo.bin") + self.git("commit", "-q", "-m", "binary") + first = self.git("rev-parse", "HEAD") + (self.repo / "logo.bin").write_bytes(b"\x00\x01new") + self.git("commit", "-q", "-am", "binary again") + second = self.git("rev-parse", "HEAD") + prior = self.session("acme-widget-pr7", first) + replacement = self.session("acme-widget-pr7-r2", second) + + linked = self.link(replacement, prior) + + self.assertEqual(linked["moved"], {"logo.bin": []}) + + def test_a_later_round_supersedes_what_an_earlier_round_left_open(self): + prior = self.reviewed() + second = self.session("acme-widget-pr7-r2", self.second) + self.link(second, prior) + store = session_store.SessionStore(second) + store.put_beat(self.flag(1, 4)) + store.produce("drop-1", 1, "drop", "fast is intended", reason="intentional") + store.ack(store.head()["seq"]) + third_head = self.commit("one\nTWO = unpinned\nthree\nFOUR = fast\nfive\nsix\n") + third = self.session("acme-widget-pr7-r3", third_head) + + linked = self.link(third, second) + + self.assertEqual( + [(entry["round"], entry["n"], entry["state"], entry.get("line_changed")) + for entry in linked["carried"]], + [(1, 1, "dropped", None), (1, 2, "flag", 2), (2, 1, "dropped", None)], + ) + + def test_a_flag_raised_over_a_drop_leaves_the_drop_carried(self): + prior = self.reviewed() + second = self.session("acme-widget-pr7-r2", self.second) + self.link(second, prior) + store = session_store.SessionStore(second) + store.put_beat(self.flag(1, 2, over_drop="the pin holds; the name shadows a builtin")) + third_head = self.commit("one\nTWO = unpinned\nthree\nFOUR = fast\nfive\nsix\n") + third = self.session("acme-widget-pr7-r3", third_head) + + linked = self.link(third, second) + + drops = [e for e in linked["carried"] if e["state"] == "dropped"] + self.assertEqual([(e["round"], e["n"]) for e in drops], [(1, 1)]) + with self.assertRaisesRegex(session_store.StoreError, "dropped this finding in round 1"): + session_store.SessionStore(third).put_beat(self.flag(1, 2)) + + def test_history_marks_order_unknown_where_creation_time_is_not_recorded(self): + self.reviewed() + current = self.session("acme-widget-pr7-r2", self.second) + result = self.history_module(lambda root: None).history(current) + + self.assertEqual([(e["session"], e["order"]) for e in result["sessions"]], [("acme-widget-pr7", "unknown")]) + + def test_confinement_refuses_a_session_outside_the_eval_root(self): + current = self.session("acme-widget-pr7-r2", self.second) + env = dict(os.environ, UNDERWRITE_CONFINE=str(self.reviews / "scratch")) + (self.reviews / "scratch").mkdir() + escape = self.reviews / "scratch" / ".." / "acme-widget-pr7-r2" + for target in (current, escape): + with self.subTest(target=str(target)): + completed = subprocess.run( + [sys.executable, str(SESSIONCTL), "get-session", str(target)], + text=True, capture_output=True, env=env, stdin=subprocess.DEVNULL, + ) + self.assertEqual(completed.returncode, 1) + self.assertIn("outside UNDERWRITE_CONFINE", completed.stderr) + + def test_history_lists_earlier_findings_on_the_paths_this_session_touches(self): + self.reviewed() + current = self.session("acme-widget-pr7-r2", self.second) + + completed = self.run_ctl("history", current) + + self.assertEqual(completed.returncode, 0, completed.stderr) + result = json.loads(completed.stdout) + self.assertEqual(result["repo"], "acme/widget") + sessions = {entry["session"]: entry for entry in result["sessions"]} + self.assertEqual(set(sessions), {"acme-widget-pr7"}) + findings = sessions["acme-widget-pr7"]["findings"] + self.assertEqual([(item["n"], item["state"]) for item in findings], [(1, "dropped"), (2, "flag")]) + self.assertEqual(findings[0]["reason"], "intentional") + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_skill.py b/tests/test_skill.py index c54dfcf..50019ca 100644 --- a/tests/test_skill.py +++ b/tests/test_skill.py @@ -602,5 +602,50 @@ def test_this_repository_asks_for_the_three_in_its_template(self): self.assertIn("which of Testing, Risk and How to revert", readme) +class ReviewerNoise(unittest.TestCase): + """The article's convergence fixes: measure noise before changing the reviewer, check + every flag with a second read, and let a moved head reopen only what moved.""" + + def skill(self): + return " ".join(SKILL.read_text(encoding="utf-8").split()) + + def readme(self): + return " ".join(README.read_text(encoding="utf-8").split()) + + def test_the_ledger_is_the_guardrail_for_skill_changes(self): + for text in (self.skill(), self.readme()): + self.assertIn("sessionctl.py stats ~/.claude/reviews", text) + self.assertIn("a change that raises the dropped share is a regression even when it finds more", text) + + def test_a_flag_is_read_twice_before_it_is_written(self): + walk = self.skill().split("## Phase 3: walk")[1].split("**Persist the beat")[0] + self.assertIn("A flag gets a second read before you write it.", walk) + self.assertIn("Did this PR introduce it? Is it real? Would a senior engineer stop at it?", walk) + self.assertIn("Do not pass your reasoning or your other beats", walk) + self.assertIn("on whether it is real, demotes the flag before it is written", walk) + self.assertIn("A no on whether a senior engineer would stop does not demote it", walk) + self.assertIn("A stored flag cannot be demoted by a rewrite", walk) + + def test_a_replacement_links_its_predecessor_and_plans_only_what_moved(self): + text = self.skill() + ingest = text.split("## Phase 0")[1].split("## Phase 1")[0] + self.assertIn('sessionctl.py link-predecessor "$R" "$P"', ingest) + self.assertIn("never in it", ingest) + self.assertIn('sessionctl.py history "$R"', ingest) + plan = text.split("## Phase 2: plan the walk")[1].split("## Phase 3")[0] + self.assertIn("A replacement plans only what moved.", plan) + self.assertIn("it is not included again", plan) + + def test_the_fourth_round_says_reconsider_the_approach(self): + orient = self.skill().split("## Phase 1: orient")[1].split("## Phase 2")[0] + self.assertIn("In round 4, the third replacement on one PR, the recommendation says to reconsider the approach", orient) + self.assertIn("the recommendation says to reconsider the approach", self.readme()) + + def test_the_review_history_promise_names_the_command_that_keeps_it(self): + text = self.skill() + self.assertNotIn("read the prior sessions for context", text) + self.assertIn("`history` is how a later session reads it", text) + + if __name__ == "__main__": unittest.main()