From f9c35c5c4415d8f922acc3bbd50b994f49c9b7d2 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 18 Aug 2026 23:36:17 +0200 Subject: [PATCH 01/22] fix(ci): keep a branch name out of the shell it is used in `ci.yml` pasted `${{ github.base_ref }}` and `${{ ... head.sha }}` into a `run:` script. A `${{ }}` is expanded before any shell exists, so the value is not an argument there - it is source code, and a branch name may carry shell metacharacters. Both travel through `env:` now and are quoted where they are used, which is what the step four lines below - "check the pull-request description" - has done since it was written, comment and all. Narrow rather than harmless, and worth naming which: `base_ref` has to be a branch that already exists in this repository, `head.sha` arrives as hex, and the job holds `contents: read` and no secrets. The exposure is "whoever may create a branch here may run code on the runner". It is fixed because it costs three lines. New guard: tests/test_repo_conventions.py collects the lines belonging to a `run:` block and refuses a `${{` among them. It also asserts the scan found script lines at all - a hand-written parser that stops matching passes every check it makes. A mutation puts the original expression back and is caught. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 12 +++++-- tests/test_mutation_registry.py | 11 ++++++ tests/test_repo_conventions.py | 59 +++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ec8d691..869d820 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,9 +60,17 @@ jobs: with: python-version: "3.14" - name: check the commit messages + # Through the environment, for the same reason as the step below: a + # `${{ }}` is pasted into the script BEFORE any shell sees it, so a + # branch name is not data there - it is source code. `base_ref` has to + # name a branch that already exists here, which makes this narrow rather + # than harmless, and the fix is three lines. Quote the variables: an + # unquoted expansion is the same fault one layer down. + env: + BASE_REF: ${{ github.base_ref }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | - python tools/check_public_text.py \ - --commits origin/${{ github.base_ref }}..${{ github.event.pull_request.head.sha }} + python tools/check_public_text.py --commits "origin/$BASE_REF..$HEAD_SHA" - name: check the pull-request description # Passed through the environment rather than interpolated into the shell: # a description is untrusted text and must never become part of a command. diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 26bc3fb..8bb3e9c 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -1021,6 +1021,17 @@ "new": ' total=str(round(t[\"total\"] / 1024.0, 1))))', "test": "test_connection_columns_tag_and_footer", }, + { + # The exact line Semgrep found, put back: a `${{ }}` expanded into a + # script is source code, not an argument. The guard has to see it + # wherever in the block it sits, so this mutates only one of the two + # variables and leaves the other in its safe form. + "label": "ci: a workflow interpolates a GitHub expression into a script", + "file": ".github/workflows/ci.yml", + "old": 'python tools/check_public_text.py --commits \"origin/$BASE_REF..$HEAD_SHA\"', + "new": 'python tools/check_public_text.py --commits origin/${{ github.base_ref }}..$HEAD_SHA', + "test": "test_no_workflow_puts_a_github_expression_inside_a_shell_script", + }, ] # The runner's own check: a patch that cannot compile must be reported as BROKEN, not diff --git a/tests/test_repo_conventions.py b/tests/test_repo_conventions.py index 46bfedb..1113669 100644 --- a/tests/test_repo_conventions.py +++ b/tests/test_repo_conventions.py @@ -452,3 +452,62 @@ def test_every_script_the_workflows_run_is_actually_in_the_repository(): check(f"{script} is tracked by git (a fresh clone must have it)", tracked.returncode == 0, "(it is ignored or untracked - internal_tools/ cannot hold a CI dependency)") + + +def _script_lines(text): + """``(line number, text)`` for every line inside a ``run:`` block. + + Hand-rolled rather than PyYAML, because the suite carries no YAML dependency + and the question is lexical anyway: is this text part of a script a runner + will execute. A ``run:`` opens the block, and the block ends at the first + non-empty line indented no further than the key itself. + """ + import re + out, block = [], None + for number, line in enumerate(text.split("\n"), 1): + stripped = line.strip() + indent = len(line) - len(line.lstrip()) + if block is not None: + if stripped and indent <= block: + block = None + else: + out.append((number, line)) + continue + match = re.match(r"-?\s*run:(.*)$", stripped) + if match: + out.append((number, match.group(1))) + block = indent + return out + + +def test_no_workflow_puts_a_github_expression_inside_a_shell_script(): + """``${{ }}`` in a ``run:`` block is TEXT SUBSTITUTION, not a variable. + + GitHub expands the expression into the script before any shell exists, so a + value carrying a shell metacharacter stops being an argument and becomes a + command. The safe form is an intermediate ``env:`` entry, quoted where it is + used - which is what GitHub's own hardening guide says, and what the + ``check the pull-request description`` step in ``ci.yml`` has always done. + + The repository had exactly one exception, four lines above that very step: + ``--commits origin/${{ github.base_ref }}..${{ ... .head.sha }}``. Narrow + rather than harmless (``base_ref`` must name a branch that already exists + here, and the job holds no secrets), and three lines to close - which is + exactly the kind of thing that survives on a memory and dies on a guard. + """ + workflows = sorted(glob.glob(os.path.join(ROOT, ".github", "workflows", "*.yml"))) + check("there are workflows to read", bool(workflows), f"({workflows})") + offenders = [] + scripts = 0 + for path in workflows: + with open(path, encoding="utf-8") as handle: + lines = _script_lines(handle.read()) + scripts += len(lines) + for number, line in lines: + if "${{" in line: + offenders.append(f"{os.path.basename(path)}:{number} {line.strip()[:60]}") + # A parser that stopped finding script lines would pass this silently, which + # is the failure mode of every hand-written scanner. + check("the scan actually read some script lines", scripts > 20, f"({scripts})") + check("no workflow interpolates a GitHub expression into a script", + not offenders, f"({offenders})") From eddaa6fd2f500ffd7f23c6f41614177d0e8bff5d Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 18 Aug 2026 23:44:05 +0200 Subject: [PATCH 02/22] ci: pin every action to a commit, and let a new release cool down Seventeen references across four workflows named a floating tag, so each one resolved to whatever commit its owner pointed it at that morning - which is exactly what the tj-actions and trivy-action compromises did. Seven distinct actions now read owner/action@<40 hex> with a comment naming the version, the format Dependabot writes and rewrites when it bumps a pin. This was an inconsistency rather than an unmade decision: the two references somebody thought about, anchore/sbom-action and actions/attest, were already pinned by SHA, while everything first-party sat on a major tag. One of those was not even a tag - actions/dependency-review-action@v5 resolves to a BRANCH named v5, checked against the API rather than assumed. Immutable releases do not retire this, which is worth saying because they sound like they should: they lock the release tag (v7.0.1), while the floating major is designed to move and GitHub tells action authors to move it. The hardening guide still calls a full-length SHA the only immutable reference. dependabot.yml also gains cooldown: default-days: 7 for both ecosystems, so a version published in the last week is not proposed here. Read in the options reference rather than assumed, because the obvious objection is that this delays security work: it does not. Cooldown applies to version updates only, and security updates are a separate mechanism. github-actions supports default-days alone; the semver-*-days variants are pip's. New guard: every `uses:` must carry a 40-character SHA and a comment naming its version. Two mutations - a floating tag put back, and the comment stripped - are caught by it. Co-Authored-By: Claude Opus 5 --- .github/dependabot.yml | 15 +++++++++ .github/workflows/ci.yml | 16 ++++----- .github/workflows/dependency-review.yml | 4 +-- .github/workflows/pages.yml | 10 +++--- .github/workflows/release.yml | 4 +-- tests/test_mutation_registry.py | 19 +++++++++++ tests/test_repo_conventions.py | 44 +++++++++++++++++++++++++ 7 files changed, 95 insertions(+), 17 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index b161ccc..976b04f 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -5,11 +5,24 @@ version: 2 # security fixes). Grouped so one PR carries the whole bump, not four. # - pip: requirements*.txt (pydivert, psutil, pytest, hypothesis, ...). Review these # before merging - pydivert/psutil touch the packet path and the build. +# `cooldown` holds a newly published version back for a week before it is +# proposed here. It closes the window the tj-actions and trivy-action +# compromises used - a poisoned release pulled in within hours of publication - +# and it costs a week of latency on updates nobody is waiting for. +# 🔴 It does NOT delay security fixes: cooldown applies to version updates only, +# and Dependabot security updates are a separate mechanism (GitHub docs, +# "Dependabot options reference"). Checked there rather than assumed, because +# a week of silence on a CVE would be the opposite of the point. +# github-actions supports `default-days` alone; the `semver-*-days` variants +# exist for pip, and are left off until there is a reason to treat a major +# differently from a patch here. updates: - package-ecosystem: github-actions directory: "/" schedule: interval: weekly + cooldown: + default-days: 7 groups: github-actions: patterns: @@ -19,4 +32,6 @@ updates: directory: "/" schedule: interval: weekly + cooldown: + default-days: 7 open-pull-requests-limit: 5 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 869d820..cf9c08e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,11 +52,11 @@ jobs: if: github.event_name == 'pull_request' runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: # the whole range being merged, not just its tip fetch-depth: 0 - - uses: actions/setup-python@v7 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: "3.14" - name: check the commit messages @@ -96,7 +96,7 @@ jobs: os: [ubuntu-latest, windows-latest] python-version: ["3.14"] steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Verify required release files are in the checkout # These are git-ignored by pattern and re-included by exception, so a # missing force-add or a broken .gitignore makes them vanish on a fresh @@ -117,7 +117,7 @@ jobs: exit 1 fi echo "All required release files are present." - - uses: actions/setup-python@v7 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: ${{ matrix.python-version }} cache: pip @@ -150,7 +150,7 @@ jobs: - name: Coverage report if: always() - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: coverage-${{ matrix.os }}-py${{ matrix.python-version }} path: coverage.xml @@ -223,11 +223,11 @@ jobs: timeout-minutes: 30 needs: tests steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 # Must match the interpreter release.yml builds with: PyInstaller freezes # THIS Python into the bundle, so a CI build on a different one is not # smoke-testing the artefact users get. - - uses: actions/setup-python@v7 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: "3.14" cache: pip @@ -286,7 +286,7 @@ jobs: shell: bash run: python tools/sbom.py --audit-bundle bundle-scan.json - - uses: actions/upload-artifact@v7 + - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: BeanNetworkTester-windows path: dist/BeanNetworkTester diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index e6dd884..774b76a 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -18,8 +18,8 @@ jobs: name: review new dependencies runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 - - uses: actions/dependency-review-action@v5 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/dependency-review-action@a1d282b36b6f3519aa1f3fc636f609c47dddb294 # v5.0.0 with: # A tool that loads a kernel driver has no business gaining a # vulnerable dependency quietly. Anything at or above "moderate" diff --git a/.github/workflows/pages.yml b/.github/workflows/pages.yml index 01db682..d336fb6 100644 --- a/.github/workflows/pages.yml +++ b/.github/workflows/pages.yml @@ -60,8 +60,8 @@ jobs: contents: read pages: read steps: - - uses: actions/checkout@v7 - - uses: actions/setup-python@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: "3.14" cache: pip @@ -71,7 +71,7 @@ jobs: # the run instead of publishing a site whose every canonical points elsewhere. - name: Read the Pages configuration id: pages - uses: actions/configure-pages@v6 + uses: actions/configure-pages@45bfe0192ca1faeb007ade9deae92b16b8254a0d # v6.0.0 - name: The address it publishes to must be the address the pages claim env: @@ -113,7 +113,7 @@ jobs: - name: Build run: python tools/build_site.py --out _site - - uses: actions/upload-pages-artifact@v5 + - uses: actions/upload-pages-artifact@fc324d3547104276b827a68afc52ff2a11cc49c9 # v5.0.0 with: path: _site @@ -131,7 +131,7 @@ jobs: name: github-pages url: ${{ steps.deployment.outputs.page_url }} steps: - - uses: actions/deploy-pages@v5 + - uses: actions/deploy-pages@cd2ce8fcbc39b97be8ca5fce6e763baed58fa128 # v5.0.0 id: deployment # A deployment that reports success proves the upload happened, not that the diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index d47c70c..14e46fa 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -33,12 +33,12 @@ jobs: runs-on: windows-latest timeout-minutes: 30 steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 # The interpreter PyInstaller freezes into the shipped bundle. Keep it in # step with the build job in ci.yml, or CI smoke-tests one artefact and # users download another. - - uses: actions/setup-python@v7 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: "3.14" cache: pip diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 8bb3e9c..98f7efe 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -1032,6 +1032,25 @@ "new": 'python tools/check_public_text.py --commits origin/${{ github.base_ref }}..$HEAD_SHA', "test": "test_no_workflow_puts_a_github_expression_inside_a_shell_script", }, + { + # One action slides back onto a floating tag - the state the whole + # repository was in, and the one a hand-written `uses:` falls into. + "label": "ci: an action goes back to a movable tag", + "file": ".github/workflows/dependency-review.yml", + "old": "actions/dependency-review-action@a1d282b36b6f3519aa1f3fc636f609c47dddb294 # v5.0.0", + "new": "actions/dependency-review-action@v5", + "test": "test_every_action_a_workflow_uses_is_pinned_to_a_commit", + }, + { + # The other half of the same rule: a digest with nothing saying which + # version it is. Legal YAML, unreadable diff, and Dependabot has + # nothing to rewrite when it bumps the pin. + "label": "ci: a pinned action stops saying which version it is", + "file": ".github/workflows/dependency-review.yml", + "old": "actions/dependency-review-action@a1d282b36b6f3519aa1f3fc636f609c47dddb294 # v5.0.0", + "new": "actions/dependency-review-action@a1d282b36b6f3519aa1f3fc636f609c47dddb294", + "test": "test_every_action_a_workflow_uses_is_pinned_to_a_commit", + }, ] # The runner's own check: a patch that cannot compile must be reported as BROKEN, not diff --git a/tests/test_repo_conventions.py b/tests/test_repo_conventions.py index 1113669..f2104d1 100644 --- a/tests/test_repo_conventions.py +++ b/tests/test_repo_conventions.py @@ -511,3 +511,47 @@ def test_no_workflow_puts_a_github_expression_inside_a_shell_script(): check("the scan actually read some script lines", scripts > 20, f"({scripts})") check("no workflow interpolates a GitHub expression into a script", not offenders, f"({offenders})") + + +def test_every_action_a_workflow_uses_is_pinned_to_a_commit(): + """A tag is a movable reference, and that movement IS the attack. + + ``actions/checkout@v7`` names whichever commit that tag points at today, and + the owner of the tag may repoint it at any time - which is what the + tj-actions and trivy-action compromises did. GitHub's hardening guide is + blunt about the remedy: a full-length commit SHA is the only way to use an + action as an immutable release. + + Immutable releases (generally available since October 2025) do not retire + this rule, and it is worth writing down why, because they sound like they + should. They lock the release tag - ``v7.0.1`` - while the floating major + ``v7`` is DESIGNED to move and GitHub's own documentation tells action + authors to move it. One reference in this repository was not even a tag: + ``actions/dependency-review-action@v5`` resolved to a BRANCH of that name. + + The comment after the SHA is part of the rule rather than decoration. It is + what tells a reader which version the digest is, and it is exactly the format + Dependabot writes and rewrites when it bumps a pin - so pinning costs no + upkeep, it only moves the decision to update from the action's owner to us. + """ + import re + workflows = sorted(glob.glob(os.path.join(ROOT, ".github", "workflows", "*.yml"))) + check("there are workflows to read", bool(workflows), f"({workflows})") + seen, unpinned, uncommented = 0, [], [] + for path in workflows: + with open(path, encoding="utf-8") as handle: + for number, line in enumerate(handle, 1): + match = re.search(r"uses:\s*([\w.-]+/[\w.-]+)@(\S+)(.*)$", line) + if not match: + continue + seen += 1 + action, ref, rest = match.group(1), match.group(2), match.group(3) + where = f"{os.path.basename(path)}:{number} {action}@{ref[:12]}" + if not re.fullmatch(r"[0-9a-f]{40}", ref): + unpinned.append(where) + elif not re.search(r"#\s*v?\d", rest): + uncommented.append(where) + # A regex that stopped matching would report a clean sweep of nothing. + 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})") From 74cbd245cbb76c4499ef970a84043f8bfc8be5a7 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 18 Aug 2026 23:49:17 +0200 Subject: [PATCH 03/22] chore: answer the last three scanner findings, two by fixing one thing The crash fingerprint moves from sha1 to sha256. Substantively the finding is wrong - this is a dedup key built from an exception type and four frames, cut to twelve characters, where a collision costs one merged crash record and nothing else. It changes anyway because "sha1" in a source file is a finding in every scanner that looks, and answering the same false alarm every quarter costs more than the one word does. Nothing persists these ids between runs, so no stored record is invalidated. tools/downloads.py now checks that --repo is an owner/name before it becomes part of a URL. The rule's usual worry, a smuggled file:// scheme, cannot happen here because the scheme is a literal - but the value does land in the path, so "../../gists" would ask a different endpoint and print the answer as though those were releases. Checked in fetch_releases, so every caller is covered rather than only the CLI. Left alone, with the reason written down rather than remembered: legal._module_version passes a name to importlib.import_module, and that name comes from a hardcoded table in the same module. There is no path from input to that argument, so the rule is seeing a shape it cannot resolve. Guards: the downloads argument gets six shapes that must be refused and three that must pass, and the crash-id test now pins the SHAPE of the id - twelve hex characters - which is what a person reads off a record. Both have a mutation and both are caught. Co-Authored-By: Claude Opus 5 --- beantester/crashlog.py | 9 +++++++- tests/test_crashlog.py | 7 +++++++ tests/test_mutation_registry.py | 20 ++++++++++++++++++ tests/test_version_and_release.py | 35 +++++++++++++++++++++++++++++++ tools/downloads.py | 15 ++++++++++++- 5 files changed, 84 insertions(+), 2 deletions(-) diff --git a/beantester/crashlog.py b/beantester/crashlog.py index da628dd..02c4c7f 100644 --- a/beantester/crashlog.py +++ b/beantester/crashlog.py @@ -166,7 +166,14 @@ def _fingerprint(exc_type, frames): parts = [getattr(exc_type, "__name__", str(exc_type))] for frame in frames[-4:]: parts.append(f"{os.path.basename(frame.filename)}:{frame.name}:{frame.lineno}") - return hashlib.sha1("|".join(parts).encode("utf-8", "replace")).hexdigest()[:12] + # sha256, and NOT because this is a signature - it is a dedup key, where a + # collision would cost one merged crash record and nothing else. The reason + # is cheaper than that: "sha1" in a source file is a finding in every scanner + # that looks (Semgrep raised it here), and answering the same false alarm + # every quarter costs more than the one-word change ever will. Nothing + # persists these ids between runs, so no stored record is invalidated - a + # crash text file written by an older version simply carries the old id. + return hashlib.sha256("|".join(parts).encode("utf-8", "replace")).hexdigest()[:12] def _subsystem_of(frames): diff --git a/tests/test_crashlog.py b/tests/test_crashlog.py index 8f05484..0922b00 100644 --- a/tests/test_crashlog.py +++ b/tests/test_crashlog.py @@ -98,6 +98,13 @@ def other(): other() prints = {e["fingerprint"] for e in _entries(isolated)} assert len(prints) == 2, "two different bugs must not be merged into one" + # The id is printed in the record a user may paste into a report, so its + # SHAPE is the part worth pinning: twelve hex characters, whatever hash is + # behind it. (It moved from sha1 to sha256 in August 2026 - not for strength, + # this is a dedup key and not a signature, but because "sha1" in a source + # file is a finding in every scanner that looks.) + for one in prints: + assert len(one) == 12 and all(c in "0123456789abcdef" for c in one), one # -- 3) it never raises, whatever it is handed ------------------------------- # diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 98f7efe..f10f623 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -1051,6 +1051,26 @@ "new": "actions/dependency-review-action@a1d282b36b6f3519aa1f3fc636f609c47dddb294", "test": "test_every_action_a_workflow_uses_is_pinned_to_a_commit", }, + { + # The check that keeps `--repo` inside its own meaning. Without it the + # value still lands in the PATH of an api.github.com URL, so + # `../../gists` asks a different endpoint and prints the answer as if + # those were releases. + "label": "tools: the downloads repository argument stops being checked", + "file": "tools/downloads.py", + "old": " if not REPO.match(str(repo or \"\")):", + "new": " if False and not REPO.match(str(repo or \"\")):", + "test": "test_the_downloads_tool_refuses_anything_that_is_not_owner_slash_name", + }, + { + # The crash id is printed in a record a user may paste into a report, + # so its shape is the contract - not the hash behind it. + "label": "crashlog: the crash id stops being twelve characters", + "file": "beantester/crashlog.py", + "old": ".hexdigest()[:12]", + "new": ".hexdigest()", + "test": "test_different_faults_get_different_fingerprints", + }, ] # The runner's own check: a patch that cannot compile must be reported as BROKEN, not diff --git a/tests/test_version_and_release.py b/tests/test_version_and_release.py index e138d5e..5eae713 100644 --- a/tests/test_version_and_release.py +++ b/tests/test_version_and_release.py @@ -445,3 +445,38 @@ def test_both_workflows_install_the_same_pinned_builder(): and pin_file not in ln] check(f"{path}: never installs pyinstaller unpinned", not loose, f"({loose} - put the version in {pin_file}, not on the command line)") + + +def test_the_downloads_tool_refuses_anything_that_is_not_owner_slash_name(): + """``--repo`` lands in the PATH of an api.github.com URL. + + Semgrep flagged the `urlopen` call (`dynamic-urllib-use-detected`) for the + reason it usually flags one: a dynamic value could carry a `file://` scheme. + It cannot here - the scheme is a literal - but the value does become part of + the path, so `--repo ../../gists` would quietly ask a different endpoint the + question and print whatever came back as if those were releases. Nobody is + attacked by that (it is a maintainer's own tool), and it is still three lines + to make the argument mean what its name says. + """ + import sys + sys.path.insert(0, os.path.join(ROOT, "tools")) + import downloads + + for good in ("donislawdev/BeanNetworkTester", "a/b", "Some.Owner/repo-name_1"): + check(f"{good} is accepted as a repository", bool(downloads.REPO.match(good))) + + for bad in ("../../gists", "owner", "owner/name/extra", "owner/name?x=1", + "https://example.test/o/n", "owner name", ""): + rejected = True + try: + downloads.fetch_releases(bad) + except ValueError: + pass + except Exception as exc: # noqa: BLE001 - any other error means it TRIED + rejected = False + reason = exc + else: + rejected = False + reason = "no error at all" + check(f"{bad!r} is refused before it becomes a URL", rejected, + "" if rejected else f"({reason})") diff --git a/tools/downloads.py b/tools/downloads.py index 383cfe2..022f6e0 100644 --- a/tools/downloads.py +++ b/tools/downloads.py @@ -12,14 +12,27 @@ """ import argparse import json +import re import sys import urllib.request DEFAULT_REPO = "donislawdev/BeanNetworkTester" +REPO = re.compile(r"^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$") + + def fetch_releases(repo): - """Every published release for ``owner/name`` (newest first), via the public API.""" + """Every published release for ``owner/name`` (newest first), via the public API. + + ``repo`` is checked before it becomes part of a URL. Not because a scheme + could be smuggled in - the scheme here is a literal - but because the value + lands in the PATH, so `--repo ../../gists` would quietly ask a different + endpoint the question and print whatever came back as if it were releases. + An owner and a name, nothing else. + """ + if not REPO.match(str(repo or "")): + raise ValueError("expected owner/name, got %r" % (repo,)) url = "https://api.github.com/repos/%s/releases?per_page=100" % repo req = urllib.request.Request(url, headers={ "Accept": "application/vnd.github+json", From 3c9a94e846f2465b17aac7d6bfa1503d204babb9 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 01:12:44 +0200 Subject: [PATCH 04/22] chore: adopt ruff, and close the 56 findings it opens with Four rule families, decided rather than defaulted: F (pyflakes), B (bugbear), S (bandit) and ASYNC. F and B are meant to block a pull request; S is a report, and that split is what the two measure rather than softness. Measured here: S raises 306 findings, 287 of them in tests, and they are things this project does on purpose - a test asserts (180), a packet mangler randomises (89), a CI script runs git (18). Per-file ignores name each with its reason, so a NEW subprocess call or eval in the package still lands as a finding. ASYNC was zero from the start and is selected to keep it that way. The findings were not all dead imports: * tests/fake_tk.py defined bind_class twice - an alias, and a real method 145 lines below whose docstring explains why the recording version must exist. The def won; the alias was dead. * tests/test_site.py used a backslash inside an f-string expression, legal only from Python 3.12, while requires-python says 3.10. CI runs 3.14, so nothing caught it; ruff reads the floor from pyproject. * Eight zip() calls now say which they are: strict=True where the two sides are the same length by contract, strict=False where they deliberately differ (pairwise over a list and its own tail, an AST tuple assignment). * Thirteen raises inside except keep their cause. Each replaces a parse error with a translated message; nothing user-visible changes, and a crash record that names the original failure is worth more than one that does not. * Ten findings were FALSE and are annotated rather than "fixed": closures in loops that return in the same iteration, and bare expressions whose value is discarded on purpose - one re-arms the target resolver, the other forces the tkinter-backed import. Two things the tool caught the moment it ran, which is the argument for it: a rename landed on the wrong loop and came back as F821, and strict=True exposed a fixture that had been feeding a nine-value row to a seventeen-column table since the columns were added. Co-Authored-By: Claude Opus 5 --- bean_network_tester.py | 2 +- beantester/cli.py | 4 +-- beantester/crashlog.py | 1 - beantester/gui/pages/conns.py | 5 ++- beantester/matchers.py | 18 ++++++----- beantester/scenario.py | 10 +++--- beantester/settings.py | 9 +++--- beantester/validators.py | 8 ++--- build.py | 2 +- pyproject.toml | 40 ++++++++++++++++++++++++ tests/fake_tk.py | 1 - tests/test_concurrency_chaos.py | 2 +- tests/test_core.py | 5 +-- tests/test_crashlog.py | 1 - tests/test_gui_file_actions.py | 2 -- tests/test_gui_state.py | 15 ++++++--- tests/test_i18n.py | 2 +- tests/test_license_surface.py | 3 +- tests/test_matchers_windivert.py | 2 +- tests/test_packet_mutation_properties.py | 2 +- tests/test_passthrough.py | 1 - tests/test_prefs.py | 2 +- tests/test_readme_guards.py | 2 +- tests/test_repo_conventions.py | 1 - tests/test_site.py | 7 +++-- tests/test_target_resolver.py | 6 ++-- tests/test_validators_settings.py | 2 +- tests/test_version_and_release.py | 5 +-- tests/test_view_scope.py | 3 -- tools/build_site.py | 18 ++++++----- tools/ci_blocking_smoke.py | 2 +- 31 files changed, 116 insertions(+), 67 deletions(-) diff --git a/bean_network_tester.py b/bean_network_tester.py index b8b285c..e3249b3 100644 --- a/bean_network_tester.py +++ b/bean_network_tester.py @@ -27,7 +27,7 @@ def __getattr__(name): if name == "_HAS_TK": try: from beantester import gui - gui.App # force the tkinter-backed import to run + _ = gui.App # force the tkinter-backed import to run return True except Exception: return False diff --git a/beantester/cli.py b/beantester/cli.py index dc7939d..277a5b1 100644 --- a/beantester/cli.py +++ b/beantester/cli.py @@ -19,7 +19,7 @@ from . import appinfo, clilog, driver, exitcodes, winenv from .appinfo import APP_NAME, command_name, program_name, __version__ -from .clilog import LOG_PREFIX, CliLog +from .clilog import CliLog from . import crashlog from .engine import BeanEngine from .fields import BOOL, FIELD_DEFS @@ -32,7 +32,7 @@ from .scenario import load_scenario_file from .settings import (DEFAULT_SETTINGS, apply_settings, build_matchers, load_config_file, parse_schedule, save_config_file, - range_errors, validate_ranges, warn_if_unbounded) + range_errors, warn_if_unbounded) from .synthetic import SyntheticDivert from .utils import bytes_to_mb diff --git a/beantester/crashlog.py b/beantester/crashlog.py index 02c4c7f..12b5d02 100644 --- a/beantester/crashlog.py +++ b/beantester/crashlog.py @@ -59,7 +59,6 @@ import platform import sys import threading -import time import traceback from contextlib import contextmanager from datetime import datetime, timezone diff --git a/beantester/gui/pages/conns.py b/beantester/gui/pages/conns.py index aad9ad2..74a7258 100644 --- a/beantester/gui/pages/conns.py +++ b/beantester/gui/pages/conns.py @@ -364,7 +364,10 @@ def _selected(self): if not values: return None keys = list(COLUMNS) - return dict(zip(keys, values)) + # strict: the row and the column registry are the same length by + # contract (tests/test_conns_columns.py pins it). Zipping leniently + # would hand the menu a row with its last columns missing instead. + return dict(zip(keys, values, strict=True)) def _copy_row(self): # every selected row, tab separated - the same thing Ctrl+C puts on the diff --git a/beantester/matchers.py b/beantester/matchers.py index 08bd615..57c4140 100644 --- a/beantester/matchers.py +++ b/beantester/matchers.py @@ -441,8 +441,8 @@ def _compile_regex(pattern, field, term): warnings.simplefilter("ignore", FutureWarning) warnings.simplefilter("ignore", DeprecationWarning) return re.compile(pattern, re.IGNORECASE) - except re.error: - raise _err("errors.bad_filter_regex", field, term) + except re.error as exc: + raise _err("errors.bad_filter_regex", field, term) from exc def _is_glob(body): @@ -478,14 +478,16 @@ def _parse_ip_term(body, term, field): raise _err("errors.bad_filter_ip", field, term) version, num = operand.version, operand.num base = _compare_predicate(op, num) - return ((lambda c: c is not None and c.version == version and base(c.num)), + # noqa: the loop returns in this iteration, so `version` and `base` + # cannot change under the lambda - B023 needs a second pass to bite. + return ((lambda c: c is not None and c.version == version and base(c.num)), # noqa: B023 ("ip_cmp", version, op, num)) # CIDR if "/" in body: try: net = ipaddress.ip_network(body, strict=False) - except (ValueError, TypeError): - raise _err("errors.bad_filter_ip", field, term) + except (ValueError, TypeError) as exc: + raise _err("errors.bad_filter_ip", field, term) from exc lo = int(net.network_address) hi = int(net.broadcast_address) version = net.version @@ -537,7 +539,7 @@ def _parse_process_term(body, term, field): if not operand.isdigit(): raise _err("errors.bad_filter_compare_name", field, term) base = _compare_predicate(op, int(operand)) - return (lambda c: c.pid is not None and base(c.pid)), None + return (lambda c: c.pid is not None and base(c.pid)), None # noqa: B023 # numeric atoms (literal PID / PID range) reuse the int parser numeric = _parse_int_atom(body, term, field, None) if numeric is not None: @@ -567,8 +569,8 @@ def parse_matcher(text, kind, field="fields.filter", bounds=None): return text try: cls = _MATCHER_CLASSES[kind] - except KeyError: - raise ValueError(f"unknown matcher kind: {kind!r}") + except KeyError as exc: + raise ValueError(f"unknown matcher kind: {kind!r}") from exc terms = [] for raw_term in split_terms(text): diff --git a/beantester/scenario.py b/beantester/scenario.py index 045b5ee..ca3b4ae 100644 --- a/beantester/scenario.py +++ b/beantester/scenario.py @@ -40,8 +40,8 @@ def _validate_step(index, step): raise _err("errors.scenario_step_at", step=where) try: at = float(step["at"]) - except (TypeError, ValueError): - raise _err("errors.scenario_step_at", step=where) + except (TypeError, ValueError) as exc: + raise _err("errors.scenario_step_at", step=where) from exc if at < 0: raise _err("errors.scenario_step_at", step=where) @@ -81,8 +81,8 @@ def _validate_step(index, step): raise _err("errors.scenario_duration_without_action", step=where) try: duration = float(step["duration"]) - except (TypeError, ValueError): - raise _err("errors.scenario_step_duration", step=where) + except (TypeError, ValueError) as exc: + raise _err("errors.scenario_step_duration", step=where) from exc if duration < 0: raise _err("errors.scenario_step_duration", step=where) @@ -158,5 +158,5 @@ def load_scenario_file(path): try: data = json.load(f) except ValueError as e: - raise _err("errors.scenario_bad_json", error=e) + raise _err("errors.scenario_bad_json", error=e) from e return parse_scenario(data) diff --git a/beantester/settings.py b/beantester/settings.py index 1e08f5e..6ce7e56 100644 --- a/beantester/settings.py +++ b/beantester/settings.py @@ -53,8 +53,9 @@ def parse_schedule(text): raise ValueError(translate("errors.bad_schedule_step", None, part=part)) try: dur, dn, up = (float(bits[0]), float(bits[1]), float(bits[2])) - except ValueError: - raise ValueError(translate("errors.bad_schedule_step", None, part=part)) + except ValueError as exc: + raise ValueError(translate("errors.bad_schedule_step", None, + part=part)) from exc steps.append((dur, dn, up)) return steps @@ -565,14 +566,14 @@ def _coerce_setting(key, value): if isinstance(value, bool): raise ValueError return float(value) - except (TypeError, ValueError): + except (TypeError, ValueError) as exc: # Say what the setting DOES take, not just that this is not it. The # registry already knows - the form has been telling people "must be # between 0 and 100" for as long as it has existed, while the config # loader said only "invalid" for the very same value. raise ValueError(translate("errors.bad_config_value", None, field=key, value=repr(value), - expected=_expected_shape(key))) + expected=_expected_shape(key))) from exc return str(value) diff --git a/beantester/validators.py b/beantester/validators.py index 283d624..a151740 100644 --- a/beantester/validators.py +++ b/beantester/validators.py @@ -22,8 +22,8 @@ def parse_number(value, field_key=None, bounds=None, lang=None): text = str("" if value is None else value).strip().replace(",", ".") try: number = float(text) - except (TypeError, ValueError): - raise ValueError(translate("errors.field_number", lang, name=name)) + except (TypeError, ValueError) as exc: + raise ValueError(translate("errors.field_number", lang, name=name)) from exc if number != number or number in (float("inf"), float("-inf")): # NaN / inf raise ValueError(translate("errors.field_number", lang, name=name)) if bounds: @@ -43,5 +43,5 @@ def parse_seed(value, lang=None): return -1 try: return int(text) - except (TypeError, ValueError): - raise ValueError(translate("errors.seed_integer", lang)) + except (TypeError, ValueError) as exc: + raise ValueError(translate("errors.seed_integer", lang)) from exc diff --git a/build.py b/build.py index 49a3f7e..22ebdf9 100644 --- a/build.py +++ b/build.py @@ -102,7 +102,7 @@ def verify_package(): # The WinDivert driver must travel with the app, never in %TEMP% - and its # presence is also what makes the LGPL library replaceable in place. hits = [] - for dirpath, _dirs, files in os.walk(DIST): + for _dirpath, _dirs, files in os.walk(DIST): hits += [f for f in files if f.lower().startswith("windivert")] if not hits: print("build.py: WARNING: no WinDivert* files found in the bundle " diff --git a/pyproject.toml b/pyproject.toml index 5a8733e..ef56496 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -43,6 +43,46 @@ include = ["beantester*"] testpaths = ["tests"] addopts = "-q" +[tool.ruff] +# Ruff: four rule families and no more, decided 2026-08-19. +# +# F pyflakes - dead imports, dead variables, redefinitions +# B bugbear - the bug shapes a reader misses (late binding, zip truncation) +# S bandit - security patterns +# ASYNC flake8-async - blocking calls in async code (zero findings here: there is +# no async code, and this keeps it that way) +# +# F and B BLOCK a pull request. S is a report, and the difference is not +# softness - it is what the two measure. Measured 2026-08-19: S raises 306 +# findings here, 287 of them in tests, and they are things this project does on +# purpose - a test asserts (180), a packet mangler randomises (89), a CI script +# runs git (18). A gate that fires on all of them is a gate nobody reads, and it +# would bury the ten findings in the product that are worth a look. +target-version = "py310" # matches requires-python; ruff reads it anyway, said out loud + +[tool.ruff.lint] +select = ["F", "B", "S", "ASYNC"] + +[tool.ruff.lint.per-file-ignores] +# A test suite for a packet mangler asserts, randomises, spawns subprocesses and +# parses XML on purpose, and none of it ships. S on tests/ is 287 lines of noise. +"tests/**" = ["S"] +# The one module allowed to swallow an exception, by this repository's own rule +# (tests/test_repo_conventions.py): a crash reporter that raises while reporting +# a crash is worse than one that stays quiet. +"beantester/crashlog.py" = ["S110"] +# The randomness IS the product: loss, jitter and duplication are drawn here, and +# nothing about them is cryptographic. `seed` exists to make them repeatable. +"beantester/engine.py" = ["S311"] +"beantester/synthetic.py" = ["S311"] +# Both continue past a widget or an adapter that answered nothing, in a loop whose +# point is to survive exactly that. +"beantester/gui/widgets/sortable_tree.py" = ["S112"] +"beantester/winenv.py" = ["S112"] +# Maintainer and CI scripts: they run `git` and `python` by design, and the only +# URL opened is a literal api.github.com endpoint whose path is validated first. +"tools/**" = ["S603", "S607", "S310"] + [tool.coverage.run] source = ["beantester"] parallel = true # the GUI tests run in subprocesses; each writes its own data file diff --git a/tests/fake_tk.py b/tests/fake_tk.py index 6e1730e..2cc8fc1 100644 --- a/tests/fake_tk.py +++ b/tests/fake_tk.py @@ -154,7 +154,6 @@ def bind(self, sequence, func=None, add=None): self.bindings.setdefault(sequence, []).append(func) bind_all = bind - bind_class = bind def unbind(self, sequence, funcid=None): self.bindings.pop(sequence, None) diff --git a/tests/test_concurrency_chaos.py b/tests/test_concurrency_chaos.py index d3e0b35..f127b6c 100644 --- a/tests/test_concurrency_chaos.py +++ b/tests/test_concurrency_chaos.py @@ -569,7 +569,7 @@ def resolver(): # the rebuild, and the adoption path def reader(): # the packet path while not stop.is_set(): try: - 19999 in targeting + _ = 19999 in targeting # the lookup itself is the point except Exception as exc: # pragma: no cover - the bug problems.append(f"reader: {type(exc).__name__}: {exc}") return diff --git a/tests/test_core.py b/tests/test_core.py index b63e9e3..ce45da9 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -517,7 +517,8 @@ class NoPayload: # exactly one bit flipped (payload length preserved, hamming distance == 1) p = FakePacket(payload=b"\x00" * 16) BeanCore.corrupt_packet(p, random.Random(5)) - diff_bits = sum(bin(a ^ b).count("1") for a, b in zip(b"\x00" * 16, p.payload)) + diff_bits = sum(bin(a ^ b).count("1") + for a, b in zip(b"\x00" * 16, p.payload, strict=True)) check("corrupt: exactly 1 bit flipped", diff_bits == 1 and len(p.payload) == 16, f"(bits={diff_bits}, len={len(p.payload)})") @@ -628,7 +629,7 @@ def test_a_long_rst_cooldown_is_honoured_not_truncated_by_the_flow_table(): if d.emit_rst: resets.append(now) now += 0.05 - gaps = [round(b - a, 1) for a, b in zip(resets, resets[1:])] + gaps = [round(b - a, 1) for a, b in zip(resets, resets[1:], strict=False)] check("RST: the cooldown between resets is the one that was configured", gaps and all(abs(g - cooldown) < 0.5 for g in gaps), f"(cooldown={cooldown}, gaps={gaps})") diff --git a/tests/test_crashlog.py b/tests/test_crashlog.py index 0922b00..57c6123 100644 --- a/tests/test_crashlog.py +++ b/tests/test_crashlog.py @@ -158,7 +158,6 @@ def test_an_unhandled_main_thread_exception_is_recorded(isolated): crashlog.install(native=False) chained = [] try: - previous = sys.excepthook exc = _boom("main thread died") sys.excepthook(type(exc), exc, exc.__traceback__) diff --git a/tests/test_gui_file_actions.py b/tests/test_gui_file_actions.py index 968db2d..2fdfaff 100644 --- a/tests/test_gui_file_actions.py +++ b/tests/test_gui_file_actions.py @@ -12,8 +12,6 @@ dialog returning "" must do nothing at all, and that branch is how the user backs out of every one of these. """ -import json -import os from gui_harness import run_gui diff --git a/tests/test_gui_state.py b/tests/test_gui_state.py index 591ebe2..3f476d3 100644 --- a/tests/test_gui_state.py +++ b/tests/test_gui_state.py @@ -280,8 +280,16 @@ class Ev: # a real row. The table is virtualised, so identify_row() gives back a # VIEWPORT SLOT id (__v0, __v1, ...), which the table maps to the model key - # the widget ids are recycled and say nothing about which row was clicked. - page.table.sync([("r1", ("chrome.exe", "TCP", "1.2.3.4", "443", - "5000", "10", "1.0", "2.0", "0.1"))]) + # + # Built FROM the column registry rather than by hand: this fixture used to + # carry nine values for a seventeen-column table and nobody noticed, + # because the row-to-dict zip truncated in silence. + def row(proc): + values = ["-"] * len(page.table.columns) + values[0] = proc + return values + + page.table.sync([("r1", row("chrome.exe"))]) tree.row_at = page.table._slots[0] page._popup(Ev()) assert page.table.selected_keys() == ["r1"] @@ -289,8 +297,7 @@ class Ev: assert page.menu.entry_states[page.TARGET_INDEX]["state"] == "normal" # a row whose process could not be resolved cannot be targeted - page.table.sync([("r2", ("?", "TCP", "1.2.3.4", "443", - "5000", "10", "1.0", "2.0", "0.1"))]) + page.table.sync([("r2", row("?"))]) tree.row_at = page.table._slots[0] page._popup(Ev()) assert page.menu.entry_states[page.TARGET_INDEX]["state"] == "disabled" diff --git a/tests/test_i18n.py b/tests/test_i18n.py index a84efe3..0484b19 100644 --- a/tests/test_i18n.py +++ b/tests/test_i18n.py @@ -122,7 +122,7 @@ def test_the_language_files_stay_sorted(): for code in ("en", "pl"): with open(os.path.join(LANG_DIR, f"{code}.json"), encoding="utf-8") as f: keys = list(_json.load(f)) - out_of_order = [(a, b) for a, b in zip(keys, keys[1:]) if a > b] + out_of_order = [(a, b) for a, b in zip(keys, keys[1:], strict=False) if a > b] check(f"{code}.json keys are sorted", not out_of_order, f"(first offender: {out_of_order[:1]})") check(f"{code}.json opens with _meta", keys and keys[0] == "_meta", diff --git a/tests/test_license_surface.py b/tests/test_license_surface.py index ddeb68e..1a1e386 100644 --- a/tests/test_license_surface.py +++ b/tests/test_license_surface.py @@ -153,7 +153,7 @@ def test_the_registry_and_the_notices_describe_the_same_set(): report = legal.cli_report() check("report opens with the licence itself", "GNU GENERAL PUBLIC LICENSE" in report) - for name, version, _lic, url in legal.component_rows(): + for name, _version, _lic, url in legal.component_rows(): check(f"report names {name}", name in report) check(f"report names the source of {name}", url in report) check("report states there is no telemetry (convention 36)", @@ -244,7 +244,6 @@ def test_the_notices_name_the_windivert_files_that_are_really_shipped(): PyInstaller lays a collected package out exactly as it is installed, so the installed tree is the authority here and no PyInstaller import is needed. """ - import os result = _windivert_binaries() if result is None: return # pydivert is Windows-only; nothing shipped here diff --git a/tests/test_matchers_windivert.py b/tests/test_matchers_windivert.py index 83d42ee..c301fa0 100644 --- a/tests/test_matchers_windivert.py +++ b/tests/test_matchers_windivert.py @@ -132,7 +132,7 @@ def test_the_fragment_never_excludes_what_the_matcher_accepts(): Run in full outside the suite over 27 648 packet views on 16 expressions with zero violations (2026-07-28); the sweep here is trimmed to keep the suite fast. """ - pydivert = pytest.importorskip("pydivert") + pytest.importorskip("pydivert") import ctypes from pydivert import windivert_dll as w from pydivert.windivert_dll import WinDivertAddress diff --git a/tests/test_packet_mutation_properties.py b/tests/test_packet_mutation_properties.py index db9b2b5..2b6708d 100644 --- a/tests/test_packet_mutation_properties.py +++ b/tests/test_packet_mutation_properties.py @@ -39,7 +39,7 @@ def _bit_difference(a, b): """Total number of differing bits between two equal-length byte strings.""" - return sum(bin(x ^ y).count("1") for x, y in zip(a, b)) + return sum(bin(x ^ y).count("1") for x, y in zip(a, b, strict=True)) # --------------------------------------------------------------------------- # diff --git a/tests/test_passthrough.py b/tests/test_passthrough.py index 9b20971..268fc6a 100644 --- a/tests/test_passthrough.py +++ b/tests/test_passthrough.py @@ -25,7 +25,6 @@ import random import time -import pytest from hypothesis import given, settings from hypothesis import strategies as st diff --git a/tests/test_prefs.py b/tests/test_prefs.py index c405ec8..ad6c9fa 100644 --- a/tests/test_prefs.py +++ b/tests/test_prefs.py @@ -7,7 +7,7 @@ what the switch promises. """ from beantester.gui import prefs -from beantester.gui.prefs import PREFS, PREFS_BY_KEY, PREF_GROUPS, BOOL, NUMBER, coerce +from beantester.gui.prefs import PREFS, PREFS_BY_KEY, PREF_GROUPS, coerce from beantester.i18n import set_language, translate from fakes import check from gui_harness import run_gui diff --git a/tests/test_readme_guards.py b/tests/test_readme_guards.py index 3d7b068..bdb3fbb 100644 --- a/tests/test_readme_guards.py +++ b/tests/test_readme_guards.py @@ -135,7 +135,7 @@ def test_both_readmes_document_every_connections_column(): import json import os as _os from beantester.gui.pages.conns import COLUMNS - for readme, lang in zip(READMES, ("en", "pl")): + for readme, lang in zip(READMES, ("en", "pl"), strict=True): with open(_os.path.join(ROOT, "lang", f"{lang}.json"), encoding="utf-8") as f: names = json.load(f) text = _read(readme) diff --git a/tests/test_repo_conventions.py b/tests/test_repo_conventions.py index f2104d1..ce7184b 100644 --- a/tests/test_repo_conventions.py +++ b/tests/test_repo_conventions.py @@ -142,7 +142,6 @@ def test_no_silent_exception_swallowing_outside_the_crash_logger(): for node in ast.walk(tree): if not isinstance(node, ast.ExceptHandler): continue - body = [n for n in node.body if not isinstance(n, ast.Pass)] # `except ...: pass` with nothing else is the silent swallow only_pass = all(isinstance(n, ast.Pass) for n in node.body) if only_pass: diff --git a/tests/test_site.py b/tests/test_site.py index 7d4a6bd..0e5ac7f 100644 --- a/tests/test_site.py +++ b/tests/test_site.py @@ -220,8 +220,11 @@ def test_every_page_has_one_headline_and_no_image_without_a_description(tmp_path out, written = _build(tmp_path) for rel in [p for p in written if p.endswith(".html")]: page = _read(os.path.join(out, rel.replace("/", os.sep))) - check(f"{rel}: exactly one h1", len(re.findall(r"]", page)) == 1, - f"({len(re.findall(r']', page))})") + # One search, one number. The second copy of the pattern used to live + # inside an f-string EXPRESSION, where a backslash is legal only from + # Python 3.12 - and `requires-python` here says 3.10. + headings = re.findall(r"]", page) + check(f"{rel}: exactly one h1", len(headings) == 1, f"({len(headings)})") for tag in re.findall(r"]*>", page): check(f"{rel}: every image describes itself ({tag[:60]})", re.search(r'\balt="', tag)) diff --git a/tests/test_target_resolver.py b/tests/test_target_resolver.py index 4e4a542..172c862 100644 --- a/tests/test_target_resolver.py +++ b/tests/test_target_resolver.py @@ -288,7 +288,7 @@ def test_constant_misses_cannot_turn_into_a_continuous_scan(): def storm(): # unrelated traffic: a miss every time while not stop.is_set(): - 9999 in targeting + _ = 9999 in targeting # the lookup itself is the point noise = threading.Thread(target=storm, daemon=True) noise.start() @@ -363,7 +363,7 @@ def test_a_child_spawned_mid_session_starts_being_impaired(): # ...and a grandchild, two levels down table.info[300] = ("renderer.exe", 200) table.ports[7002] = 300 - 9999 in targeting # any packet re-arms the miss + _ = 9999 in targeting # any packet re-arms the miss check("a grandchild is targeted too", _wait(lambda: 7002 in targeting)) check("the whole tree is in scope", targeting.ports() == {5001, 7001, 7002}, @@ -383,7 +383,7 @@ def test_an_excluded_child_is_not_pulled_back_in_by_its_parent(): check("the resolver settled", _wait(lambda: resolver.rebuilds >= 1)) table.info[200] = ("myapp-helper.exe", 100) table.ports[7001] = 200 - 9999 in targeting + _ = 9999 in targeting # re-arms the miss check("a rebuild happened", _wait(lambda: resolver.rebuilds >= 2)) time.sleep(0.05) check("the excluded child stays out despite its matching parent", diff --git a/tests/test_validators_settings.py b/tests/test_validators_settings.py index 8e59062..6036745 100644 --- a/tests/test_validators_settings.py +++ b/tests/test_validators_settings.py @@ -126,7 +126,7 @@ def test_a_preset_always_yields_every_profile_field(): def test_every_preset_is_within_the_declared_bounds(): - for key, preset in PRESETS.items(): + for _key, preset in PRESETS.items(): validate_ranges(dict(DEFAULT_SETTINGS, **preset_to_settings(preset))) check("presets: all inside the field bounds", True) diff --git a/tests/test_version_and_release.py b/tests/test_version_and_release.py index 5eae713..7ce5570 100644 --- a/tests/test_version_and_release.py +++ b/tests/test_version_and_release.py @@ -125,7 +125,8 @@ def test_breaking_sections_come_first(): def close(version, sections): if version and "### BREAKING" in sections and sections[0] != "### BREAKING": - problems.append(f"{name} {version}: BREAKING is #{sections.index('### BREAKING') + 1}" + # noqa: one file, one iteration - the closure cannot outlive it + problems.append(f"{name} {version}: BREAKING is #{sections.index('### BREAKING') + 1}" # noqa: B023 f" of {len(sections)} (first is {sections[0]!r})") for line in lines: @@ -226,7 +227,7 @@ def test_no_user_facing_entry_grows_into_an_essay(): with open(path, encoding="utf-8") as f: lines = f.read().splitlines() - version, entry, offenders, entries = None, None, [], 0 + entry, offenders, entries = None, [], 0 def close(entry): if not entry: diff --git a/tests/test_view_scope.py b/tests/test_view_scope.py index 633050d..559aea2 100644 --- a/tests/test_view_scope.py +++ b/tests/test_view_scope.py @@ -702,9 +702,6 @@ def test_the_stats_csv_records_which_world_each_row_was_measured_in(): what `packets_seen` counted - so without this column two rows under one header could describe completely different traffic with no way to tell. """ - import csv as _csv - import os - import tempfile out = run_gui(""" import csv, os, tempfile diff --git a/tools/build_site.py b/tools/build_site.py index 5315053..9cf03b0 100644 --- a/tools/build_site.py +++ b/tools/build_site.py @@ -90,18 +90,18 @@ def _read_json(path): try: with open(path, encoding="utf-8") as handle: return json.load(handle) - except FileNotFoundError: - raise SiteError("missing file: %s" % path) + except FileNotFoundError as exc: + raise SiteError("missing file: %s" % path) from exc except json.JSONDecodeError as exc: - raise SiteError("%s is not valid JSON: %s" % (path, exc)) + raise SiteError("%s is not valid JSON: %s" % (path, exc)) from exc def _read_text(path): try: with open(path, encoding="utf-8") as handle: return handle.read() - except FileNotFoundError: - raise SiteError("missing file: %s" % path) + except FileNotFoundError as exc: + raise SiteError("missing file: %s" % path) from exc def load_registry(root): @@ -134,7 +134,7 @@ def load_registry(root): if reg["default_language"] not in codes: raise SiteError("site.json: default_language %r is not one of %s" % (reg["default_language"], codes)) - if dict(zip(codes, dirs))[reg["default_language"]] != "": + if dict(zip(codes, dirs, strict=True))[reg["default_language"]] != "": raise SiteError("site.json: the default language must live at the root (dir \"\")") return reg @@ -166,7 +166,9 @@ def palette(root, mapping): if isinstance(target, ast.Name) and isinstance(value, ast.Constant): found[target.id] = value.value elif isinstance(target, ast.Tuple) and isinstance(value, ast.Tuple): - for name, item in zip(target.elts, value.elts): + # `a, b = 1, 2, 3` parses, so the two sides may differ in length: + # read the pairs that exist rather than raising on the rest. + for name, item in zip(target.elts, value.elts, strict=False): if isinstance(name, ast.Name) and isinstance(item, ast.Constant): found[name.id] = item.value @@ -896,7 +898,7 @@ def social_links(registry, texts): def page_context(page, code, registry, texts, home, colours, root, pages): """Everything a page's template and body may refer to, for one language.""" entry = page["languages"][code] - lang = _language(registry, code) + _language(registry, code) # raises on a code site.json does not know repo = registry["repo_url"].rstrip("/") context = dict(texts[code]) _merge(context, load_app_strings(root, code), "program strings [%s]" % code) diff --git a/tools/ci_blocking_smoke.py b/tools/ci_blocking_smoke.py index 055887a..965f274 100644 --- a/tools/ci_blocking_smoke.py +++ b/tools/ci_blocking_smoke.py @@ -169,7 +169,7 @@ def main(): "took more than it named" % (label, other, ROUNDS)) print("phase named bystander") - for label, ip, port, named, other, bystander_counts in results: + for label, _ip, _port, named, other, bystander_counts in results: note = "" if bystander_counts else " (bystander N/A: a one-octet " \ "loopback prefix covers every 127.x)" print(" %-38s %2d/%-2d %2d/%-2d%s" From aaf941c03bfbf5cb0ed6d5f821d86c6e39083268 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 01:16:23 +0200 Subject: [PATCH 05/22] ci: run ruff on every pull request, blocking on F and B Two steps, because the families answer different questions. `--select F,B` is the gate: dead code and bug shapes fail the run. `--select S,ASYNC --exit-zero` is the report: it annotates the diff and never fails it. `--exit-zero` rather than continue-on-error on the step, because a step that "succeeded but failed" reads as a broken job to whoever looks at it later. `--select` on the command line replaces the families named in pyproject.toml and nothing else, so the per-file ignores and the target version still come from there - the CI run cannot drift from what a developer sees locally. One platform rather than the test matrix: ruff reads source, and source does not differ between the runners. requirements-lint.txt pins the tool, and the pin means something different from the one in requirements.txt: there it records which library a release shipped, here it records which rules a pull request was judged by. A linter's rule set moves between releases, and an unpinned tool turns "green yesterday, red today" into a mystery with no commit behind it. Also rewrites two explanatory comments that began with "# noqa:" - a comment starting that way is parsed as a directive, and ruff warned about an invalid code list on every run. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 28 ++++++++++++++++++++++++++++ beantester/matchers.py | 5 +++-- requirements-lint.txt | 14 ++++++++++++++ tests/test_version_and_release.py | 3 ++- 4 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 requirements-lint.txt diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cf9c08e..2dc3f87 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -80,6 +80,34 @@ jobs: printf '%s' "$PR_BODY" > pr-body.txt python tools/check_public_text.py --text-file pr-body.txt + # Static analysis, one platform: ruff reads source, and source does not differ + # between the runners. Kept out of the `tests` matrix for exactly that reason - + # in there it would run twice and say the same thing. + lint: + name: ruff (F and B block, S and ASYNC report) + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.14" + - name: Install the linter + run: pip install -r requirements-lint.txt + # The gate. `--select` on the command line replaces the families named in + # pyproject.toml and nothing else: the per-file ignores and the target + # version still come from there, so the two runs below cannot drift apart + # from what a developer sees locally. + - name: Bugs and dead code (blocking) + run: ruff check --select F,B --output-format github + # The report. S is 306 findings on this repository and 287 of them are + # things a test suite for a packet mangler does on purpose - so it + # annotates the diff and never fails the run. `ruff check` exits 1 on any + # finding, hence --exit-zero rather than a `continue-on-error` on the step: + # a step that "succeeded but failed" reads as a broken job to everybody. + - name: Security patterns and async misuse (report only) + run: ruff check --select S,ASYNC --exit-zero --output-format github + tests: name: tests (${{ matrix.os }}, py${{ matrix.python-version }}) runs-on: ${{ matrix.os }} diff --git a/beantester/matchers.py b/beantester/matchers.py index 57c4140..8b6e9df 100644 --- a/beantester/matchers.py +++ b/beantester/matchers.py @@ -478,8 +478,9 @@ def _parse_ip_term(body, term, field): raise _err("errors.bad_filter_ip", field, term) version, num = operand.version, operand.num base = _compare_predicate(op, num) - # noqa: the loop returns in this iteration, so `version` and `base` - # cannot change under the lambda - B023 needs a second pass to bite. + # B023 is false here: the loop RETURNS in this iteration, so `version` + # and `base` cannot change under the lambda - late binding needs a + # second pass to bite. return ((lambda c: c is not None and c.version == version and base(c.num)), # noqa: B023 ("ip_cmp", version, op, num)) # CIDR diff --git a/requirements-lint.txt b/requirements-lint.txt new file mode 100644 index 0000000..d849ce6 --- /dev/null +++ b/requirements-lint.txt @@ -0,0 +1,14 @@ +# Static analysis, CI only. Nothing here is imported by the program. +# +# 🔴 PINNED, and for a different reason than requirements.txt is pinned. There the +# pin says which library a release shipped; here it says which RULES a pull request +# was judged by. A linter's rule set moves between releases, so an unpinned tool +# turns "green yesterday, red today" into a mystery with no commit behind it - the +# same failure that made `pyinstaller` a pinned dependency in requirements-build.txt +# after the same commit built a working exe on CI and a crashing one on a developer +# machine. +# +# Raise these by hand, deliberately, and read the release notes when you do: a new +# ruff minor may add rules to a family we select, and that is a decision about what +# blocks a pull request, not a routine bump. +ruff==0.16.3 diff --git a/tests/test_version_and_release.py b/tests/test_version_and_release.py index 7ce5570..5b1df10 100644 --- a/tests/test_version_and_release.py +++ b/tests/test_version_and_release.py @@ -125,7 +125,8 @@ def test_breaking_sections_come_first(): def close(version, sections): if version and "### BREAKING" in sections and sections[0] != "### BREAKING": - # noqa: one file, one iteration - the closure cannot outlive it + # B023 is false here: one file, one iteration - the closure + # cannot outlive the loop that made it. problems.append(f"{name} {version}: BREAKING is #{sections.index('### BREAKING') + 1}" # noqa: B023 f" of {len(sections)} (first is {sections[0]!r})") From 61a328e5db03ad6520a497ca7b9e750f443b0309 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 01:27:27 +0200 Subject: [PATCH 06/22] chore: adopt mypy, and answer the thirty things it says Thirty findings on 66 modules, and none of them needed a type: ignore. Fifteen were one block: App._export_csv = App.export_csv and fourteen more, growing attributes onto a class from outside its body. They are a loop over the names now - which is what the property block ten lines above them already did, and which takes ten lines out of the largest module in the package. The file ceiling in tests/test_code_shape.py moves 1202 -> 1192 with it, because down is the routine door. Nine were empty containers whose comment already said what goes in them (_translations = {} # language code -> {key: text}). The comment is the annotation now, and a wrong one gets caught rather than read. Six were one base class declaring kind = None and BLAST_PROBES = (), which mypy reads as "always None" and "always empty" - making every subclass that fills them an error instead of the point of the base class. Nothing strict is turned on, and that is a decision. This package carries no annotations, so disallow_untyped_defs would mean annotating 66 modules before anything could be checked at all. The default level found the three classes of thing above on its own. Tighten per module as modules gain annotations, never repository-wide in one go: a check that has to be silenced everywhere teaches that silence is normal. Scope is the package. tests/ and tools/ hold same-named modules under no package, which mypy refuses without --explicit-package-bases - a later decision, not this one. Co-Authored-By: Claude Opus 5 --- beantester/crashlog.py | 4 ++-- beantester/engine.py | 2 +- beantester/gui/app.py | 25 +++++++++---------------- beantester/gui/dialogs.py | 2 +- beantester/gui/theme.py | 2 +- beantester/gui/tooltip.py | 2 +- beantester/gui/windows.py | 2 +- beantester/i18n.py | 4 ++-- beantester/matchers.py | 7 +++++-- pyproject.toml | 29 +++++++++++++++++++++++++++++ tests/test_code_shape.py | 5 ++++- 11 files changed, 56 insertions(+), 28 deletions(-) diff --git a/beantester/crashlog.py b/beantester/crashlog.py index 12b5d02..707ba60 100644 --- a/beantester/crashlog.py +++ b/beantester/crashlog.py @@ -82,7 +82,7 @@ DEBUG = "debug" _lock = threading.Lock() -_seen = {} # fingerprint -> record (with a count) +_seen: dict[str, dict] = {} # fingerprint -> record (with a count) _context_provider = None # set by the App/CLI: returns a dict of state _installed = False _enabled = True @@ -325,7 +325,7 @@ def note(exc, subsystem, message=""): record(exc, source="swallowed", subsystem=subsystem, severity=DEBUG, note=message) -_once_seen = set() +_once_seen: set[tuple] = set() def once(subsystem, exc): diff --git a/beantester/engine.py b/beantester/engine.py index 44e95cb..efbc4cf 100644 --- a/beantester/engine.py +++ b/beantester/engine.py @@ -172,7 +172,7 @@ def corruption_pct(stats): # Every running engine, so the interpreter can never exit with an open divert # (a leaked handle keeps the WinDivert driver - and its .sys file - loaded). -_LIVE_ENGINES = weakref.WeakSet() +_LIVE_ENGINES: weakref.WeakSet = weakref.WeakSet() def deadline_reached(deadline, now): diff --git a/beantester/gui/app.py b/beantester/gui/app.py index c30c3f5..0a113b6 100644 --- a/beantester/gui/app.py +++ b/beantester/gui/app.py @@ -1847,19 +1847,12 @@ def _var_property(key): setattr(App, _name, _var_property(_key)) -# older private names kept working (docs, scripts, the GUI smoke) -App._apply_if_running = App.apply_if_running -App._export_csv = App.export_csv -App._save_profile = App.save_profile -App._delete_profile = App.delete_profile -App._load_selected_profile = App.load_selected_profile -App._load_scenario = App.load_scenario -App._clear_scenario = App.clear_scenario -App._load_config_file = App.load_config_file -App._save_config_file = App.save_config_file -App._mark_bug = App.mark_bug -App._save_repro = App.save_repro -App._copy_repro_cli = App.copy_repro_cli -App._reset_now_click = App.reset_now_click -App._on_close = App.on_close -App._profile_names = App.profile_names +# Older private names kept working (docs, scripts, the GUI smoke). Written as a +# loop for the same reason the property block above it is: fifteen assignment +# statements say one thing fifteen times, and every one of them was also a +# type error - a class does not grow attributes from outside its own body. +for _alias in ("apply_if_running", "export_csv", "save_profile", "delete_profile", + "load_selected_profile", "load_scenario", "clear_scenario", + "load_config_file", "save_config_file", "mark_bug", "save_repro", + "copy_repro_cli", "reset_now_click", "on_close", "profile_names"): + setattr(App, "_" + _alias, getattr(App, _alias)) diff --git a/beantester/gui/dialogs.py b/beantester/gui/dialogs.py index 9e82ad6..d14d7a8 100644 --- a/beantester/gui/dialogs.py +++ b/beantester/gui/dialogs.py @@ -18,7 +18,7 @@ from .. import crashlog WRAP = 380 -_result = {} # per-dialog result, keyed by the toplevel +_result: dict = {} # per-dialog result, keyed by the toplevel def _center(win, parent, focus=None): diff --git a/beantester/gui/theme.py b/beantester/gui/theme.py index 5f1689f..7d95fe4 100644 --- a/beantester/gui/theme.py +++ b/beantester/gui/theme.py @@ -602,7 +602,7 @@ def unhighlight_combobox(event=None, widget=None): # -- hand-drawn checkbox indicator -------------------------------------------- # -_CHECK_IMAGES = [] # Tk only keeps a weak grip on PhotoImages +_CHECK_IMAGES: list = [] # Tk only keeps a weak grip on PhotoImages _INDICATOR_READY = [False] diff --git a/beantester/gui/tooltip.py b/beantester/gui/tooltip.py index 34209a0..dcab0c5 100644 --- a/beantester/gui/tooltip.py +++ b/beantester/gui/tooltip.py @@ -17,7 +17,7 @@ from .theme import FONT, TIP_BG, TIP_FG from .. import crashlog -_BUBBLES = {} # toplevel name -> (window, label) +_BUBBLES: dict[str, tuple] = {} # toplevel name -> (window, label) def _alive(entry): diff --git a/beantester/gui/windows.py b/beantester/gui/windows.py index fdf67fb..873fee7 100644 --- a/beantester/gui/windows.py +++ b/beantester/gui/windows.py @@ -50,7 +50,7 @@ def build(self, body): from .theme import BG, apply_dark_titlebar, disable_maximize from .. import crashlog -WINDOWS = {} # id -> PanelWindow subclass (the registry) +WINDOWS: dict[str, type] = {} # id -> PanelWindow subclass (the registry) MIN_W, MIN_H = 480, 320 # unscaled; a window smaller than this is unusable diff --git a/beantester/i18n.py b/beantester/i18n.py index fefdd9a..f0e1150 100644 --- a/beantester/i18n.py +++ b/beantester/i18n.py @@ -12,8 +12,8 @@ FALLBACK_LANGUAGE = "en" -_translations = {} # language code -> {key: translated text} -_language_names = {} # language code -> display name (from "_meta") +_translations: dict[str, dict[str, str]] = {} # language code -> {key: text} +_language_names: dict[str, str] = {} # code -> name (from "_meta") _LANG = None # resolved lazily on first use (see _resolve_language) diff --git a/beantester/matchers.py b/beantester/matchers.py index 8b6e9df..f8d8d57 100644 --- a/beantester/matchers.py +++ b/beantester/matchers.py @@ -151,7 +151,10 @@ def __repr__(self): # pragma: no cover # -- matchers ----------------------------------------------------------------- # class Matcher: """A compiled field expression. Compile once, call ``matches()`` per packet.""" - kind = None + # Annotated because subclasses fill both in, and an unannotated `None` + # (or `()`) is read as "this attribute is always None" - which makes + # every subclass an error rather than the point of the base class. + kind: str | None = None def __init__(self, raw, terms): self.raw = str(raw or "").strip() @@ -184,7 +187,7 @@ def selects_nothing_in_particular(self): # the whole of IPv4 and none of IPv6, and that still bounds nothing worth # having. Covering any ONE group completely is enough. Kinds without such a # split declare a single group. - BLAST_PROBES = () + BLAST_PROBES: tuple[tuple[tuple, ...], ...] = () @property def covers_everything(self): diff --git a/pyproject.toml b/pyproject.toml index ef56496..4a046e8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -83,6 +83,35 @@ select = ["F", "B", "S", "ASYNC"] # URL opened is a literal api.github.com endpoint whose path is validated first. "tools/**" = ["S603", "S607", "S310"] +[tool.mypy] +# The PACKAGE only. `tests/` and `tools/` are not a matter of taste here: both +# hold modules with the same basename under no package (`downloads`, `fakes`), +# and mypy refuses that layout without `--explicit-package-bases`. They can be +# added later, deliberately; the product is what ships. +files = ["beantester"] +# The floor from `requires-python`, said out loud: mypy otherwise checks against +# the interpreter it happens to run on, and the two are not the same question. +python_version = "3.10" +# A `# type: ignore` that has stopped being needed is a lie about the code that +# outlives whatever it was hiding. +warn_unused_ignores = true + +# Nothing strict is turned on, and that is a decision rather than an oversight. +# This package carries no annotations, so `disallow_untyped_defs` would mean +# annotating 66 modules before anything could be checked at all. What the default +# level DOES catch is worth having on its own: it found fifteen attributes being +# grown onto a class from outside its body, nine containers whose type nobody +# could infer, and a base class that declared its own attributes as None. +# Tighten it per module when a module is annotated, never repository-wide in one +# go - a check that must be silenced everywhere teaches that silence is normal. + +[[tool.mypy.overrides]] +# Neither ships type information: pydivert has no `py.typed` marker, and psutil's +# stubs live in a separate `types-psutil` distribution. Ignoring the import is a +# smaller lie than pretending we know those signatures. +module = ["pydivert", "pydivert.*", "psutil"] +ignore_missing_imports = true + [tool.coverage.run] source = ["beantester"] parallel = true # the GUI tests run in subprocesses; each writes its own data file diff --git a/tests/test_code_shape.py b/tests/test_code_shape.py index 2793b82..80ca023 100644 --- a/tests/test_code_shape.py +++ b/tests/test_code_shape.py @@ -54,12 +54,15 @@ # there for a week and were found only because somebody printed the numbers. That is # the same defect the crowd counts below exist to catch, one level up. FUNCTION_CEILING = 133 # beantester/cli.py::_run_session +# Lowered 2026-08-19 from 1202: fifteen compatibility aliases assigned one per line +# became a loop over their names, which is also what mypy asked for (a class does not +# grow attributes from outside its own body). Ten lines out, ten lines off the ceiling. # Lowered 2026-08-12 from 1287, the same routine door: moving the user files out of # the install directory needed three lines in `app.py`, which was pinned to the # ceiling exactly, so the two CSV exports moved to `gui/csv_export.py` instead of the # number moving up. The crowd band below was re-measured after the drop (`engine.py` # is 779, still clear of it) - lowering a ceiling tightens that band too. -FILE_CEILING = 1202 # beantester/gui/app.py +FILE_CEILING = 1192 # beantester/gui/app.py # 🔴 THE SECOND KNOB. A ceiling on the worst single item sees one thing growing # to a record and is blind to everything creeping upward together: five files at From eae348c18d5507b068b959e21a8fd5d77b9df451 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 01:30:22 +0200 Subject: [PATCH 07/22] ci: type check the package on every pull request Its own job rather than a step inside the ruff one, so the two gates carry two check names: a branch-protection rule can require either on its own, and a red one says which it was without opening the log. The job installs no project dependencies, and that is the part worth checking rather than assuming. pydivert is Windows-only, psutil is not needed to read this package, and both are ignored by name in [tool.mypy] - so mypy was run in exactly that shape before this job was written: Linux, neither installed, 66 modules, no issues. A missing module and an installed-but-untyped one are different code paths in mypy, and only the second had been seen on the developer machine. mypy is pinned next to ruff for the same reason: a release can start reporting a class of error the previous one did not, and that is a decision about what blocks a pull request rather than a routine bump. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 23 +++++++++++++++++++++++ requirements-lint.txt | 7 +++++++ 2 files changed, 30 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2dc3f87..36cc4f5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -108,6 +108,29 @@ jobs: - name: Security patterns and async misuse (report only) run: ruff check --select S,ASYNC --exit-zero --output-format github + # Its own job rather than a step in `lint`: two gates, two check names, so a + # branch-protection rule can require either one on its own and a red one says + # which of the two it was without opening the log. + types: + name: mypy + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.14" + - name: Install the type checker + run: pip install -r requirements-lint.txt + # No project dependencies installed on purpose: `pydivert` is Windows-only + # and `psutil` is not needed to READ this package, and both are ignored by + # name in [tool.mypy]. Verified in that exact shape (Linux, neither + # installed) before this job was written - a missing module and an untyped + # one are different code paths in mypy, and only one of them was measured + # on the developer machine. + - name: Type check the package + run: mypy + tests: name: tests (${{ matrix.os }}, py${{ matrix.python-version }}) runs-on: ${{ matrix.os }} diff --git a/requirements-lint.txt b/requirements-lint.txt index d849ce6..33ef3e4 100644 --- a/requirements-lint.txt +++ b/requirements-lint.txt @@ -12,3 +12,10 @@ # ruff minor may add rules to a family we select, and that is a decision about what # blocks a pull request, not a routine bump. ruff==0.16.3 + +# Same pin, same reason: a mypy release can start reporting a class of error the +# previous one did not, and that is a decision about what blocks a pull request. +# Verified 2026-08-19 on Linux WITHOUT pydivert or psutil installed - the shape a +# runner has - because both are ignored by name in [tool.mypy] and a missing +# module is a different code path from an untyped one. +mypy==2.3.1 From 2327d33b5518ae7783a98892b9b579515211ab52 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 01:45:40 +0200 Subject: [PATCH 08/22] ci: scan with semgrep on every pull request, and gate it ourselves The scan is `semgrep scan --config p/default --metrics=off --oss-only --json` and tools/semgrep_gate.py decides the run. Measured before any of it was written: the registry serves p/default anonymously - no account, no token, 343 rules over 184 files in 35 seconds - and scanning the commit before this branch reproduces the platform report exactly, the same 23 findings with the same rule ids. The free path gives what the dashboard gave. The gate is a script and not a flag, because the flag lies. --severity accepts INFO, WARNING and ERROR only, while registry rules also carry the newer scale: two findings in this repository's own report came back as MEDIUM. Measured with a probe rule declaring severity: HIGH - unfiltered it produced 19 findings, and --severity ERROR produced zero. A gate built on that flag would silently ignore the severities it was asked to block. The script reads the JSON and blocks on ERROR, HIGH or CRITICAL, and on a scan error, because a rule that could not run is not a rule that found nothing. Verified end to end on both scans: the pre-branch commit gives "1 blocking, 22 other" and exit 1, today's tree gives "0 blocking, 2 other" and exit 0. Licensing is why the rules are fetched and never carried here. The CLI is LGPL-2.1, which we run rather than ship. The rules are under the Semgrep Rules License v1.0, which permits use for our own purposes and forbids redistributing them, and vendoring p/default into a public repository would be exactly that. So the pin fixes the engine and not the ruleset, and a re-graded rule can turn a pull request red with no commit behind it. That is the cost of not vendoring, and it is the cheaper side. The Windows marker on the pin is measured as well: the wheel installs and then semgrep-core fails to scan at all on this machine, an OCaml backtrace on a three-line local rule. Local scans go through WSL. Both READMEs and CONTRIBUTING now say what the three static gates do. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 27 ++++++++++ CONTRIBUTING.md | 10 +++- README.md | 8 +++ README.pl.md | 8 +++ requirements-lint.txt | 14 +++++ tests/test_semgrep_gate.py | 101 +++++++++++++++++++++++++++++++++++++ tools/semgrep_gate.py | 97 +++++++++++++++++++++++++++++++++++ 7 files changed, 263 insertions(+), 2 deletions(-) create mode 100644 tests/test_semgrep_gate.py create mode 100644 tools/semgrep_gate.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 36cc4f5..f804f67 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -131,6 +131,33 @@ jobs: - name: Type check the package run: mypy + # Semgrep, with no account and no token: `p/default` is fetched anonymously + # from the registry (measured - 343 rules, 184 files, 35 s). The rules are used + # here and never carried in the repository, because the Semgrep Rules License + # allows the first and forbids the second. + semgrep: + name: semgrep (ERROR, HIGH and CRITICAL block) + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.14" + - name: Install the scanner + run: pip install -r requirements-lint.txt + # No --error and no --severity here: the scan reports, and tools/semgrep_gate.py + # decides. `--severity` knows INFO/WARNING/ERROR only, while registry rules also + # carry HIGH and CRITICAL - measured with a probe rule, `--severity ERROR` + # returned zero findings for a rule marked HIGH. Metrics are off: this repository + # does not send telemetry about its own source anywhere. + - name: Scan + run: | + semgrep scan --config p/default --metrics=off --oss-only \ + --json --output semgrep.json --quiet + - name: Decide + run: python tools/semgrep_gate.py semgrep.json + tests: name: tests (${{ matrix.os }}, py${{ matrix.python-version }}) runs-on: ${{ matrix.os }} diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 937350f..0cc2108 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -8,8 +8,11 @@ testable on any OS. ```bash pip install -r requirements-dev.txt +pip install -r requirements-lint.txt # ruff and mypy (semgrep on Linux and macOS) python -m pytest tests # full suite - no Windows, no driver, no admin rights python smoke_gui.py # GUI smoke with a fake tkinter +ruff check # F and B fail a pull request, S and ASYNC are a report +mypy # types, over the package python bean_network_tester.py --simulate --loss 10 --duration 3 # CLI demo python bean_network_tester.py --doctor # environment report ``` @@ -57,5 +60,8 @@ onedir, `asInvoker`. Do not reintroduce `--noconsole` / `--onefile` / `--uac-adm ## Pull requests 1. Run `python -m pytest tests` - everything must pass. -2. Add tests for new behavior (see `tests/` for the style). -3. Update both `lang/en.json` and `lang/pl.json` when adding UI texts. +2. Run `ruff check` and `mypy` - both are gates on the pull request. + Semgrep runs in CI and needs no local setup. On Windows it installs but does not + scan, so run it from WSL if you want it locally. +3. Add tests for new behavior (see `tests/` for the style). +4. Update both `lang/en.json` and `lang/pl.json` when adding UI texts. diff --git a/README.md b/README.md index 463af17..e205d5c 100644 --- a/README.md +++ b/README.md @@ -1157,6 +1157,14 @@ exit-code assertions, an NDJSON check, `--doctor` and `--license`, and then **an a smoke test of the built file** (`--version`, `--simulate`, a bad config -> code 3) plus a check that the WinDivert driver really shipped next to the exe, with a downloadable artifact. +**Three static checks run beside the tests**, on Linux only, because they read the source rather +than run it. **ruff** fails a pull request on a dead-code or bug-shape finding (`F` and `B`) and +reports the security family (`S`, `ASYNC`) as annotations that never block. **mypy** type-checks +the package. **semgrep** scans with its default registry ruleset and a finding at ERROR, HIGH or +CRITICAL fails the run, while everything below that is printed in full. The three tool versions +are pinned in `requirements-lint.txt`, so a new release of a linter cannot redden a pull request +that changed nothing. + One step is worth knowing about because no unit test can do its job: a **GUI render check on real Tk** under a virtual screen, at the minimum supported 1366x768, **in every language**. It builds the actual window, walks every page, opens the About window and fails the build when any button is diff --git a/README.pl.md b/README.pl.md index 3fc7d93..18640bb 100644 --- a/README.pl.md +++ b/README.pl.md @@ -1012,6 +1012,14 @@ a na koniec **build `.exe` i smoke zbudowanego pliku** (`--version`, `--simulate konfiguracja → kod 3) plus kontrola, że sterownik WinDivert naprawdę trafił obok exe, z artefaktem do pobrania. +**Obok testów chodzą trzy analizy statyczne**, wyłącznie na Linuksie, bo czytają kod, a nie go +uruchamiają. **ruff** wywraca pull requesta na martwym kodzie i na kształtach błędów (`F` i `B`), +a rodzinę bezpieczeństwa (`S`, `ASYNC`) tylko wypisuje w diffie i nigdy nie blokuje. **mypy** +sprawdza typy w pakiecie. **semgrep** skanuje domyślnym zestawem reguł z rejestru, przy czym +znalezisko na poziomie ERROR, HIGH albo CRITICAL wywraca przebieg, a wszystko niżej ląduje w logu. +Wersje tych trzech narzędzi są przypięte w `requirements-lint.txt`, więc nowe wydanie lintera nie +zaczerwieni pull requesta, w którym nic się nie zmieniło. + Jeden krok wart jest osobnego zdania, bo żaden test jednostkowy go nie zastąpi: **render GUI na prawdziwym Tk** pod wirtualnym ekranem, w minimalnej wspieranej rozdzielczości 1366x768 i **w każdym języku**. Buduje prawdziwe okno, obchodzi wszystkie strony, otwiera okno „O programie” i diff --git a/requirements-lint.txt b/requirements-lint.txt index 33ef3e4..02a40d2 100644 --- a/requirements-lint.txt +++ b/requirements-lint.txt @@ -19,3 +19,17 @@ ruff==0.16.3 # runner has - because both are ignored by name in [tool.mypy] and a missing # module is a different code path from an untyped one. mypy==2.3.1 + +# Linux and macOS only, deliberately: the Windows wheel installs (57 MB) and then +# `semgrep-core` fails to run a scan at all on this machine - an OCaml backtrace +# on a three-line local rule, measured 2026-08-19. Local scans go through WSL, +# CI runs on ubuntu. The marker keeps `pip install -r` honest on Windows instead +# of downloading an engine that cannot start. +# +# 🔴 The pin fixes the ENGINE, not the rules: `--config p/default` fetches those +# from the registry at scan time, so a new or re-graded rule can turn a pull +# request red with no commit behind it. That is the trade for not vendoring them, +# and vendoring is not open to us - the Semgrep Rules License permits use for our +# own purposes and forbids redistributing the rules, which is what putting them in +# a public repository would be. +semgrep==1.173.0; sys_platform != "win32" diff --git a/tests/test_semgrep_gate.py b/tests/test_semgrep_gate.py new file mode 100644 index 0000000..a3d6084 --- /dev/null +++ b/tests/test_semgrep_gate.py @@ -0,0 +1,101 @@ +"""The Semgrep gate: which severities stop a pull request, and which do not. + +The gate exists because ``semgrep --severity ERROR --error`` does not do what it +reads like - the flag knows INFO/WARNING/ERROR only, while registry rules also +carry the newer LOW/MEDIUM/HIGH/CRITICAL scale, so a HIGH rule is filtered OUT by +a filter asked to keep the worst findings. That was measured with a probe rule +before this file was written; what is guarded here is the decision that replaced +it, on reports shaped exactly like semgrep's own. +""" +import json +import os +import sys + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "tools")) + +import semgrep_gate # noqa: E402 +from fakes import check # noqa: E402 + + +def _finding(severity, path="beantester/x.py", line=1, check_id="rule.id"): + return {"check_id": check_id, "path": path, "start": {"line": line}, + "extra": {"severity": severity, "message": "something happened"}} + + +def _report(findings=(), errors=()): + return {"results": list(findings), "errors": list(errors)} + + +def _write(tmp_path, report): + path = tmp_path / "semgrep.json" + path.write_text(json.dumps(report), encoding="utf-8") + return str(path) + + +def test_the_new_severity_names_block_as_well_as_the_old_one(): + """The whole reason this file exists: HIGH and CRITICAL are not ERROR, and a + gate that only knows ERROR passes them through.""" + for severity in ("ERROR", "HIGH", "CRITICAL"): + blocking, passing, _ = semgrep_gate.split(_report([_finding(severity)])) + check(f"{severity} blocks", len(blocking) == 1 and not passing, + f"({len(blocking)} blocking, {len(passing)} passing)") + # ...and the ones that must not, including the value that actually appeared in + # this repository's own report. + for severity in ("MEDIUM", "WARNING", "LOW", "INFO"): + blocking, passing, _ = semgrep_gate.split(_report([_finding(severity)])) + check(f"{severity} does not block", not blocking and len(passing) == 1, + f"({len(blocking)} blocking, {len(passing)} passing)") + + +def test_severity_is_read_case_insensitively(): + """Nothing promises the case of that field, and a gate that misses "high" + because it expected "HIGH" fails open - the direction that never gets noticed.""" + blocking, passing, _ = semgrep_gate.split(_report([_finding("high")])) + check("lower-case high still blocks", len(blocking) == 1 and not passing) + + +def test_a_scan_error_is_a_failed_gate_not_a_clean_one(): + """A rule that could not run is not a rule that found nothing.""" + report = _report(errors=[{"level": "error", "type": "SemgrepError", + "message": "rule validation failed"}]) + blocking, passing, errors = semgrep_gate.split(report) + check("the scan error is collected", len(errors) == 1 and not blocking and not passing) + + +def test_a_warning_level_scan_note_is_not_an_error(tmp_path): + report = _report([_finding("WARNING")], + errors=[{"level": "warn", "type": "Note", "message": "skipped a file"}]) + code = semgrep_gate.main([_write(tmp_path, report)]) + check("a warning finding and a warn-level note pass", code == 0, f"(exit {code})") + + +def test_the_exit_code_is_the_verdict(tmp_path): + clean = semgrep_gate.main([_write(tmp_path, _report())]) + check("an empty report passes", clean == 0, f"(exit {clean})") + + mixed = _report([_finding("WARNING"), _finding("HIGH", line=7)]) + code = semgrep_gate.main([_write(tmp_path, mixed)]) + check("one HIGH among warnings fails", code == 1, f"(exit {code})") + + +def test_a_missing_or_broken_report_fails_the_gate(tmp_path): + """The failure mode that would otherwise be silent: the scan step wrote + nothing, and a gate reading nothing decides everything is fine.""" + missing = semgrep_gate.main([str(tmp_path / "not-written.json")]) + check("a missing report is not a pass", missing == 2, f"(exit {missing})") + + broken = tmp_path / "broken.json" + broken.write_text("{not json", encoding="utf-8") + code = semgrep_gate.main([str(broken)]) + check("an unparsable report is not a pass", code == 2, f"(exit {code})") + + +def test_the_printed_report_names_every_finding(tmp_path): + """CI shows the log and nothing else, so the log has to carry the verdict.""" + report = _report([_finding("HIGH", path="a.py", line=3, check_id="rules.bad"), + _finding("WARNING", path="b.py", line=9, check_id="rules.meh")], + errors=[{"level": "error", "type": "SemgrepError", "message": "boom"}]) + text = "\n".join(semgrep_gate.report_lines(*semgrep_gate.split(report))) + for expected in ("a.py:3", "rules.bad", "b.py:9", "rules.meh", "SemgrepError", + "1 blocking, 1 other, 1 scan error"): + check(f"the report says {expected!r}", expected in text, f"({text[:200]})") diff --git a/tools/semgrep_gate.py b/tools/semgrep_gate.py new file mode 100644 index 0000000..46d1f4a --- /dev/null +++ b/tools/semgrep_gate.py @@ -0,0 +1,97 @@ +#!/usr/bin/env python3 +"""Decide a pull request from a Semgrep JSON report. + + semgrep scan --config p/default --json --output semgrep.json + python tools/semgrep_gate.py semgrep.json + +Why this exists rather than ``semgrep --severity ERROR --error`` +--------------------------------------------------------------- +Because that flag does not mean what it reads like. ``--severity`` accepts +exactly ``INFO``, ``WARNING`` and ``ERROR`` (semgrep 1.173.0, ``--help``), while +a rule may declare the newer scale - and rules in the registry do: two findings +in this repository's own report came back as ``MEDIUM``. + +Measured 2026-08-19 with a probe rule carrying ``severity: HIGH``: unfiltered it +produced 19 findings, and with ``--severity ERROR`` it produced **zero**. So the +flag-based gate silently ignores exactly the severities it is asked to block. +Reading the report and deciding here is the only version that cannot lie. + +What blocks +----------- +Any finding whose severity is ERROR, HIGH or CRITICAL. Everything else is +printed and passes, because a WARNING here is usually a rule seeing a shape it +cannot resolve (a dynamic import from a hardcoded table, a URL built from a +validated argument) and a gate that fires on those is a gate nobody reads. + +A scan ERROR also blocks. A rule that failed to run is not a rule that found +nothing, and "the scan was green" must not mean "the scan did not happen". +""" +import argparse +import json +import sys + +BLOCKING = ("ERROR", "HIGH", "CRITICAL") + + +def load(path): + with open(path, encoding="utf-8") as handle: + return json.load(handle) + + +def split(report): + """``(blocking, passing, scan_errors)`` out of a semgrep JSON report.""" + blocking, passing = [], [] + for result in report.get("results") or []: + severity = str((result.get("extra") or {}).get("severity", "")).upper() + (blocking if severity in BLOCKING else passing).append(result) + errors = [e for e in report.get("errors") or [] + if str(e.get("level", "")).lower() == "error"] + return blocking, passing, errors + + +def describe(result): + extra = result.get("extra") or {} + start = result.get("start") or {} + message = " ".join(str(extra.get("message", "")).split()) + return "%s:%s [%s] %s\n %s" % ( + result.get("path", "?"), start.get("line", "?"), + extra.get("severity", "?"), result.get("check_id", "?"), message[:300]) + + +def report_lines(blocking, passing, errors): + lines = [] + if errors: + lines.append("scan errors (a rule that could not run is not a rule that passed):") + lines += [" %s: %s" % (e.get("type", "?"), + " ".join(str(e.get("message", "")).split())[:200]) + for e in errors] + if blocking: + lines.append("blocking findings (%s):" % ", ".join(BLOCKING)) + lines += [" " + describe(r) for r in blocking] + if passing: + lines.append("other findings (reported, not blocking):") + lines += [" " + describe(r) for r in passing] + lines.append("semgrep: %d blocking, %d other, %d scan error(s)" + % (len(blocking), len(passing), len(errors))) + return lines + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("report", help="the JSON file semgrep --output wrote") + args = parser.parse_args(argv) + try: + report = load(args.report) + except (OSError, ValueError) as exc: + # A missing or unparsable report is a failed gate, never a pass: it means + # the scan step did not produce what this one was promised. + print("semgrep gate: cannot read %s: %s" % (args.report, exc), file=sys.stderr) + return 2 + blocking, passing, errors = split(report) + for line in report_lines(blocking, passing, errors): + print(line) + return 1 if (blocking or errors) else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From ede3333c2e3b8f61bd83546d502a590910a9e1d6 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 08:30:22 +0200 Subject: [PATCH 09/22] feat(release): sign where the download came from, not just what is in it The SBOM attestation already said what is inside the release zip. This adds the other half: a build-provenance attestation saying the zip came out of this repository, from this commit, through this workflow, on a GitHub-hosted runner. Both are signed with the same short-lived OIDC identity and land in the same store, so a user answers both questions with one command: gh attestation verify BeanNetworkTester--windows-x64.zip \ -R donislawdev/BeanNetworkTester This is a different question from the code signature being prepared separately, and that is why both are worth having. A signature says who signed the binary. Provenance says which source and which build produced it. A stolen signing key cannot forge provenance, and a forked workflow cannot claim to be this repository. A separate action from actions/attest, checked rather than assumed: that one attaches a predicate you hand it, while the provenance predicate is 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 worth trusting. The permissions were already there, so nothing new is granted. New guard: both subject-path values must name the same variable, and that variable must be what `gh release create` uploads. A statement signed about a file nobody downloads is invisible on the release page. Co-Authored-By: Claude Opus 5 --- .github/workflows/release.yml | 23 ++++++++++++++++++++ README.md | 12 +++++++++++ README.pl.md | 12 +++++++++++ tests/test_version_and_release.py | 36 +++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 14e46fa..b9b8a59 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -161,6 +161,29 @@ jobs: subject-path: ${{ env.ASSET }} sbom-path: ${{ env.SBOM }} + # WHO built it, and FROM WHAT. The SBOM attestation above says what is inside + # the zip; this one says the zip came out of this repository, from this commit, + # through this workflow, on a GitHub-hosted runner - signed with the same + # short-lived OIDC identity and stored in the same attestation store. A user + # checks both with one command: + # + # gh attestation verify BeanNetworkTester--windows-x64.zip \ + # -R donislawdev/BeanNetworkTester + # + # It is a different question from a code signature, which is why both are + # worth having: a signature says who signed the binary, provenance says which + # source and which build produced it. A stolen signing key cannot forge this, + # and a forked workflow cannot claim to be this repository. + # + # Separate action from `actions/attest` above on purpose: that one attaches a + # predicate you hand it (here, the SBOM), while the provenance predicate is + # 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 + uses: actions/attest-build-provenance@4d101475d8b20a2381f78447822ac1eab6504dd8 # v4.2.2 + with: + subject-path: ${{ env.ASSET }} + # 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". # diff --git a/README.md b/README.md index e205d5c..bdb4b85 100644 --- a/README.md +++ b/README.md @@ -1454,6 +1454,18 @@ and some antivirus tools may raise a false alarm. The **WinDivert driver itself by its author**. You can compare the release's SHA-256 checksum (`SHA256SUMS.txt`) to confirm the file has not been modified. +**You can also check where the 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": + +```bash +gh attestation verify BeanNetworkTester-v0.5.0-windows-x64.zip -R donislawdev/BeanNetworkTester +``` + +A checksum proves the file matches what the release page says. This proves the release page itself +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. + ### 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 18640bb..ca2635b 100644 --- a/README.pl.md +++ b/README.pl.md @@ -1316,6 +1316,18 @@ mogą zgłosić fałszywy alarm. Sam sterownik **WinDivert jest podpisany cyfrow przez jego autora**. Sumę kontrolną SHA-256 wydania (`SHA256SUMS.txt`) możesz porównać, żeby potwierdzić, że plik nie został zmodyfikowany. +**Możesz też sprawdzić, skąd ten plik pochodzi, a nie tylko czy się nie zmienił.** Każde archiwum +wydania niesie podpisaną atestację builda, więc jedno polecenie odpowiada na pytanie „czy to +naprawdę zbudowano z tego kodu, tym workflow": + +```bash +gh attestation verify BeanNetworkTester-v0.5.0-windows-x64.zip -R donislawdev/BeanNetworkTester +``` + +Suma kontrolna dowodzi, że plik zgadza się z tym, co mówi strona wydania. To dowodzi, że sama +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. + ### 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 5b1df10..aebfe1e 100644 --- a/tests/test_version_and_release.py +++ b/tests/test_version_and_release.py @@ -482,3 +482,39 @@ def test_the_downloads_tool_refuses_anything_that_is_not_owner_slash_name(): reason = "no error at all" check(f"{bad!r} is refused before it becomes a URL", rejected, "" if rejected else f"({reason})") + + +def test_the_release_attests_exactly_the_archive_it_publishes(): + """Two attestations, one subject, and the subject is the download. + + A release carries two signed statements about the zip: an SBOM attestation + (what is inside it) and a build-provenance attestation (which repository, + commit and workflow produced it). Both are worth nothing if they name a + different file from the one `gh release create` uploads, and that mismatch is + invisible on the release page - the attestation store simply ends up holding a + statement about a digest nobody downloads. + + So this pins the shape rather than the wording: every `subject-path` in the + workflow names the same variable the publish step uploads, and both actions + are still there. + """ + import re + with open(os.path.join(ROOT, ".github", "workflows", "release.yml"), + encoding="utf-8") as handle: + text = handle.read() + + subjects = re.findall(r"subject-path:\s*(\S.*?)\s*$", text, re.MULTILINE) + 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) + 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" ...` + name = subjects[0].strip("${} ").replace("env.", "").strip() + check(f"the attested subject ({name}) is what gets uploaded", + ("$" + name) in uploaded or ("${" + name + "}") in uploaded, + f"({uploaded[:120]})") + + for action in ("actions/attest@", "actions/attest-build-provenance@"): + check(f"{action} is still in the release workflow", action in text) From f8701c5b2792b41c34f8016ef38a20d180906a58 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 08:40:25 +0200 Subject: [PATCH 10/22] feat(deps): pin the bytes of the runtime dependencies, not just the version pydivert==3.1.3 pins a number and says nothing about what comes back. An index, or a publishing account that has been taken over, can serve different bytes under the same version - and that wheel carries the WinDivert kernel driver this tool installs on a user's machine. requirements.txt now carries the artefact hashes (2 for pydivert, 21 for psutil), generated by tools/pin_hashes.py from the PyPI JSON API. Measured against the real index, because the failure mode is not the obvious one. Corrupting one artefact's hash does NOT fail the install: pip falls back to another artefact of the same version and succeeds. That is why every published artefact is listed. Corrupting all of them is what produces "THESE PACKAGES DO NOT MATCH THE HASHES" and exit 1. requirements-dev.txt no longer includes -r requirements.txt, and that is pip's rule rather than a preference: hash-checking applies to the whole install as soon as one requirement carries a hash, so including the hashed file would demand hashes for pytest, hypothesis and everything underneath - which are deliberately unpinned so the weekly run can watch them drift. Every install site is two commands now, in CI, in the release, on Pages and in both READMEs. The build file is not hashed yet, so the release installs it separately. That is the next step rather than an oversight: the freezer pulls eight transitive dependencies, one of which ships monthly. Guards: every pinned requirement must carry at least two hashes in the shape pip reads, and the dev file must not include the hashed one. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 7 +- .github/workflows/pages.yml | 1 + .github/workflows/release.yml | 6 +- CONTRIBUTING.md | 1 + README.md | 1 + README.pl.md | 1 + requirements-dev.txt | 11 ++- requirements.txt | 27 +++++++- tests/test_mutation_registry.py | 4 +- tests/test_version_and_release.py | 69 +++++++++++++++++++ tools/pin_hashes.py | 110 ++++++++++++++++++++++++++++++ 11 files changed, 231 insertions(+), 7 deletions(-) create mode 100644 tools/pin_hashes.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f804f67..93b901f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -202,6 +202,7 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip + pip install --require-hashes -r requirements.txt pip install -r requirements-dev.txt # ONE run of the whole suite, under coverage. testpaths=["tests"] (pyproject) @@ -312,7 +313,11 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - pip install -r requirements.txt -r requirements-build.txt + # Two commands, not one: the runtime file is hash-checked and the + # build file is not (yet), and pip applies hash-checking to an + # entire install once any requirement in it carries a hash. + pip install --require-hashes -r requirements.txt + pip install -r requirements-build.txt # onedir + console subsystem + asInvoker - see BeanNetworkTester.spec - name: Build diff --git a/.github/workflows/pages.yml b/.github/workflows/pages.yml index d336fb6..095dc30 100644 --- a/.github/workflows/pages.yml +++ b/.github/workflows/pages.yml @@ -102,6 +102,7 @@ jobs: - name: Install the test dependencies run: | python -m pip install --upgrade pip + pip install --require-hashes -r requirements.txt pip install -r requirements-dev.txt # The generator refuses to build a broken page, but "it did not crash" is a diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b9b8a59..8e77e4e 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -46,7 +46,11 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - pip install -r requirements.txt -r requirements-build.txt + # Two commands: hash-checked runtime, then the freezer. See the same + # split in ci.yml - pip turns hash-checking on for the whole install + # as soon as one requirement carries a hash. + pip install --require-hashes -r requirements.txt + pip install -r requirements-build.txt # The version is single-sourced in VERSION.txt. The tag is v for a # final release, or v-rc.N (also -beta.N / -alpha.N) for a pre-release. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0cc2108..fae2578 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,6 +7,7 @@ testable on any OS. ## Getting started ```bash +pip install --require-hashes -r requirements.txt # runtime, pinned to exact artefacts pip install -r requirements-dev.txt pip install -r requirements-lint.txt # ruff and mypy (semgrep on Linux and macOS) python -m pytest tests # full suite - no Windows, no driver, no admin rights diff --git a/README.md b/README.md index bdb4b85..6556590 100644 --- a/README.md +++ b/README.md @@ -1130,6 +1130,7 @@ The engine is separate from WinDivert, so the tests run on any system (they need admin nor tkinter). The suite is based on **pytest**: ```bat +pip install --require-hashes -r requirements.txt pip install -r requirements-dev.txt python -m pytest tests ``` diff --git a/README.pl.md b/README.pl.md index ca2635b..c867ea0 100644 --- a/README.pl.md +++ b/README.pl.md @@ -983,6 +983,7 @@ Silnik jest oddzielony od WinDivert, więc testy działają na każdym systemie Windows, admina ani tkintera). Zestaw jest oparty o **pytest**: ```bat +pip install --require-hashes -r requirements.txt pip install -r requirements-dev.txt python -m pytest tests ``` diff --git a/requirements-dev.txt b/requirements-dev.txt index 0f80c24..6271dc3 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,4 +1,13 @@ --r requirements.txt +# 🔴 This file no longer includes `-r requirements.txt`, and that is forced +# rather than chosen. The runtime file carries artefact hashes now, and pip +# turns hash-checking on for the WHOLE install as soon as one requirement has +# a hash - so including it here would demand hashes for pytest, hypothesis and +# every transitive dependency of theirs. Those are deliberately unpinned (the +# weekly run exists to watch them drift), and the two cannot share one command. +# +# Install both, in either order: +# pip install --require-hashes -r requirements.txt +# pip install -r requirements-dev.txt pytest hypothesis # property-based tests (matchers) and CLI fuzzing pytest-cov # coverage gate (see [tool.coverage] in pyproject.toml) diff --git a/requirements.txt b/requirements.txt index 72aec63..fceed20 100644 --- a/requirements.txt +++ b/requirements.txt @@ -11,5 +11,28 @@ # deliberately - `pydivert` carries the WinDivert DLL and the kernel driver, so # bumping it changes what we ship to users and what THIRD-PARTY-NOTICES.md has to # say. Check the notices and `beantester/legal.py` in the same change. -pydivert==3.1.3; sys_platform == "win32" -psutil==7.2.2 +pydivert==3.1.3; sys_platform == "win32" \ + --hash=sha256:74ae83a5d1b4a31db8f84a55c1cf875eb4e7a0d2bc2e4730ef171819bc5e9008 \ + --hash=sha256:e5cba087f3f17eb66cf50965cf9670c55691c580f81dcad20549a0ef49af8e06 +psutil==7.2.2 \ + --hash=sha256:0746f5f8d406af344fd547f1c8daa5f5c33dbc293bb8d6a16d80b4bb88f59372 \ + --hash=sha256:076a2d2f923fd4821644f5ba89f059523da90dc9014e85f8e45a5774ca5bc6f9 \ + --hash=sha256:11fe5a4f613759764e79c65cf11ebdf26e33d6dd34336f8a337aa2996d71c841 \ + --hash=sha256:1a571f2330c966c62aeda00dd24620425d4b0cc86881c89861fbc04549e5dc63 \ + --hash=sha256:1a7b04c10f32cc88ab39cbf606e117fd74721c831c98a27dc04578deb0c16979 \ + --hash=sha256:1fa4ecf83bcdf6e6c8f4449aff98eefb5d0604bf88cb883d7da3d8d2d909546a \ + --hash=sha256:2edccc433cbfa046b980b0df0171cd25bcaeb3a68fe9022db0979e7aa74a826b \ + --hash=sha256:7b6d09433a10592ce39b13d7be5a54fbac1d1228ed29abc880fb23df7cb694c9 \ + --hash=sha256:8c233660f575a5a89e6d4cb65d9f938126312bca76d8fe087b947b3a1aaac9ee \ + --hash=sha256:917e891983ca3c1887b4ef36447b1e0873e70c933afc831c6b6da078ba474312 \ + --hash=sha256:ab486563df44c17f5173621c7b198955bd6b613fb87c71c161f827d3fb149a9b \ + --hash=sha256:ae0aefdd8796a7737eccea863f80f81e468a1e4cf14d926bd9b6f5f2d5f90ca9 \ + --hash=sha256:b0726cecd84f9474419d67252add4ac0cd9811b04d61123054b9fb6f57df6e9e \ + --hash=sha256:b58fabe35e80b264a4e3bb23e6b96f9e45a3df7fb7eed419ac0e5947c61e47cc \ + --hash=sha256:c7663d4e37f13e884d13994247449e9f8f574bc4655d509c3b95e9ec9e2b9dc1 \ + --hash=sha256:e452c464a02e7dc7822a05d25db4cde564444a67e58539a00f929c51eddda0cf \ + --hash=sha256:e78c8603dcd9a04c7364f1a3e670cea95d51ee865e4efb3556a3a63adef958ea \ + --hash=sha256:eb7e81434c8d223ec4a219b5fc1c47d0417b12be7ea866e24fb5ad6e84b3d988 \ + --hash=sha256:ed0cace939114f62738d808fdcecd4c869222507e266e574799e9c0faa17d486 \ + --hash=sha256:eed63d3b4d62449571547b60578c5b2c4bcccc5387148db46e0c2313dad0ee00 \ + --hash=sha256:fd04ef36b4a6d599bbdb225dd1d3f51e00105f6d48a28f006da7f9822f2606d8 diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index f10f623..85ca4f5 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -977,8 +977,8 @@ # the same commit built a working exe there and a crashing one here. "label": "release: a workflow installs the freezer unpinned again", "file": ".github/workflows/ci.yml", - "old": " pip install -r requirements.txt -r requirements-build.txt", - "new": " pip install -r requirements.txt pyinstaller", + "old": " pip install -r requirements-build.txt", + "new": " pip install pyinstaller", "test": "test_both_workflows_install_the_same_pinned_builder", }, { diff --git a/tests/test_version_and_release.py b/tests/test_version_and_release.py index aebfe1e..b0d0b04 100644 --- a/tests/test_version_and_release.py +++ b/tests/test_version_and_release.py @@ -518,3 +518,72 @@ def test_the_release_attests_exactly_the_archive_it_publishes(): for action in ("actions/attest@", "actions/attest-build-provenance@"): check(f"{action} is still in the release workflow", action in text) + + +def test_every_pinned_runtime_requirement_carries_its_artefact_hashes(): + """A version pins a NUMBER. Hashes pin the BYTES. + + `pydivert==3.1.3` says which release to fetch, and says nothing about what + comes back: an index or a publishing account that has been taken over can + serve different bytes under the same version, and this particular wheel + carries the WinDivert kernel driver that gets installed on a user's machine. + With hashes present pip refuses anything that does not match. + + Measured while writing this (2026-08-19), because the failure mode is not the + obvious one: corrupting the hash of ONE artefact does not fail the install - + pip falls back to another artefact of the same version, which is why every + artefact PyPI published for that version is listed. Corrupting them all is + what produces "THESE PACKAGES DO NOT MATCH THE HASHES" and exit 1. + + This runs offline, so it checks the SHAPE rather than the values: the file + that ships cannot quietly lose its hashes. Regenerate with + `python tools/pin_hashes.py requirements.txt`. + """ + import re + path = os.path.join(ROOT, "requirements.txt") + with open(path, encoding="utf-8") as handle: + lines = handle.read().splitlines() + + pinned = {} + current = None + for line in lines: + stripped = line.strip() + if not stripped or stripped.startswith("#"): + continue + if stripped.startswith("--hash="): + check("a hash line follows a requirement", current is not None, f"({stripped[:40]})") + digest = stripped[len("--hash="):].rstrip(" \\") + check(f"{current}: sha256 in the shape pip reads", + re.fullmatch(r"sha256:[0-9a-f]{64}", digest) is not None, f"({digest[:24]})") + pinned[current].append(digest) + continue + match = re.match(r"^([A-Za-z0-9._-]+)==", stripped) + check(f"every requirement is pinned with == ({stripped[:40]})", match is not None) + current = match.group(1) if match else None + pinned[current] = [] + + check("the file still names its requirements", len(pinned) >= 2, f"({sorted(pinned)})") + bare = [name for name, hashes in pinned.items() if not hashes] + check("every pinned requirement carries at least one hash", not bare, f"({bare})") + # More than one, or pip's fallback to another artefact of the same version + # would be an unhashed path back in through the front door. + thin = [name for name, hashes in pinned.items() if len(hashes) < 2] + check("each names every artefact, not just the one this machine picks", + not thin, f"({thin})") + + +def test_the_dev_requirements_do_not_pull_in_the_hashed_file(): + """The two cannot share one `pip install`, and the reason is pip's, not ours. + + Hash-checking is turned on for the WHOLE install as soon as one requirement + carries a hash. `requirements-dev.txt` deliberately tracks latest - that is + what the weekly run watches - so including the hashed runtime file would + demand hashes for pytest, hypothesis and everything underneath them. + """ + with open(os.path.join(ROOT, "requirements-dev.txt"), encoding="utf-8") as handle: + text = handle.read() + lines = [ln.strip() for ln in text.splitlines() + if ln.strip() and not ln.strip().startswith("#")] + check("requirements-dev.txt does not include requirements.txt", + not any(ln.startswith("-r requirements.txt") for ln in lines), f"({lines[:3]})") + check("it still lists the test tooling", any("pytest" in ln for ln in lines), f"({lines})") diff --git a/tools/pin_hashes.py b/tools/pin_hashes.py new file mode 100644 index 0000000..ecbeb0f --- /dev/null +++ b/tools/pin_hashes.py @@ -0,0 +1,110 @@ +#!/usr/bin/env python3 +"""Write the artefact hashes for a pinned requirements file. + + python tools/pin_hashes.py requirements.txt # rewrite it in place + python tools/pin_hashes.py --print requirements.txt # show, change nothing + +Why the file needs them +----------------------- +`pydivert==3.1.3` pins a NUMBER. It does not pin the bytes: an attacker who +reaches the index - or the account that publishes there - can serve a different +artefact under the same version, and every build afterwards is theirs. That is +not a hypothetical for this project, because the wheel it names carries the +WinDivert kernel driver we install on a user's machine. + +With hashes present pip switches to hash-checking mode and refuses anything that +does not match, so a swapped artefact fails the build instead of shipping. + +What this does +-------------- +Reads every `name==version` line, asks the PyPI JSON API for EVERY artefact of +that exact version, and writes them all as `--hash=sha256:` continuation lines. +All of them, not just the one this machine would pick: the same file installs on +Windows and Linux runners, and each picks a different wheel. + +Two things it deliberately does not do: + +* it never invents a version. The version is the pin, and the pin is a decision + made by a person - this only records what that decision resolves to; +* it does not touch a requirement without `==`. A file that is meant to track + latest (`requirements-dev.txt`) cannot be hash-pinned, and quietly freezing it + would break the weekly run that exists to watch that drift. +""" +import argparse +import io +import json +import re +import sys +import urllib.request + +API = "https://pypi.org/pypi/%s/%s/json" +PINNED = re.compile(r"^(?P[A-Za-z0-9._-]+)==(?P[^\s;\\]+)(?P.*)$") + + +def artefact_hashes(name, version, timeout=30): + """Every sha256 on PyPI for that exact version, newest artefact last.""" + with urllib.request.urlopen(API % (name, version), timeout=timeout) as response: + payload = json.load(response) + digests = [f["digests"]["sha256"] for f in payload.get("urls", [])] + if not digests: + raise SystemExit(f"pin_hashes: {name}=={version} has no artefacts on PyPI") + return sorted(set(digests)) + + +def rewrite(text, fetch=artefact_hashes): + """The file with a fresh hash block under every pinned requirement.""" + out, skipped = [], [] + lines = text.split("\n") + index = 0 + while index < len(lines): + line = lines[index] + index += 1 + match = PINNED.match(line.strip()) + if not match or line.startswith((" ", "\t", "#")): + # Not a pinned requirement: a comment, a blank, an `-r` include, or a + # continuation line from a previous run (dropped and rebuilt below). + if line.strip().startswith("--hash="): + continue + out.append(line) + continue + # Drop a stale hash block that followed this requirement. + while index < len(lines) and lines[index].strip().startswith("--hash="): + index += 1 + name, version = match.group("name"), match.group("version") + rest = match.group("rest").rstrip().rstrip("\\").rstrip() + digests = fetch(name, version) + skipped.append((name, version, len(digests))) + head = f"{name}=={version}{rest}" + out.append(head + " \\") + for position, digest in enumerate(digests): + tail = " \\" if position < len(digests) - 1 else "" + out.append(f" --hash=sha256:{digest}{tail}") + return "\n".join(out), skipped + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("path", help="the requirements file to rewrite") + parser.add_argument("--print", dest="show", action="store_true", + help="write nothing, print the result instead") + args = parser.parse_args(argv) + + with io.open(args.path, encoding="utf-8", newline="") as handle: + original = handle.read() + ending = "\r\n" if "\r\n" in original else "\n" + text, pinned = rewrite(original.replace("\r\n", "\n")) + if not pinned: + print(f"pin_hashes: nothing pinned with == in {args.path}", file=sys.stderr) + return 1 + if args.show: + print(text) + else: + with io.open(args.path, "w", encoding="utf-8", newline="") as handle: + handle.write(text.replace("\n", ending)) + for name, version, count in pinned: + print(f"{name}=={version}: {count} artefact hash(es)") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From e5e49befbbf8e0283481457ebd6bb45be3b171c6 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 08:47:39 +0200 Subject: [PATCH 11/22] feat(build): pin the freezer's whole chain, not just the freezer Pinning pyinstaller alone left seven packages free to move underneath it, one of which - pyinstaller-hooks-contrib - ships monthly and decides which files end up inside the bundle handed to users. A pin that stops at the top of the tree pins the name of the tool, not the tool. requirements-build.txt now carries the closure pip resolved on Windows, which is the only platform that installs it, with artefact hashes for every version. Verified by downloading the whole set under --require-hashes: 7 packages, exit 0. With both requirement files hashed, the split from the previous commit merges back into one hash-checked resolution in the build job and in the release, so a release cannot be built from a different set of bytes than the one CI proved. The parity guard was rewritten rather than patched: it asserted that the file pins exactly one package, which was right while that was true and would now forbid the closure it is meant to protect. It checks that every entry is pinned with == and that the freezer is still among them. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 9 +++--- .github/workflows/release.yml | 9 +++--- requirements-build.txt | 52 ++++++++++++++++++++++++++++++- tests/test_mutation_registry.py | 4 +-- tests/test_version_and_release.py | 14 ++++++--- 5 files changed, 71 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 93b901f..0cab558 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -313,11 +313,10 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - # Two commands, not one: the runtime file is hash-checked and the - # build file is not (yet), and pip applies hash-checking to an - # entire install once any requirement in it carries a hash. - pip install --require-hashes -r requirements.txt - pip install -r requirements-build.txt + # One hash-checked resolution over both files: the runtime pins and + # the freezer's whole closure. Nothing here may resolve to bytes + # that are not written down. + pip install --require-hashes -r requirements.txt -r requirements-build.txt # onedir + console subsystem + asInvoker - see BeanNetworkTester.spec - name: Build diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 8e77e4e..3713ed7 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -46,11 +46,10 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - # Two commands: hash-checked runtime, then the freezer. See the same - # split in ci.yml - pip turns hash-checking on for the whole install - # as soon as one requirement carries a hash. - pip install --require-hashes -r requirements.txt - pip install -r requirements-build.txt + # One hash-checked resolution over both files - the same command the + # build job in ci.yml runs, so a release cannot be built from a + # different set of bytes than the one CI proved. + pip install --require-hashes -r requirements.txt -r requirements-build.txt # The version is single-sourced in VERSION.txt. The tag is v for a # final release, or v-rc.N (also -beta.N / -alpha.N) for a pre-release. diff --git a/requirements-build.txt b/requirements-build.txt index a7f4f20..72ca259 100644 --- a/requirements-build.txt +++ b/requirements-build.txt @@ -29,4 +29,54 @@ # # Kept out of requirements-dev.txt on purpose: that file is deliberately unpinned # so pytest and hypothesis track latest, and this one must not. -pyinstaller==6.22.1 + +# 🔴 THE WHOLE CHAIN, not just the freezer, and with artefact hashes. +# +# Pinning `pyinstaller` alone left seven other packages free to move under it - +# including `pyinstaller-hooks-contrib`, which ships monthly and decides which +# files end up inside the bundle we hand to users. A pin that stops at the top of +# the tree is a pin on the name of the tool, not on the tool. +# +# The versions below are the closure pip resolved on Windows (`pip install +# --dry-run --report`, 2026-08-19), which is the only platform that installs this +# file: both jobs that read it run on windows-latest. The hashes cover every +# artefact PyPI published for each version, because pip picks per platform and +# falls back to a source distribution when a wheel's hash does not match - so a +# list with holes in it is a list with a way in. +# +# Regenerate after ANY bump here: +# python -m pip install --dry-run --ignore-installed --report r.json -r requirements-build.txt +# python tools/pin_hashes.py requirements-build.txt +# +# And then do what the note above already says: build the exe and launch it. +pyinstaller==6.22.1 \ + --hash=sha256:0f257d6329d90def6b96f3ad9b601f4e10e549017d2ad09e730f4a9741b42138 \ + --hash=sha256:10d009ac9ddf17b57dbfe22a453363a02be4cb0d5bd8a119ba59ef530b936296 \ + --hash=sha256:1df884760a6efc4cc9bdcd48899f1382c072ab499bb16ed0b03606f8f10380c6 \ + --hash=sha256:3b654721a9fa13abdd8a3a8e266debf750a6f50115d17addc54e3269dc944e75 \ + --hash=sha256:4e7ed495fccb9974d47cf72ef8cffc92afa05d60bc265c5585a68f3d229ca8d1 \ + --hash=sha256:9fe5d028023073b63c3e396e145c68a9c3382ab59889c749c4c248126d313945 \ + --hash=sha256:bb7a405ef0fbea9b20b7312484d2766d2aa157026a0406a8e7ce63a573748eae \ + --hash=sha256:bc964609e73f32f79b4c967d1c29a05f913629fcf54b3752ec6ec82bf17cffdd \ + --hash=sha256:bf5f4634b18add9381caa73bd1656a1f0736839aa41ca750fbd511116d5ae2ca \ + --hash=sha256:c5d7859b59c17f5c28973eb683e2236abb7a6eed1a7f354dc7f65ee3a7dff8ab \ + --hash=sha256:d519a5549bf560407a9cffa8547f278e79c1093dc1cade6d9658c67b650d66c4 \ + --hash=sha256:ff3c51c1a3b793d1917f75d0f5f31ef8bd889c0accc8478d604bbcc363fcb74b +altgraph==0.17.5 \ + --hash=sha256:c87b395dd12fabde9c99573a9749d67da8d29ef9de0125c7f536699b4a9bc9e7 \ + --hash=sha256:f3a22400bce1b0c701683820ac4f3b159cd301acab067c51c653e06961600597 +packaging==26.3 \ + --hash=sha256:94edc256424af38762eb31306eed28beb9f0efc50a8837492c9d6fd6004aed79 \ + --hash=sha256:d7193f7c8e4e93f444fde0262bf90af30e16fa0ad0ad44cb553c87339b23cd1c +pefile==2024.8.26 \ + --hash=sha256:3ff6c5d8b43e8c37bb6e6dd5085658d658a7a0bdcd20b6a07b1fcfc1c4e9d632 \ + --hash=sha256:76f8b485dcd3b1bb8166f1128d395fa3d87af26360c2358fb75b80019b957c6f +pyinstaller-hooks-contrib==2026.6 \ + --hash=sha256:bef5002c32f4f50bd55b005da12cff64eca8783e7eaf86a06a62410164bab725 \ + --hash=sha256:fd13b8ac126b35361175edacd41a0d97080b75dd5f4b594ecefefff969509dd3 +pywin32-ctypes==0.2.3 \ + --hash=sha256:8a1513379d709975552d202d942d9837758905c8d01eb82b8bcc30918929e7b8 \ + --hash=sha256:d162dc04946d704503b2edc4d55f3dba5c1d539ead017afa00142c38b9885755 +setuptools==84.0.0 \ + --hash=sha256:51a52592b3b99e102b609654876bd65f19f999935166d1352678931132b0c670 \ + --hash=sha256:f4695c21257f0d9b537ec2692c941d02ee143b7cc1276941349a546573b2ef73 diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 85ca4f5..01477b6 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -977,8 +977,8 @@ # the same commit built a working exe there and a crashing one here. "label": "release: a workflow installs the freezer unpinned again", "file": ".github/workflows/ci.yml", - "old": " pip install -r requirements-build.txt", - "new": " pip install pyinstaller", + "old": " pip install --require-hashes -r requirements.txt -r requirements-build.txt", + "new": " pip install -r requirements.txt pyinstaller", "test": "test_both_workflows_install_the_same_pinned_builder", }, { diff --git a/tests/test_version_and_release.py b/tests/test_version_and_release.py index b0d0b04..f36edb7 100644 --- a/tests/test_version_and_release.py +++ b/tests/test_version_and_release.py @@ -427,10 +427,16 @@ def test_both_workflows_install_the_same_pinned_builder(): pin_file = "requirements-build.txt" with open(os.path.join(ROOT, pin_file), encoding="utf-8") as f: pins = [ln.strip() for ln in f - if ln.strip() and not ln.lstrip().startswith("#")] - check(f"{pin_file} pins exactly one package", len(pins) == 1, f"({pins})") - check(f"{pin_file} pins it with == ", bool(re.match(r"^pyinstaller==\d", pins[0])), - f"({pins[0]!r} - a range or a bare name is not a pin)") + if ln.strip() and not ln.lstrip().startswith("#") + and not ln.strip().startswith("--hash=")] + # Since 2026-08-19 this file pins the whole CLOSURE, not just the freezer: + # pinning the top of the tree left seven packages free to move underneath + # it, one of which decides what goes inside the bundle and ships monthly. + unpinned = [p for p in pins if not re.match(r"^[A-Za-z0-9._-]+==\d", p.rstrip(" \\"))] + check(f"{pin_file}: every package in the closure is pinned with ==", + not unpinned, f"({unpinned} - a range or a bare name is not a pin)") + check(f"{pin_file}: the freezer itself is still pinned there", + any(re.match(r"^pyinstaller==\d", p) for p in pins), f"({pins[:3]})") for path in ("ci.yml", "release.yml"): with open(os.path.join(ROOT, ".github", "workflows", path), From 1af312757b0d51ff555cb435c371fb3907a7af8f Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 08:57:59 +0200 Subject: [PATCH 12/22] ci: audit the pinned dependencies weekly, and open an issue on a finding This asks a different question from dependency-review. That one looks at what a pull request adds. This one asks whether an advisory has been published since, against versions that have not moved - which no pull request can make red, so it runs on the schedule and on demand rather than per PR. It audits an installed environment rather than the requirement files, and that is measured rather than assumed: pip-audit -r over both files reports on 7 of the 9 pinned packages and prints "No known vulnerabilities found", silently skipping packaging and setuptools. The same set installed into a venv and audited with --path comes back with 10. A scanner that quietly covers less than it was asked to is worse than no scanner, because its clean report is the one that gets quoted. It runs on Windows, not Linux: pydivert carries a win32 marker, so a Linux runner would skip the one dependency that puts a kernel driver on a user's machine. Opening an issue reverses the decision of 2026-08-17 on purpose, and only for this job. A red cron usually means drift, and drift is read when somebody looks. A published advisory has a clock on it and does not resolve itself. The title carries the advisory ids, so the same finding cannot open fifty-two issues a year and a new advisory arriving while the old issue is open is not swallowed by it. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 67 +++++++++++++++++++ requirements-lint.txt | 13 ++++ tests/test_audit_issue.py | 99 ++++++++++++++++++++++++++++ tools/audit_issue.py | 132 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 311 insertions(+) create mode 100644 tests/test_audit_issue.py create mode 100644 tools/audit_issue.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0cab558..6195b4a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -158,6 +158,73 @@ jobs: - name: Decide run: python tools/semgrep_gate.py semgrep.json + # Weekly, on Windows, and only weekly: this asks whether the world changed + # under versions that did not. A pull request cannot make it red, so running it + # per pull request would burn minutes to re-answer the same question. + # + # windows-latest because that is where the shipped set actually exists: + # `pydivert` carries a `sys_platform == "win32"` marker, so a Linux runner would + # skip the one dependency that puts a kernel driver on a user's machine. + audit: + name: pip-audit (advisories against the pinned set) + if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' + runs-on: windows-latest + timeout-minutes: 15 + permissions: + contents: read + # Raised on THIS job only, and only because it opens an issue when it finds + # something. Nothing else in this workflow may write anything. + issues: write + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.14" + - name: Install the auditor + run: pip install -r requirements-lint.txt + # The pinned set, installed exactly as the build installs it, into its own + # environment - then audited BY PATH. Auditing the requirement files instead + # silently skips `packaging` and `setuptools` (measured), and a scanner that + # covers less than it was asked to is worse than no scanner at all. + - name: Install the pinned set into a clean environment + shell: bash + run: | + python -m venv audit-env + audit-env/Scripts/python -m pip install --quiet \ + --require-hashes -r requirements.txt -r requirements-build.txt + - name: Audit it + shell: bash + # pip-audit exits non-zero when it finds something, and finding something + # is what the next step is for - so the exit code is captured, not obeyed. + run: | + set +e + pip-audit --path audit-env/Lib/site-packages -f json -o audit.json + echo "pip-audit exit: $?" + set -e + test -s audit.json || { echo "no report was written"; exit 1; } + - name: Open an issue if anything was found + shell: bash + env: + GH_TOKEN: ${{ github.token }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + gh issue list --label security-scan --state open --json number,title > open.json + action=$(python tools/audit_issue.py --audit audit.json --existing open.json \ + --title-out issue-title.txt --body-out issue-body.md \ + --run-url "$RUN_URL") + echo "verdict: $action" + case "$action" in + create) + gh label create security-scan --color B60205 \ + --description "opened by the weekly dependency audit" --force + gh issue create --label security-scan \ + --title "$(cat issue-title.txt)" --body-file issue-body.md + ;; + skip) echo "an open issue already names exactly these advisories" ;; + none) echo "nothing published against the pinned set" ;; + *) echo "unexpected verdict from tools/audit_issue.py"; exit 1 ;; + esac + tests: name: tests (${{ matrix.os }}, py${{ matrix.python-version }}) runs-on: ${{ matrix.os }} diff --git a/requirements-lint.txt b/requirements-lint.txt index 02a40d2..5c12cc5 100644 --- a/requirements-lint.txt +++ b/requirements-lint.txt @@ -33,3 +33,16 @@ mypy==2.3.1 # own purposes and forbids redistributing the rules, which is what putting them in # a public repository would be. semgrep==1.173.0; sys_platform != "win32" + +# The weekly audit of what we PIN. Not the same question as dependency-review, +# which looks at what a pull request ADDS: this one asks whether an advisory has +# been published since, against versions that have not moved. +# +# 🔴 Run against an INSTALLED environment (`--path`), never against the +# requirement files. Measured 2026-08-19: `pip-audit -r requirements.txt -r +# requirements-build.txt --require-hashes` audits 7 of the 9 pinned packages and +# says "No known vulnerabilities found" - it silently skips `packaging` and +# `setuptools`. The same set installed into a venv and audited by path comes back +# with 10 (those two, plus pip itself). A scanner that quietly covers less than it +# was asked to is worse than none, because its clean report gets quoted. +pip-audit==2.10.1 diff --git a/tests/test_audit_issue.py b/tests/test_audit_issue.py new file mode 100644 index 0000000..e5c1ac8 --- /dev/null +++ b/tests/test_audit_issue.py @@ -0,0 +1,99 @@ +"""The weekly dependency audit: when it opens an issue, and when it stays quiet. + +The decision this file guards is not "is there a vulnerability" - pip-audit +answers that - but what the repository does about it. Opening an issue every +Monday for a finding somebody already read would train everyone to close them +unread, and swallowing a NEW advisory into an old issue would lose it. So the +title carries the advisory ids, and matching is exact. +""" +import json +import os +import sys + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "tools")) + +import audit_issue # noqa: E402 +from fakes import check # noqa: E402 + + +def _report(*rows): + """A pip-audit JSON report: (name, version, [(id, [fixes]), ...]).""" + return {"dependencies": [ + {"name": name, "version": version, + "vulns": [{"id": vid, "fix_versions": fixes} for vid, fixes in vulns]} + for name, version, vulns in rows]} + + +def _write(tmp_path, name, payload): + path = tmp_path / name + path.write_text(json.dumps(payload), encoding="utf-8") + return str(path) + + +def test_a_clean_audit_says_nothing(tmp_path, capsys): + code = audit_issue.main(["--audit", _write(tmp_path, "a.json", + _report(("psutil", "7.2.2", [])))]) + check("a clean report exits 0", code == 0, f"(exit {code})") + check("and asks for nothing", capsys.readouterr().out.strip() == "none") + + +def test_a_finding_asks_for_an_issue(tmp_path, capsys): + report = _report(("psutil", "7.2.2", [("GHSA-aaaa", ["7.2.3"])])) + code = audit_issue.main(["--audit", _write(tmp_path, "a.json", report), + "--title-out", str(tmp_path / "t.txt"), + "--body-out", str(tmp_path / "b.md")]) + check("it exits 0", code == 0, f"(exit {code})") + check("and asks for an issue", capsys.readouterr().out.strip() == "create") + title = (tmp_path / "t.txt").read_text(encoding="utf-8") + body = (tmp_path / "b.md").read_text(encoding="utf-8") + check("the title names the package and the advisory", + "psutil" in title and "GHSA-aaaa" in title, f"({title})") + for expected in ("7.2.2", "GHSA-aaaa", "7.2.3", "pin_hashes.py"): + check(f"the body carries {expected!r}", expected in body) + + +def test_a_finding_with_no_fix_says_so(tmp_path): + report = _report(("pydivert", "3.1.3", [("PYSEC-1", [])])) + audit_issue.main(["--audit", _write(tmp_path, "a.json", report), + "--body-out", str(tmp_path / "b.md")]) + body = (tmp_path / "b.md").read_text(encoding="utf-8") + check("an advisory without a fix is not left blank", "no fix yet" in body, f"({body[:200]})") + + +def test_the_same_finding_does_not_open_a_second_issue(tmp_path, capsys): + report = _report(("psutil", "7.2.2", [("GHSA-aaaa", ["7.2.3"])])) + title = audit_issue.title_for(audit_issue.findings(report)) + existing = _write(tmp_path, "open.json", [{"number": 7, "title": title}]) + code = audit_issue.main(["--audit", _write(tmp_path, "a.json", report), + "--existing", existing]) + check("it exits 0", code == 0, f"(exit {code})") + check("and skips, because the issue is already open", + capsys.readouterr().out.strip() == "skip") + + +def test_a_new_advisory_is_not_swallowed_by_the_open_issue(tmp_path, capsys): + """The failure that matters more than the duplicate: a second advisory + arriving while the first issue is still open.""" + old = _report(("psutil", "7.2.2", [("GHSA-aaaa", ["7.2.3"])])) + new = _report(("psutil", "7.2.2", [("GHSA-aaaa", ["7.2.3"]), ("GHSA-bbbb", [])])) + existing = _write(tmp_path, "open.json", + [{"number": 7, "title": audit_issue.title_for(audit_issue.findings(old))}]) + code = audit_issue.main(["--audit", _write(tmp_path, "a.json", new), + "--existing", existing, + "--title-out", str(tmp_path / "t.txt"), + "--body-out", str(tmp_path / "b.md")]) + check("it exits 0", code == 0, f"(exit {code})") + check("a new advisory opens its own issue", + capsys.readouterr().out.strip() == "create") + check("and the title carries both ids", + "GHSA-bbbb" in (tmp_path / "t.txt").read_text(encoding="utf-8")) + + +def test_an_unreadable_report_is_not_a_clean_one(tmp_path, capsys): + missing = audit_issue.main(["--audit", str(tmp_path / "nope.json")]) + check("a missing report fails loudly", missing == 2, f"(exit {missing})") + broken = tmp_path / "broken.json" + broken.write_text("{not json", encoding="utf-8") + code = audit_issue.main(["--audit", str(broken)]) + check("an unparsable report fails loudly", code == 2, f"(exit {code})") + capsys.readouterr() diff --git a/tools/audit_issue.py b/tools/audit_issue.py new file mode 100644 index 0000000..25f2ddc --- /dev/null +++ b/tools/audit_issue.py @@ -0,0 +1,132 @@ +#!/usr/bin/env python3 +"""Turn a pip-audit report into an issue - once per set of advisories. + + pip-audit --path -f json -o audit.json + gh issue list --label security-scan --state open --json number,title > open.json + python tools/audit_issue.py --audit audit.json --existing open.json \\ + --title-out title.txt --body-out body.md + +Prints one word for the workflow to act on: + + none nothing vulnerable in the audited set + create something is, and no open issue already says so + skip something is, and an open issue already says exactly that + +Why an issue at all +------------------- +The weekly run deliberately does NOT open an issue when it goes red - that was +decided on 2026-08-17, and the reasoning holds: a red cron usually means +something drifted, and drift is read when somebody looks. A published advisory +against a version we ship is a different animal. It has a clock on it, it does +not resolve itself, and the person who needs to see it is not necessarily +looking at Actions that week. + +Why the title carries the advisory ids +-------------------------------------- +So the same finding cannot open fifty-two issues a year, and a NEW finding is +not swallowed by an old one. Matching on the package alone would do the second; +matching on nothing would do the first. +""" +import argparse +import json +import sys + + +def findings(report): + """``(name, version, [advisory ...])`` for every vulnerable dependency.""" + out = [] + for dependency in report.get("dependencies") or []: + vulns = dependency.get("vulns") or [] + if not vulns: + continue + out.append((dependency.get("name", "?"), dependency.get("version", "?"), + sorted(vulns, key=lambda v: str(v.get("id", ""))))) + return sorted(out) + + +def advisory_ids(rows): + ids = {str(v.get("id", "?")) for _name, _version, vulns in rows for v in vulns} + return sorted(ids) + + +def title_for(rows): + ids = advisory_ids(rows) + packages = sorted({name for name, _v, _x in rows}) + return "Vulnerable dependency: %s (%s)" % (", ".join(packages), ", ".join(ids)) + + +def body_for(rows, run_url=""): + lines = ["`pip-audit` found published advisories against versions this repository pins.", + "", + "| package | pinned version | advisory | fixed in |", + "|---|---|---|---|"] + for name, version, vulns in rows: + for vuln in vulns: + fixed = ", ".join(str(f) for f in (vuln.get("fix_versions") or [])) or "no fix yet" + lines.append("| `%s` | %s | %s | %s |" + % (name, version, vuln.get("id", "?"), fixed)) + lines += ["", + "The versions are pinned with hashes, so a fix means editing the pin and", + "regenerating the hashes:", + "", + "```", + "python tools/pin_hashes.py requirements.txt", + "python tools/pin_hashes.py requirements-build.txt", + "```", + ""] + if run_url: + lines += ["Found by " + run_url, ""] + lines.append("This issue was opened by the weekly dependency audit. Closing it without a") + lines.append("pin change means deciding the advisory does not apply - say so in a comment,") + lines.append("because the next run will open it again otherwise.") + return "\n".join(lines) + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--audit", required=True, help="pip-audit JSON report") + parser.add_argument("--existing", help="`gh issue list --json number,title` output") + parser.add_argument("--title-out", help="write the issue title here") + parser.add_argument("--body-out", help="write the issue body here") + parser.add_argument("--run-url", default="", help="link back to the workflow run") + args = parser.parse_args(argv) + + try: + with open(args.audit, encoding="utf-8") as handle: + report = json.load(handle) + except (OSError, ValueError) as exc: + # Same rule as the semgrep gate: a report that cannot be read is not a + # clean report. Fail loudly rather than reporting "none". + print("audit_issue: cannot read %s: %s" % (args.audit, exc), file=sys.stderr) + return 2 + + rows = findings(report) + if not rows: + print("none") + return 0 + + title = title_for(rows) + existing = [] + if args.existing: + try: + with open(args.existing, encoding="utf-8") as handle: + existing = json.load(handle) or [] + except (OSError, ValueError) as exc: + print("audit_issue: cannot read %s: %s" % (args.existing, exc), file=sys.stderr) + return 2 + if any(str(issue.get("title", "")).strip() == title for issue in existing): + print("skip") + return 0 + + if args.title_out: + with open(args.title_out, "w", encoding="utf-8") as handle: + handle.write(title) + if args.body_out: + with open(args.body_out, "w", encoding="utf-8") as handle: + handle.write(body_for(rows, args.run_url)) + print("create") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From 5ddc177013618461629173d9f93a080c2bb69a49 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 09:00:57 +0200 Subject: [PATCH 13/22] ci: let an outside grader look at the supply chain, weekly The three analysers in ci.yml read the code. OpenSSF Scorecard reads the project: whether actions are pinned, whether tokens are least-privilege, whether releases are signed, whether a branch is protected, whether dangerous workflow patterns are present. It is the only check here that can name something we did not think to test, and it says it in a form a stranger can read before installing a tool that loads a kernel driver. Nothing it finds fails a pull request. It reports into the Security tab as SARIF and keeps the full grading as an artefact, including the checks that passed. persist-credentials: false on the checkout, because the analyser walks repository data and has no business holding a token it could push with. publish_results: true is what turns the score into a badge, and id-token: write is what makes the badge mean anything - without the OIDC proof anybody could publish a score for any repository. It also runs on branch_protection_rule, since protection is one of the things it grades. Co-Authored-By: Claude Opus 5 --- .github/workflows/scorecard.yml | 75 +++++++++++++++++++++++++++++++++ README.md | 1 + README.pl.md | 1 + 3 files changed, 77 insertions(+) create mode 100644 .github/workflows/scorecard.yml diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml new file mode 100644 index 0000000..9f60078 --- /dev/null +++ b/.github/workflows/scorecard.yml @@ -0,0 +1,75 @@ +# OpenSSF Scorecard: an OUTSIDE opinion on this repository's supply chain. +# +# Why it is worth a workflow when CI already runs three static analysers: those +# read the code, and this reads the PROJECT - whether actions are pinned, whether +# tokens are least-privilege, whether releases are signed, whether a branch is +# protected, whether dangerous workflow patterns are present. It is the only check +# here that can tell us something we did not already think to test, and it says it +# in a form a stranger can read before installing a tool that loads a kernel +# driver. +# +# Nothing here can fail a pull request. It reports. +name: Scorecard + +on: + # The default branch only. Scorecard grades a repository, not a change, and its + # own documentation marks the pull_request trigger experimental. + push: + branches: [master] + schedule: + # Monday 07:00 UTC - an hour after the CI cron, so the two do not queue behind + # each other on the same runner pool. + - cron: "0 7 * * 1" + # Branch protection is one of the things it grades, so a change to it should be + # re-graded rather than waiting a week. + branch_protection_rule: + workflow_dispatch: + +# Read-all at the top, and the job below raises exactly what it needs. Same rule +# as ci.yml: a permission is granted on the job that uses it, never globally. +permissions: read-all + +jobs: + analysis: + name: OpenSSF Scorecard + runs-on: ubuntu-latest + timeout-minutes: 15 + permissions: + # Publishes the SARIF into the Security tab, next to CodeQL's own findings. + security-events: write + # Mints the OIDC token that lets the OpenSSF API verify the results really + # came from this repository. This is what makes the badge mean anything - + # without it anyone could publish a score for any repository. + id-token: write + # Scorecard reads workflow run data to grade CI practices. + actions: read + contents: read + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # The analyser must never be handed a token it could use: it walks + # repository data, and it has no business being able to push. + persist-credentials: false + + - name: Run the analysis + uses: ossf/scorecard-action@2d1146689b8cda280b9bc96326124645441f03bc # v2.4.4 + with: + results_file: results.sarif + results_format: sarif + # Publishing is what turns the score into a badge, and it is public + # information about a public repository either way. + publish_results: true + + # Kept as an artefact as well as uploaded: the Security tab shows the + # findings, the file shows the whole grading, including the checks that + # passed. + - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: scorecard-results + path: results.sarif + retention-days: 5 + + - name: Upload to the Security tab + uses: github/codeql-action/upload-sarif@ff2f1c621b7f889edc0d3c761ac2e6a3f8cdb0dd # v4.37.7 + with: + sarif_file: results.sarif diff --git a/README.md b/README.md index 6556590..4b487b7 100644 --- a/README.md +++ b/README.md @@ -4,6 +4,7 @@ [![Latest release](https://img.shields.io/github/v/release/donislawdev/BeanNetworkTester?sort=semver)](https://github.com/donislawdev/BeanNetworkTester/releases/latest) [![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) ![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 c867ea0..c5a44eb 100644 --- a/README.pl.md +++ b/README.pl.md @@ -4,6 +4,7 @@ [![Latest release](https://img.shields.io/github/v/release/donislawdev/BeanNetworkTester?sort=semver)](https://github.com/donislawdev/BeanNetworkTester/releases/latest) [![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) ![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 e2291da1a6f3e869db34e6db84ead9ee8069dfc4 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 09:11:12 +0200 Subject: [PATCH 14/22] feat(ci): block a dependency nobody can license, and pin the driver's bytes The dependency-review action stays where it is good - known vulnerabilities - and a second gate reads the same API for the half it cannot do. Its own documentation: "If we can't detect the license for a dependency we will inform you, but the action won't fail." For a GPL-3.0 project that ships a binary, an unidentified licence is the one answer nobody can act on, so a null licence blocks exactly like a denied one. The allowlist is SPDX ids a GPL-3.0 project may distribute alongside. GPL-2.0-only is deliberately absent: it is incompatible with GPL-3.0 and is exactly the kind of entry that looks fine in a list of open source licences. A compound expression is judged by its worst half, because being generous with an OR means accepting the worse one. Only what the pull request adds is judged, and a gate that cannot read its input exits 2 rather than reporting an empty list of problems. Separately, the WinDivert driver is now pinned by its bytes. WINDIVERT_VERSION answers which driver the notices describe, and the existing test proves the driver agrees - but neither notices a file changing while the version still reads 2.2, which is what a swapped kernel driver looks like. The wheel is already hash-pinned, so this is the second line: it catches a file replaced in site-packages after the install, on the machine that builds the release. Both new guards have a mutation and both are caught. Co-Authored-By: Claude Opus 5 --- .github/workflows/dependency-review.yml | 20 ++++ beantester/legal.py | 19 ++++ tests/test_dependency_gate.py | 96 ++++++++++++++++++ tests/test_license_surface.py | 39 ++++++++ tests/test_mutation_registry.py | 18 ++++ tools/dependency_gate.py | 124 ++++++++++++++++++++++++ 6 files changed, 316 insertions(+) create mode 100644 tests/test_dependency_gate.py create mode 100644 tools/dependency_gate.py diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index 774b76a..8e6f84f 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -26,3 +26,23 @@ jobs: # fails the pull request rather than warning in a log nobody reads. fail-on-severity: moderate comment-summary-in-pr: always + + # The half the action above cannot do. Its own documentation: "If we can't + # detect the license for a dependency we will inform you, but the action + # won't fail." For a GPL-3.0 project that ships a binary, an unidentified + # licence is the one answer nobody can act on - so the same data is read + # again here, from the API the action itself uses, and an unknown licence + # blocks exactly like a denied one. + # + # `gh` is preinstalled and uses the job token. The gate exits 2 when it + # cannot read the answer (the endpoint refuses some repository shapes), so + # a 403 fails the run instead of looking like an empty list of problems. + - name: Licences of what this pull request adds + env: + GH_TOKEN: ${{ github.token }} + BASE: ${{ github.event.pull_request.base.sha }} + HEAD: ${{ github.event.pull_request.head.sha }} + REPO: ${{ github.repository }} + run: | + gh api "repos/$REPO/dependency-graph/compare/$BASE...$HEAD" > deps.json + python tools/dependency_gate.py deps.json diff --git a/beantester/legal.py b/beantester/legal.py index 0d15e0b..1475ac6 100644 --- a/beantester/legal.py +++ b/beantester/legal.py @@ -21,6 +21,25 @@ # that source and rebuild it. WINDIVERT_VERSION = "2.2" +# The exact bytes of the two files that version resolves to, as shipped by the +# pinned `pydivert` wheel (sha256, recorded 2026-08-19 from pydivert 3.1.3). +# +# The version above answers "which WinDivert is this" for the licence notices. +# These answer a different question, and it is the one an attacker cares about: +# are these the same bytes the pinned wheel contained. `requirements.txt` pins +# the wheel by hash, so this is defence in depth rather than the first line - +# it catches a driver swapped in `site-packages` AFTER the install, on the +# machine that builds the release. A kernel driver is exactly the file worth +# spending a second guard on. +# +# When the pydivert pin moves: rebuild these from the new wheel, and read the +# version resource again - a driver whose bytes changed without its version +# changing is the case this exists to make loud. +WINDIVERT_SHA256 = { + "WinDivert64.dll": "c1e060ee19444a259b2162f8af0f3fe8c4428a1c6f694dce20de194ac8d7d9a2", + "WinDivert64.sys": "8da085332782708d8767bcace5327a6ec7283c17cfb85e40b03cd2323a90ddc2", +} + # The components we ship, in the order a reader cares about. ``module`` is the # import name used to report the real version at run time (None = not a Python # package, so the version is fixed or reported by other means). diff --git a/tests/test_dependency_gate.py b/tests/test_dependency_gate.py new file mode 100644 index 0000000..7eec10f --- /dev/null +++ b/tests/test_dependency_gate.py @@ -0,0 +1,96 @@ +"""The licence gate: what a new dependency may be licensed under, and what +happens when nobody can tell. + +The official dependency-review action covers vulnerabilities and says so about +licences it cannot resolve: it informs, and does not fail. For a GPL-3.0 project +that ships a binary, "we could not determine the licence" is the one answer +nobody can act on, so it blocks here. +""" +import json +import os +import sys + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "tools")) + +import dependency_gate # noqa: E402 +from fakes import check # noqa: E402 + + +def _dep(name="thing", version="1.0", licence="MIT", change="added", scope="runtime"): + return {"change_type": change, "name": name, "version": version, + "license": licence, "scope": scope, "manifest": "requirements.txt", + "ecosystem": "pip", "vulnerabilities": []} + + +def _write(tmp_path, payload, name="deps.json"): + path = tmp_path / name + path.write_text(json.dumps(payload), encoding="utf-8") + return str(path) + + +def test_a_permissive_licence_passes(): + for licence in ("MIT", "Apache-2.0", "BSD-3-Clause", "MPL-2.0", "LGPL-3.0-or-later"): + blocked, passed = dependency_gate.split([_dep(licence=licence)]) + check(f"{licence} passes", not blocked and len(passed) == 1, + f"(blocked={blocked})") + + +def test_an_unknown_licence_blocks(): + """The case the official action explicitly does not fail on.""" + for licence in (None, "", " "): + blocked, _passed = dependency_gate.split([_dep(licence=licence)]) + check(f"licence={licence!r} blocks", len(blocked) == 1 and blocked[0][0] == "unknown", + f"({blocked})") + + +def test_gpl2_only_blocks_because_it_cannot_be_shipped_with_gpl3(): + """The finding that looks like a false alarm and is not: GPL-2.0-only is + incompatible with this project's own licence.""" + blocked, _passed = dependency_gate.split([_dep(licence="GPL-2.0-only")]) + check("GPL-2.0-only blocks", len(blocked) == 1 and blocked[0][0] == "denied", f"({blocked})") + + +def test_a_compound_expression_is_judged_by_its_worst_half(): + ok, _ = dependency_gate.split([_dep(licence="MIT OR Apache-2.0")]) + check("both halves allowed passes", not ok, f"({ok})") + bad, _ = dependency_gate.split([_dep(licence="MIT OR GPL-2.0-only")]) + check("one bad half blocks", len(bad) == 1, f"({bad})") + + +def test_only_added_dependencies_are_judged(): + """A removed dependency creates no obligation, and re-judging the whole tree + would make every pull request answer for decisions taken years ago.""" + blocked, passed = dependency_gate.split([_dep(licence=None, change="removed")]) + check("a removed dependency is ignored", not blocked and not passed, + f"(blocked={blocked}, passed={passed})") + + +def test_the_exit_code_is_the_verdict(tmp_path): + clean = dependency_gate.main([_write(tmp_path, [_dep()])]) + check("a permissive addition exits 0", clean == 0, f"(exit {clean})") + dirty = dependency_gate.main([_write(tmp_path, [_dep(), _dep(name="mystery", licence=None)])]) + check("an unknown licence exits 1", dirty == 1, f"(exit {dirty})") + + +def test_a_gate_that_cannot_read_its_input_has_not_passed_anything(tmp_path): + """The API answers 403 for some repository shapes. That must look like a + failure, not like an empty list of problems.""" + missing = dependency_gate.main([str(tmp_path / "nope.json")]) + check("a missing file exits 2", missing == 2, f"(exit {missing})") + + broken = tmp_path / "broken.json" + broken.write_text("{not json", encoding="utf-8") + check("unparsable input exits 2", dependency_gate.main([str(broken)]) == 2) + + # A 403 body is a JSON OBJECT with a message - not the list the gate expects. + forbidden = _write(tmp_path, {"message": "Forbidden"}, "403.json") + check("an error object exits 2 rather than passing", + dependency_gate.main([forbidden]) == 2) + + +def test_the_report_names_what_it_blocked_and_why(tmp_path, capsys): + dependency_gate.main([_write(tmp_path, [_dep(name="mystery", licence=None), + _dep(name="fine", licence="MIT")])]) + out = capsys.readouterr().out + for expected in ("mystery", "unknown", "fine", "MIT", "1 blocked, 1 allowed"): + check(f"the report says {expected!r}", expected in out, f"({out[:200]})") diff --git a/tests/test_license_surface.py b/tests/test_license_surface.py index 1a1e386..f4ffcee 100644 --- a/tests/test_license_surface.py +++ b/tests/test_license_surface.py @@ -282,3 +282,42 @@ def test_the_written_offer_lasts_as_long_as_the_licence_demands(): check("notices: the written offer names the three-year floor", "three years" in notices, "(an offer weaker than GPLv3 section 6(b) allows)") + + +def test_the_shipped_driver_is_byte_for_byte_the_one_we_recorded(): + """A version resource is a claim. A hash is not. + + `WINDIVERT_VERSION` says which WinDivert the notices describe, and the test + above proves the driver agrees. Neither notices anything if the FILE changes + while the version stays "2.2" - which is what a swapped driver looks like, + and this one loads into the kernel. + + `requirements.txt` pins the pydivert wheel by hash, so this is the second + line rather than the first: it catches a file replaced in `site-packages` + after the install, on the machine that builds the release. + + Skips where pydivert is absent - it is a Windows-only dependency, so the + Linux runner has nothing to hash. + """ + import hashlib + import os + from beantester import legal as _legal + directory, names = _windivert_binaries() or (None, None) + if not directory: + return # no pydivert here: nothing to check + import pydivert + root = os.path.dirname(os.path.dirname(os.path.abspath(pydivert.__file__))) + recorded = _legal.WINDIVERT_SHA256 + + check("the registry records a hash for every file it ships", + set(recorded) == set(names), + f"(recorded {sorted(recorded)}, shipped {sorted(names)})") + + for name in sorted(names): + path = os.path.join(root, directory.replace("/", os.sep), name) + with open(path, "rb") as handle: + digest = hashlib.sha256(handle.read()).hexdigest() + check(f"{name} is the file we recorded", + digest == recorded.get(name), + f"(got {digest[:16]}..., recorded {str(recorded.get(name))[:16]}... - " + f"if the pydivert pin moved, re-record it deliberately)") diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 01477b6..39e3353 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -1071,6 +1071,24 @@ "new": ".hexdigest()", "test": "test_different_faults_get_different_fingerprints", }, + { + # One byte of the recorded driver hash. The version resource still reads + # 2.2 - which is exactly what a swapped kernel driver looks like. + "label": "legal: the recorded WinDivert driver hash stops matching", + "file": "beantester/legal.py", + "old": "8da085332782708d8767bcace5327a6ec7283c17cfb85e40b03cd2323a90ddc2", + "new": "0da085332782708d8767bcace5327a6ec7283c17cfb85e40b03cd2323a90ddc2", + "test": "test_the_shipped_driver_is_byte_for_byte_the_one_we_recorded", + }, + { + # The line that makes an undetectable licence block. Without it this gate + # agrees with the official action: informs, and passes. + "label": "deps: an undetectable licence stops blocking", + "file": "tools/dependency_gate.py", + "old": " if licence is None or not str(licence).strip():", + "new": " if False and (licence is None or not str(licence).strip()):", + "test": "test_an_unknown_licence_blocks", + }, ] # The runner's own check: a patch that cannot compile must be reported as BROKEN, not diff --git a/tools/dependency_gate.py b/tools/dependency_gate.py new file mode 100644 index 0000000..3b5ec49 --- /dev/null +++ b/tools/dependency_gate.py @@ -0,0 +1,124 @@ +#!/usr/bin/env python3 +"""Decide a pull request from GitHub's dependency review data. + + gh api "repos/$REPO/dependency-graph/compare/$BASE...$HEAD" > deps.json + python tools/dependency_gate.py deps.json + +Why this exists next to `actions/dependency-review-action` +---------------------------------------------------------- +The action already fails a pull request that ADDS a dependency with a known +vulnerability, and it does that well. It cannot do the other half. Its own +documentation is explicit: *"If we can't detect the license for a dependency we +will inform you, but the action won't fail."* + +For a GPL-3.0 project that ships a binary, an unidentified licence is not an +informational note - it is the one case where nobody can say whether the thing +may be distributed at all. So the same data is read here and an unknown licence +blocks, exactly like a denied one. The REST field is documented as "string or +null", and `null` is what "not determined" looks like. + +Scope +----- +Only dependencies that the pull request ADDS (`change_type == "added"`). +Removing something never creates an obligation, and re-checking what is already +in the tree would make every pull request answer for decisions taken years ago. +""" +import argparse +import json +import sys + +# SPDX identifiers a GPL-3.0 project may distribute alongside its own code. +# +# GPL-2.0-only is deliberately ABSENT: it is famously incompatible with GPL-3.0, +# and a dependency under it would be exactly the kind of thing that looks fine in +# a list of "open source licences" and is not. Anything not named here is a +# decision for a person, which is what blocking means. +ALLOWED = frozenset({ + "0BSD", "Apache-2.0", "BSD-2-Clause", "BSD-3-Clause", "CC0-1.0", "ISC", + "MIT", "MIT-0", "MPL-2.0", "PSF-2.0", "Python-2.0", "Unlicense", "Zlib", + "LGPL-2.1-only", "LGPL-2.1-or-later", "LGPL-3.0-only", "LGPL-3.0-or-later", + "GPL-3.0-only", "GPL-3.0-or-later", +}) + +# Packages whose licence GitHub cannot resolve and which a person has already +# looked at. A name here is a decision with a reason, not a way to make a red +# build green - and it names the package, never a whole ecosystem. +EXCEPTIONS = { + # (empty on purpose - add "name": "why this is fine" when it happens) +} + + +def added(review): + return [d for d in review if str(d.get("change_type", "")) == "added"] + + +def verdict(dependency): + """``("ok" | "unknown" | "denied", licence)`` for one added dependency.""" + licence = dependency.get("license") + name = str(dependency.get("name", "?")) + if name in EXCEPTIONS: + return "ok", licence + if licence is None or not str(licence).strip(): + return "unknown", licence + # A compound expression ("MIT OR Apache-2.0") passes only if every part is + # allowed. Being generous with an OR would mean accepting the worse half. + parts = [p.strip("() ") for p in str(licence).replace(" AND ", " OR ").split(" OR ")] + if all(part in ALLOWED for part in parts if part): + return "ok", licence + return "denied", licence + + +def split(review): + blocked, passed = [], [] + for dependency in added(review): + state, licence = verdict(dependency) + row = (state, str(dependency.get("name", "?")), + str(dependency.get("version", "?")), licence, + str(dependency.get("scope", "?"))) + (passed if state == "ok" else blocked).append(row) + return blocked, passed + + +def report_lines(blocked, passed): + lines = [] + if blocked: + lines.append("blocked - a licence that is denied or could not be determined:") + for state, name, version, licence, scope in blocked: + lines.append(" %-8s %s %s licence=%s scope=%s" + % (state, name, version, licence, scope)) + lines.append("") + lines.append("An unknown licence blocks on purpose: this project is GPL-3.0 and ships a") + lines.append("binary, so 'we could not tell' is the one answer nobody can act on. Record") + lines.append("the decision in tools/dependency_gate.py (ALLOWED or EXCEPTIONS).") + if passed: + lines.append("allowed (%d): %s" % ( + len(passed), ", ".join("%s %s (%s)" % (n, v, lic) for _s, n, v, lic, _sc in passed))) + lines.append("dependency gate: %d blocked, %d allowed" % (len(blocked), len(passed))) + return lines + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("review", help="JSON from the dependency review API") + args = parser.parse_args(argv) + try: + with open(args.review, encoding="utf-8") as handle: + review = json.load(handle) + except (OSError, ValueError) as exc: + # A gate that cannot read its input has not passed anything. The API + # answers 403 for some repository shapes, and that must look like a + # failure rather than an empty list of problems. + print("dependency gate: cannot read %s: %s" % (args.review, exc), file=sys.stderr) + return 2 + if not isinstance(review, list): + print("dependency gate: expected a list of dependencies, got %s" + % type(review).__name__, file=sys.stderr) + return 2 + blocked, passed = split(review) + for line in report_lines(blocked, passed): + print(line) + return 1 if blocked else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From adbf02ebca8f736e182c73285f7a659939bb4578 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 09:21:16 +0200 Subject: [PATCH 15/22] ci: run the mutation registry instead of quoting it What ships is a registry whose entries the suite checks for pointing at code that exists. Nothing in the repository ever ran one. So "116 caught, 0 survived" was true on the day somebody typed it and unverifiable every day after - the same class of claim the registry exists to refuse. The runner moves from internal_tools/ into tools/, which makes it a tracked CI dependency, and a job executes it. Two modes. A pull request runs --changed against its base: only the entries whose file the branch touched, which is seconds. The weekly run does the lot, which is about thirteen minutes for 117 entries on a developer machine. The canary rides along on the full run only, because demanding it on a filtered run would fail a pull request for the shape of the run rather than its result. It runs on Windows, and not for tidiness: pydivert carries a win32 marker, so on Linux an entry whose test needs the driver comes back SURVIVED - the test passes there by returning early, which is indistinguishable from a guard that does not guard. The old path still works and redirects, because it is in muscle memory and in the notes. This reverses the "it is a rig, not a dependency of anything" rule deliberately, and the repository's own guard against ${{ }} inside a run block refused the first version of the job - which is the cheapest possible evidence that the guard works. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 46 +++++++++++ tools/mutate.py | 174 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 220 insertions(+) create mode 100644 tools/mutate.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6195b4a..d368891 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -225,6 +225,52 @@ jobs: *) echo "unexpected verdict from tools/audit_issue.py"; exit 1 ;; esac + # The registry that ships is checked by the suite for POINTING at code that + # exists. Nothing in the repository ever RAN one, so "116 caught, 0 survived" + # was true on the day somebody typed it and unverifiable afterwards. This is + # that sentence, executed. + # + # windows-latest, and not to be tidy: `pydivert` carries a win32 marker, and an + # entry whose test needs the driver would report SURVIVED on a Linux runner - + # the test passes there by returning early, which is indistinguishable from a + # guard that does not guard. + mutations: + name: mutation registry + if: github.event_name != 'push' + runs-on: windows-latest + timeout-minutes: 60 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # `--changed` diffs against the base branch, which has to be here. + fetch-depth: 0 + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.14" + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install --require-hashes -r requirements.txt + pip install -r requirements-dev.txt + # A pull request pays for what it touched; the weekly run pays for + # everything. The full pass is minutes (~13 for 117 entries on a developer + # machine), which is fine once a week and absurd on every push. + - name: Break each guarded behaviour and prove its test reddens + shell: bash + env: + BASE_REF: ${{ github.base_ref }} + # Through the environment like every other context value here - the + # repository's own guard refuses a `${{ }}` inside a run block, and it + # refused this one while it was being written. + EVENT_NAME: ${{ github.event_name }} + run: | + if [ "$EVENT_NAME" = "pull_request" ]; then + git fetch --no-tags --depth=0 origin "$BASE_REF" + python tools/mutate.py --changed "origin/$BASE_REF" + else + python tools/mutate.py + fi + tests: name: tests (${{ matrix.os }}, py${{ matrix.python-version }}) runs-on: ${{ matrix.os }} diff --git a/tools/mutate.py b/tools/mutate.py new file mode 100644 index 0000000..f498c6a --- /dev/null +++ b/tools/mutate.py @@ -0,0 +1,174 @@ +"""Run the mutation registry: break a guarded behaviour, prove the named test reddens. + +What question this closes +------------------------- +"Is this test actually guarding what its name says?" A green suite proves nothing +about a behaviour whose guard never fails. The only proof is the sentence "this test +would be red if I broke this" - executed, not asserted. + +The registry lives in `tests/test_mutation_registry.py`, because it must ship and +must be checked by the normal suite. This runner used to live outside git as a rig +(PROJECT_NOTES rule 3) - it moved here on 2026-08-19, and the reason is the gap +that move closes: what ships is a registry whose entries are only checked for +POINTING at code that exists. Nothing in the repository ever ran one. "116 caught, +0 survived" was true on the day somebody typed it and unverifiable every day +after, which is the same class of claim this whole registry exists to refuse. + +Traps this already fell into, all paid for elsewhere and worth keeping +---------------------------------------------------------------------- +1. **A pattern that went stale reports a false "caught".** If the search string no + longer occurs, the file is unchanged, the test passes for the ordinary reason and + the entry looks proven. So a hit count != 1 is `SKIP`, never a result. The normal + suite catches this too (the registry test counts occurrences), which is why that + check is in both places. +2. **Restore by BYTES, never by rewriting text.** Opening a file in text mode on + Windows turns LF into CRLF, `.gitattributes` says the repo is LF, and the tree + ends up dirty in a way `git diff` does not show. Never `git checkout --` either: + for a file with uncommitted work that destroys the very fix being tested. +3. **Aim at ONE test.** Naming two tests with `or` and seeing red proves only that + one of them fell. The entry that names the other would then report "not caught" + in a full run and look like a hole in the product. +4. **The canary.** One entry is deliberately broken syntax and MUST come back as + BROKEN. Without it, a runner whose subprocess call is misconfigured reports + "everything caught" and the whole run is a lie. + +Usage +----- + python tools/mutate.py # every entry, plus the canary + python tools/mutate.py gui # entries whose label contains "gui" + python tools/mutate.py --changed origin/master + # only entries whose FILE changed + # against that ref - what a pull + # request runs, usually seconds + +`--changed` exists because the full run is minutes (~13 on the developer machine, +117 entries) and a pull request touches a handful of files. It compares with +`git diff --name-only ...HEAD`, the three-dot form, so a change on the base +branch does not drag unrelated entries in. With no entry matching, it says so and +exits 0 - a pull request that touches nothing guarded is not a failure. +""" +import os +import subprocess +import sys + +ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +sys.path.insert(0, os.path.join(ROOT, "tests")) + +from test_mutation_registry import CANARY, MUTATIONS # noqa: E402 + + +def run_test(test_name): + """Return True when the named test FAILS - i.e. the mutation was caught.""" + proc = subprocess.run( + [sys.executable, "-m", "pytest", "tests", "-k", test_name, "-q", + "-p", "no:randomly"], + cwd=ROOT, capture_output=True, text=True) + # "Did pytest actually run this test?" has to be read from pytest's own exit + # code, not from scanning its output for the word ERROR. + # + # 🔴 The substring check was WRONG in the one direction that matters: it turned + # a genuine `caught` into a SKIP whenever the failing test's traceback happened + # to print an upper-case ERROR - and a test about Win32 error constants prints + # `_ERROR_SERVICE_MARKED_FOR_DELETE` in its own source line. A rig that reports + # "unproven" for a guard that works is the same lie as one that reports + # "caught" for a guard that does not, only quieter. + # + # Pytest's codes: 0 passed, 1 failed, 2 interrupted, 3 internal, 4 usage, + # 5 nothing collected. Only 0 and 1 are answers to the question. + if proc.returncode == 5 or "no tests ran" in proc.stdout: + return None # the name matched nothing: not a result + if proc.returncode not in (0, 1) or "errors during collection" in proc.stdout: + return None # the tree did not run: also not a result + return proc.returncode != 0 + + +def apply_one(entry): + path = os.path.join(ROOT, entry["file"]) + with open(path, "rb") as handle: + original = handle.read() + # Newlines are NORMALISED before matching, and put back before writing. + # + # 🔴 Without this, every entry aiming at one of the files that still carry CRLF + # in this tree reported SKIP - and the registry test in `tests/` did not notice, + # because it reads in TEXT mode, where universal newlines turn `\r\n` into `\n` + # and the pattern matches. So the suite said the entry was healthy while the + # runner quietly refused to run it, which is the exact shape of a guard that + # proves nothing while looking fine. Found on `sortable_tree.py` (600 CRLF). + crlf = b"\r\n" in original + text = original.decode("utf-8").replace("\r\n", "\n") + if text.count(entry["old"]) != 1: + return "SKIP", "pattern occurs %d times, not 1" % text.count(entry["old"]) + try: + mutated = text.replace(entry["old"], entry["new"], 1) + if crlf: + mutated = mutated.replace("\n", "\r\n") + with open(path, "wb") as handle: + handle.write(mutated.encode("utf-8")) + compiled = subprocess.run([sys.executable, "-m", "compileall", "-q", path], + cwd=ROOT, capture_output=True, text=True) + if compiled.returncode != 0: + return "BROKEN", "the mutated tree does not compile" + caught = run_test(entry["test"]) + if caught is None: + return "SKIP", "no test matched %r" % entry["test"] + return ("caught" if caught else "SURVIVED"), entry["test"] + finally: + with open(path, "wb") as handle: # bytes, so line endings survive + handle.write(original) + + +def changed_files(ref): + """Repository-relative paths that differ from ``ref``, in the three-dot sense.""" + proc = subprocess.run(["git", "diff", "--name-only", "%s...HEAD" % ref], + cwd=ROOT, capture_output=True, text=True) + if proc.returncode != 0: + # Not a result: a runner that cannot read the diff must not report that + # nothing changed, because that reads exactly like a clean run. + print("mutate: git diff against %r failed: %s" + % (ref, proc.stderr.strip()[:200]), file=sys.stderr) + raise SystemExit(2) + return {line.strip().replace("\\", "/") for line in proc.stdout.splitlines() if line.strip()} + + +def main(argv): + argv = list(argv[1:]) + ref = None + if "--changed" in argv: + position = argv.index("--changed") + try: + ref = argv[position + 1] + except IndexError: + print("mutate: --changed needs a ref", file=sys.stderr) + return 2 + del argv[position:position + 2] + needle = argv[0] if argv else "" + + entries = [m for m in MUTATIONS if needle in m["label"]] + if ref is not None: + touched = changed_files(ref) + entries = [m for m in entries if m["file"].replace("\\", "/") in touched] + print("changed against %s: %d file(s), %d matching registry entr(ies)" + % (ref, len(touched), len(entries))) + if not entries: + print("nothing guarded by the registry was touched") + return 0 + elif not needle: + entries = entries + [CANARY] + width = max(len(m["label"]) for m in entries) + counts = {} + for entry in entries: + state, detail = apply_one(entry) + counts[state] = counts.get(state, 0) + 1 + print("%-*s %-9s %s" % (width, entry["label"], state, detail)) + print("\n" + " ".join("%s: %d" % kv for kv in sorted(counts.items()))) + # The canary only rides along on a FULL run. A filtered run (a label, or + # --changed) has none, and demanding one there would make every pull request + # fail for the shape of the run rather than for its result. + canary_ok = needle or ref is not None or counts.get("BROKEN") == 1 + if not canary_ok: + print("CANARY DID NOT FIRE - this whole run proves nothing") + return 0 if counts.get("SURVIVED", 0) == 0 and canary_ok else 1 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) From 40d810e59068cc86b2acb7dcba82d4efca290ea3 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 09:28:06 +0200 Subject: [PATCH 16/22] chore: put a ceiling on branching, and stop the CI prose drifting Two ratchets that were missing. The size ratchet watches how long a function is. Complexity watches how many ways through it there are, and the two do not move together: a hundred lines of straight-line setup is readable, forty lines with eight nested branches is not. The ceiling is 29 because that is core.decide today, the twelve-step packet pipeline, with cli._run_session at 27 behind it and nothing else in the package past 24. The number IS the maximum, so the guard asserts both directions - nothing exceeds it, and lowering it by one must produce a finding. It skips itself when ruff is absent, because a red that means "you did not install a tool" teaches people to ignore red. The workflow gained four jobs in two days and nothing was watching what the READMEs say about them. Both now carry a job table between explicit markers, and the guard compares it with the workflow in both directions: an undocumented job fails, and so does a documented job that no longer exists. The markers exist because the first version of the guard read every table in the file - both READMEs are full of tables whose first column is a backticked lowercase name. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 4 +-- README.md | 15 +++++++++++ README.pl.md | 15 +++++++++++ pyproject.toml | 18 ++++++++++++- tests/test_code_shape.py | 50 +++++++++++++++++++++++++++++++++++++ tests/test_readme_guards.py | 43 +++++++++++++++++++++++++++++++ 6 files changed, 142 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d368891..28caf42 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -98,8 +98,8 @@ jobs: # pyproject.toml and nothing else: the per-file ignores and the target # version still come from there, so the two runs below cannot drift apart # from what a developer sees locally. - - name: Bugs and dead code (blocking) - run: ruff check --select F,B --output-format github + - name: Bugs, dead code and the complexity ceiling (blocking) + run: ruff check --select F,B,C90 --output-format github # The report. S is 306 findings on this repository and 287 of them are # things a test suite for a packet mangler does on purpose - so it # annotates the diff and never fails the run. `ruff check` exits 1 on any diff --git a/README.md b/README.md index 4b487b7..4c40714 100644 --- a/README.md +++ b/README.md @@ -1159,6 +1159,21 @@ exit-code assertions, an NDJSON check, `--doctor` and `--license`, and then **an a smoke test of the built file** (`--version`, `--simulate`, a bad config -> code 3) plus a check that the WinDivert driver really shipped next to the exe, with a downloadable artifact. + +Every job in that workflow, and what a red one means: + +| job | what it does | +|---|---| +| `public-text` | commit messages and the pull-request description: English, plain hyphens, nothing private to a machine | +| `lint` | **ruff**. Dead code, bug shapes and the complexity ceiling fail the run. The security family is reported and never blocks | +| `types` | **mypy** over the package | +| `semgrep` | the default registry ruleset. ERROR, HIGH and CRITICAL fail the run, the rest is printed | +| `mutations` | breaks each guarded behaviour and proves its test reddens. A pull request runs the entries it touched, the weekly run does all of them | +| `audit` | weekly only: **pip-audit** against the pinned set, and it opens an issue when an advisory lands | +| `tests` | the suite, the GUI smoke, the real-Tk render check and the CLI assertions, on Linux and Windows | +| `build` | the Windows executable, smoke-tested, with the driver check and the licence registry scan | + + **Three static checks run beside the tests**, on Linux only, because they read the source rather than run it. **ruff** fails a pull request on a dead-code or bug-shape finding (`F` and `B`) and reports the security family (`S`, `ASYNC`) as annotations that never block. **mypy** type-checks diff --git a/README.pl.md b/README.pl.md index c5a44eb..b6b92b5 100644 --- a/README.pl.md +++ b/README.pl.md @@ -1014,6 +1014,21 @@ a na koniec **build `.exe` i smoke zbudowanego pliku** (`--version`, `--simulate konfiguracja → kod 3) plus kontrola, że sterownik WinDivert naprawdę trafił obok exe, z artefaktem do pobrania. + +Wszystkie joby tego workflow i co znaczy czerwony: + +| job | co robi | +|---|---| +| `public-text` | treść commitów i opisu PR-a: po angielsku, płaskie łączniki, nic prywatnego z maszyny | +| `lint` | **ruff**. Martwy kod, kształty błędów i sufit złożoności wywracają przebieg. Rodzina bezpieczeństwa tylko raportuje | +| `types` | **mypy** na pakiecie | +| `semgrep` | domyślny zestaw reguł z rejestru. ERROR, HIGH i CRITICAL wywracają przebieg, reszta ląduje w logu | +| `mutations` | psuje każde pilnowane zachowanie i dowodzi, że jego test się czerwieni. PR odpala wpisy, których dotknął, cotygodniowy przebieg wszystkie | +| `audit` | tylko co tydzień: **pip-audit** na przypiętym zestawie, a przy znalezisku zakłada issue | +| `tests` | suite, smoke GUI, render na prawdziwym Tk i asercje CLI, na Linuksie i Windowsie | +| `build` | plik .exe dla Windowsa, ze smoke'iem, sprawdzeniem sterownika i skanem rejestru licencji | + + **Obok testów chodzą trzy analizy statyczne**, wyłącznie na Linuksie, bo czytają kod, a nie go uruchamiają. **ruff** wywraca pull requesta na martwym kodzie i na kształtach błędów (`F` i `B`), a rodzinę bezpieczeństwa (`S`, `ASYNC`) tylko wypisuje w diffie i nigdy nie blokuje. **mypy** diff --git a/pyproject.toml b/pyproject.toml index 4a046e8..5ac0f65 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -61,7 +61,23 @@ addopts = "-q" target-version = "py310" # matches requires-python; ruff reads it anyway, said out loud [tool.ruff.lint] -select = ["F", "B", "S", "ASYNC"] +select = ["F", "B", "S", "ASYNC", "C90"] + +[tool.ruff.lint.mccabe] +# 🔴 THE CEILING IS THE MEASUREMENT, like FILE_CEILING in tests/test_code_shape.py. +# 29 is `core.decide` today - the twelve-step packet pipeline, which is branchy +# because the thing it describes is - and 27 is `cli._run_session` behind it. +# Nothing else in the package passes 24. +# +# The size ratchet already watches how LONG a function is. This watches how many +# ways through it there are, which is the number that decides whether a person +# can hold it in their head - and the two do not move together: a hundred-line +# function of straight-line setup is readable, a forty-line one with eight +# nested branches is not. +# +# Down is routine, up is a decision. Splitting something means lowering this in +# the same change. +max-complexity = 29 [tool.ruff.lint.per-file-ignores] # A test suite for a packet mangler asserts, randomises, spawns subprocesses and diff --git a/tests/test_code_shape.py b/tests/test_code_shape.py index 80ca023..f4b1996 100644 --- a/tests/test_code_shape.py +++ b/tests/test_code_shape.py @@ -37,6 +37,7 @@ """ import ast import os +import sys from fakes import ROOT, check @@ -270,3 +271,52 @@ def test_the_ceilings_are_not_set_so_loosely_that_they_never_fire(): biggest_function[0] == FUNCTION_CEILING, f"({biggest_function[1]} is {biggest_function[0]}, ceiling is " f"{FUNCTION_CEILING} - move the ceiling to {biggest_function[0]})") + + +# Ruff is not in requirements-dev.txt: it lives in requirements-lint.txt, which a +# contributor may not have installed. The two checks below skip themselves rather +# than fail in that case - the same choice the admin-only tests make, and for the +# same reason: a red that means "you did not install a tool" teaches people to +# ignore red. +def _ruff_complexity_findings(limit): + """How many functions ruff reports above ``limit``, or None if ruff is absent.""" + import subprocess + try: + proc = subprocess.run( + [sys.executable, "-m", "ruff", "check", "--select", "C901", + "--config", f"lint.mccabe.max-complexity = {limit}", + "--output-format", "concise", "--no-cache", "."], + cwd=ROOT, capture_output=True, text=True) + except OSError: + return None + if "No module named" in proc.stderr or proc.returncode not in (0, 1): + return None + return len([ln for ln in proc.stdout.splitlines() if "C901" in ln]) + + +def test_the_complexity_ceiling_is_the_measurement_not_a_number_above_it(): + """The same rule the file and function ceilings live by, on the third axis. + + A ceiling parked above the truth grants headroom nobody decided to grant, and + the next arrival slips in under it in silence. So `max-complexity` has to BE + the most branching function in the tree: nothing may exceed it, and lowering + it by one must produce a finding. + + Complexity is not length. The size ratchet already watches how long a function + is, and the two do not move together - a hundred lines of straight-line setup + is readable, forty lines with eight nested branches is not. + """ + import tomllib + with open(os.path.join(ROOT, "pyproject.toml"), "rb") as handle: + ceiling = tomllib.load(handle)["tool"]["ruff"]["lint"]["mccabe"]["max-complexity"] + + at_ceiling = _ruff_complexity_findings(ceiling) + if at_ceiling is None: + return # ruff not installed here: nothing to measure + check(f"nothing in the tree is more complex than {ceiling}", + at_ceiling == 0, f"({at_ceiling} function(s) over the ceiling)") + + below = _ruff_complexity_findings(ceiling - 1) + check(f"the ceiling {ceiling} IS a real measurement, not headroom", + below and below > 0, + f"(nothing reaches {ceiling} - lower max-complexity to the real maximum)") diff --git a/tests/test_readme_guards.py b/tests/test_readme_guards.py index bdb3fbb..46262a6 100644 --- a/tests/test_readme_guards.py +++ b/tests/test_readme_guards.py @@ -206,3 +206,46 @@ def test_polish_readme_pipeline_keeps_lan_and_blocking(): ("celowanie", "tryb LAN", "blokada", "NAT") if sec.find(w) >= 0)] check("README.pl.md 'Jak to działa' keeps celowanie -> tryb LAN -> blokada -> NAT", order == ["celowanie", "tryb LAN", "blokada", "NAT"], f"(got {order})") + + +def test_both_readmes_list_every_job_the_ci_workflow_runs(): + """Prose about CI is the first thing to go stale, and nothing was watching it. + + The workflow gained four jobs in two days. A reader deciding whether to trust + this project reads the README, not the YAML - so the table there has to be the + set of jobs, exactly: a new job that nobody documented reddens this, and so + does a documented job that no longer exists. + + Parsed by hand rather than with PyYAML, which is deliberately not a test + dependency (see the same choice in test_site.py). + """ + import re + with open(os.path.join(ROOT, ".github", "workflows", "ci.yml"), encoding="utf-8") as f: + lines = f.read().splitlines() + inside, jobs = False, [] + for line in lines: + if re.match(r"^jobs:\s*$", line): + inside = True + continue + if inside: + if line and not line.startswith((" ", "\t", "#")): + break # a new top-level key + match = re.match(r"^ ([A-Za-z0-9_-]+):\s*$", line) + if match: + jobs.append(match.group(1)) + check("the workflow scan found its jobs", len(jobs) >= 5, f"({jobs})") + + for readme in READMES: + text = _read(readme) + # Only the marked table: both READMEs are full of tables whose first + # column is a backticked lowercase name (CLI flags, CSV columns), and a + # guard that reads all of them measures the wrong thing. + block = re.search(r"(.*?)", text, re.S) + check(f"{readme} carries the marked CI table", block is not None) + documented = set(re.findall(r"^\| `([a-z0-9_-]+)` \|", block.group(1) if block else "", + re.MULTILINE)) + missing = sorted(set(jobs) - documented) + extra = sorted(documented - set(jobs)) + check(f"{readme} documents every CI job", not missing, f"(missing: {missing})") + check(f"{readme} documents no job that no longer exists", not extra, + f"(stale: {extra})") From cf818aa2fd0168a3d48f3e28f96d929ec9b8cb67 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 09:36:53 +0200 Subject: [PATCH 17/22] chore(types): make three modules strict, and make the list one-way Gradual typing without a ratchet is a plan, not a property. disallow_untyped_ defs is on for beantester.utils, gui.rates and gui.scope - and nothing stopped the next change from dropping one out of pyproject.toml to turn a red build green, with nothing to say so afterwards, because a check that vanishes cannot fail. So the list lives in two places, the configuration and the test, and the test compares them in both directions: growing it is free, shrinking it reddens. A mutation removes one module from the configured list and is caught. Those three because they are pure, small and headless, and utils is read by every layer so a wrong type there travels furthest. Annotating them turned up nothing broken, which is the expected result for three hundred lines of arithmetic and exactly why the ratchet starts there rather than in app.py. Co-Authored-By: Claude Opus 5 --- beantester/gui/rates.py | 15 +++++++------ beantester/gui/scope.py | 9 ++++---- beantester/utils.py | 34 ++++++++++++++++++----------- pyproject.toml | 17 +++++++++++++++ tests/test_code_shape.py | 38 +++++++++++++++++++++++++++++++++ tests/test_mutation_registry.py | 9 ++++++++ 6 files changed, 99 insertions(+), 23 deletions(-) diff --git a/beantester/gui/rates.py b/beantester/gui/rates.py index dfce926..0e9d49f 100644 --- a/beantester/gui/rates.py +++ b/beantester/gui/rates.py @@ -24,13 +24,15 @@ anchor the window, so the span is always >= WINDOW once the session is warm. """ from collections import deque +from typing import Optional WINDOW_S = 1.0 # average over this much time WARMUP_S = 0.8 # below this the window is too young to trust AVG_MIN_S = 0.5 # session average needs at least this much elapsed time -def average_kbps(total_bytes, elapsed_s, min_elapsed_s=AVG_MIN_S): +def average_kbps(total_bytes: float, elapsed_s: float, + min_elapsed_s: float = AVG_MIN_S) -> float: """Session-average throughput in KB/s (1024-based), or 0 while too young. ``total_bytes / elapsed`` is the honest lifetime average, but dividing by an @@ -45,18 +47,19 @@ def average_kbps(total_bytes, elapsed_s, min_elapsed_s=AVG_MIN_S): class PeakWindow: """Sliding window over cumulative byte counters -> KB/s, averaged over ~1 s.""" - def __init__(self, window_s=WINDOW_S, warmup_s=WARMUP_S): + def __init__(self, window_s: float = WINDOW_S, warmup_s: float = WARMUP_S) -> None: self.window_s = float(window_s) self.warmup_s = float(warmup_s) - self._samples = deque() # (t, bytes_in, bytes_out) + self._samples: deque[tuple[float, int, int]] = deque() - def reset(self): + def reset(self) -> None: self._samples.clear() - def __len__(self): + def __len__(self) -> int: return len(self._samples) - def add(self, now, bytes_in, bytes_out): + def add(self, now: float, bytes_in: int, + bytes_out: int) -> Optional[tuple[float, float]]: """Record a snapshot; return ``(down_kbs, up_kbs)`` or ``None`` if too young. ``None`` means "no honest answer yet", not "zero" - the caller must not diff --git a/beantester/gui/scope.py b/beantester/gui/scope.py index ecfa005..1fe7c2d 100644 --- a/beantester/gui/scope.py +++ b/beantester/gui/scope.py @@ -35,7 +35,7 @@ Pure (no tkinter, no engine): ``gui/__init__`` is lazy, so this imports and unit-tests headlessly, like ``scaling`` and ``rates``. """ -from typing import NamedTuple +from typing import NamedTuple, Optional # The four answers - and the whole set. A surface maps these to its own wording # through a table keyed by all of STATES, so a fifth state goes red at its @@ -48,7 +48,7 @@ STATES = (ALL, CAPTURE, CAPTURE_PROCESS, VIEW) -def keys_for_states(mapping): +def keys_for_states(mapping: dict[str, str]) -> dict[str, str]: """Freeze a ``state -> i18n key`` table, refusing an incomplete one. Each surface keeps its own table, because each says this in its own words - @@ -79,7 +79,8 @@ class Coverage(NamedTuple): process_target: bool -def coverage(capture_narrowed, view_scoped, process_target): +def coverage(capture_narrowed: bool, view_scoped: bool, + process_target: bool) -> "Coverage": """Derive what the on-screen numbers cover. Pure; the single decider. ``view_scoped`` wins over ``capture_narrowed`` because it is the stronger @@ -104,7 +105,7 @@ def coverage(capture_narrowed, view_scoped, process_target): return Coverage(state, capture_narrowed, view_scoped, process_target) -def capture_scope_note(settings, capture_narrowed): +def capture_scope_note(settings: dict, capture_narrowed: bool) -> Optional[str]: """i18n key of the line to log about the capture's scope, or ``None``. Asked for and got it, or asked for and did NOT: both have to be said, and diff --git a/beantester/utils.py b/beantester/utils.py index 744c115..b841d68 100644 --- a/beantester/utils.py +++ b/beantester/utils.py @@ -1,13 +1,21 @@ -"""Small dependency-free helpers shared across the engine, GUI and CLI.""" +"""Small dependency-free helpers shared across the engine, GUI and CLI. + +Annotated on purpose, and one of only three modules mypy is strict about +(`[[tool.mypy.overrides]]` in pyproject.toml). The list is a ratchet: it may +grow, and `tests/test_code_shape.py` refuses to let it shrink. This module is +in it because it is pure, small and read by every layer - the cheapest place to +start, and the place where a wrong type travels furthest. +""" import math +from typing import Any, Optional -def clamp01(x): +def clamp01(x: float) -> float: """Clamp a number into the ``[0.0, 1.0]`` range.""" return max(0.0, min(1.0, x)) -def to_number(value): +def to_number(value: Any) -> float: """Lenient float conversion: ``None`` / garbage -> ``0.0``.""" try: return float(value) @@ -15,13 +23,13 @@ def to_number(value): return 0.0 -def number_string(value): +def number_string(value: Any) -> str: """Compact string for a number: ``5.0`` -> ``'5'``, ``2.5`` -> ``'2.5'``.""" f = to_number(value) return str(int(f)) if f == int(f) else str(f) -def bytes_to_mb(n): +def bytes_to_mb(n: Any) -> float: """Bytes -> megabytes (MB = 1024*1024 B), rounded to 2 decimals.""" return round(to_number(n) / (1024.0 * 1024.0), 2) @@ -33,7 +41,7 @@ def bytes_to_mb(n): BYTE_UNITS = ("B", "KB", "MB", "GB", "TB", "PB") -def _byte_decimals(value, index): +def _byte_decimals(value: float, index: int) -> int: """How many decimals ``value`` gets in unit ``index``: three significant digits, except that whole bytes are never fractional.""" if index == 0: @@ -43,7 +51,7 @@ def _byte_decimals(value, index): return 1 if value < 100 else 0 -def human_bytes(n): +def human_bytes(n: Any) -> str: """Bytes as a string a person reads at a glance: ``5.24 GB``, ``90 B``. The connection table used to render every traffic column as ``bytes / 1024`` @@ -76,7 +84,7 @@ def human_bytes(n): return f"{sign}{value:.{_byte_decimals(value, index)}f} {BYTE_UNITS[index]}" -def nice_ceiling(v): +def nice_ceiling(v: Any) -> float: """Round up to a 'nice' axis value (1/2/2.5/5 x 10^k).""" v = to_number(v) if v <= 0: @@ -89,7 +97,7 @@ def nice_ceiling(v): return 10 * base -def canonical_ip(ip): +def canonical_ip(ip: Any) -> Optional[str]: """Canonical text form of an IPv4/IPv6 address, or ``None`` if invalid. Used to validate user-entered destination IPs and to compare them against @@ -102,7 +110,7 @@ def canonical_ip(ip): return None -def is_local_ip(ip): +def is_local_ip(ip: Any) -> bool: """True for local addresses (RFC1918, loopback, link-local, CGNAT...). Public (internet) addresses return False. Missing/error = treated as local. @@ -116,7 +124,7 @@ def is_local_ip(ip): return True -def _route_source_ip(family, probe): +def _route_source_ip(family: int, probe: str) -> str: """The local address the OS would use to reach ``probe`` - no packet is sent. A connected UDP socket only records a default peer, so this asks the routing @@ -133,7 +141,7 @@ def _route_source_ip(family, probe): return "-" -def host_identity(): +def host_identity() -> tuple[str, str, str]: """Hostname and this machine's private IPv4 / IPv6 addresses. The addresses belong to the adapter that would route to the internet, found @@ -161,7 +169,7 @@ def host_identity(): _num = to_number -def human_duration(seconds): +def human_duration(seconds: Any) -> str: """A session length a human can read, at any length. It used to be ``f"{minutes}m {seconds}s"``, which is fine for the ten-minute diff --git a/pyproject.toml b/pyproject.toml index 5ac0f65..6915365 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -121,6 +121,23 @@ warn_unused_ignores = true # Tighten it per module when a module is annotated, never repository-wide in one # go - a check that must be silenced everywhere teaches that silence is normal. +[[tool.mypy.overrides]] +# 🔴 THE STRICT LIST, and it is a RATCHET: it may grow, and +# `tests/test_code_shape.py::test_the_strictly_typed_modules_only_ever_grow` +# refuses to let it shrink. Deleting a name here is deleting a guarantee, which +# is a decision somebody has to make on purpose rather than to get unblocked. +# +# Why these three first: `utils` is pure, small and read by every layer, so a +# wrong type there travels furthest; `rates` and `scope` are the other two +# modules that already import headlessly and have no tkinter in them. Growing the +# list means annotating a module, not flipping a switch - which is the point, +# because a flag turned on over 66 unannotated modules would have to be silenced +# everywhere, and a check that is silenced everywhere teaches that silence is +# normal. +module = ["beantester.utils", "beantester.gui.rates", "beantester.gui.scope"] +disallow_untyped_defs = true +disallow_incomplete_defs = true + [[tool.mypy.overrides]] # Neither ships type information: pydivert has no `py.typed` marker, and psutil's # stubs live in a separate `types-psutil` distribution. Ignoring the import is a diff --git a/tests/test_code_shape.py b/tests/test_code_shape.py index f4b1996..c878fd4 100644 --- a/tests/test_code_shape.py +++ b/tests/test_code_shape.py @@ -320,3 +320,41 @@ def test_the_complexity_ceiling_is_the_measurement_not_a_number_above_it(): check(f"the ceiling {ceiling} IS a real measurement, not headroom", below and below > 0, f"(nothing reaches {ceiling} - lower max-complexity to the real maximum)") + + +# The modules mypy is strict about, recorded here so the list in pyproject.toml +# cannot quietly shrink. Add to BOTH when a module gains annotations; this one is +# the ratchet, and like every ratchet here it may rise and may not fall. +STRICTLY_TYPED = { + "beantester.utils", + "beantester.gui.rates", + "beantester.gui.scope", +} + + +def test_the_strictly_typed_modules_only_ever_grow(): + """Gradual typing without a ratchet is a plan, not a property. + + `disallow_untyped_defs` is on for three modules. Nothing stops the next + change from dropping one out of `pyproject.toml` to make a red build green - + and nothing would ever say so, because the check that vanished cannot fail. + + So the list is recorded twice: in the configuration, and here. Growing it is + free (add to both), shrinking it reddens. That is the same shape as + FILE_CEILING, one axis over. + """ + import tomllib + with open(os.path.join(ROOT, "pyproject.toml"), "rb") as handle: + config = tomllib.load(handle) + strict = set() + for override in config["tool"]["mypy"].get("overrides", []): + if override.get("disallow_untyped_defs"): + module = override.get("module") + strict |= set(module if isinstance(module, list) else [module]) + + lost = sorted(STRICTLY_TYPED - strict) + check("no module has quietly lost its strict typing", not lost, + f"({lost} - dropping one is a decision, so change STRICTLY_TYPED too)") + gained = sorted(strict - STRICTLY_TYPED) + check("a newly strict module is recorded here as well", not gained, + f"({gained} - add it to STRICTLY_TYPED, that is what makes it stick)") diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 39e3353..0876484 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -1089,6 +1089,15 @@ "new": " if False and (licence is None or not str(licence).strip()):", "test": "test_an_unknown_licence_blocks", }, + { + # A module quietly dropping out of the strict list. The check that + # vanishes cannot fail, which is why the list is recorded twice. + "label": "types: a module loses its strict typing quietly", + "file": "pyproject.toml", + "old": 'module = ["beantester.utils", "beantester.gui.rates", "beantester.gui.scope"]', + "new": 'module = ["beantester.gui.rates", "beantester.gui.scope"]', + "test": "test_the_strictly_typed_modules_only_ever_grow", + }, ] # The runner's own check: a patch that cannot compile must be reported as BROKEN, not From f9fbd619a85cff71040f83cf981bedc4ac4974e7 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 09:40:56 +0200 Subject: [PATCH 18/22] ci: gate the coverage of what a pull request changes The whole-repository gate answers "is this project tested". It cannot answer "is this change tested": at around 1200 tests the average barely moves, so a pull request can add a hundred untested lines and still pass it - and those are the lines nobody has run twice yet. diff-cover reads the coverage.xml the suite already produces and reports on the changed lines only, at 80 percent. Not 100, deliberately: parts of the GUI are reachable only through a real Tk event loop the fake tkinter cannot drive, and demanding every changed line would push people towards writing the test that is easy instead of the one that matters. Linux only, because both matrix legs would give the same answer, and pull requests only, because a push to master has nothing to diff against. The job's checkout gains fetch-depth: 0, since a shallow clone has no base branch. Verified locally against a real coverage.xml. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 27 +++++++++++++++++++++++++++ requirements-lint.txt | 6 ++++++ 2 files changed, 33 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 28caf42..a85374a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -288,6 +288,10 @@ jobs: python-version: ["3.14"] steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # The diff-coverage step below compares against the base branch, and a + # shallow clone has no base branch to compare against. + fetch-depth: 0 - name: Verify required release files are in the checkout # These are git-ignored by pattern and re-included by exception, so a # missing force-add or a broken .gitignore makes them vanish on a fresh @@ -317,6 +321,9 @@ jobs: python -m pip install --upgrade pip pip install --require-hashes -r requirements.txt pip install -r requirements-dev.txt + # diff-cover only: the rest of that file is for the analysis jobs, and + # installing it whole here would drag semgrep onto both matrix legs. + pip install "$(grep -E '^diff-cover==' requirements-lint.txt)" # ONE run of the whole suite, under coverage. testpaths=["tests"] (pyproject) # means this already includes the suites that used to own separate steps: @@ -340,6 +347,26 @@ jobs: COVERAGE_PROCESS_START: ${{ github.workspace }}/pyproject.toml run: python -m pytest tests --cov=beantester --cov-report=term --cov-report=xml + # The whole-repository gate (pyproject: fail_under) answers "is this project + # tested". It cannot answer "is this CHANGE tested": at ~1200 tests the + # average barely moves, so a pull request can add a hundred untested lines + # and still pass it - and those are the lines nobody has run twice yet. + # + # 80 rather than 100, deliberately. Parts of the GUI are only reachable + # through a real Tk event loop that the fake tkinter cannot drive, so + # demanding every changed line would push people towards writing the test + # that is easy instead of the one that matters. Ubuntu only, because the two + # matrix legs produce the same answer, and pull requests only, because there + # is nothing to diff against on a push to master. + - name: Coverage of the lines this pull request changed + if: github.event_name == 'pull_request' && matrix.os == 'ubuntu-latest' + shell: bash + env: + BASE_REF: ${{ github.base_ref }} + run: | + git fetch --no-tags origin "$BASE_REF" + diff-cover coverage.xml --compare-branch "origin/$BASE_REF" --fail-under 80 + - name: Coverage report if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 diff --git a/requirements-lint.txt b/requirements-lint.txt index 5c12cc5..a26a915 100644 --- a/requirements-lint.txt +++ b/requirements-lint.txt @@ -46,3 +46,9 @@ semgrep==1.173.0; sys_platform != "win32" # with 10 (those two, plus pip itself). A scanner that quietly covers less than it # was asked to is worse than none, because its clean report gets quoted. pip-audit==2.10.1 + +# Coverage of the CHANGE, on top of the whole-repository gate in pyproject. The +# average moves so slowly at this size that a pull request can add a hundred +# untested lines without pushing it below the threshold - which is precisely the +# code most worth a test, because it is the code nobody has run twice yet. +diff-cover==10.5.1 From 4b80d7e7be2f2848c99b4a9cb89485944acf4da1 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 10:03:33 +0200 Subject: [PATCH 19/22] fix(ci): skip actions in the licence gate, and stop asking git for depth zero Two things the first real run found, and neither could have failed locally. The licence gate blocked four GitHub Actions. Actions are dependencies in that API too, and GitHub reports license: null for every one of them - measured on this pull request, where checkout, upload-artifact, codeql-action and scorecard-action all came back unknown while all five pip packages came back with real licences. The actions ecosystem is skipped now, and not for convenience: an action is CI machinery that never reaches a user, so it creates none of the distribution obligation an unknown licence is dangerous for, and keeping it would block every pull request that touches a workflow for ever. A gate that always fires is a gate people learn to bypass. Actions are held to a stricter rule elsewhere: pinned to a commit SHA, checked by the suite, graded weekly. `git fetch --depth=0` is an error rather than a no-op - "depth 0 is not a positive number" - so the mutation job died on its first command in 25 seconds. The checkout already takes the full history, so the flag was never needed. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 5 ++++- tests/test_dependency_gate.py | 23 +++++++++++++++++++++++ tools/dependency_gate.py | 20 +++++++++++++++++++- 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a85374a..4f605d6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -265,7 +265,10 @@ jobs: EVENT_NAME: ${{ github.event_name }} run: | if [ "$EVENT_NAME" = "pull_request" ]; then - git fetch --no-tags --depth=0 origin "$BASE_REF" + # No --depth here: `git fetch --depth=0` is an error ("depth 0 is + # not a positive number"), and the checkout above already took the + # full history with fetch-depth: 0. + git fetch --no-tags origin "$BASE_REF" python tools/mutate.py --changed "origin/$BASE_REF" else python tools/mutate.py diff --git a/tests/test_dependency_gate.py b/tests/test_dependency_gate.py index 7eec10f..94ae3c9 100644 --- a/tests/test_dependency_gate.py +++ b/tests/test_dependency_gate.py @@ -94,3 +94,26 @@ def test_the_report_names_what_it_blocked_and_why(tmp_path, capsys): out = capsys.readouterr().out for expected in ("mystery", "unknown", "fine", "MIT", "1 blocked, 1 allowed"): check(f"the report says {expected!r}", expected in out, f"({out[:200]})") + + +def test_github_actions_are_not_judged_by_this_gate(): + """Measured on the first real run: GitHub reports `license: null` for EVERY + action, while every pip package came back with a real licence. + + An action is CI machinery that never reaches a user, so it creates none of + the distribution obligation an unknown licence is dangerous for. Keeping them + would block every pull request that touches a workflow, for ever - and a gate + that always fires is one people learn to bypass. Actions are held to a + stricter rule elsewhere: pinned to a commit SHA, and graded weekly. + """ + review = [_dep(name="actions/checkout", licence=None), + _dep(name="mypy", licence="MIT")] + review[0]["ecosystem"] = "actions" + blocked, passed = dependency_gate.split(review) + check("an action with no licence does not block", not blocked, f"({blocked})") + check("and it is not counted as allowed either", len(passed) == 1, f"({passed})") + + # ...while a pip package with no licence still blocks, which is the point. + unknown = [_dep(name="mystery", licence=None)] + check("a package with no licence still blocks", + len(dependency_gate.split(unknown)[0]) == 1) diff --git a/tools/dependency_gate.py b/tools/dependency_gate.py index 3b5ec49..ada3a48 100644 --- a/tools/dependency_gate.py +++ b/tools/dependency_gate.py @@ -48,8 +48,26 @@ } +# 🔴 GitHub Actions are dependencies in this data too, and GitHub reports +# `license: null` for every one of them - measured on the first real run, where +# this gate blocked `actions/checkout`, `actions/upload-artifact`, +# `github/codeql-action` and `ossf/scorecard-action` while every pip package came +# back with a real licence. +# +# They are skipped, and the reason is not convenience. An action is CI machinery +# that never reaches a user, so it creates no distribution obligation - the thing +# an unknown licence is dangerous for. Keeping them would mean blocking every +# pull request that touches a workflow, for ever, and a gate that always fires is +# a gate people learn to bypass. What actions ARE checked for lives elsewhere and +# is stricter: every one must be pinned to a commit SHA +# (tests/test_repo_conventions.py), and OpenSSF Scorecard grades them weekly. +SKIPPED_ECOSYSTEMS = frozenset({"actions"}) + + def added(review): - return [d for d in review if str(d.get("change_type", "")) == "added"] + return [d for d in review + if str(d.get("change_type", "")) == "added" + and str(d.get("ecosystem", "")).lower() not in SKIPPED_ECOSYSTEMS] def verdict(dependency): From 0d001cea0f906eaed985a49c9851fd2dd716f2c5 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 10:18:38 +0200 Subject: [PATCH 20/22] fix(tests): give the scanner guard something to guard on a runner The mutation registry job came back with one SURVIVED, and there was no defect behind it. test_the_repository_scanners_stay_out_of_what_is_not_in_the_ repository asserts that internal_tools/, .claude/ and crashes/ are never scanned - and those directories exist on the maintainer's machine and in no checkout. On a runner there was nothing under them to leak, so the assertion passed whatever SKIP_DIRS held, and dropping internal_tools from it changed nothing observable. The same test already fixed this exact fault once, for HANDOFF-*.md, and its docstring says so: an assertion that could not fail, and only a mutation run said so. So the fix follows that precedent - plant a file in each of those directories for the length of the scan, so the skip has something to skip - and clean up afterwards, including a directory the test created. Verified the way the failure was found rather than by argument: a clean git archive export of HEAD, which is what a runner checks out, passes with this fix and reddens with the mutation applied. Co-Authored-By: Claude Opus 5 --- tests/test_repo_conventions.py | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/tests/test_repo_conventions.py b/tests/test_repo_conventions.py index ce7184b..3dd7029 100644 --- a/tests/test_repo_conventions.py +++ b/tests/test_repo_conventions.py @@ -316,16 +316,44 @@ def test_the_repository_scanners_stay_out_of_what_is_not_in_the_repository(): probe = os.path.join(ROOT, "HANDOFF-scanner-probe.md") check("the planted brief is not overwriting a real one", not os.path.exists(probe), f"({probe})") - # An em-dash: what the dash scan would object to if it ever read this file. + # 🔴 And the same trick for the DIRECTORIES, for the same reason one level up. + # `internal_tools/`, `.claude/` and `crashes/` exist on the maintainer's machine + # and in no checkout, so on a runner there is nothing under them to leak - the + # assertion below passes whatever SKIP_DIRS holds. Measured on CI (2026-08-19): + # the mutation that drops `internal_tools` from SKIP_DIRS came back SURVIVED, + # with no defect behind it. Planting one file in each makes the guard mean the + # same thing in both places. + planted = [] + for directory in ("internal_tools", ".claude", "crashes"): + target = os.path.join(ROOT, directory, "scanner-probe.md") + if os.path.exists(target): + continue # never overwrite something real + os.makedirs(os.path.dirname(target), exist_ok=True) + planted.append((target, os.path.dirname(target))) + # An em-dash: what the dash scan would object to if it ever read these files. # Built with chr() rather than written out, because THIS file is repository text # and is scanned by the very rule it defines - a literal one fails the suite. + text = "planted by the suite " + chr(0x2014) + " removed immediately\n" with open(probe, "w", encoding="utf-8", newline="\n") as handle: - handle.write("planted by the suite " + chr(0x2014) + " removed immediately\n") + handle.write(text) + for target, _parent in planted: + with open(target, "w", encoding="utf-8", newline="\n") as handle: + handle.write(text) try: scanned = {os.path.relpath(p, ROOT).replace(os.sep, "/") for p in repo_text_files((".py", ".md", ".json", ".txt"))} finally: os.remove(probe) + for target, parent in planted: + os.remove(target) + # Only a directory this test created: a maintainer's own is left alone. + try: + os.rmdir(parent) + except OSError: + pass + check("the scan had something to find under each skipped directory", + len(planted) == 3 or os.path.isdir(os.path.join(ROOT, "internal_tools")), + f"(planted {len(planted)})") for stray in ("PROJECT_NOTES.md", "CLAUDE.md", "HISTORY_NOTES.md", "CHANGELOG-INTERNAL.md", os.path.basename(probe)): check(f"{stray} is not scanned (it is not in the repository)", From eb1d5ed1909632da3882e7078419b6d689a84481 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 11:13:16 +0200 Subject: [PATCH 21/22] fix(ci): stop a dead package mirror eating the whole test job tests (ubuntu) hit its 20-minute timeout three times in a row, always in the GUI render step, and never got as far as starting the render. The timestamps settle it: the last line of apt-get update was at 08:49:50 and the job was cancelled at 09:05:29 - fifteen minutes and thirty-nine seconds of silence, after four "Ign: http://azure.archive.ubuntu.com/..." per index list. The same step took seventeen seconds on master the day before. Nothing here changed: the runner image points apt at a mirror that stopped answering, and apt-get update has no timeout of its own. So the step now touches apt only when a package is actually missing, rewrites that mirror to the one apt was already limping towards, bounds every apt call, and bounds the render itself - because a hanging GUI must not look identical to a hanging mirror. It also runs Python unbuffered. The first diagnosis of this was wrong, and that is why: the archived log of a cancelled job holds only what was flushed, and Python buffers into a pipe, so "no render output" could equally have meant "the render hung with its output still in the buffer". Proving which took a WSL run, a comparison against master and the line timestamps. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 41 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4f605d6..38d5579 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -385,15 +385,48 @@ jobs: # smoke cannot see truncation or DPI/layout breakage; this builds the real App # at the minimum supported resolution in BOTH languages (Polish strings are the # longest - that is where layouts crack) and fails on any clipped key widget. + # 🔴 Everything here except the last line is armour against the runner's + # package mirror, and it was paid for: on 2026-08-19 this step burned the + # job's whole 20-minute budget three times in a row without ever starting + # the render. The log said `Ign: http://azure.archive.ubuntu.com/...` four + # times per index list, then went silent for 15 minutes 39 seconds. The + # same step took 17 seconds on master the day before. Nothing in this + # repository changed - GitHub's image points apt at a mirror that stopped + # answering, and `apt-get update` has no timeout of its own. + # + # * touch apt at all only when a package is actually missing; + # * rewrite that mirror to the one that answered (the fallback apt was + # already limping towards); + # * bound every apt call, so a dead mirror costs a minute and a clear + # error rather than the job's whole budget; + # * bound the render too, because a GUI that hangs must look different + # from a mirror that hangs; + # * and run Python UNBUFFERED, because when this hung its output was + # still in a pipe buffer and the archived log showed nothing at all - + # which is why the first diagnosis blamed the wrong command. - name: GUI render check (real Tk, Xvfb, 1366x768, PL+EN) if: runner.os == 'Linux' env: BEAN_NO_ELEVATE: "1" run: | - sudo apt-get update - sudo apt-get install -y python3-tk xvfb - xvfb-run -a --server-args="-screen 0 1366x768x24" \ - python tools/ci_gui_render.py + missing="" + for package in python3-tk xvfb; do + dpkg -s "$package" >/dev/null 2>&1 || missing="$missing $package" + done + if [ -n "$missing" ]; then + echo "installing:$missing" + sudo sed -i 's|azure.archive.ubuntu.com|archive.ubuntu.com|g' \ + /etc/apt/sources.list /etc/apt/sources.list.d/*.sources \ + /etc/apt/sources.list.d/*.list 2>/dev/null || true + sudo timeout 240 apt-get update \ + -o Acquire::Retries=2 \ + -o Acquire::http::Timeout=15 -o Acquire::https::Timeout=15 + sudo timeout 240 apt-get install -y --no-install-recommends $missing + else + echo "python3-tk and xvfb are already on the image" + fi + timeout 300 xvfb-run -a --server-args="-screen 0 1366x768x24" \ + python -u tools/ci_gui_render.py # The CLI contract itself: --simulate needs neither Windows, WinDivert nor # admin, so every runner can prove it end to end. From 50ae9ebca1dc3ac41d2fd4c9454c9f0ce5c275c2 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 19 Aug 2026 11:36:52 +0200 Subject: [PATCH 22/22] fix(ci): rewrite the mirror where it actually lives, and skip apt when it can The previous fix bounded the damage - the job died at four minutes with exit 124 instead of eating twenty - but left the cause alone, because it rewrote the wrong file. The log had already said where the mirror lives: "Get:1 file:/etc/apt/apt-mirrors.txt Mirrorlist [144 B]". On these images sources.list points at a mirrorlist, so rewriting sources.list changes nothing. That file is rewritten now. The step also tries the cheap path first: the image ships package lists, so apt-get install is attempted without refreshing them, and apt-get update - the network-bound half that hung - happens only if that fails. Both branches were proved with a stubbed shell harness rather than by reading the code: one install and no update on the cheap path, update plus a second install on the fallback. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 38d5579..3f91e9f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -415,13 +415,29 @@ jobs: done if [ -n "$missing" ]; then echo "installing:$missing" - sudo sed -i 's|azure.archive.ubuntu.com|archive.ubuntu.com|g' \ - /etc/apt/sources.list /etc/apt/sources.list.d/*.sources \ - /etc/apt/sources.list.d/*.list 2>/dev/null || true - sudo timeout 240 apt-get update \ - -o Acquire::Retries=2 \ - -o Acquire::http::Timeout=15 -o Acquire::https::Timeout=15 - sudo timeout 240 apt-get install -y --no-install-recommends $missing + # 🔴 The mirror is NOT in sources.list on these images - it is in a + # MIRRORLIST that sources.list points at, and the first rewrite here + # missed it. The log said so plainly and it took a second failure to + # read it: `Get:1 file:/etc/apt/apt-mirrors.txt Mirrorlist [144 B]`, + # then `Ign: http://azure.archive.ubuntu.com/...` for every index. + for source in /etc/apt/apt-mirrors.txt /etc/apt/sources.list \ + /etc/apt/sources.list.d/*.sources \ + /etc/apt/sources.list.d/*.list; do + if [ -f "$source" ]; then + sudo sed -i 's|azure.archive.ubuntu.com|archive.ubuntu.com|g' "$source" + fi + done + # The image ships package lists, so the cheap path is to install + # without refreshing them at all. `apt-get update` is the expensive, + # network-bound half, and it is only worth paying for when the lists + # really are too old for the package we need. + if ! sudo timeout 180 apt-get install -y --no-install-recommends $missing; then + echo "install from the shipped lists failed - refreshing them" + sudo timeout 240 apt-get update \ + -o Acquire::Retries=2 \ + -o Acquire::http::Timeout=15 -o Acquire::https::Timeout=15 + sudo timeout 240 apt-get install -y --no-install-recommends $missing + fi else echo "python3-tk and xvfb are already on the image" fi