From 728729f0fa87c42d8d8621cf88735b0047a8a598 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 17:19:21 +0200 Subject: [PATCH 1/3] docs(review): tell the reviewer where this repository's bugs come from The first version of the briefing carried the conventions and nothing about failure modes, so a review arrived knowing what it must not break and not where to look. The five classes it now names are read off the mutation registry rather than invented - that file records every behaviour broken on purpose to prove a test catches it, and the entries cluster hard: lifecycle races around targeting, view state drifting from fact, a warning that stops matching the state it describes, a column registry drifting from the table it renders, and empty or just-changed states. First among them is the one no gate can see: a guard that cannot fail. An assertion that would also pass over an empty set, a collector that returns nothing, a walk rooted where a runner has no directory. The briefing gives the question to ask of every added assertion - what input makes this red - and says outright that a test which would still pass with the behaviour removed is a finding. Two blind spots are named because a diff hides them. The fake tkinter models one widget class with no style validation and no geometry, so changes to styles, geometry or column mapping are not covered by the tests that look like they cover them. And the suite runs on two platforms while a developer checks one, so a symbol named after one system can raise on the other. Paid for in tokens on every run, so it is not only longer: the meta paragraph and the CI list are cut back, and a "what not to propose" section is added, because the fastest way to make a review worthless is to let it repeat the linter. No counts are written into the prose - an unguarded number goes stale. Also drops the mention of where the maintainer's own notes live. The reason a reviewer needs is that the file is not part of this repository, and that is all it now says. Co-Authored-By: Claude Opus 5 --- .github/claude-review-rules.md | 86 ++++++++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 20 deletions(-) diff --git a/.github/claude-review-rules.md b/.github/claude-review-rules.md index 70ae58a..e6ff090 100644 --- a/.github/claude-review-rules.md +++ b/.github/claude-review-rules.md @@ -1,13 +1,9 @@ # 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, and nothing in it is new - every rule +below is already stated in `CONTRIBUTING.md` or the READMEs. ## What the tool is @@ -34,7 +30,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 +39,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 +54,70 @@ 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 has blind spots.** The suite drives a stand-in for Tk with one widget + class, no style validation and no geometry. Changes to styles, geometry or column mapping + are therefore **not** covered by the tests that appear to cover them, and 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 From 081cbcbe8ae36e8e2d789545fed6a5aa1cdf8b8f Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 17:32:15 +0200 Subject: [PATCH 2/3] docs(review): correct what the fake tkinter cannot see The review on this pull request caught it, with file and line citations, and it was right: the paragraph described the PREVIOUS fake. `tests/fake_tk.py` says so itself, in the past tense, at the top of its own docstring. Verified against the source before changing anything. It defines eleven classes, records `pack_info` and `grid_info`, raises when `before=`/`after=` names a non-sibling, and `Treeview.column` resolves a `"#N"` spec against the columns currently shown - which was added precisely so the hidden-column header-tooltip bug would stop being invisible, the same bug this file names two paragraphs above. Layout and column mapping are covered, and `tests/test_gui_layout.py` asserts on `pack_info` directly. Two blind spots are real and they are the two now named: `ttk.Style` is mapped to a bare widget, so style names and options are never validated; and `winfo_width`, `winfo_height` and `winfo_geometry` return constants, so nothing measures wrapping, clipping or overflow. Steering a reviewer away from the best-covered part of the GUI is worse than saying nothing, and this file becomes the project memory of every future run - so the error would have compounded. Also fixes a second claim in the same file, found while checking the first: the header said every rule below was already stated in CONTRIBUTING.md or the READMEs. After the failure classes were added that was no longer true. It now says what is true - every line comes from files already public here, the registry included. Co-Authored-By: Claude Opus 5 --- .github/claude-review-rules.md | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/.github/claude-review-rules.md b/.github/claude-review-rules.md index e6ff090..dd9640f 100644 --- a/.github/claude-review-rules.md +++ b/.github/claude-review-rules.md @@ -2,8 +2,9 @@ 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, and nothing in it is new - every rule -below is already stated in `CONTRIBUTING.md` or the READMEs. +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 @@ -84,10 +85,15 @@ broken on purpose to prove its test catches it, and the entries cluster. Look he Two more things a diff hides: -- **The fake tkinter has blind spots.** The suite drives a stand-in for Tk with one widget - class, no style validation and no geometry. Changes to styles, geometry or column mapping - are therefore **not** covered by the tests that appear to cover them, and deserve a closer - read than their green suite suggests. +- **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". From efdd14548e074897cdc9383e54b3e645cc5ff89e Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 17:44:28 +0200 Subject: [PATCH 3/3] ci: make the review something you ask for, not something that happens It started as a review on every pull request. One run cost $2.92 - Opus 5, 13 turns, 6 minutes 42 seconds of model time - on a pull request carrying a single Markdown file. The cost tracks how far the reviewer explores, not how large the diff is: that run read the fake tkinter, the layout suite and their docstrings to check one claim. At this project's rate that is a standing monthly bill against a subscription with better uses. So the trigger is inverted rather than tuned. The workflow now waits for `@claude` in a comment and runs no other way: interactive mode, which is what dropping the `prompt` input selects, so it answers the question asked instead of running a fixed task. Reach for it on a change big enough that a second reader earns its keep. The turn count is deliberately not capped. When it is asked for, the deep read is the whole point; `timeout-minutes` is the backstop. Still read-only, still the maintainer only, and the rule digest is still copied into place - that step earns itself: on the run measured above the reviewer was pointed at the briefing and found that one of its own paragraphs described a stand-in that had since been rewritten, with file and line citations. The guard gets stronger rather than weaker. It used to say "must not review on every push"; it now says "must not run by itself at all" - no `pull_request` trigger, no schedule - because half a trigger is the same bill. Mutation entry replaced to match, and caught. Co-Authored-By: Claude Opus 5 --- .github/workflows/claude-review.yml | 122 +++++++++++++--------------- tests/test_mutation_registry.py | 13 +-- tests/test_repo_conventions.py | 53 ++++++------ 3 files changed, 93 insertions(+), 95 deletions(-) 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",