diff --git a/.github/claude-review-rules.md b/.github/claude-review-rules.md index 70ae58a..dd9640f 100644 --- a/.github/claude-review-rules.md +++ b/.github/claude-review-rules.md @@ -1,13 +1,10 @@ # What a review of this repository has to know -This file is the reviewer's briefing. It is copied to `CLAUDE.md` on the CI runner before -the review runs, because the maintainer's own `CLAUDE.md` is not in the repository - it -lives in a private notes repo and a runner never sees it. Without this file the review -arrives with no idea what this project holds itself to and spends its findings on textbook -advice that is already handled. - -Everything below is already visible in `CONTRIBUTING.md` and the READMEs. Nothing private -belongs here: this file is public and permanent, like every other file in a public repo. +The maintainer's own `CLAUDE.md` is not part of this repository, so a CI runner checks out +a tree without it. This file is the public stand-in: it is copied to `CLAUDE.md` for the +length of a review run. Nothing private belongs here: every line below is drawn from files +that are already public in this repository - `CONTRIBUTING.md`, the READMEs, and the suite's +own mutation registry. ## What the tool is @@ -34,7 +31,8 @@ Two consequences worth carrying into every review: local paths, machine names, addresses or tokens, in comments either. 3. **Anything visible from outside goes in the changelog.** `CHANGELOG.md` for users, `CHANGELOG-INTERNAL.md` for maintainers; a GUI change counts as visible. Entries go under - `[Unreleased]`, and `VERSION.txt` is never bumped in a pull request. + `[Unreleased]`, a user-facing entry is capped at 100 words, and `VERSION.txt` is never + bumped in a pull request. 4. **Never break traffic globally.** A real interception needs a narrow target (`--target` / `--dst-ip` / `--dst-port`) and a short `--duration`. `--loss` or `--latency` with no target is a defect, not a default. @@ -42,7 +40,6 @@ Two consequences worth carrying into every review: instead. Traffic is released on failure, never held. 6. **New behaviour arrives with the test that guards it.** A new failure mode gets an exit code, a test and a README row. A new mechanism in the decision pipeline gets unit tests. - A test that cannot fail is worse than no test. ## Contracts that changes must not break silently @@ -58,20 +55,75 @@ Two consequences worth carrying into every review: - **The project website's page addresses are a contract.** The site is published; names on its pages come from the language files, not typed by hand. +## Where this project's bugs actually come from + +This is not a guess. `tests/test_mutation_registry.py` records every behaviour that has been +broken on purpose to prove its test catches it, and the entries cluster. Look here first. + +1. 🔴 **A guard that cannot fail.** The single most valuable finding available in this + repository, and no linter can see it. An assertion that would also pass over an empty + set, a collector that returns nothing, a walk rooted at a directory that does not exist + on a runner, a search pattern that quietly stops matching - each looks like coverage and + is coverage of nothing. **A new test that would still pass with the behaviour removed is + a finding.** Ask of every added assertion: what input makes this red? +2. **A sentence that stops agreeing with the state it describes.** A note, tooltip, chart + caption, log line or warning has to be derived from the state, never from a nearby proxy + that is usually the same. Past defects of exactly this shape: an unbounded run judged + bounded, a session that becomes unbounded and says nothing, a filter of pure exclusions + passing as a target. Numbers and the words beside them must come from one source. +3. **Lifecycle and ordering around targeting.** The largest group by far. A socket that + arrives while a rebuild is in flight, a process adopted and then never re-judged, a + pending entry nobody drains, a failure on one item that kills the thread handling the + rest. Any change here deserves the question "what happens if this arrives during that". +4. **Tables and their column registry drifting apart.** A header describing its neighbour + once a column is hidden, a count that includes hidden columns, a row marked by colour + alone, a number left touching the text beside it. If a change touches columns, check the + registry, the header, the tooltip and the export together. +5. **Empty and just-changed states.** An empty table that renders as a blank rectangle, an + unsearched table blaming a search nobody made. The first and last iteration are where + this code breaks, not the middle. + +Two more things a diff hides: + +- **The fake tkinter models behaviour, not pixels.** `tests/fake_tk.py` models the widgets + the app leans on and records geometry and column calls, so layout and column mapping **are** + covered: `tests/test_gui_layout.py` asserts on `pack_info` directly, and `Treeview.column` + resolves a `"#N"` spec against the columns currently shown, exactly as Tk does. What it + cannot see needs a real renderer. `ttk.Style` is mapped to a bare widget, so style names and + options are never validated and `theme_use` / `map` / `layout` do nothing; and + `winfo_width` / `winfo_height` / `winfo_geometry` return constants, so nothing measures real + wrapping, clipping or overflow. Changes to `gui/theme.py`, or to anything that depends on + measured text, deserve a closer read than their green suite suggests. +- **One platform is not both.** The suite runs on Linux and Windows. A symbol, a keysym or a + path habit named after one system can raise on the other, and "checked locally" here means + "checked on Windows". + +## What a complete pull request looks like here + +The change, the guard that catches its absence, an entry in the mutation registry when it +guards a behaviour, and the changelog lines. **A pull request that adds behaviour with no +guard is itself a finding**, and so is one that changes behaviour without touching the prose +that describes it - the READMEs describe current state, and drift there is invisible. + ## What CI already enforces, so a review need not -ruff (bug shapes, dead code, a measured complexity ceiling), mypy, semgrep, CodeQL, a -coverage gate on the whole repository plus 80 percent on the lines a pull request changes, -a mutation registry that re-breaks each guarded behaviour to prove its test reddens, a -licence gate on new dependencies, a weekly dependency audit, and a check that commit -messages and the pull request body obey rule 2. Every action is pinned to a commit SHA and -no `${{ }}` is ever interpolated into a `run:` script. +ruff, mypy, semgrep and CodeQL; a coverage gate on the repository plus 80 percent on the +lines a pull request changes; the mutation registry; a licence gate on new dependencies; a +weekly dependency audit; and a check that commit messages and the pull request body obey +rule 2. Every action is pinned to a commit SHA and no `${{ }}` reaches a `run:` script. + +**A finding that repeats one of those is noise.** The valuable finding is the one no gate can +see. + +## What not to propose -Findings that repeat one of those are noise. The valuable finding is the one no gate can -see: a wrong answer, a broken edge case, a contract quietly changed, a test that passes for -the wrong reason, a comment that no longer matches the code beneath it. +- **A new dependency.** This project ships a kernel driver and pins its dependencies by + artefact hash; adding one is a deliberate decision with a licence gate in front of it, not + a review suggestion. +- **A broad refactor.** Judge the change that is here. +- **Style already settled by the linter**, or anything the section above covers. -## How to write a finding here +## How to write a finding Say what breaks, with the input or state that breaks it. "This could be clearer" is not a finding; "with `--duration 0` this loops forever, and no test covers it" is. If a rule above diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index adaaa2f..e49979e 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -1,70 +1,69 @@ -# A second reader on every pull request, with a different context from the session -# that wrote the code. +# A second reader, on request only. # -# Why it is worth a workflow when four analysers already run: they read the code for -# shapes. This reads the CHANGE for meaning - a wrong answer, an edge case nobody +# 🔴 This workflow does NOT run by itself. It waits for `@claude` in a comment on a +# pull request or an issue. That is a deliberate reversal of how it started: an +# automatic review on every pull request cost $2.92 per run (measured 2026-08-19, +# Opus 5, 13 turns, on a pull request carrying ONE Markdown file), and cost tracks +# how far the reviewer explores, not how big the diff is. At this project's rate +# that is a standing bill for a subscription that has better uses. +# +# So it is the emergency exit rather than the front door: reach for it on a change +# big enough that a second reader earns its keep, and ignore it the rest of the time. +# The turn count is deliberately NOT capped - when it is asked for, the deep read is +# the whole point. `timeout-minutes` is the backstop. +# +# What it is for: the change read for MEANING - a wrong answer, an edge case nobody # tried, a contract quietly altered, a comment that stopped matching the code under -# it. None of those redden a linter. +# it. The four analysers in ci.yml read the code for shapes and cannot see any of that. # -# 🔴 It cannot fail a pull request. It comments. +# 🔴 It cannot fail a pull request, and it cannot write to the repository. It comments. # -# 🔴 **A pull request that CHANGES THIS FILE gets no review, and the job still goes -# green.** Measured on the pull request that introduced it (2026-08-19): the app -# refuses to hand out a token unless the workflow file is byte-identical to the copy -# on the default branch, and the action then exits with -# *"Exiting due to workflow validation skip"* - a success, in 18 seconds, with no -# model call and nothing spent. That is the app's anti-abuse rule, and it is the -# right one: without it a pull request could edit this file to walk off with the -# token. The consequence to remember rather than re-derive: this workflow cannot be -# tested before it is merged, and every later change to it skips its own review. +# 🔴 A pull request that CHANGES THIS FILE gets no answer, and the job still goes +# green. Measured on the pull request that introduced it: the app refuses to hand out +# a token unless the workflow file is byte-identical to the copy on the default +# branch, and the action exits with *"Exiting due to workflow validation skip"* - a +# success, in 18 seconds, with no model call. That is the app's anti-abuse rule and it +# is the right one; without it a pull request could edit this file and walk off with +# the token. Remember it rather than re-derive it: every later change to this workflow +# skips its own review. name: Claude review on: - # `opened` and `ready_for_review` only, deliberately - NOT `synchronize`. - # `synchronize` fires on every push, and at this project's rate (several pull - # requests a day, several pushes each) that multiplies the bill by the number of - # times somebody amends a branch. The cost of the choice is named rather than - # hidden: a review reads the pull request as opened, so a finding introduced by a - # later push is not seen. Ask for a fresh pass with an `@claude` comment when a - # branch changes substantially. - pull_request: - types: [opened, ready_for_review] + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] -# Read-all at the top; the job raises what it needs. Same rule as every other +# Read-all at the top; the job raises only what it needs. Same rule as every other # workflow here. permissions: contents: read -# One review per pull request. Reopening or marking ready while a review is still -# running replaces it rather than paying for both. +# One conversation per pull request or issue. A second `@claude` while the first is +# still working replaces it rather than paying for both. concurrency: - group: claude-review-${{ github.event.pull_request.number }} + group: claude-review-${{ github.event.issue.number || github.event.pull_request.number }} cancel-in-progress: true jobs: review: name: Claude review - # 🔴 THE COST GATE, and it is three locks rather than one, because this is the - # only job here that spends money per run. - # - # 1. this condition - the pull request must be the maintainer's; - # 2. the action's own check - it refuses an actor without write access, and - # refuses bot actors outright, so Dependabot never triggers it; - # 3. GitHub itself - a public repository withholds secrets from workflows - # triggered by a fork's pull request, so a stranger's branch cannot spend - # the token even if the two above were removed. - # - # Drafts are skipped: `ready_for_review` is in the trigger precisely so the - # review happens once, when the change is finished. + # Two locks, and neither is decoration. The comment must ask for it, and the + # asker must be the maintainer. Beyond these the action refuses an actor without + # write access and refuses bot actors outright, and a public repository withholds + # secrets from anything a fork triggers - but those are invisible in this file, + # and this condition is the one a reader can check. if: >- - github.event.pull_request.user.login == 'donislawdev' - && github.event.pull_request.draft == false + contains(github.event.comment.body, '@claude') + && github.event.comment.user.login == 'donislawdev' runs-on: ubuntu-latest - timeout-minutes: 20 + timeout-minutes: 25 permissions: contents: read pull-requests: read issues: read + # Lets the reviewer read the CI results on the pull request it is looking at. + actions: read # Required by the action's default GitHub App authentication. id-token: write steps: @@ -74,38 +73,33 @@ jobs: # 🔴 The reviewer's briefing, and the reason it needs a step at all. # - # Claude Code reads `CLAUDE.md` from the checkout as project memory - that is - # the documented way to give it a project's rules. This repository's real - # `CLAUDE.md` is git-ignored: it lives in a private notes repo, so a runner + # Claude Code reads `CLAUDE.md` from the checkout as project memory - the + # documented way to give it a project's rules. This repository's real + # `CLAUDE.md` is git-ignored: it is not part of the repository, so a runner # checks out a tree without it and the review would arrive knowing nothing - # about flat hyphens, English-only history, the changelog rule or the fail-open - # contract, and would spend its findings on advice CI already enforces. + # about flat hyphens, English-only history, the changelog rule or fail-open, + # and would spend its findings on what CI already enforces. # # So the public digest in `.github/` is copied into place for the length of the # run. Nothing private crosses over: that file is in the repository, and every - # rule in it is already stated in CONTRIBUTING.md and the READMEs. + # line of it comes from files already public here. + # + # It earns this step. On the run that introduced it the briefing was itself + # reviewed, and the finding was that one of its paragraphs described a stand-in + # that had since been rewritten - caught with file and line citations. - name: Put the public rule digest where Claude reads project memory shell: bash run: cp .github/claude-review-rules.md CLAUDE.md - - name: Review the pull request + # No `prompt` input on purpose: that is what selects interactive mode, where + # Claude answers the comment that summoned it instead of running a fixed task. + # Ask for what you want in the comment - "@claude review this" for the whole + # change, or a narrower question when only one part worries you. + - name: Answer the comment uses: anthropics/claude-code-action@d40ddef4c030e508327d6e35a9c45f3368482c50 # v1.0.195 with: # The subscription token, not an API key: runs bill against the # maintainer's Claude subscription instead of opening a second meter. # Generated with `claude setup-token`. claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} - plugin_marketplaces: "https://github.com/anthropics/claude-code.git" - plugins: "code-review@claude-code-plugins" - # `--comment` is what puts the review on the pull request - an inline - # comment per finding, or one summary comment when there are none. Without - # it the findings stay in the run log, where nobody reads them. - prompt: "/code-review:code-review --comment ${{ github.repository }}/pull/${{ github.event.pull_request.number }}" - # `--model` because the default is whatever Claude Code ships; this project - # would rather pay for the better reader on a change that ships a kernel - # driver. `--allowedTools` has to name the inline-comment tool even though - # the skill's own frontmatter does: the action starts that MCP server only - # when this argument asks for it. - claude_args: | - --model claude-opus-5 - --allowedTools "mcp__github_inline_comment__create_inline_comment" + claude_args: --model claude-opus-5 diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 2537a94..529a0ad 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -1059,13 +1059,14 @@ "test": "test_the_provenance_bundle_ships_as_a_release_asset", }, { - # The one job here that costs money per run, and the one word that decides - # how often it runs. `synchronize` fires on every push. - "label": "review: the paid review starts running on every push", + # The one job here that costs money per run, and the line that decides whether + # it runs at all. Measured at $2.92 a run before it was made optional, so an + # automatic trigger put back "while tidying" is a standing bill nobody chose. + "label": "review: the optional review goes back to running by itself", "file": ".github/workflows/claude-review.yml", - "old": " types: [opened, ready_for_review]", - "new": " types: [opened, ready_for_review, synchronize]", - "test": "test_the_paid_review_keeps_its_cost_gate", + "old": " issue_comment:", + "new": " pull_request:\n types: [opened]\n issue_comment:", + "test": "test_the_optional_review_never_runs_by_itself", }, { "label": "supply chain: release.yml grants write at the file level again", diff --git a/tests/test_repo_conventions.py b/tests/test_repo_conventions.py index 2fe4424..bfd1a09 100644 --- a/tests/test_repo_conventions.py +++ b/tests/test_repo_conventions.py @@ -583,28 +583,28 @@ def test_every_action_a_workflow_uses_is_pinned_to_a_commit(): check("every action is pinned to a full commit SHA", not unpinned, f"({unpinned})") check("every pin says which version it is", not uncommented, f"({uncommented})") +def test_the_optional_review_never_runs_by_itself(): + """The one job here that spends money, and the reason it is not automatic. -def test_the_paid_review_keeps_its_cost_gate(): - """The only workflow here that spends money per run, and what stops it running. + It started as a review on every pull request. Measured on 2026-08-19: **$2.92 for + one run** - Opus 5, 13 turns, on a pull request carrying a single Markdown file - + because the cost tracks how far the reviewer explores, not how large the diff is. + At this project's rate that is a standing bill against a subscription with better + uses, so the trigger was inverted: it now waits to be asked. - Three things keep it cheap, and each one is a line somebody could delete while - tidying and never notice the bill: + What this guards is the shape of that decision, because each piece is one line + somebody could restore while tidying and only notice on the invoice: - * it triggers on `opened` and `ready_for_review` and **not** `synchronize`. - `synchronize` fires on every push, so adding it multiplies the cost by how - many times a branch gets amended - which, at this project's rate, is the - difference between a review per pull request and five; - * it runs only for the maintainer's own pull requests. The action refuses - non-write actors and bots by itself, and a public repository withholds - secrets from fork pull requests, but neither of those is visible in this file - - the condition is, so it is the one a reader can check; - * it holds no write permission. It comments through the app, and nothing here - can push. + * it must not trigger on `pull_request` AT ALL. Not `opened`, not `synchronize` - + any automatic trigger puts the standing bill back; + * it answers a comment, and only one that asks for it by name; + * only the maintainer can summon it; + * it holds no write permission. It reads and comments; it cannot push. Also guarded: the rule digest is still copied into place. Without that step the - review runs with no project context at all - `CLAUDE.md` is git-ignored, so a - runner checks out a tree without it - and the run still succeeds, just uselessly. - That is the failure mode worth a test: not a red job, a wasted one. + review runs with no project context at all - `CLAUDE.md` is not part of this + repository, so a runner checks out a tree without it - and the run still + SUCCEEDS, just uselessly. A wasted run is harder to notice than a red one. """ path = os.path.join(ROOT, ".github", "workflows", "claude-review.yml") check("the review workflow is still here", os.path.exists(path)) @@ -614,14 +614,17 @@ def test_the_paid_review_keeps_its_cost_gate(): lines = handle.read().splitlines() code = [ln.split("#", 1)[0] for ln in lines] body = "\n".join(code) - - check("it does not review on every push", "synchronize" not in body, - "(`synchronize` fires per push - that is the expensive trigger)") - check("it still reviews an opened pull request", "opened" in body) - check("it still reviews one marked ready for review", "ready_for_review" in body) - check("it runs only for the maintainer's pull requests", - "github.event.pull_request.user.login == 'donislawdev'" in body) - check("it skips drafts", "draft == false" in body) + triggers = body.split("jobs:", 1)[0] + + check("nothing makes the review run by itself", + "pull_request:" not in triggers and "schedule:" not in triggers, + "(an automatic trigger is the standing bill this was reversed to avoid)") + check("it is summoned by a comment", + "issue_comment:" in triggers, f"({triggers[-200:]})") + check("the comment has to ask for it by name", + "contains(github.event.comment.body, '@claude')" in body) + check("only the maintainer can summon it", + "github.event.comment.user.login == 'donislawdev'" in body) check("the rule digest is copied where Claude reads project memory", "cp .github/claude-review-rules.md CLAUDE.md" in body) check("the digest it copies exists",