ci: make the review something you ask for, and tell it where the bugs are

.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,15 +31,15 @@ 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.
 5. **Fail open.** Anything that could leave the WinDivert handle open must stop the engine
    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

.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

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",