diff --git a/corpus/skills/principle-prove-it/SKILL.md b/corpus/skills/principle-prove-it/SKILL.md index 237048ff..48fffdff 100644 --- a/corpus/skills/principle-prove-it/SKILL.md +++ b/corpus/skills/principle-prove-it/SKILL.md @@ -34,6 +34,22 @@ check now, not lower the confidence and continue. **Absence of output is not proof of success.** A command that printed nothing needs its exit code shown. +**The output must entail the sentence, not merely agree with it.** Before +writing "verified," read the claim and the pasted output side by side and ask +what the output actually rules out. A run on one version, one host, one image, +one input proves the claim *for that instance*; it does not prove the general +or version-boundary statement the sentence made. When the check comes back +narrower than the claim — and it usually will, because the cheap check is the +reachable instance — rewrite the claim down to what ran, and say the wider one +is still open. Filing a narrow result under a wide heading is the error, even +when every word of the output is true. This binds hardest in a correction: +restating the original overclaim while pasting a narrower proof relabels the +mistake as a fix. Named in logic as hasty generalization, *secundum quid* +(Aristotle, *Sophistical Refutations*, Bk. I ch. 5, trans. W.A. +Pickard-Cambridge, http://classics.mit.edu/Aristotle/sophist_refut.html); in +software it is the difference between a witness and a proof, since one passing +instance witnesses existence and never universality. + **Blaming a gate is a causal claim.** "The hook is wrong," "the check misfired," "the classifier blocked it for no reason" — each one needs the gate's rule read this turn and quoted, with its `file:line`, next to the diff --git a/corpus/skills/principle-prove-it/tests/fires_narrow_output_wide_claim.md b/corpus/skills/principle-prove-it/tests/fires_narrow_output_wide_claim.md new file mode 100644 index 00000000..844c4b67 --- /dev/null +++ b/corpus/skills/principle-prove-it/tests/fires_narrow_output_wide_claim.md @@ -0,0 +1,17 @@ +An agent runs `docker run --rm node:25-slim corepack --version` and pastes the +real output: `corepack: not found`. It then writes "verified: corepack was +removed from Node 25+" into the PR body. One image, one tag, one run. Nothing +checked a second 25.x image, a later version, or the release notes. + +This skill fires. The pasted output is real and every word of it is true, but +it does not entail the sentence. It rules out corepack in that one image; it +says nothing about the version boundary the claim draws. The rule that the +output must entail the sentence, not merely agree with it, is what the reply +needs: rewrite the claim down to what ran ("the `node:25-slim` image has no +corepack") and mark the wider statement as open, or run the check that would +actually cover it. + +The same shape fires on a correction. If the agent later says "I overstated +it earlier, here is the proof" and pastes the same single-image run under the +same "removed from Node 25+" heading, the correction has relabeled the +overclaim as a fix. The narrower proof needs the narrower sentence. diff --git a/engine/hooks/gh-write-verification/detect.py b/engine/hooks/gh-write-verification/detect.py index 47b73b81..b10cf01b 100644 --- a/engine/hooks/gh-write-verification/detect.py +++ b/engine/hooks/gh-write-verification/detect.py @@ -63,6 +63,7 @@ import json import os import re +from typing import Optional, Tuple TRUST_PR_EDIT_ENV = "GH_WRITE_VERIFICATION_TRUST_PR_EDIT" @@ -213,6 +214,7 @@ def self_match_message(hits: list[str]) -> str: r"|\bgit\s+branch\s+-r\s+--contains\b" r"|\bgit\s+branch\s+--contains\b[^\n]*\s-r\b" ) +LANDING_OK_RE = re.compile(r"(?m)^\s*OK:") VERIFY_SCRIPT_RELPATH = "gh-write-verification/verify_pr_landed_on_trunk.sh" UNVERIFIED_MERGE_MESSAGE = ( @@ -288,22 +290,33 @@ def silenced_mutations(raw_text: str) -> list[str]: return hits -def _proves_landing(command: str, number: str | None) -> bool: +CommandRecord = Tuple[str, Optional[str]] + + +def _proves_landing(command: str, number: str | None, result: str | None) -> bool: """True when this command checks where a merge commit actually landed. An invocation of the shipped verification script must name the PR it is - vouching for; a hand-rolled ancestry check is accepted as written, since - it takes a commit sha rather than a PR number. + vouching for, and the paired tool result must report the passing verdict. """ command = command or "" if not LANDING_PROOF_RE.search(command): return False if "verify_pr_landed_on_trunk" in command and number is not None: - return number in command - return True + if number not in command: + return False + return bool(result and LANDING_OK_RE.search(result)) + + +def _command_text(record: str | CommandRecord) -> str: + return record[0] if isinstance(record, tuple) else record + + +def _command_result(record: str | CommandRecord) -> str | None: + return record[1] if isinstance(record, tuple) else None -def merges_missing_landing_proof(commands: list[str]) -> list[str]: +def merges_missing_landing_proof(commands: list[str | CommandRecord]) -> list[str]: """PR subjects merged in this turn with no landing check run afterwards. Returns the merged subjects (a PR number, or "the current branch's PR" @@ -312,13 +325,17 @@ def merges_missing_landing_proof(commands: list[str]) -> list[str]: proof ran after the merge. """ subjects: list[str] = [] - for index, command in enumerate(commands): + for index, record in enumerate(commands): + command = _command_text(record) match = GH_PR_MERGE_RE.search(command or "") if not match: continue number = match.group("number") subject = f"PR #{number}" if number else "the current branch's PR" - if any(_proves_landing(later, number) for later in commands[index + 1:]): + if any( + _proves_landing(_command_text(later), number, _command_result(later)) + for later in commands[index + 1:] + ): continue if subject not in subjects: subjects.append(subject) @@ -359,6 +376,17 @@ def _text_content(data: dict) -> str: return "" +def _tool_result_text(block: dict) -> str: + content = block.get("content") + if isinstance(content, str): + return content + if isinstance(content, list): + return "\n".join( + b.get("text", "") for b in content if isinstance(b, dict) and b.get("type") == "text" + ) + return "" + + def _is_human_user_line(data: dict) -> bool: if data.get("type") != "user": return False @@ -366,7 +394,7 @@ def _is_human_user_line(data: dict) -> bool: return bool(text.strip()) and not text.lstrip().startswith("<") -def bash_commands_this_turn(raw_lines) -> list[str]: +def bash_commands_this_turn(raw_lines) -> list[CommandRecord]: """Bash tool commands issued since the last authored user message.""" parsed: list[dict] = [] for raw in raw_lines: @@ -380,8 +408,21 @@ def bash_commands_this_turn(raw_lines) -> list[str]: for index, data in enumerate(parsed): if _is_human_user_line(data): turn_start = index - commands: list[str] = [] - for data in parsed[turn_start:]: + records = parsed[turn_start:] + results: dict[str, str] = {} + for data in records: + message = data.get("message") + content = message.get("content") if isinstance(message, dict) else None + if not isinstance(content, list): + continue + for block in content: + if not isinstance(block, dict) or block.get("type") != "tool_result": + continue + tool_id = block.get("tool_use_id") + if tool_id: + results[str(tool_id)] = _tool_result_text(block) + commands: list[CommandRecord] = [] + for data in records: if data.get("type") != "assistant": continue message = data.get("message") @@ -395,7 +436,9 @@ def bash_commands_this_turn(raw_lines) -> list[str]: continue tool_input = block.get("input") if isinstance(tool_input, dict): - commands.append(str(tool_input.get("command") or "")) + tool_id = block.get("id") or block.get("tool_use_id") + result = results.get(str(tool_id)) if tool_id else None + commands.append((str(tool_input.get("command") or ""), result)) return commands diff --git a/engine/hooks/gh-write-verification/tests/test_hooks.py b/engine/hooks/gh-write-verification/tests/test_hooks.py index c329b15a..3e6a7f6e 100644 --- a/engine/hooks/gh-write-verification/tests/test_hooks.py +++ b/engine/hooks/gh-write-verification/tests/test_hooks.py @@ -57,15 +57,24 @@ def bash_payload(command: str) -> dict: return {"tool_name": "Bash", "cwd": HOOK_DIR, "tool_input": {"command": command}} -def transcript(commands: list[str]) -> str: +def transcript(commands: list[str | tuple[str, str | None]]) -> str: lines = [json.dumps({"type": "user", "message": {"role": "user", "content": "land the stack"}})] - for command in commands: + for index, item in enumerate(commands): + command, result = item if isinstance(item, tuple) else (item, None) + tool_id = f"bash-{index}" lines.append(json.dumps({ "type": "assistant", "message": {"content": [ - {"type": "tool_use", "name": "Bash", "input": {"command": command}} + {"type": "tool_use", "id": tool_id, "name": "Bash", "input": {"command": command}} ]}, })) + if result is not None: + lines.append(json.dumps({ + "type": "user", + "message": {"content": [ + {"type": "tool_result", "tool_use_id": tool_id, "content": result} + ]}, + })) handle = tempfile.NamedTemporaryFile("w", suffix=".jsonl", delete=False) handle.write("\n".join(lines) + "\n") handle.close() @@ -228,6 +237,9 @@ def test_entrypoint_denies_the_incident_wait_loop(self): class TestUnverifiedLanding(unittest.TestCase): + def proven(self, command: str) -> tuple[str, str]: + return (command, "OK: abc123 is an ancestor of origin/main\n") + def test_a_merge_with_no_landing_check_is_flagged(self): self.assertEqual( merges_missing_landing_proof(["gh pr merge 291 --squash --admin", "gh pr view 291"]), @@ -238,7 +250,7 @@ def test_every_unproven_merge_in_the_turn_is_flagged(self): commands = [ "gh pr merge 291 --squash", "gh pr merge 292 --squash", - "bash verify_pr_landed_on_trunk.sh 292", + self.proven("bash verify_pr_landed_on_trunk.sh 292"), ] self.assertEqual(merges_missing_landing_proof(commands), ["PR #291"]) @@ -251,9 +263,18 @@ def test_a_verified_landing_stays_silent(self): "git branch -r --contains 314f0447", ): self.assertEqual( - merges_missing_landing_proof(["gh pr merge 291 --squash", proof]), [], proof + merges_missing_landing_proof(["gh pr merge 291 --squash", self.proven(proof)]), [], proof ) + def test_a_landing_check_without_a_result_is_flagged(self): + self.assertEqual( + merges_missing_landing_proof([ + "gh pr merge 291 --squash", + "bash verify_pr_landed_on_trunk.sh 291", + ]), + ["PR #291"], + ) + def test_a_turn_with_no_merge_stays_silent(self): self.assertEqual(merges_missing_landing_proof(["git status", "gh pr view 291"]), []) @@ -271,7 +292,11 @@ def test_stop_entrypoint_denies_an_unproven_merge(self): def test_stop_entrypoint_allows_a_proven_merge(self): path = transcript([ "gh pr merge 291 --squash --admin", - 'bash "$HOME/.claude/hooks/gh-write-verification/verify_pr_landed_on_trunk.sh" 291', + ( + 'bash "$HOME/.claude/hooks/gh-write-verification/verify_pr_landed_on_trunk.sh" 291', + "pr=#291 repo=acme/widgets merged=true base=main merge_commit=abc123\n" + "OK: abc123 is an ancestor of origin/main\n", + ), ]) try: result = run_entrypoint(STOP_CHECK, {"transcript_path": path}) @@ -279,6 +304,37 @@ def test_stop_entrypoint_allows_a_proven_merge(self): finally: os.unlink(path) + def test_stop_entrypoint_denies_a_failed_landing_check(self): + path = transcript([ + "gh pr merge 291 --squash --admin", + ( + 'bash "$HOME/.claude/hooks/gh-write-verification/verify_pr_landed_on_trunk.sh" 291', + "pr=#291 repo=acme/widgets merged=true base=stack merge_commit=abc123\n" + "FAIL: PR #291 in acme/widgets reports MERGED but abc123 is not on origin/main\n", + ), + ]) + try: + result = run_entrypoint(STOP_CHECK, {"transcript_path": path}) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertIn("PR #291", result.stderr) + finally: + os.unlink(path) + + def test_stop_entrypoint_denies_an_unchecked_landing_check(self): + path = transcript([ + "gh pr merge 291 --squash --admin", + ( + 'bash "$HOME/.claude/hooks/gh-write-verification/verify_pr_landed_on_trunk.sh" 291', + "UNCHECKED: gh cannot resolve a repository here\n", + ), + ]) + try: + result = run_entrypoint(STOP_CHECK, {"transcript_path": path}) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertIn("PR #291", result.stderr) + finally: + os.unlink(path) + def test_a_missing_transcript_fails_open(self): self.assertIsNone(decide_stop({"transcript_path": "/nonexistent/transcript.jsonl"})) self.assertIsNone(decide_stop({})) diff --git a/engine/hooks/llm-judge/README.md b/engine/hooks/llm-judge/README.md index 31eba814..ec80ca2d 100644 --- a/engine/hooks/llm-judge/README.md +++ b/engine/hooks/llm-judge/README.md @@ -165,7 +165,9 @@ hold up the reply. `inbox.messages(transcript)` drains that transcript's verdicts and turns each one into a line of text: -- **hit**: the job's `on_hit` text, word for word. +- **hit**: the job's `on_hit` text, word for word, followed by a space and the + answer's `report` string when the answer has a non-blank one, clipped to 600 + characters. - **unchecked**: `llm-judge: could not judge the last reply: ` then `: ` for each try, joined by `; `. If there were no tries (the judge broke, or the verdict file was unreadable), the verdict's own diff --git a/engine/hooks/llm-judge/inbox.py b/engine/hooks/llm-judge/inbox.py index fd9766fa..ed285123 100644 --- a/engine/hooks/llm-judge/inbox.py +++ b/engine/hooks/llm-judge/inbox.py @@ -6,6 +6,7 @@ import judge NO_TRANSCRIPT = "llm-judge: {harness} payload has no transcript path, so finished verdicts were not checked" +REPORT_LIMIT = 600 def resolve_transcript(payload: dict) -> str: @@ -49,6 +50,11 @@ def messages(transcript: str) -> list[str]: text = item.get("on_hit") if not isinstance(text, str) or not text.strip(): text = f"llm-judge: {item.get('hook') or 'unknown hook'} flagged the last reply: {item.get('reason')}" + answer = item.get("answer") + if isinstance(answer, dict): + report = answer.get("report") + if isinstance(report, str) and report.strip(): + text = f"{text} {report.strip()[:REPORT_LIMIT]}" out.append(text) continue out.append(unchecked_message(item)) diff --git a/engine/hooks/llm-judge/judge_test_base.py b/engine/hooks/llm-judge/judge_test_base.py index 01249d18..9bb41eb8 100644 --- a/engine/hooks/llm-judge/judge_test_base.py +++ b/engine/hooks/llm-judge/judge_test_base.py @@ -2,6 +2,7 @@ import json import os +import sys import tempfile import unittest from unittest.mock import patch @@ -13,10 +14,14 @@ class JudgeTestCase(unittest.TestCase): def setUp(self): super().setUp() self.state = tempfile.TemporaryDirectory() - self.judge_env = patch.dict(os.environ, {judge.STATE_ENV: self.state.name}) + self.judge_env = patch.dict(os.environ, { + judge.STATE_ENV: self.state.name, + judge.RUNNERS_ENV: json.dumps([ + ["stub", [sys.executable, "-c", "print('{\"match\": false}')", judge.PROMPT_SLOT]], + ]), + }) self.judge_env.start() os.environ.pop(judge.CHILD_ENV, None) - os.environ.pop(judge.RUNNERS_ENV, None) def tearDown(self): self.judge_env.stop() diff --git a/engine/hooks/llm-judge/tests/test_inbox.py b/engine/hooks/llm-judge/tests/test_inbox.py index b142bdcb..af270bd4 100644 --- a/engine/hooks/llm-judge/tests/test_inbox.py +++ b/engine/hooks/llm-judge/tests/test_inbox.py @@ -56,6 +56,9 @@ def seed(self, *runner_entries, job_id="job-1"): }) return judge.run_job(job_path) + def seed_verdict(self, verdict, job_id="job-1"): + judge.write_json_atomic(os.path.join(judge.verdict_dir(self.transcript), f"{job_id}.json"), verdict) + def run_claude(self, stdin_text): out, err = io.StringIO(), io.StringIO() with patch.object(sys, "stdin", io.StringIO(stdin_text)), redirect_stdout(out), redirect_stderr(err): @@ -84,6 +87,31 @@ def test_hit_yields_the_exact_on_hit_text_once(self): self.assertEqual(inbox.messages(self.transcript), [ON_HIT]) self.assertEqual(inbox.messages(self.transcript), []) + def test_hit_with_report_appends_report(self): + report = "model saw a quoted rollback" + self.seed_verdict({"outcome": "hit", "hook": "demo-hook", "on_hit": ON_HIT, "answer": {"report": report}}) + found = inbox.messages(self.transcript) + self.assertEqual(found, [f"{ON_HIT} {report}"]) + self.assertTrue(found[0].endswith(f" {report}"), found) + + def test_hit_without_report_equals_on_hit_exactly(self): + self.seed_verdict({"outcome": "hit", "hook": "demo-hook", "on_hit": ON_HIT, "answer": {"match": True}}) + self.assertEqual(inbox.messages(self.transcript), [ON_HIT]) + + def test_hit_report_is_clipped_to_600_characters(self): + report = "x" * 700 + self.seed_verdict({"outcome": "hit", "hook": "demo-hook", "on_hit": ON_HIT, "answer": {"report": report}}) + self.assertEqual(inbox.messages(self.transcript), [f"{ON_HIT} {'x' * 600}"]) + + def test_hit_number_or_list_report_is_ignored(self): + self.seed_verdict({"outcome": "hit", "hook": "demo-hook", "on_hit": ON_HIT, "answer": {"report": 5}}, job_id="a") + self.seed_verdict({"outcome": "hit", "hook": "demo-hook", "on_hit": ON_HIT, "answer": {"report": ["detail"]}}, job_id="b") + self.assertEqual(inbox.messages(self.transcript), [ON_HIT, ON_HIT]) + + def test_clean_with_report_yields_nothing(self): + self.seed_verdict({"outcome": "clean", "hook": "demo-hook", "on_hit": ON_HIT, "answer": {"report": "ignored"}}) + self.assertEqual(inbox.messages(self.transcript), []) + def test_unchecked_yields_one_reason_per_runner(self): self.assertEqual(self.seed(MISSING, CRASHES)["outcome"], "unchecked") self.assertEqual( diff --git a/engine/hooks/llm-judge/tests/test_judge.py b/engine/hooks/llm-judge/tests/test_judge.py index 52246b69..a41cf24d 100644 --- a/engine/hooks/llm-judge/tests/test_judge.py +++ b/engine/hooks/llm-judge/tests/test_judge.py @@ -108,10 +108,17 @@ def test_malformed_runners_env_refuses_instead_of_running_defaults(self): with self.assertRaises(ValueError): judge.ask("x") + def test_test_base_runs_only_the_local_stub(self): + self.assertEqual([name for name, _ in judge.runners()], ["stub"]) + self.assertEqual(judge.ask("x")["answer"], {"match": False}) + def test_default_runner_order_is_codex_then_claude_then_cursor(self): - self.assertEqual([name for name, _ in judge.runners()], ["codex", "claude", "cursor"]) + with patch.dict(os.environ): + os.environ.pop(judge.RUNNERS_ENV) + self.assertEqual([name for name, _ in judge.runners()], ["codex", "claude", "cursor"]) def test_investigate_runner_argv_is_read_only_and_excludes_cursor(self): + os.environ.pop(judge.RUNNERS_ENV) self.assertEqual( judge.runners("investigate"), [ @@ -159,6 +166,7 @@ def test_investigate_runners_env_replaces_investigate_defaults(self): self.assertEqual(judge.runners("investigate"), [("probe", custom[1])]) def test_investigate_job_threads_timeout_and_cwd_to_runner(self): + os.environ.pop(judge.RUNNERS_ENV) path = os.path.join(self.state.name, "jobs", "investigate-job.json") with tempfile.TemporaryDirectory() as cwd: judge.write_json_atomic(path, self.job(id="investigate-job", mode="investigate", timeout_seconds=123, cwd=cwd)) @@ -220,6 +228,18 @@ def test_unchecked_when_ask_was_unchecked(self): class TestBackground(JudgeBehaviorTestCase): + def test_enqueue_writes_only_to_temporary_state_directory(self): + with tempfile.TemporaryDirectory() as home: + with patch.dict(os.environ, {"HOME": home}): + with patch.dict(os.environ): + os.environ.pop(judge.STATE_ENV) + default_state = judge.state_root() + os.makedirs(default_state) + with patch.object(judge.subprocess, "Popen"): + self.assertEqual(judge.enqueue(self.job(id="isolated-job")), "isolated-job") + self.assertTrue(os.path.isfile(os.path.join(self.state.name, "jobs", "isolated-job.json"))) + self.assertEqual(os.listdir(default_state), []) + def test_enqueue_as_judge_child_returns_none_and_starts_nothing(self): os.environ[judge.CHILD_ENV] = "1" self.use_runners(ANSWER_MATCH) diff --git a/product/skills/measure-then-optimize/SKILL.md b/product/skills/measure-then-optimize/SKILL.md deleted file mode 100644 index 31998b6d..00000000 --- a/product/skills/measure-then-optimize/SKILL.md +++ /dev/null @@ -1,14 +0,0 @@ ---- -name: measure-then-optimize -description: >- - Measure and verify work that makes something faster, cheaper, or lighter. - Use before diagnosing or changing performance, cost, resource use, latency, - throughput, startup time, or build time. ---- - -# Measure then optimize - -Follow [Measure, identify, fix, verify](playbooks/measure-identify-fix-verify.md) -before making a performance change. A performance fix is done only when the -user's symptom was measured before and after under the same real conditions by -a committed, rerunnable program, and the ledger's `judge` verdict is `pass`. diff --git a/product/skills/measure-then-optimize/playbooks/measure-identify-fix-verify.md b/product/skills/measure-then-optimize/playbooks/measure-identify-fix-verify.md deleted file mode 100644 index b224228d..00000000 --- a/product/skills/measure-then-optimize/playbooks/measure-identify-fix-verify.md +++ /dev/null @@ -1,77 +0,0 @@ -# Measure, identify, fix, verify - -Rob Pike's rule 2 in [*Notes on Programming in C* (1989)](https://users.ece.utexas.edu/~adnan/pike.html) -is the starting constraint: "Measure. Don't tune for speed until you've -measured." Brendan Gregg's [Performance Analysis Methodology](https://www.brendangregg.com/methodology.html) -provides methods for moving from the observed problem toward measured causes -instead of changing things at random. - -## 1. Measure - -Define the metric for the user's own symptom, not a convenient proxy. Exercise -the real path with the real workload, configuration, hardware, and other -settings that matter. Write a program that reruns that measurement and commit -it as the lever before changing the system; [Build the Lever](../../../../corpus/skills/principle-build-the-lever/SKILL.md) -explains why the program must remain reviewable and rerunnable. - -Record the baseline with [perf_ledger.py](../scripts/perf_ledger.py), using at -least three runs and naming every relevant setting: - -```sh -python3 product/skills/measure-then-optimize/scripts/perf_ledger.py record \ - --ledger perf-ledger.json \ - --phase baseline \ - --metric boot_seconds \ - --runs 5 \ - --setting workload=production \ - --symptom-metric boot_seconds \ - --lever-path scripts/measure_boot.py \ - -- python3 scripts/measure_boot.py -``` - -Use `--from-stdout` when the program prints the metric; otherwise the ledger -records wall-clock seconds. Preserve the baseline ledger with the change. - -## 2. Identify - -Name a cause only after a measurement attributes time or cost to it. Break the -symptom into measured components, then rank candidates by their measured share -of the total. Profiles, traces, counters, and time-division measurements can -support attribution; intuition and code proximity cannot. - -Investigate the largest measured candidate first. If the measurements cannot -distinguish candidates, improve the measurement before choosing a cause. - -## 3. Fix - -Change one measured cause at a time so the next measurement can attribute any -difference to that change. Keep the measurement program, workload, and settings -fixed. - -Raising a timeout, memory ceiling, retry budget, batch limit, or other limit is -not a performance fix unless the before and after symptom numbers justify it. -Treat a limit increase without that evidence as moving the boundary, not -removing the cost. - -## 4. Verify - -Rerun the same committed program with the same settings and record the result as -the after phase: - -```sh -python3 product/skills/measure-then-optimize/scripts/perf_ledger.py record \ - --ledger perf-ledger.json \ - --phase after \ - --metric boot_seconds \ - --runs 5 \ - --setting workload=production \ - -- python3 scripts/measure_boot.py - -python3 product/skills/measure-then-optimize/scripts/perf_ledger.py judge \ - perf-ledger.json -``` - -Report the `judge` output verbatim, including its verdict and reason set. Only a -`pass` verdict completes the performance fix. `unchecked` is not done: repair -the missing or unreadable measurement and rerun it rather than treating the -absence of evidence as success. diff --git a/product/skills/measure-then-optimize/scripts/perf_ledger.py b/product/skills/measure-then-optimize/scripts/perf_ledger.py deleted file mode 100644 index aae7614f..00000000 --- a/product/skills/measure-then-optimize/scripts/perf_ledger.py +++ /dev/null @@ -1,130 +0,0 @@ -import argparse -import json -import subprocess -import sys -import time -from pathlib import Path - - -def number(value): - return isinstance(value, (int, float)) and not isinstance(value, bool) - - -def valid_entry(entry): - if not isinstance(entry, dict): - return entry is None - required = ("metric", "cmd", "settings", "runs") - if any(key not in entry for key in required): - return False - return (isinstance(entry["metric"], str) and isinstance(entry["cmd"], str) - and isinstance(entry["settings"], dict) and isinstance(entry["runs"], list) - and bool(entry["runs"]) and all(number(value) for value in entry["runs"])) - - -def judge(ledger): - if not isinstance(ledger, dict) or "symptom_metric" not in ledger: - return "unchecked", ["unreadable-entry"] - baseline = ledger.get("baseline") - after = ledger.get("after") - if not valid_entry(baseline) or not valid_entry(after): - return "unchecked", ["unreadable-entry"] - symptom = ledger["symptom_metric"] - reasons = [] - if baseline is None: - reasons.append("no-baseline") - if after is None: - reasons.append("no-real-after") - for entry in (baseline, after): - if entry is not None and entry["metric"] != symptom: - reasons.append("proxy-metric") - if baseline is not None and after is not None and baseline["metric"] == symptom and after["metric"] == symptom: - if baseline["cmd"] != after["cmd"] or baseline["settings"] != after["settings"]: - reasons.append("different-conditions") - for entry in (baseline, after): - if entry is not None and len(entry["runs"]) < 3: - reasons.append("single-run") - limit_change = ledger.get("limit_change") - if (isinstance(limit_change, dict) and number(limit_change.get("old")) - and limit_change.get("old") is not None and number(limit_change.get("new")) - and limit_change.get("new") is not None and limit_change["new"] > limit_change["old"]): - reasons.append("limit-loosened") - if not ledger.get("lever_path"): - reasons.append("no-lever") - reasons = sorted(set(reasons)) - return ("fail" if reasons else "pass"), reasons - - -def record(args): - values = [] - for _ in range(args.runs): - started = time.monotonic() - result = subprocess.run(args.program, capture_output=True, text=True) - if result.returncode: - sys.stderr.write(result.stderr) - return 2 - if args.from_stdout: - value = None - for line in reversed(result.stdout.splitlines()): - try: - candidate = float(line.strip()) - except ValueError: - continue - value = int(candidate) if candidate.is_integer() else candidate - break - if value is None: - sys.stderr.write("no numeric stdout value\n") - return 2 - else: - value = time.monotonic() - started - values.append(value) - path = Path(args.ledger) - if path.exists(): - data = json.loads(path.read_text()) - else: - data = {} - if not isinstance(data, dict): - data = {} - data[args.phase] = {"metric": args.metric, "cmd": " ".join(args.program), "settings": dict(item.split("=", 1) for item in args.setting), "runs": values} - if args.symptom_metric is not None: - data["symptom_metric"] = args.symptom_metric - if args.lever_path is not None: - data["lever_path"] = args.lever_path - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(json.dumps(data, indent=2) + "\n") - return 0 - - -def main(argv=None): - parser = argparse.ArgumentParser() - subparsers = parser.add_subparsers(dest="command", required=True) - judge_parser = subparsers.add_parser("judge") - judge_parser.add_argument("ledger") - record_parser = subparsers.add_parser("record") - record_parser.add_argument("--ledger", required=True) - record_parser.add_argument("--phase", choices=("baseline", "after"), required=True) - record_parser.add_argument("--metric", required=True) - record_parser.add_argument("--runs", type=int, default=5) - record_parser.add_argument("--setting", action="append", default=[]) - record_parser.add_argument("--from-stdout", action="store_true") - record_parser.add_argument("--symptom-metric") - record_parser.add_argument("--lever-path") - record_parser.add_argument("program", nargs=argparse.REMAINDER) - args = parser.parse_args(argv) - if args.command == "judge": - try: - data = json.loads(Path(args.ledger).read_text()) - except (OSError, ValueError, TypeError): - verdict, reasons = "unchecked", ["unreadable-entry"] - else: - verdict, reasons = judge(data) - print(json.dumps({"verdict": verdict, "reasons": reasons}, separators=(",", ":"))) - return {"pass": 0, "fail": 1, "unchecked": 2}[verdict] - if args.program and args.program[0] == "--": - args.program = args.program[1:] - if not args.program or args.runs < 1 or any("=" not in item for item in args.setting): - parser.error("record requires a program, positive runs, and key=value settings") - return record(args) - - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/product/skills/measure-then-optimize/tests/cases.json b/product/skills/measure-then-optimize/tests/cases.json deleted file mode 100644 index 4537c5d8..00000000 --- a/product/skills/measure-then-optimize/tests/cases.json +++ /dev/null @@ -1,21 +0,0 @@ -[ -{"id":"s01-palette-open","note":"Palette open lag; timed only in a fake browser, limit later raised 50 to 150.","ledger":{"symptom_metric":"palette_open_ms_real_app","baseline":null,"after":{"metric":"palette_open_ms_jsdom","cmd":"vitest command-palette","settings":{"env":"jsdom"},"runs":[31,29,33]},"limit_change":{"metric":"palette_open_ms_jsdom","old":50,"new":150},"lever_path":"packages/ui/src/__tests__/command-palette.test.tsx"},"expected":{"verdict":"fail","reasons":["no-baseline","proxy-metric","limit-loosened"]}}, -{"id":"s02-dag-click","note":"DAG click beachball; cause matched to an old bug, proof on a synthetic 20k-event fixture.","ledger":{"symptom_metric":"dag_click_stall_ms_real_app","baseline":null,"after":{"metric":"get_events_ms_synthetic","cmd":"vitest dag-click-get-events-cost","settings":{"events":20000},"runs":[12,11,13]},"limit_change":null,"lever_path":"packages/app/src/__tests__/dag-click-get-events-cost.test.ts"},"expected":{"verdict":"fail","reasons":["no-baseline","proxy-metric"]}}, -{"id":"s03-focus-switch","note":"Focus-switch beachball; real repro did not reproduce, fix proven on a synthetic snapshot, limit raised 100 to 150.","ledger":{"symptom_metric":"focus_switch_stall_ms","baseline":{"metric":"focus_switch_stall_ms","cmd":"playwright focus-switch","settings":{"app":"electron"},"runs":[34,30,36]},"after":{"metric":"action_graph_snapshot_ms","cmd":"vitest snapshot","settings":{"fixture":"synthetic"},"runs":[8,8,9]},"limit_change":{"metric":"focus_switch_stall_ms","old":100,"new":150},"lever_path":"packages/app/e2e/main-process-hitch-responsiveness.spec.ts"},"expected":{"verdict":"fail","reasons":["proxy-metric","limit-loosened"]}}, -{"id":"s04-worker-start-stop","note":"Worker start/stop beachball; fixed from reading code, 200ms timing added only after the fix.","ledger":{"symptom_metric":"worker_toggle_ack_ms","baseline":null,"after":{"metric":"worker_toggle_ack_ms","cmd":"playwright ui-action-responsiveness-battery","settings":{"app":"electron"},"runs":[120,115,130]},"limit_change":null,"lever_path":"packages/app/e2e/ui-action-responsiveness-battery.spec.ts"},"expected":{"verdict":"fail","reasons":["no-baseline"]}}, -{"id":"s05-terminal-lag","note":"Terminal lag; measured upserts per chunk outside the app, never keystroke latency.","ledger":{"symptom_metric":"terminal_keystroke_latency_ms","baseline":{"metric":"sqlite_upsert_ms_per_chunk","cmd":"node upsert-harness","settings":{"cap_kb":64},"runs":[0.35,0.36,0.34]},"after":{"metric":"upserts_per_chunk","cmd":"vitest terminal-upsert","settings":{"cap_kb":64},"runs":[1,1,1]},"limit_change":null,"lever_path":"packages/app/e2e/terminal-upsert-hitch-responsiveness.spec.ts"},"expected":{"verdict":"fail","reasons":["proxy-metric"]}}, -{"id":"s07-drag-lag","note":"Drag lag; real slow-query logs used, but drag was never re-timed after the fix.","ledger":{"symptom_metric":"dag_drag_lag_ms","baseline":{"metric":"events_query_ms","cmd":"slow-query log","settings":{"source":"live"},"runs":[1591,1402,1510]},"after":null,"limit_change":null,"lever_path":"scripts/repro/repro-get-events-by-types-temp-btree.sh"},"expected":{"verdict":"fail","reasons":["proxy-metric","no-real-after"]}}, -{"id":"s09-boot-vacuum","note":"10-minute boot; one boot timed, fix measured by database file size, next boot still stalled.","ledger":{"symptom_metric":"boot_seconds","baseline":{"metric":"boot_seconds","cmd":"owner restart","settings":{"host":"do1"},"runs":[739]},"after":{"metric":"db_file_mb","cmd":"stat invoker.db","settings":{"host":"do1"},"runs":[1450]},"limit_change":null,"lever_path":null},"expected":{"verdict":"fail","reasons":["single-run","proxy-metric","no-lever"]}}, -{"id":"s10-boot-reload","note":"Boot should be under 5s; fix proven by reload call count, boot never re-timed.","ledger":{"symptom_metric":"boot_seconds","baseline":null,"after":{"metric":"reload_call_count","cmd":"vitest reload-count","settings":{"fixture":"unit"},"runs":[4,4,4]},"limit_change":null,"lever_path":"packages/app/src/__tests__/reload-count.test.ts"},"expected":{"verdict":"fail","reasons":["no-baseline","proxy-metric"]}}, -{"id":"s12-relaunch-timeout","note":"Relaunch failed at 90s; timeout raised to 20 minutes from two boots; proof was the constant's value.","ledger":{"symptom_metric":"boot_seconds","baseline":{"metric":"boot_seconds","cmd":"owner relaunch","settings":{"host":"do1"},"runs":[90,780]},"after":{"metric":"timeout_constant_ms","cmd":"vitest launch-health-timeout","settings":{"fixture":"unit"},"runs":[1200000]},"limit_change":{"metric":"boot_seconds","old":90,"new":1200},"lever_path":null},"expected":{"verdict":"fail","reasons":["single-run","proxy-metric","limit-loosened","no-lever"]}}, -{"id":"s14-token-simulator","note":"Real fix-ci sessions burned 1.24B tokens; the after number came from a simulator.","ledger":{"symptom_metric":"tokens_per_fixci_session","baseline":{"metric":"tokens_per_fixci_session","cmd":"codex-session-audit","settings":{"source":"real_sessions"},"runs":[21313030,19800000,22400000]},"after":{"metric":"tokens_per_fixci_session","cmd":"fix-ci-token-bench","settings":{"source":"simulator"},"runs":[966000,970000,960000]},"limit_change":null,"lever_path":"scripts/fix-ci-token-bench.mjs"},"expected":{"verdict":"fail","reasons":["different-conditions"]}}, -{"id":"s16-runaway-spend","note":"Runaway Codex spend; a cap was claimed but never checked on a real session, spend recurred.","ledger":{"symptom_metric":"tokens_per_session","baseline":{"metric":"tokens_per_session","cmd":"fleet_cost_report","settings":{"source":"real_sessions"},"runs":[99500000,41000000,39458756]},"after":null,"limit_change":null,"lever_path":"scripts/codex-session-audit.py"},"expected":{"verdict":"fail","reasons":["no-real-after"]}}, -{"id":"s17-sqljs-oom","note":"Out of memory; the only win used a 15s window against a 20s baseline window.","ledger":{"symptom_metric":"peak_rss_mb","baseline":{"metric":"peak_rss_mb","cmd":"run-oom-repro","settings":{"timeout_sec":20},"runs":[582.8,579.1,584.0]},"after":{"metric":"peak_rss_mb","cmd":"run-oom-repro","settings":{"timeout_sec":15},"runs":[477.6,480.2,475.9]},"limit_change":null,"lever_path":"scripts/run-oom-benchmark-matrix.mjs"},"expected":{"verdict":"fail","reasons":["different-conditions"]}}, -{"id":"s18-cold-start-bytes","note":"UI cold start; measured bundle bytes, never startup time; size budget set between before and after.","ledger":{"symptom_metric":"ui_cold_start_ms","baseline":{"metric":"entry_chunk_bytes","cmd":"repro-ui-startup-bundle-size","settings":{"build":"prod"},"runs":[1770000]},"after":{"metric":"entry_chunk_bytes","cmd":"repro-ui-startup-bundle-size","settings":{"build":"prod"},"runs":[340000]},"limit_change":{"metric":"entry_chunk_bytes","old":null,"new":1300000},"lever_path":"scripts/repro/repro-ui-startup-bundle-size.sh"},"expected":{"verdict":"fail","reasons":["proxy-metric","single-run"]}}, -{"id":"s19-submission-storm","note":"The good run: real submission path timed before and after with the same script, committed and wired into the proof gate.","ledger":{"symptom_metric":"workflow_submit_ms","baseline":{"metric":"workflow_submit_ms","cmd":"bench-workflow-submission-storm","settings":{"path":"real","submissions":20},"runs":[61000,350,352,348]},"after":{"metric":"workflow_submit_ms","cmd":"bench-workflow-submission-storm","settings":{"path":"real","submissions":20},"runs":[900,340,351,347]},"limit_change":null,"lever_path":"scripts/bench-workflow-submission-storm.sh"},"expected":{"verdict":"pass","reasons":[]}}, -{"id":"s20-image-cache-first","note":"Image disk cache counted done for 8 days because its class had a passing check; never switched on.","ledger":{"symptom_metric":"images_downloaded_on_cold_start","baseline":null,"after":{"metric":"disk_cache_class_exists","cmd":"flutter check image_loader","settings":{"device":"none"},"runs":[1,1,1]},"limit_change":null,"lever_path":null},"expected":{"verdict":"fail","reasons":["no-baseline","proxy-metric","no-lever"]}}, -{"id":"s21-hourly-scan","note":"Hourly job too slow; real scheduled run before and after, but one run each and no timing script kept.","ledger":{"symptom_metric":"scan_runtime_seconds","baseline":{"metric":"scan_runtime_seconds","cmd":"launchd hourly job","settings":{"path":"production"},"runs":[104]},"after":{"metric":"scan_runtime_seconds","cmd":"launchd hourly job","settings":{"path":"production"},"runs":[5]},"limit_change":null,"lever_path":null},"expected":{"verdict":"fail","reasons":["single-run","no-lever"]}}, -{"id":"s23-ci-shards","note":"Slow CI shards; one run before and after, shard groups copied by hand.","ledger":{"symptom_metric":"ci_job_seconds","baseline":{"metric":"ci_job_seconds","cmd":"github actions run","settings":{"branch":"feature"},"runs":[1157]},"after":{"metric":"ci_job_seconds","cmd":"github actions run","settings":{"branch":"feature"},"runs":[503]},"limit_change":null,"lever_path":null},"expected":{"verdict":"fail","reasons":["single-run","no-lever"]}}, -{"id":"s24-corpus-scan-hang","note":"Corpus search hung on a 146MB file; fix proven with a mocked timeout, search time never measured.","ledger":{"symptom_metric":"corpus_scan_seconds","baseline":null,"after":{"metric":"mocked_timeout_raised","cmd":"unittest corpus_scan","settings":{"fixture":"mock"},"runs":[1,1,1]},"limit_change":null,"lever_path":"engine/skills/reflect/scripts/corpus_scan.py"},"expected":{"verdict":"fail","reasons":["no-baseline","proxy-metric"]}}, -{"id":"x01-unreadable-after","note":"A ledger whose after entry is missing its runs field cannot be judged.","ledger":{"symptom_metric":"boot_seconds","baseline":{"metric":"boot_seconds","cmd":"owner restart","settings":{"host":"do1"},"runs":[739,700,720]},"after":{"metric":"boot_seconds","cmd":"owner restart","settings":{"host":"do1"}},"limit_change":null,"lever_path":"scripts/bench-boot.sh"},"expected":{"verdict":"unchecked","reasons":["unreadable-entry"]}} -] diff --git a/product/skills/measure-then-optimize/tests/test_perf_ledger.py b/product/skills/measure-then-optimize/tests/test_perf_ledger.py deleted file mode 100644 index c79d4aca..00000000 --- a/product/skills/measure-then-optimize/tests/test_perf_ledger.py +++ /dev/null @@ -1,68 +0,0 @@ -import json -import pathlib -import subprocess -import sys -import tempfile -import unittest - - -ROOT = pathlib.Path(__file__).resolve().parents[4] -SCRIPT = ROOT / "product/skills/measure-then-optimize/scripts/perf_ledger.py" -CASES = pathlib.Path(__file__).with_name("cases.json") - - -class PerfLedgerTests(unittest.TestCase): - @classmethod - def setUpClass(cls): - cls.cases = json.loads(CASES.read_text()) - - def run_cli(self, *args): - return subprocess.run([sys.executable, str(SCRIPT), *args], capture_output=True, text=True) - - def judge_case(self, case): - with tempfile.TemporaryDirectory() as directory: - ledger = pathlib.Path(directory) / "ledger.json" - ledger.write_text(json.dumps(case["ledger"])) - result = self.run_cli("judge", str(ledger)) - self.assertEqual(result.returncode, {"pass": 0, "fail": 1, "unchecked": 2}[case["expected"]["verdict"]]) - self.assertEqual(json.loads(result.stdout), {"verdict": case["expected"]["verdict"], "reasons": sorted(case["expected"]["reasons"])}) - - def test_record_wall_clock(self): - with tempfile.TemporaryDirectory() as directory: - ledger = pathlib.Path(directory) / "ledger.json" - result = self.run_cli("record", "--ledger", str(ledger), "--phase", "baseline", "--metric", "boot_seconds", "--runs", "3", "--setting", "host=local", "--", sys.executable, "-c", "pass") - self.assertEqual(result.returncode, 0) - data = json.loads(ledger.read_text()) - self.assertEqual(data["baseline"]["metric"], "boot_seconds") - self.assertEqual(data["baseline"]["settings"], {"host": "local"}) - self.assertEqual(len(data["baseline"]["runs"]), 3) - self.assertTrue(all(isinstance(value, (int, float)) for value in data["baseline"]["runs"])) - - def test_record_from_stdout(self): - with tempfile.TemporaryDirectory() as directory: - ledger = pathlib.Path(directory) / "ledger.json" - result = self.run_cli("record", "--ledger", str(ledger), "--phase", "after", "--metric", "boot_seconds", "--runs", "2", "--from-stdout", "--symptom-metric", "boot_seconds", "--lever-path", "bench.py", "--", sys.executable, "-c", "print('ignored'); print(12.5)") - self.assertEqual(result.returncode, 0) - self.assertEqual(json.loads(ledger.read_text())["after"]["runs"], [12.5, 12.5]) - - def test_record_failure_leaves_ledger_untouched(self): - with tempfile.TemporaryDirectory() as directory: - ledger = pathlib.Path(directory) / "ledger.json" - original = '{"sentinel": true}\n' - ledger.write_text(original) - result = self.run_cli("record", "--ledger", str(ledger), "--phase", "after", "--metric", "boot_seconds", "--runs", "2", "--", sys.executable, "-c", "import sys; print('bad', file=sys.stderr); sys.exit(3)") - self.assertEqual(result.returncode, 2) - self.assertIn("bad", result.stderr) - self.assertEqual(ledger.read_text(), original) - - -def make_case_test(case): - return lambda self: self.judge_case(case) - - -for case in json.loads(CASES.read_text()): - setattr(PerfLedgerTests, "test_case_" + case["id"], make_case_test(case)) - - -if __name__ == "__main__": - unittest.main()