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..adaaa2f --- /dev/null +++ b/.github/workflows/claude-review.yml @@ -0,0 +1,111 @@ +# 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. +# +# 馃敶 **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: + # `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/.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..b89151a 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 @@ -39,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/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 diff --git a/README.md b/README.md index 2b2844c..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 @@ -1485,6 +1486,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..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 @@ -1347,6 +1348,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/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 2a965ca..2537a94 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -1029,6 +1029,45 @@ { # 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 + # 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)") 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.