From d52fdfd5f6bae355cefe18800fc08f32f52850bb Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 15:49:52 +0200 Subject: [PATCH 1/6] docs(contributing): state the testing policy as a policy The rule was already followed and already written down twice - a new failure mode gets an exit code, a test and a README row; a new mechanism gets a numbered spot in the pipeline plus unit tests - but only as two examples of what happens, never as the policy behind them. A reader looking for the policy found instances of it. Says it once, and names what enforces it: the mutation registry records which broken behaviour each test is supposed to catch, and CI re-breaks them to prove the test actually reddens. Replaces a line added in #136 that repeated the two bullets above it verbatim. Co-Authored-By: Claude Opus 5 --- CONTRIBUTING.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 56ca31d..166a325 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -58,7 +58,10 @@ onedir, `asInvoker`. Do not reintroduce `--noconsole` / `--onefile` / `--uac-adm - Presets are ordered best (top) -> worst (bottom). - Keep `BeanCore.decide()` pure and covered by tests; new mechanisms get a numbered spot in the pipeline plus unit tests. -- A new failure mode gets an exit code, a test and a README row. A new mechanism gets a numbered spot in the pipeline plus unit tests +- **New functionality is merged with the test that guards it.** That is the policy, and the + two bullets above are what it looks like in practice. It is not left to good intentions: + `tests/test_mutation_registry.py` records which broken behaviour each test is supposed to + catch, and CI re-breaks them to prove the test actually reddens. ## Pull requests From e89e59b657b80b5c0883deffe1663ad09179c627 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 15:50:13 +0200 Subject: [PATCH 2/6] ci(release): ship the provenance bundle beside the archive Both attestations went into GitHub's attestation store and nowhere else, which is enough for `gh attestation verify -R ` and not enough for two other readers. A person holding the download and no path to that API cannot check it, and neither can anyone who took the files from a mirror - the proof has to travel with the archive. The release now uploads the bundle as `BeanNetworkTester-.sigstore.json`, and `--bundle` verifies offline. The second reader is a scanner, and it is why this was worth finding: OpenSSF Scorecard's Signed-Releases check reads release assets BY FILE EXTENSION and never opens the attestation store, so it scored 0 out of 10 on releases that already carried two signed statements. Read in probes/releasesAreSigned and probes/releasesHaveProvenance rather than guessed. The name says what the file is. The action writes a Sigstore bundle, so the asset is `.sigstore.json`. `.intoto.jsonl` would score two points higher on that check and is a different format, not another name for this one. Worth knowing before reading the first number: the check scores floor(sum / releases) over the last five releases, so one signed release scores 2, not 10. The full value arrives after five in a row. The step reads `bundle-path` off the provenance action through `env:`, never interpolated into the script, and checks the file parses as JSON before uploading a name that promises it does. Guarded by a test that also pins the extension; the existing attestation guard had to learn that the publish command now spans lines, which it was silently not reading. Co-Authored-By: Claude Opus 5 --- .github/workflows/release.yml | 35 +++++++++++++++++++++++- CHANGELOG.md | 9 ++++--- README.md | 10 +++++++ README.pl.md | 10 +++++++ tests/test_version_and_release.py | 44 ++++++++++++++++++++++++++++++- 5 files changed, 102 insertions(+), 6 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 53ae2af..87ffeb1 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -190,10 +190,42 @@ jobs: # built BY the action from the workflow's own context - there is nothing to # pass in, and nothing we could pass in that would be trustworthy. - name: Attest the build provenance of the release archive + id: provenance uses: actions/attest-build-provenance@4d101475d8b20a2381f78447822ac1eab6504dd8 # v4.2.2 with: subject-path: ${{ env.ASSET }} + # The same bundle, published as an ASSET rather than left only in the + # attestation store. Two reasons, one for a person and one for a scanner. + # + # For a person: the file travels with the archive it describes, so + # `gh attestation verify --bundle ` answers without asking an + # API - which is the difference between "GitHub says this is fine" and "these + # bytes say so". A mirror that copies the release page copies the proof too. + # + # 馃敶 For a scanner: OpenSSF Scorecard's Signed-Releases check reads release + # assets BY FILE EXTENSION (`.sigstore.json`, `.asc`, `.sig`, `.intoto.jsonl`) + # and never looks in the attestation store. Read in probes/releasesAreSigned + # on 2026-08-19, after the check scored 0/10 on releases that already carried + # two attestations. Producing evidence nobody can find is the same as not + # producing it. + # + # The name says what the file IS: a Sigstore bundle, which is what the action + # writes. `.intoto.jsonl` would score two points higher there and would be a + # different format - not a rename. + - name: Publish the provenance bundle beside the archive + shell: bash + env: + # Through the environment, never interpolated into the script: `${{ }}` is + # expanded before a shell exists, so it is source code rather than an + # argument. Guarded by tests/test_repo_conventions.py. + BUNDLE_PATH: ${{ steps.provenance.outputs.bundle-path }} + run: | + bundle="BeanNetworkTester-${GITHUB_REF_NAME}.sigstore.json" + cp "$BUNDLE_PATH" "$bundle" + python -c "import json,sys; json.load(open(sys.argv[1])); print('bundle parses as JSON')" "$bundle" + echo "BUNDLE=$bundle" >> "$GITHUB_ENV" + # gh is preinstalled on the runner - no third-party action, uses the job token. # A -rc/-beta/-alpha tag publishes as a "Pre-release"; a plain tag as "Latest". # @@ -212,4 +244,5 @@ jobs: head -5 RELEASE_NOTES.md flags=(--title "$TITLE" --notes-file RELEASE_NOTES.md) if [ "$PRERELEASE" = "true" ]; then flags+=(--prerelease); else flags+=(--latest); fi - gh release create "$GITHUB_REF_NAME" "$ASSET" SHA256SUMS.txt "$SBOM" "${flags[@]}" + gh release create "$GITHUB_REF_NAME" \ + "$ASSET" SHA256SUMS.txt "$SBOM" "$BUNDLE" "${flags[@]}" diff --git a/CHANGELOG.md b/CHANGELOG.md index ce786c4..c7c50ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,11 +23,12 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol - **You can now check where a download came from, not just that it is unchanged.** Every release archive carries a signed build attestation, so one command answers "was this really - built from that source, by that workflow": + built from that source": `gh attestation verify BeanNetworkTester-v0.5.0-windows-x64.zip -R donislawdev/BeanNetworkTester`. - A checksum proves the file matches the release page. This proves the release page itself came - out of this repository's own workflow, from a specific commit. The same command also verifies - the SBOM shipped beside the archive. + A checksum proves the file matches the release page; this proves the page came out of this + repository's workflow, from a specific commit. It covers the SBOM too. The proof now ships + **as a file** as well, `BeanNetworkTester-vX.Y.Z.sigstore.json`, so `--bundle` checks the + archive from a mirror, or with no network at all. ### Changed diff --git a/README.md b/README.md index 2b2844c..e891ad2 100644 --- a/README.md +++ b/README.md @@ -1485,6 +1485,16 @@ A checksum proves the file matches what the release page says. This proves the r was produced by this repository's own workflow, from a specific commit, on a GitHub-hosted runner. The same command also verifies the SBOM that ships beside the archive. +That command asks GitHub. The proof also ships **as a file**, `BeanNetworkTester-vX.Y.Z.sigstore.json`, +so you can check the archive without one: + +```bash +gh attestation verify BeanNetworkTester-v0.5.0-windows-x64.zip --bundle BeanNetworkTester-v0.5.0.sigstore.json +``` + +Useful if you got the files from a mirror, or from a machine that cannot reach the API - the +evidence travelled with the download instead of living somewhere you have to trust separately. + ### What is inside the download, and how to check it Every release carries an **SBOM** - a list, in the standard SPDX format, of every third-party diff --git a/README.pl.md b/README.pl.md index 41758c1..56a7de3 100644 --- a/README.pl.md +++ b/README.pl.md @@ -1347,6 +1347,16 @@ Suma kontrolna dowodzi, 偶e plik zgadza si臋 z tym, co m贸wi strona wydania. To strona wydania powsta艂a z workflow tego repozytorium, z konkretnego commita, na maszynie GitHuba. Tym samym poleceniem sprawdzisz te偶 SBOM, kt贸ry jedzie obok archiwum. +To polecenie pyta GitHuba. Dow贸d jedzie te偶 **jako plik**, `BeanNetworkTester-vX.Y.Z.sigstore.json`, +wi臋c archiwum sprawdzisz bez pytania kogokolwiek: + +```bash +gh attestation verify BeanNetworkTester-v0.5.0-windows-x64.zip --bundle BeanNetworkTester-v0.5.0.sigstore.json +``` + +Przydaje si臋, gdy pliki masz z kopii lustrzanej albo na maszynie bez dost臋pu do API - dow贸d +przyjecha艂 razem z pobranym plikiem, zamiast le偶e膰 w miejscu, kt贸remu trzeba osobno ufa膰. + ### Co jest w 艣rodku pobranego pliku i jak to sprawdzi膰 Ka偶de wydanie niesie **SBOM** - list臋, w standardowym formacie SPDX, wszystkich diff --git a/tests/test_version_and_release.py b/tests/test_version_and_release.py index d3c01f3..6e1ac44 100644 --- a/tests/test_version_and_release.py +++ b/tests/test_version_and_release.py @@ -513,7 +513,9 @@ def test_the_release_attests_exactly_the_archive_it_publishes(): check("both attestations name a subject", len(subjects) == 2, f"({subjects})") check("both attest the same file", len(set(subjects)) == 1, f"({subjects})") - publish = re.search(r"gh release create[^\n]*", text) + # The command spans lines: match it through its backslash continuations, or the + # asset list looks empty and every check below passes on nothing. + publish = re.search(r"gh release create(?:[^\n]*\\\n)*[^\n]*", text) check("the workflow publishes a release", publish is not None) uploaded = publish.group(0) if publish else "" # `subject-path: ${{ env.ASSET }}` against `gh release create ... "$ASSET" ...` @@ -526,6 +528,46 @@ def test_the_release_attests_exactly_the_archive_it_publishes(): check(f"{action} is still in the release workflow", action in text) +def test_the_provenance_bundle_ships_as_a_release_asset(): + """An attestation nobody can find is an attestation nobody has. + + Both attestations went into GitHub's attestation store and nowhere else, which + is enough for `gh attestation verify -R ` and not enough for two + other readers: + + * a person holding the download and no network path to that API, or a mirror + that copied the release page - the proof has to travel with the archive; + * 馃敶 OpenSSF Scorecard, whose Signed-Releases check reads release assets BY + FILE EXTENSION and never opens the attestation store. Measured 2026-08-19: + the check scored 0/10 on releases that already carried two attestations. + `probes/releasesAreSigned` accepts `.asc`, `.minisig`, `.sig`, `.sign`, + `.sigstore` and `.sigstore.json`; `probes/releasesHaveProvenance` accepts + `.intoto.jsonl` and nothing else. + + So the bundle is copied to a named asset and uploaded. The extension is part of + what this guards: renaming it to `.intoto.jsonl` would score two points higher + and would be a lie about the format, because the action writes a Sigstore + bundle. + """ + import re + with open(os.path.join(ROOT, ".github", "workflows", "release.yml"), + encoding="utf-8") as handle: + text = handle.read() + + check("the provenance step is addressable (it needs an id for its output)", + re.search(r"id:\s*provenance\b", text) is not None) + check("the bundle path is read from that step's output", + "steps.provenance.outputs.bundle-path" in text) + check("the bundle is named as a Sigstore bundle", + ".sigstore.json" in text, + "(the action writes a Sigstore bundle - the name has to say so)") + + publish = re.search(r"gh release create(?:[^\n]*\\\n)*[^\n]*", text) + uploaded = publish.group(0) if publish else "" + check("the bundle is uploaded with the other assets", + '"$BUNDLE"' in uploaded, f"({uploaded[:160]})") + + def _check_every_requirement_carries_hashes(filename): """Shared by the two hash-checked files: every line pinned, every pin hashed. From 53f11b1c2ef5731b1506a22100ebbf7114bba628 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 15:50:35 +0200 Subject: [PATCH 3/6] ci: review each pull request with a second reader Four analysers already read the code for shapes. This reads the change for meaning - a wrong answer, an edge case nobody tried, a contract quietly altered, a comment that stopped matching the code under it - with a context the session that wrote the change does not have. It comments; it cannot fail a pull request. The trigger is the cost control, not the model. `opened` and `ready_for_review` only, never `synchronize`, which fires on every push and would multiply the bill by how often a branch is amended. The cost of that choice is stated rather than hidden: a problem introduced by a later push is not seen, and a fresh pass is asked for with a comment. Opus 5 deliberately, on a project that ships a kernel driver. Three locks on who can spend it: the condition in the workflow, the action's own refusal of actors without write access and of bots, and GitHub withholding secrets from fork pull requests in a public repository. The job holds no write permission. The part that needed finding: `CLAUDE.md` is git-ignored here, so a runner checks out a tree without it and the review would arrive knowing none of this repository's rules - flat hyphens, English-only history, the changelog rule, fail-open - and would spend its findings on what CI already enforces. `.github/claude-review-rules.md` is a public digest of those rules, every line of it already in CONTRIBUTING and the READMEs, and the workflow copies it into place for the length of the run. That is also the failure mode the guard is for: without the copy the job still succeeds, just uselessly. A wasted run is harder to notice than a red one, so the test pins the copy step, the trigger, the actor condition and the absence of write permission. Two mutations, both caught. Setup this needs on the repository side: the Claude GitHub App installed, and a `CLAUDE_CODE_OAUTH_TOKEN` secret so runs bill against a subscription rather than opening a second meter. Co-Authored-By: Claude Opus 5 --- .github/claude-review-rules.md | 78 +++++++++++++++++++++ .github/workflows/claude-review.yml | 101 ++++++++++++++++++++++++++++ tests/test_mutation_registry.py | 20 ++++++ tests/test_repo_conventions.py | 47 +++++++++++++ 4 files changed, 246 insertions(+) create mode 100644 .github/claude-review-rules.md create mode 100644 .github/workflows/claude-review.yml diff --git a/.github/claude-review-rules.md b/.github/claude-review-rules.md new file mode 100644 index 0000000..70ae58a --- /dev/null +++ b/.github/claude-review-rules.md @@ -0,0 +1,78 @@ +# 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. + +## What the tool is + +Bean Network Tester simulates poor network conditions on Windows - latency, packet loss, +jitter, bandwidth limits, dropped connections - so a developer can see how their program +behaves on a bad link. It has a GUI and a CLI, and it works by loading the WinDivert kernel +driver and mangling packets in flight. + +Two consequences worth carrying into every review: + +- **It runs on the machine it is testing.** A change that widens what gets intercepted can + take the user's own network down with it. Impairment must always be narrow - a target + process, address or port - and bounded by a duration. +- **It ships a kernel driver to strangers.** Supply chain, signatures and pinned bytes are + not paperwork here; they are the product. + +## Rules that a reviewer should treat as blocking + +1. **Flat hyphen only.** No em dash, no en dash, anywhere in the repository - code, + comments, docs, changelogs. A test enforces it. +2. **Everything in git is English.** Commit messages, pull request titles and bodies. Quote + the program - interface text, command output, code - and never a person: a sentence from + a conversation is somebody else's words, and a public commit cannot be unpublished. No + 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. +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 + +- **The CLI is a CI/CD interface.** Every outcome has an exit code from + `beantester/exitcodes.py`. Logs go to stderr, data goes to stdout, as text or NDJSON. + Changing a code, a stream or the NDJSON schema is a breaking change. +- **UI text lives in `lang/.json`, never in code.** Code carries i18n keys. A new key + goes into `lang/en.json` **and** `lang/pl.json` in the same change, or the other language + falls back silently. +- **`BeanCore.decide()` stays pure.** It is the decision pipeline and it is covered + position by position. +- **Presets are ordered best at the top, worst at the bottom.** +- **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. + +## 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. + +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. + +## How to write a finding here + +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 +is broken, name the rule. If nothing is wrong, say so plainly rather than filling the space. diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml new file mode 100644 index 0000000..f740ec8 --- /dev/null +++ b/.github/workflows/claude-review.yml @@ -0,0 +1,101 @@ +# A second reader on every pull request, with a different context from the session +# that wrote the code. +# +# 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 +# tried, a contract quietly altered, a comment that stopped matching the code under +# it. None of those redden a linter. +# +# 馃敶 It cannot fail a pull request. It comments. +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] + +# Read-all at the top; the job raises 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. +concurrency: + group: claude-review-${{ 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. + if: >- + github.event.pull_request.user.login == 'donislawdev' + && github.event.pull_request.draft == false + runs-on: ubuntu-latest + timeout-minutes: 20 + permissions: + contents: read + pull-requests: read + issues: read + # Required by the action's default GitHub App authentication. + id-token: write + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 1 + + # 馃敶 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 + # 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. + # + # 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. + - 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 + 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" diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 2a965ca..cee8a33 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -1029,6 +1029,26 @@ { # The permission that outlives the job it was written for: back at the top # of release.yml, where every job added later inherits it. + # Evidence that exists and cannot be found is evidence nobody has. The + # attestation stayed in GitHub's store, where the archive's own readers - + # a person offline, a mirror, a scanner reading assets by extension - never + # look. + "label": "release: the provenance bundle stops shipping as an asset", + "file": ".github/workflows/release.yml", + "old": '"$ASSET" SHA256SUMS.txt "$SBOM" "$BUNDLE"', + "new": '"$ASSET" SHA256SUMS.txt "$SBOM"', + "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", + "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", + }, + { "label": "supply chain: release.yml grants write at the file level again", "file": ".github/workflows/release.yml", "old": "permissions:\n contents: read", diff --git a/tests/test_repo_conventions.py b/tests/test_repo_conventions.py index 3dd7029..2fe4424 100644 --- a/tests/test_repo_conventions.py +++ b/tests/test_repo_conventions.py @@ -582,3 +582,50 @@ def test_every_action_a_workflow_uses_is_pinned_to_a_commit(): check("the scan found the actions the workflows use", seen >= 15, f"({seen})") 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_paid_review_keeps_its_cost_gate(): + """The only workflow here that spends money per run, and what stops it running. + + Three things keep it cheap, and each one is a line somebody could delete while + tidying and never notice the bill: + + * 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. + + 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. + """ + path = os.path.join(ROOT, ".github", "workflows", "claude-review.yml") + check("the review workflow is still here", os.path.exists(path)) + if not os.path.exists(path): + return + with open(path, encoding="utf-8") as handle: + 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) + 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", + os.path.exists(os.path.join(ROOT, ".github", "claude-review-rules.md"))) + check("the review job holds no write permission", + "write" not in body.split("jobs:", 1)[1].replace("id-token: write", ""), + "(a reviewer that can push is not a reviewer)") From 3968a4ca348cd310d3b02b737503050f0f1489da Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 15:54:08 +0200 Subject: [PATCH 4/6] docs(readme): carry the OpenSSF Best Practices badge The project is registered and passes at 100 percent of that level, so the badge row says so next to the Scorecard one. Scorecard's own CII-Best-Practices check reads the bestpractices.dev API by repository URL rather than the README, so this changes nothing about the score - it is there for a person deciding whether to install a tool that loads a kernel driver. Co-Authored-By: Claude Opus 5 --- README.md | 1 + README.pl.md | 1 + 2 files changed, 2 insertions(+) diff --git a/README.md b/README.md index e891ad2..6f042fa 100644 --- a/README.md +++ b/README.md @@ -5,6 +5,7 @@ [![Downloads](https://img.shields.io/github/downloads/donislawdev/BeanNetworkTester/total)](https://github.com/donislawdev/BeanNetworkTester/releases) [![License: GPLv3](https://img.shields.io/badge/License-GPLv3-blue.svg)](LICENSE) [![OpenSSF Scorecard](https://api.securityscorecards.dev/projects/github.com/donislawdev/BeanNetworkTester/badge)](https://scorecard.dev/viewer/?uri=github.com/donislawdev/BeanNetworkTester) +[![OpenSSF Best Practices](https://www.bestpractices.dev/projects/14154/badge)](https://www.bestpractices.dev/projects/14154) ![Platform: Windows](https://img.shields.io/badge/platform-Windows-0078D6) **Bean Network Tester** is a tool for testers and developers: check how your application behaves diff --git a/README.pl.md b/README.pl.md index 56a7de3..b899e37 100644 --- a/README.pl.md +++ b/README.pl.md @@ -5,6 +5,7 @@ [![Downloads](https://img.shields.io/github/downloads/donislawdev/BeanNetworkTester/total)](https://github.com/donislawdev/BeanNetworkTester/releases) [![License: GPLv3](https://img.shields.io/badge/License-GPLv3-blue.svg)](LICENSE) [![OpenSSF Scorecard](https://api.securityscorecards.dev/projects/github.com/donislawdev/BeanNetworkTester/badge)](https://scorecard.dev/viewer/?uri=github.com/donislawdev/BeanNetworkTester) +[![OpenSSF Best Practices](https://www.bestpractices.dev/projects/14154/badge)](https://www.bestpractices.dev/projects/14154) ![Platform: Windows](https://img.shields.io/badge/platform-Windows-0078D6) **Bean Network Tester** to narz臋dzie dla tester贸w i deweloper贸w: sprawd藕, jak aplikacja zachowuje From c9ca14d8dbf1c2c9a8a7bec7b658612616b2d878 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 16:36:38 +0200 Subject: [PATCH 5/6] fix(matchers): stop a trailing escape from swallowing the next filter term `Matcher.describe` joins terms with ", ", so a term ending in a backslash escaped that comma on the way back in and two terms silently became one. The filter a user reads stopped being the filter that runs, which is the class of lie the project's fifth convention exists to prevent, and it reaches them through the filter display, the log line and the repro report. Measured on the shape this tool meets most often - a Windows path typed with its trailing separator, then a process name. Two terms in, one term out. Only an ODD run of trailing backslashes is dangerous, so the fix drops one rather than stripping the tail: an even run escapes itself and already round-tripped. Measured across eleven shapes, three broken before and none after. The dropped character carries no meaning in any kind this language has - a regex ending in a backslash is not a valid regex at all - so this repairs a term rather than truncating one. `split_terms` is exported from the facade, so this is a change to public behaviour rather than only an internal repair. Its readers are `parse_matcher`, which builds every filter the GUI, the CLI and saved profiles use, and the GUI's add-to-filter path. A stored profile with such a term now parses one character shorter; it already displayed something other than what it did. Found by the property test that found the comma half of this - and then that test PASSED on CI the same afternoon it failed locally, because Hypothesis explores at random and its counterexample lives in a local database. So the regression test here is deterministic and spells both inputs out. Two mutations, both caught: the fix removed, and the fix widened into a plain rstrip, which is the likelier future mistake because it looks tidier. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 6 +++++ beantester/matchers.py | 37 ++++++++++++++++++++++++++++++- tests/test_matchers.py | 39 +++++++++++++++++++++++++++++++++ tests/test_mutation_registry.py | 19 ++++++++++++++++ 4 files changed, 100 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c7c50ea..b89151a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,12 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol ### Fixed +- **A filter ending in a backslash no longer swallows the one after it.** Type a Windows path with + its trailing separator and a second name - `C:\, chrome.exe` - and the tool showed two filters + while using one, because that backslash ate the comma between them. The stray backslash is now + dropped as the filter is read. It never meant anything on its own, so nothing you can usefully + write is affected, and what the filter line says is what the filter does. + - **Column tooltips in Connections no longer describe the wrong column.** With any column hidden, every header to its right explained its neighbour instead - and with only a couple of columns left, the tooltip could describe a column that was not on screen at all. The tooltip now follows diff --git a/beantester/matchers.py b/beantester/matchers.py index f8d8d57..9666fcc 100644 --- a/beantester/matchers.py +++ b/beantester/matchers.py @@ -342,7 +342,42 @@ def split_terms(text): if escaped: buf.append("\\") parts.append("".join(buf)) - return [p.strip() for p in parts if p.strip()] + terms = [] + for part in parts: + term = _without_a_dangling_escape(part.strip()) + if term: + terms.append(term) + return terms + + +def _without_a_dangling_escape(term): + """``term`` with a trailing backslash that cannot survive being described. + + 馃敶 The round trip this protects: ``Matcher.describe`` joins terms with ``", "``, + so a term ENDING in a backslash escapes that separator on the way back in, and + two terms silently become one. Measured 2026-08-19 with the exact filter this + tool meets most often:: + + C:\\ ,chrome.exe -> ['C:\\', 'chrome.exe'] two terms + describe -> 'C:\\, chrome.exe' + parsed again -> ['C:, chrome.exe'] ONE term + + A filter that reads as two names and behaves as one is the class of lie + convention 5 exists to stop, and it reaches the user through the display of the + filter, the log line and the repro report. + + **Only an ODD count is dangerous**, which is why this drops one rather than + stripping the tail: an even run escapes itself and already round-trips, so + ``a\\\\`` is left exactly as it is. The dropped character carries no meaning in + any kind this language has - a ``re:`` pattern ending in a backslash is not a + valid regex at all (*bad escape (end of pattern)*), and no address, port or + process name ends in one - so this repairs a term rather than truncating it. + + Found by ``test_describe_reparses_to_the_same_matcher``, the same property test + that found the comma-escape half of this in ``describe``. + """ + trailing = len(term) - len(term.rstrip("\\")) + return term[:-1] if trailing % 2 else term def add_term(text, term): diff --git a/tests/test_matchers.py b/tests/test_matchers.py index 0b8aadd..326d8c3 100644 --- a/tests/test_matchers.py +++ b/tests/test_matchers.py @@ -318,6 +318,45 @@ def test_matcher_never_raises_from_matches(): check("Matcher is the exported base class", isinstance(m, Matcher)) +def test_a_term_may_not_end_in_an_escape_that_swallows_the_separator(): + """The other half of the same fault, and the half that reaches a real filter. + + ``describe()`` joins terms with ``", "``. A term ENDING in a backslash escapes + that comma on the way back in, so two terms silently become one - and the filter + a user reads is not the filter that runs. + + Measured before the fix, with the shape this tool meets most often - a Windows + path typed with its trailing separator, then a second name:: + + C:\\ ,chrome.exe -> ['C:\\', 'chrome.exe'] two terms + describe -> 'C:\\, chrome.exe' + parsed again -> ['C:, chrome.exe'] ONE term + + Deliberately NOT left to the property test that found it. That test explores at + random and passed on CI the same afternoon it failed here, so on its own it + reports this fault as weather. These three assertions report it as a bug. + + Only an ODD run of trailing backslashes is dangerous - an even run escapes + itself and already round-tripped - so the even case is pinned too. Widening the + fix to strip the whole tail would redden that one. + """ + B = chr(92) + + check("a trailing escape is dropped, and the two terms stay two", + split_terms("C:" + B + " ,chrome.exe") == ["C:", "chrome.exe"], + f"({split_terms('C:' + B + ' ,chrome.exe')})") + check("an odd run loses exactly one backslash, not the run", + split_terms("a" + B * 3 + " ,b") == ["a" + B * 2, "b"], + f"({split_terms('a' + B * 3 + ' ,b')})") + check("an even run is untouched - it already round-tripped", + split_terms("a" + B * 2 + " ,b") == ["a" + B * 2, "b"], + f"({split_terms('a' + B * 2 + ' ,b')})") + + described = ", ".join(split_terms("C:" + B + " ,chrome.exe")) + check("and the described text parses back to the same two terms", + split_terms(described) == ["C:", "chrome.exe"], f"({described!r})") + + def test_describe_round_trips_an_escaped_comma(): """``describe()`` is the CANONICAL text - it must parse back to the same matcher. diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index cee8a33..2537a94 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -1029,6 +1029,25 @@ { # The permission that outlives the job it was written for: back at the top # of release.yml, where every job added later inherits it. + # The filter a user reads stops being the filter that runs: a term ending + # in a backslash escapes the separator `describe()` writes after it, and + # two terms silently become one. + "label": "matchers: a term keeps a trailing escape that eats the separator", + "file": "beantester/matchers.py", + "old": " term = _without_a_dangling_escape(part.strip())", + "new": " term = part.strip()", + "test": "test_a_term_may_not_end_in_an_escape_that_swallows_the_separator", + }, + { + # The OVER-correction, which is the likelier future mistake: stripping the + # whole tail looks tidier and breaks `a\\`, which already round-tripped. + "label": "matchers: the escape fix widens into stripping every trailing backslash", + "file": "beantester/matchers.py", + "old": " return term[:-1] if trailing % 2 else term", + "new": r' return term.rstrip("\\")', + "test": "test_a_term_may_not_end_in_an_escape_that_swallows_the_separator", + }, + { # Evidence that exists and cannot be found is evidence nobody has. The # attestation stayed in GitHub's store, where the archive's own readers - # a person offline, a mirror, a scanner reading assets by extension - never From 028723440a159b3e1c78fd6f812a45204c5178b2 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 16:48:26 +0200 Subject: [PATCH 6/6] ci: record why the review skips its own pull request Measured on this branch: the app refuses to hand out a token unless the workflow file is byte-identical to the copy on the default branch, so the action exits with "Exiting due to workflow validation skip" - a success, in 18 seconds, with no model call and nothing spent. That is the right rule. Without it a pull request could edit this file and walk off with the token. But it has two consequences worth a comment rather than a rediscovery: the workflow cannot be tested before it is merged, and every later change to it skips its own review. Worth knowing for a different reason too: a green "Claude review" check is not evidence that a review happened. The run duration is. Co-Authored-By: Claude Opus 5 --- .github/workflows/claude-review.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index f740ec8..adaaa2f 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -7,6 +7,16 @@ # it. None of those redden a linter. # # 馃敶 It cannot fail a pull request. 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. name: Claude review on: