From f534a4fcac1aca95f29cdedfabc2e6f7844d38d6 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 15:48:46 -0500 Subject: [PATCH 1/5] ci(gate): make the merge gate knowable, and not neuterable in one line Branch protection lives on the server, so "is this check blocking?" could not be answered from a clone -- and five in-repo statements answered it differently. docs/CI.md said 8 contexts (naming the CLA one "CLA Assistant", which matches no status check at all), manifest-lint.yml said 4, freethread.md said 7, cla.yml told a reader to require that non-matching string, and test_push_guard.py said 12. The live API says 12. With required_approving_review_count 0 and auto-merge armed, required-set membership is the only thing separating "reviewed" from "merged unread" -- so a session reasoning from docs/CI.md would conclude that gitleaks, semgrep, npm-audit and crypto-inventory were advisory, i.e. that four blocking security gates were safe to weaken. .github/required-contexts.txt is now the checked-in claim, and tests/test_required_contexts.py asserts every prose statement agrees with it -- plus a reality check that each context resolves to a real job name (expanding matrix templates). That last part matters: a doc-drift test comparing two wrong things passes happily, and a required context that nothing reports blocks every PR forever. tests/test_security_posture.py closes the second hole. security.yml's own header documents the downgrade -- "add `continue-on-error: true` back to its job" -- which is exactly the edit that makes a REQUIRED context report SUCCESS while scanning nothing. Nothing guarded it: the three tests that read a workflow's continue-on-error cover quality-advisory.yml and freethread-smoke.yml, and test_lint_scope_parity.py opens security.yml only to compare scan scope. The new suite refuses continue-on-error (job and step), `|| true` / `--exit-zero`, and a skippable job-level `if:` for every job backing a required context; it pins the advisory jobs the other way so an accidental promotion is equally deliberate; and test_every_security_job_is_classified fails on any new security.yml job, so the module cannot quietly stop covering the file it is named for. Verified by mutation rather than by being green. Seven injected regressions -- job- and step-level continue-on-error, `|| true` on the bandit scan, a schedule-only `if:` on the leak guard, trivy silently promoted, a new unclassified scanner, a dropped required context -- are each caught by the intended assertion; reverting the doc fixes reproduces 5 of the 8 drift failures. Both mutated files were confirmed restored byte-identical. One assertion caught a modelling error of my own on the way: 12 contexts resolve to 10 distinct jobs, because the three `test` legs share one matrix job. Also wires actionlint, which docs/CI.md has prescribed for months while nothing ran it. An invalid `${{ }}` anywhere in a `run:` body -- comments included -- aborts workflow compilation, so no jobs are created and required contexts simply never appear. The PR does not go red, it goes stuck, and the tempting remedy for a stuck PR is relaxing branch protection. Now a version-pinned pre-commit hook (the load-bearing half; zizmor.yml is deliberately not a required check) plus a checksum-verified step in zizmor.yml, which is already paths-filtered to .github/**. Its first run produced one real finding -- a custom self-hosted runner label -- declared in .github/actionlint.yaml rather than by disabling the runner-label rule, which would have traded one finding for the whole class. backlog-hygiene is recorded as PENDING PROMOTION rather than added: this file mirrors the live server, so adding it before branch protection changes would make it lie in the one direction it exists to prevent. That promotion is the owner's call. --- .github/actionlint.yaml | 15 ++ .github/required-contexts.txt | 73 ++++++++ .github/workflows/cla.yml | 7 +- .github/workflows/manifest-lint.yml | 7 +- .github/workflows/security.yml | 8 + .github/workflows/zizmor.yml | 24 +++ .pre-commit-config.yaml | 23 +++ docs/CI.md | 50 ++++-- docs/design/freethread.md | 15 +- tests/_workflow_contexts.py | 95 +++++++++++ tests/test_required_contexts.py | 232 +++++++++++++++++++++++++ tests/test_security_posture.py | 255 ++++++++++++++++++++++++++++ 12 files changed, 781 insertions(+), 23 deletions(-) create mode 100644 .github/actionlint.yaml create mode 100644 .github/required-contexts.txt create mode 100644 tests/_workflow_contexts.py create mode 100644 tests/test_required_contexts.py create mode 100644 tests/test_security_posture.py diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml new file mode 100644 index 00000000..447c92ae --- /dev/null +++ b/.github/actionlint.yaml @@ -0,0 +1,15 @@ +# actionlint configuration. +# +# actionlint validates `runs-on:` against GitHub's HOSTED runner labels, so a custom self-hosted label +# is reported as unknown. Declaring the label here is actionlint's own documented remedy and keeps the +# runner-label check LIVE for genuine typos ("windows-2O25", "ubunutu-latest") — the alternative, +# disabling the `runner-label` rule, would trade one real finding for the whole class. +self-hosted-runner: + labels: + # .github/workflows/selfhosted-win2025-sql.yml — SQL Server 2025 suites on real hardware. + # NOTE: the NucBox self-hosted runners are RETIRED (ci.yml runs every leg on hosted runners now), + # so this label may currently resolve to no registered runner. Confirm with + # `gh api repos/MEFORORG/MessageFoundry/actions/runners` before relying on that workflow; if the + # runner is gone for good, delete the workflow and this label together rather than leaving a + # declaration that outlives its subject. + - mefor-win2025-sql diff --git a/.github/required-contexts.txt b/.github/required-contexts.txt new file mode 100644 index 00000000..a65bf94b --- /dev/null +++ b/.github/required-contexts.txt @@ -0,0 +1,73 @@ +# The status-check contexts REQUIRED to merge into `main`. +# +# WHY THIS FILE EXISTS. Branch protection lives on the server, so "is this check blocking?" was +# unanswerable from a clone -- and five places in this repo answered it differently: docs/CI.md +# listed 8 contexts, .github/workflows/manifest-lint.yml said 4, docs/design/freethread.md said 7, +# tests/test_push_guard.py said 12. The live API says 12. With `required_approving_review_count: 0` +# and auto-merge armed, required-set membership is the ONLY thing separating "reviewed" from +# "merged unread", so a wrong answer here mis-ranks which gates a change can safely touch. +# +# This file is not the enforcement -- the server is. It is the CHECKED-IN CLAIM that every in-repo +# statement must agree with, which is what `tests/test_required_contexts.py` asserts. When branch +# protection changes, change this file in the same PR; the test will name every prose claim that +# then needs updating. +# +# FORMAT. One context string per line, `#` starts a comment, blank lines ignored. These are the +# exact strings branch protection matches -- the JOB NAME (`name:`), not the workflow name and not +# the workflow file. Two ways that bites: +# * The CLA context is `cla` (the job key, because cla.yml's job declares no `name:`), NOT +# "CLA Assistant" (the workflow name). docs/CI.md claimed the latter for months. +# * A job name containing " #" must be QUOTED in YAML or it truncates at the comment marker -- +# see .github/workflows/backlog-hygiene.yml, whose name is deliberately quoted for this reason. +# +# ADDING ONE: see the required-but-absent trap in docs/CI.md. A required check that never reports +# blocks every PR forever, so confirm the job reports on a real PR (no `paths:` filter that can skip +# it, no `if:` gated on a non-PR event) BEFORE it lands here or in branch protection. + +# --- ci.yml --------------------------------------------------------------------------------------- +CI gate +test (ubuntu-latest, py3.14) +test (windows-2022, py3.14) +test (windows-2025, py3.14) + +# --- security.yml (every non-advisory job in that file; the advisory `sbom` and `trivy` jobs are +# --- deliberately absent -- they declare `continue-on-error: true`) ------------------------------- +bandit (Python SAST) +pip-audit (dependency vulnerabilities) +npm-audit (ide dependency vulnerabilities) +gitleaks (secret scan) +semgrep (project SAST rules) +crypto-inventory (ASVS 11.1.3 discovery gate) +forbidden-content (customer/PHI leak guard) + +# --- cla.yml -------------------------------------------------------------------------------------- +cla + +# PENDING PROMOTION -- owner's decision, NOT yet applied. This file mirrors the LIVE server, so the +# context below is deliberately absent until branch protection actually changes; adding it here first +# would make the file lie in the one direction it exists to prevent. +# +# "a PR that implements BACKLOG #N must update BACKLOG.md" (backlog-hygiene.yml) +# +# WHY PROMOTE: it hard-fails today and nothing waits for it, so it is decoration -- auto-merge only +# blocks on REQUIRED contexts. It exists because #60 shipped while its banner still read +# "PRE-RESERVED", and that stale banner was then repeated as fact in a merged PR. In a repo where the +# ledger is the only memory shared across isolated sessions, a stale banner makes another session +# rebuild work that is already merged. +# SAFE TO PROMOTE: it triggers on `pull_request: branches: [main]` with NO paths filter and no `if:`, +# so it always reports -- no required-but-absent trap. (Its STRUCTURAL half, "every item declares +# exactly one status", is already enforced inside the required test legs by +# tests/test_backlog_status_check.py; only the behavioural half is unguarded.) +# TO APPLY: add the context to branch protection, then move the line above into the ci/security list +# and update the count pinned in tests/test_required_contexts.py. + +# DELIBERATELY NOT REQUIRED (do not add without the owner's decision; each has a recorded reason): +# codeql.yml, scorecard.yml -- their SARIF upload needs `security-events: write`, which fork-PR +# tokens lack, so requiring them would block every fork PR. +# zizmor.yml -- paths-filtered to `.github/**`, so it does not report on most PRs. +# quality-advisory.yml -- advisory BY DESIGN and must never be promoted; the invariant is +# pinned by tests/test_quality_advisory_invariants.py. +# freethread-smoke.yml -- a 3.14t canary; informational only. +# manifest-lint.yml -- paths-filtered to the k8s manifests. +# nightly / path-gated legs -- server-DB store, load/throughput, service-smoke, DICOM/FHIR +# breadth: they do not run on every PR (required-but-absent trap). diff --git a/.github/workflows/cla.yml b/.github/workflows/cla.yml index 1c0e5142..5b599c61 100644 --- a/.github/workflows/cla.yml +++ b/.github/workflows/cla.yml @@ -5,8 +5,11 @@ name: CLA Assistant # needed — the default GITHUB_TOKEN (with contents: write below) is sufficient. # # NOTE: pull_request_target workflows only run from the DEFAULT branch, so this takes effect -# once it is merged to main. To make signing mandatory, add the "CLA Assistant" status check -# to main's branch-protection required checks. +# once it is merged to main. Signing IS already mandatory: the required status-check context is +# `cla` -- the JOB key below, because that job declares no `name:`. It is NOT "CLA Assistant", +# which is this WORKFLOW's name and matches no status check; adding that string to branch +# protection would wedge every PR forever (docs/CI.md, "the required-but-absent trap"). +# See .github/required-contexts.txt. on: issue_comment: diff --git a/.github/workflows/manifest-lint.yml b/.github/workflows/manifest-lint.yml index 3830c0a1..2ef50fc1 100644 --- a/.github/workflows/manifest-lint.yml +++ b/.github/workflows/manifest-lint.yml @@ -8,8 +8,11 @@ # * It runs ONLY when a manifest, this workflow, or the docker/ docs change (paths filter), so docs/code # PRs that don't touch manifests skip it entirely. # * It is a SEPARATE workflow, deliberately not part of ci.yml's required `ci-gate`. The required set -# stays: the `test` matrix + bandit + pip-audit + cla (see docs/.. main required-checks). Do NOT add -# "manifest lint" to branch-protection required checks unless the owner decides to promote it. +# is recorded in .github/required-contexts.txt — read it there rather than trusting a copy in a +# comment; the copy that used to live on this line named 4 of the 12 and went stale. Do NOT add +# "manifest lint" to branch-protection required checks unless the owner decides to promote it: it is +# paths-filtered, so on a PR that touches no manifest it would never report and would block the PR +# forever (the required-but-absent trap, docs/CI.md). # * It is fast + dependency-light (a single static binary + grep assertions), so it is cheap to run. name: manifest lint diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index b7328c43..9195a622 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -5,6 +5,14 @@ name: Security # All four are BLOCKING — their first CI run was clean (bandit: 0; gitleaks: 0; semgrep: 0 across # 5 rules; pip-audit: no known CVEs), so each now fails the build on a regression rather than # merging unnoticed. To temporarily downgrade one, add `continue-on-error: true` back to its job. +# +# READ THAT LAST SENTENCE AS A TRAP, not a procedure. Every job here except `sbom` and `trivy` is a +# REQUIRED context, and GitHub reports a continue-on-error job as SUCCESS — so that one line leaves +# branch protection green while the scanner's findings are discarded, and with auto-merge armed and +# zero required approvals the PR merges unread. `tests/test_security_posture.py` now REFUSES it (and +# `|| true`, `--exit-zero`, and a skippable job-level `if:`) for any job in the required set. To take +# a gate off the merge path, remove its context from branch protection AND +# `.github/required-contexts.txt` — deliberately, in a reviewed diff. Never neuter it in place. # CodeQL and native push-protection secret scanning need GitHub Advanced Security on this private # repo; gitleaks below is the no-GHAS secret scanner. See docs/SECURITY.md. # diff --git a/.github/workflows/zizmor.yml b/.github/workflows/zizmor.yml index bdfc780d..3f345f98 100644 --- a/.github/workflows/zizmor.yml +++ b/.github/workflows/zizmor.yml @@ -40,6 +40,30 @@ jobs: run: | # Pin deliberately; verify the tag at https://github.com/zizmorcore/zizmor/releases if it 404s. python -m pip install --upgrade pip "zizmor==1.5.2" + # actionlint FIRST: it catches the class zizmor structurally cannot. An invalid `${{ }}` anywhere + # in a `run:` body -- comments included -- aborts workflow COMPILATION, so no jobs are created and + # every required context silently never appears (the PR looks stuck, not red). zizmor parses the + # file it is given and has nothing to say about a file GitHub will refuse to compile. + # + # Keep the version in step with .pre-commit-config.yaml's actionlint `rev`. This step is the + # backstop for `git commit --no-verify`; the pre-commit hook is the load-bearing half, because + # this workflow is deliberately NOT a required check (see the header). + - name: Lint workflow syntax (actionlint, pinned) + run: | + # Pin the release; bump deliberately. Verify the tag exists at + # https://github.com/rhysd/actionlint/releases if this step 404s. Verified against the + # release's own checksum file -- the same posture as the sbomqs download in security.yml, + # because a curl|tar of a release asset is dependency intake that no lockfile covers. + VER=1.7.12 + asset="actionlint_${VER}_linux_amd64.tar.gz" + base="https://github.com/rhysd/actionlint/releases/download/v${VER}" + curl -sSfL "${base}/${asset}" -o "$asset" + curl -sSfL "${base}/actionlint_${VER}_checksums.txt" -o actionlint_checksums.txt + grep " ${asset}$" actionlint_checksums.txt | sha256sum -c - + tar -xzf "$asset" actionlint + sudo install -m 0755 actionlint /usr/local/bin/actionlint + actionlint --version + actionlint - name: Analyze the workflows env: # Lets zizmor run its online audits (e.g. unpinned / known-problematic action refs). diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 59d6d67e..6410238c 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -64,6 +64,29 @@ repos: hooks: - id: gitleaks + # Workflow SYNTAX — the check that stops a PR from silently having no required contexts at all. + # + # GitHub interpolates `${{ }}` ANYWHERE in a `run:` script — comments included — before the shell + # sees it, so one invalid expression aborts workflow compilation: no jobs are created, the run is + # attributed to a phantom event, and every required context simply never appears. The PR does not go + # red, it goes STUCK, and the tempting remedy for a stuck PR is relaxing branch protection — which + # would permanently weaken the gate to work around a syntax error. `zizmor` does not catch this class + # at all; `actionlint` does. + # + # docs/CI.md carried this as an instruction ("run actionlint on every ci.yml edit") aimed at human + # memory. That is the wrong mechanism for a failure whose only symptom is silence, so it is a hook. + # The hook is the LOAD-BEARING half: the matching CI step lives in zizmor.yml, which is deliberately + # NOT a required check (it is paths-filtered to `.github/**`, so requiring it would wedge every PR + # that touches no workflow). Keep the `rev` in step with the version zizmor.yml installs. + # + # The upstream hook already scopes itself to `types: [yaml]` + `files: ^\.github/workflows/`, so no + # `files:` override is needed here — and adding one would be the scope drift + # tests/test_lint_scope_parity.py exists to prevent. + - repo: https://github.com/rhysd/actionlint + rev: v1.7.12 + hooks: + - id: actionlint + # Python SAST — the SAME skips AND the same excluded paths as the CI bandit job (security.yml). # It said "same ... as CI" before and was not: CI scanned `-r messagefoundry tee` while this hook # scanned everything but tests/harness/samples, so scripts/ was gated here and by nothing in CI — diff --git a/docs/CI.md b/docs/CI.md index dbea20eb..bac6f8b3 100644 --- a/docs/CI.md +++ b/docs/CI.md @@ -2,8 +2,15 @@ **Audience:** contributors and maintainers working on MessageFoundry. This page describes what Continuous Integration runs on a pull request, which checks must pass before a change can merge, and a -few gotchas that have cost real debugging time. Branch protection on `main` is the source of truth for -which checks are *required* — this page describes the intended layout. +few gotchas that have cost real debugging time. + +Branch protection on `main` is the **server-side** source of truth for which checks are *required*. +Because that is unreadable from a clone, the set is mirrored in +[`.github/required-contexts.txt`](../.github/required-contexts.txt) — the checked-in claim that this +page and every other in-repo statement must agree with, asserted by `tests/test_required_contexts.py`. +Prose lists are what drift: this page understated the required set by four blocking security gates and +named the CLA context by the wrong string. Edit the canonical file, and let the test tell you which +claims move with it. ## Workflows @@ -14,7 +21,7 @@ which checks are *required* — this page describes the intended layout. | `codeql.yml` | GitHub CodeQL analysis (python / javascript-typescript). | | `scorecard.yml` | OpenSSF Scorecard analysis. | | `cla.yml` | CLA Assistant — records the Contributor License Agreement signature on each PR. | -| `zizmor.yml` | Lints the workflow files themselves for insecure patterns (template injection, over-broad tokens). **Blocking.** | +| `zizmor.yml` | Lints the workflow files themselves for insecure patterns (template injection, over-broad tokens), and runs `actionlint` on the workflow syntax. Hard-fails, but **not a required check** — it is paths-filtered to `.github/**`, so it does not report on a PR that touches no workflow, and requiring it would wedge every such PR. The `actionlint` pre-commit hook is the local half. | | `quality-advisory.yml` | Advisory quality measurement — complexity (ruff `C901`), duplication (`jscpd`), diff-coverage (`diff-cover`) and mutation testing (`mutmut`). **Every job is advisory and none is in branch protection.** See below for how each signal reaches a reviewer. | Several heavier legs (server-DB store tests, load/throughput, service-smoke, DICOM/FHIR breadth) run @@ -24,7 +31,8 @@ would otherwise wedge the PR — see the gotcha below). ## Checks required to merge -The stable contexts required on `main` are: +The stable contexts required on `main` are — mirroring +[`.github/required-contexts.txt`](../.github/required-contexts.txt), which is the file to edit: - `CI gate` - `test (ubuntu-latest, py3.14)` @@ -32,12 +40,24 @@ The stable contexts required on `main` are: - `test (windows-2025, py3.14)` - `bandit (Python SAST)` - `pip-audit (dependency vulnerabilities)` +- `npm-audit (ide dependency vulnerabilities)` +- `gitleaks (secret scan)` +- `semgrep (project SAST rules)` +- `crypto-inventory (ASVS 11.1.3 discovery gate)` - `forbidden-content (customer/PHI leak guard)` -- `CLA Assistant` +- `cla` + +That last string is the **job key** in `cla.yml`, whose job declares no `name:`. Branch protection +matches the job name, never the workflow name — so the context is `cla`, not "CLA Assistant". Every +non-advisory job in `security.yml` is in the set; the two that are not (`sbom`, `trivy`) declare +`continue-on-error: true`, and `tests/test_security_posture.py` pins which side of that line each one +is on. -CodeQL and Scorecard run on PRs but are **advisory** (not in the required set) — their SARIF upload needs -`security-events: write`, which fork-PR tokens do not have, so requiring them would block PRs from forks. -Nightly / path-gated legs (service-smoke, load, SQL/Postgres store) are deliberately **not** required. +CodeQL is **advisory** (not in the required set) — its SARIF upload needs `security-events: write`, +which fork-PR tokens do not have, so requiring it would block PRs from forks. Scorecard is advisory for +the same reason and additionally **does not run on PRs at all** (`scorecard.yml` has no `pull_request` +trigger — it runs on push-to-main, a schedule, and branch-protection changes). Nightly / path-gated +legs (service-smoke, load, SQL/Postgres store) are deliberately **not** required. The `quality-advisory.yml` jobs create **no code-scanning category** and **no _required_ check context** — they do report as ordinary advisory checks, and they **must never be added to the required list**. Two @@ -115,11 +135,15 @@ an unrelated PR without turning the gate red. ## Gotchas -- **Run `actionlint` on every `ci.yml` edit.** GitHub interpolates `${{ }}` expressions *anywhere* in a - `run:` script — comments included — before the shell sees it, so a stray/invalid expression aborts - workflow compilation: **no jobs are created**, the run is attributed to a phantom event, and required - contexts silently never appear (the PR just looks stuck). `zizmor` does not catch this; `actionlint` - does. +- **`actionlint` runs on every workflow edit — let it.** GitHub interpolates `${{ }}` expressions + *anywhere* in a `run:` script — comments included — before the shell sees it, so a stray/invalid + expression aborts workflow compilation: **no jobs are created**, the run is attributed to a phantom + event, and required contexts silently never appear (the PR just looks stuck). `zizmor` does not catch + this; `actionlint` does. This used to be an instruction aimed at human memory, which is the wrong + mechanism for a failure whose symptom is "the PR is stuck" and whose tempting remedy is relaxing + branch protection. It is now a **pre-commit hook** scoped to `.github/workflows/**`, plus a step in + `zizmor.yml` (which is already paths-filtered to `.github/**`). The hook is the load-bearing half — + `zizmor.yml` is not a required check. - **Pass matrix/expression values through `env:`, don't inline them in `run:`.** A dynamic `matrix: ${{ fromJSON(...) }}` defeats zizmor's static analysis, which then flags its expansion inside `run:` as template injection. The fix is to route the value through `env:` — the remedy endorsed in diff --git a/docs/design/freethread.md b/docs/design/freethread.md index 76964bf2..c195dd8f 100644 --- a/docs/design/freethread.md +++ b/docs/design/freethread.md @@ -222,12 +222,15 @@ of `ci.yml`: * It is **not** in `ci.yml`'s `ci-gate` `needs:` list, so it is **not** the required "CI gate" context and **cannot** block a PR. -* Every job step that could fail under an immature `3.14t` is wrapped in `continue-on-error: true`, - and the job itself is `continue-on-error: true`, so a red canary reports a red **informational** - check, never a failed required one. -* It must **not** be added to branch protection's required checks (see §"required checks" in memory — - the 7 required contexts are the `test` matrix + bandit + pip-audit + cla). **Do not** add this - context there. +* Every job step that could fail under an immature `3.14t` is wrapped in `continue-on-error: true`. + The **job** deliberately is not: see the header comment in `freethread-smoke.yml`, which explains + that a job allowed to go red is the only way a dead tripwire stays distinguishable from a healthy + one. Its triggers already guarantee it cannot block anything, so a red canary reports a red + **informational** check, never a failed required one. +* It must **not** be added to branch protection's required checks. The required set is recorded in + `.github/required-contexts.txt` — read it there rather than trusting a copy in prose; the copy that + used to live on this line named 7 contexts when the live set was 12. **Do not** add this context + there. What it does (kept minimal — install + import + the fastest pure-Python test subset): diff --git a/tests/_workflow_contexts.py b/tests/_workflow_contexts.py new file mode 100644 index 00000000..51d41895 --- /dev/null +++ b/tests/_workflow_contexts.py @@ -0,0 +1,95 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Resolve branch-protection status-check CONTEXTS to the jobs that report them. + +Shared by ``test_required_contexts.py`` (does every in-repo claim about the required set agree?) and +``test_security_posture.py`` (can a required job be neutered without turning red?). Both need the same +context -> job mapping, and two copies of that mapping would be free to drift apart — which is the +class of bug both suites exist to catch. + +THE MAPPING IS NOT OBVIOUS, which is why it lives in one place: + +* The context string is the job's ``name:`` if it declares one, else the job KEY. That is why the CLA + context is ``cla`` and not "CLA Assistant" (cla.yml's workflow name, which matches no status check). +* A matrix job's name is a TEMPLATE — ``test (${{ matrix.os }}, py${{ matrix.python-version }})`` + reports as ``test (ubuntu-latest, py3.14)``. Comparing the template literally would resolve nothing, + so ``${{ ... }}`` becomes a wildcard. +""" + +from __future__ import annotations + +import re +from pathlib import Path +from typing import Any + +import pytest + +# The importorskip lives HERE rather than in each importing test module. Done there, it would be a +# statement before the `from tests._workflow_contexts import ...` line and every caller would need an +# E402 dance; done here, both suites keep ordinary top-of-file imports and PyYAML stays optional +# exactly as tests/test_lint_scope_parity.py treats it. +yaml = pytest.importorskip("yaml") + +ROOT = Path(__file__).resolve().parents[1] +WORKFLOWS = ROOT / ".github" / "workflows" +CANONICAL_CONTEXTS = ROOT / ".github" / "required-contexts.txt" + +_EXPR = re.compile(r"\$\{\{[^}]*\}\}") +_PLACEHOLDER = "\x00" + + +def required_contexts() -> list[str]: + """The contexts recorded in ``.github/required-contexts.txt`` (comments/blanks stripped).""" + lines = CANONICAL_CONTEXTS.read_text(encoding="utf-8").splitlines() + return [s for line in lines if (s := line.strip()) and not s.startswith("#")] + + +def load_workflow(name: str) -> dict[str, Any]: + """Parse one workflow file. ``name`` is the file name, e.g. ``security.yml``.""" + parsed = yaml.safe_load((WORKFLOWS / name).read_text(encoding="utf-8")) + assert isinstance(parsed, dict), f"{name} did not parse to a mapping" + return parsed + + +def jobs_of(name: str) -> dict[str, dict[str, Any]]: + return {k: (v or {}) for k, v in (load_workflow(name).get("jobs") or {}).items()} + + +def context_of(job_key: str, job: dict[str, Any]) -> str: + """The status-check context string this job reports (still templated, if it is a matrix job).""" + return str(job.get("name", job_key)) + + +def _context_pattern(declared: str) -> re.Pattern[str]: + expr = _EXPR.sub(_PLACEHOLDER, declared) + body = "".join( + ".+" if part == _PLACEHOLDER else re.escape(part) + for part in re.split(f"({_PLACEHOLDER})", expr) + if part + ) + return re.compile(f"^{body}$") + + +def reportable_contexts() -> dict[str, tuple[str, str]]: + """Every context a workflow here CAN report -> (workflow file, job key). + + Keys are the declared (possibly templated) strings; use :func:`resolve` to match a concrete + context against them. + """ + found: dict[str, tuple[str, str]] = {} + for wf_path in sorted(WORKFLOWS.glob("*.yml")): + for job_key, job in jobs_of(wf_path.name).items(): + found[context_of(job_key, job)] = (wf_path.name, job_key) + return found + + +def resolve(context: str) -> tuple[str, str] | None: + """Locate the job that reports ``context``, or None if no workflow can ever report it. + + None is the required-but-absent trap (docs/CI.md): a required context nothing reports blocks every + PR forever. Callers must treat it as a failure, never skip it. + """ + for declared, where in reportable_contexts().items(): + if _context_pattern(declared).match(context): + return where + return None diff --git a/tests/test_required_contexts.py b/tests/test_required_contexts.py new file mode 100644 index 00000000..94884aeb --- /dev/null +++ b/tests/test_required_contexts.py @@ -0,0 +1,232 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Doc-vs-server drift guard for the REQUIRED status-check set. + +THE DEFECT THIS EXISTS FOR. Branch protection lives on the server, so "is this check blocking?" could +not be answered from a clone -- and five places in this repo answered it differently at once: + + docs/CI.md 8 contexts (and named the CLA one "CLA Assistant") + .github/workflows/manifest-lint.yml 4 ("the `test` matrix + bandit + pip-audit + cla") + docs/design/freethread.md 7 ("the 7 required contexts are ...") + .github/workflows/cla.yml "add the \"CLA Assistant\" status check" -- already required, + and under a different string than its own line 18 gives + tests/test_push_guard.py 12 + +The live API says 12. That question is not trivia here: ``required_approving_review_count`` is 0 and +auto-merge is armed, so required-set membership is the ONLY thing separating "reviewed" from "merged +unread". A session reasoning from docs/CI.md would conclude that gitleaks, semgrep, npm-audit and +crypto-inventory are advisory -- i.e. that four blocking security gates were safe to weaken. + +WHAT THIS PINS. ``.github/required-contexts.txt`` is the checked-in claim; these tests assert every +in-repo statement agrees with it. The file is NOT the enforcement -- the server is -- so when branch +protection changes, change the file in the same PR and this suite names each prose claim that must +move with it. + +WHY THE REALITY CHECK MATTERS. A doc-drift test comparing prose to a file passes just as happily when +BOTH are wrong -- a context string that matches no job in any workflow is the required-but-absent trap +(docs/CI.md), which blocks every PR forever. So ``test_every_required_context_matches_a_real_job`` +resolves each string against the actual job names, expanding ``${{ }}`` templates. That is the +assertion that makes the rest of this file mean something. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +from tests._workflow_contexts import ( + CANONICAL_CONTEXTS as _CANONICAL, +) +from tests._workflow_contexts import ( + ROOT as _ROOT, +) +from tests._workflow_contexts import ( + WORKFLOWS as _WORKFLOWS, +) +from tests._workflow_contexts import ( + load_workflow, + reportable_contexts, + required_contexts, + resolve, +) + +_CI_DOC = _ROOT / "docs" / "CI.md" + +# Files that make a statement about the required set. Every one is READ, and a missing path is an +# ERROR rather than a skip: a claim file that got renamed must be re-pointed here, not silently +# dropped from the scan (that is how a doc-drift guard comes to guard nothing). +_CLAIM_FILES = ( + Path("docs/CI.md"), + Path("docs/design/freethread.md"), + Path(".github/workflows/manifest-lint.yml"), + Path(".github/workflows/cla.yml"), + Path(".github/workflows/zizmor.yml"), + Path(".github/workflows/freethread-smoke.yml"), + Path("tests/test_push_guard.py"), + Path("scripts/hooks/push_guard.py"), +) + +# Contexts that must NEVER appear in the canonical file. Each is a job that cannot report on an +# ordinary PR (fork-token SARIF, a paths filter, a non-PR `if:`) or is advisory by design, so +# requiring it walks straight into the required-but-absent trap. +_MUST_NOT_BE_REQUIRED = ( + "zizmor (GitHub Actions static analysis)", + "Scorecard analysis", + "kubeconform + HA policy lint", + "freethread smoke (3.14t)", + "complexity triage (advisory)", + "clone detection (advisory)", + "diff-coverage (advisory)", + "mutation (advisory)", + "gate liveness (advisory)", + "SBOMs (CycloneDX, multi-ecosystem)", + "trivy (container image vulnerabilities)", +) + + +def _canonical() -> list[str]: + return required_contexts() + + +def test_the_canonical_file_parses_and_names_the_live_set() -> None: + """A liveness receipt: report what was actually parsed, so an empty read cannot look like a pass.""" + contexts = _canonical() + print(f"[required-contexts] parsed {len(contexts)} contexts from {_CANONICAL.name}") + assert contexts, f"{_CANONICAL} parsed to ZERO contexts — the format changed under the parser" + assert len(set(contexts)) == len(contexts), ( + f"duplicate context in {_CANONICAL.name}: {contexts}" + ) + # Pinned so that ADDING or REMOVING a required check is a deliberate, reviewed edit here rather + # than a silent one. Verified against `gh api repos/MEFORORG/MessageFoundry/branches/main/protection`. + assert len(contexts) == 12, ( + f"the canonical required set changed to {len(contexts)} contexts. If branch protection really " + "changed, update this count AND every claim this suite checks; if it did not, revert the file." + ) + + +def test_every_required_context_matches_a_real_job() -> None: + """The required-but-absent trap, made mechanical. + + A required context that no job can ever report blocks every PR forever. Resolving each string + against the real job names also catches the quieter version: a job renamed while branch + protection (and this file) kept the old string. + """ + reportable = reportable_contexts() + print(f"[required-contexts] resolving against {len(reportable)} job names in {_WORKFLOWS}") + unresolved = [ctx for ctx in _canonical() if resolve(ctx) is None] + assert not unresolved, ( + f"required context(s) match no job in .github/workflows/: {unresolved}. A required check that " + "never reports blocks every PR forever (docs/CI.md, 'the required-but-absent trap'). Either a " + "job was renamed without updating branch protection, or the string here is a typo." + ) + + +def test_advisory_and_path_gated_jobs_are_not_required() -> None: + """Promoting one of these is the required-but-absent trap, or breaks a pinned invariant.""" + contexts = set(_canonical()) + wrongly_required = sorted(contexts & set(_MUST_NOT_BE_REQUIRED)) + assert not wrongly_required, ( + f"{wrongly_required} must not be a required context. These jobs either cannot report on an " + "ordinary PR (fork-PR token, paths filter, non-PR `if:`) or are advisory by design — " + "quality-advisory.yml's advisory status is separately pinned by " + "tests/test_quality_advisory_invariants.py." + ) + + +def test_ci_doc_required_list_matches_the_canonical_file() -> None: + """docs/CI.md's 'Checks required to merge' bullet list is the claim readers actually act on.""" + text = _CI_DOC.read_text(encoding="utf-8") + m = re.search(r"^## Checks required to merge$(.*?)^#", text, re.MULTILINE | re.DOTALL) + assert m, "docs/CI.md lost its '## Checks required to merge' section — re-point this test" + documented = set(re.findall(r"^- `([^`]+)`$", m.group(1), re.MULTILINE)) + assert documented, ( + "parsed ZERO contexts out of docs/CI.md's required-checks section. The bullet format changed; " + "an empty parse must not read as agreement." + ) + canonical = set(_canonical()) + assert documented == canonical, ( + "docs/CI.md disagrees with .github/required-contexts.txt.\n" + f" documented but not required: {sorted(documented - canonical)}\n" + f" required but not documented: {sorted(canonical - documented)}\n" + "This drift previously understated the required set by four blocking security gates." + ) + + +def test_numeric_required_set_claims_agree() -> None: + """'the 7 required contexts', '12 status checks' — a count is a claim, and three were stale.""" + expected = len(_canonical()) + pattern = re.compile( + r"(\d+)\s+(?:required\s+(?:status\s+)?(?:checks?|contexts?)|status\s+checks?)", + re.IGNORECASE, + ) + wrong: list[str] = [] + examined = 0 + for rel in _CLAIM_FILES: + path = _ROOT / rel + assert path.exists(), f"claim file {rel} no longer exists — re-point _CLAIM_FILES" + examined += 1 + for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + for found in pattern.finditer(line): + if int(found.group(1)) != expected: + wrong.append(f"{rel}:{lineno} claims {found.group(1)}, live set is {expected}") + print(f"[required-contexts] scanned {examined} claim files for numeric required-set counts") + assert examined == len(_CLAIM_FILES) + assert not wrong, "stale required-set counts:\n " + "\n ".join(wrong) + + +def test_the_two_workflows_that_restated_the_set_now_point_at_the_file() -> None: + """An inline enumeration is what went stale; a pointer cannot. + + manifest-lint.yml and freethread.md each carried their own copy of the required set ("the `test` + matrix + bandit + pip-audit + cla") to warn a reader not to add themselves to it. Both copies + aged out. They must reference the canonical file instead. + """ + for rel in (Path(".github/workflows/manifest-lint.yml"), Path("docs/design/freethread.md")): + text = (_ROOT / rel).read_text(encoding="utf-8") + assert "required-contexts.txt" in text, ( + f"{rel} warns against joining the required set but no longer points at " + ".github/required-contexts.txt. Reference the canonical file — do not re-inline the list, " + "which is what went stale here twice." + ) + + +def test_no_claim_file_names_the_cla_context_by_its_workflow_name() -> None: + """The context is `cla` (the job key). "CLA Assistant" is the WORKFLOW name and matches nothing. + + docs/CI.md listed the wrong string in its required set, and cla.yml told a reader to add that + string to branch protection — a change that would have wedged every PR. + """ + offenders: list[str] = [] + banned = re.compile(r"[\"`']CLA Assistant[\"`']\s+status check|- `CLA Assistant`") + for rel in _CLAIM_FILES: + for lineno, line in enumerate((_ROOT / rel).read_text(encoding="utf-8").splitlines(), 1): + if banned.search(line): + offenders.append(f"{rel}:{lineno}: {line.strip()}") + assert not offenders, ( + "the required CLA context string is `cla`, not `CLA Assistant` (that is cla.yml's workflow " + "name; branch protection matches the JOB name, and cla.yml's job declares none):\n " + + "\n ".join(offenders) + ) + + +def test_ci_doc_does_not_contradict_the_zizmor_and_scorecard_workflows() -> None: + """Two doc claims that the workflows themselves refute.""" + doc = _CI_DOC.read_text(encoding="utf-8") + + zizmor_row = next((ln for ln in doc.splitlines() if ln.startswith("| `zizmor.yml`")), None) + assert zizmor_row, "docs/CI.md lost its zizmor.yml workflow row — re-point this test" + assert "**Blocking.**" not in zizmor_row, ( + "docs/CI.md calls zizmor.yml '**Blocking.**' — .github/workflows/zizmor.yml states it is NOT " + "a required check, and the live protection rules agree. It is also paths-filtered to " + "`.github/**`, so requiring it would wedge every PR that does not touch a workflow." + ) + + scorecard = load_workflow("scorecard.yml") + # `on:` parses to the boolean True under the YAML 1.1 spec PyYAML implements. + triggers = scorecard.get("on") or scorecard.get(True) or {} + if "pull_request" not in triggers: + assert not re.search(r"Scorecard[^.\n]*run(s)? on PRs", doc), ( + "docs/CI.md says Scorecard runs on PRs, but scorecard.yml has no `pull_request` trigger " + f"(it has {sorted(triggers)}). The advisory-vs-required reasoning in that sentence is " + "sound for CodeQL and does not apply to a workflow that never reports on a PR at all." + ) diff --git a/tests/test_security_posture.py b/tests/test_security_posture.py new file mode 100644 index 00000000..950030ff --- /dev/null +++ b/tests/test_security_posture.py @@ -0,0 +1,255 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""A REQUIRED gate must be able to go red. This pins that it still can. + +THE DEFECT THIS EXISTS FOR. ``security.yml``'s own header documents the one-line downgrade: + + "To temporarily downgrade one, add `continue-on-error: true` back to its job." + +That is a fair note for a human doing it on purpose. It is also the exact edit that makes a **required** +context report SUCCESS while scanning nothing — GitHub takes a job's conclusion, and +``continue-on-error`` rewrites a failure into success before branch protection ever sees it. With +``required_approving_review_count: 0`` and auto-merge armed, the PR then merges unread. Every scanner +this project relies on — bandit, pip-audit, npm-audit, gitleaks, semgrep, crypto-inventory and the +customer/PHI leak guard — sits behind that one line, and nothing guarded it: the three tests in this +repo that read a workflow's ``continue-on-error`` cover ``quality-advisory.yml`` and +``freethread-smoke.yml``, and ``test_lint_scope_parity.py`` opens ``security.yml`` only to compare scan +*scope*. + +This is the artifact of a broader lesson the project already learned twice — a gate that reports green +while measuring nothing (``scripts/quality/liveness.py``, ``tests/test_gate_liveness.py``). Liveness +catches a gate that RAN and measured nothing. This catches a gate that was told its findings do not +count. + +WHY IT ALSO PINS THE ADVISORY JOBS. The check runs in both directions on purpose. ``sbom`` and +``trivy`` are advisory *by design* and are scheduled for promotion; if one silently loses +``continue-on-error`` it starts blocking merges from a cron-only trigger, which is the +required-but-absent trap wearing the opposite hat. Either move must be a deliberate edit to the lists +below. + +NOT VACUOUS BY CONSTRUCTION. ``test_every_security_job_is_classified`` fails when a job is added to +``security.yml`` without being named blocking or advisory here, so a new scanner cannot arrive +unguarded — the failure mode that would otherwise make this whole module a decoration. +""" + +from __future__ import annotations + +import re + +from tests._workflow_contexts import ( + WORKFLOWS, + context_of, + jobs_of, + required_contexts, + resolve, +) + +_SECURITY = "security.yml" + +# BLOCKING: a finding here must fail the build. Each of these job names is asserted to be in +# .github/required-contexts.txt below, so "blocking" is a checked claim rather than a label. +_BLOCKING_SECURITY_JOBS = frozenset( + { + "pip-audit", + "npm-audit", + "bandit", + "gitleaks", + "semgrep", + "crypto-inventory", + "forbidden-content", + } +) + +# ADVISORY by design: these MUST keep continue-on-error. Both are cron/dispatch-only, so promoting one +# without also removing its `if:` would wedge every PR (see security.yml's own notes on trivy). +_ADVISORY_SECURITY_JOBS = frozenset({"sbom", "trivy"}) + +# Job-level `if:` expressions that CANNOT skip the job on a pull_request, with the reason each is safe. +# Anything else on a required job is a way for the context to silently not report. +_JOB_IF_ALLOWLIST = { + ("ci.yml", "ci-gate"): "always()", # the roll-up must run even when a gated leg failed +} + +# Idioms that discard a non-zero exit, i.e. neuter the step without touching continue-on-error. +_NEUTERING = ( + (re.compile(r"\|\|\s*true\b"), "|| true"), + (re.compile(r"\|\|\s*:\s*(?:$|[;&\n])"), "|| :"), + (re.compile(r"\|\|\s*exit\s+0\b"), "|| exit 0"), + (re.compile(r"--exit-zero\b"), "--exit-zero"), + (re.compile(r"--fail-under=0\b"), "--fail-under=0"), +) + + +def _required_jobs() -> dict[tuple[str, str], dict]: + """Every job backing a required context, keyed by (workflow file, job key).""" + resolved: dict[tuple[str, str], dict] = {} + unresolved: list[str] = [] + for ctx in required_contexts(): + where = resolve(ctx) + if where is None: + unresolved.append(ctx) + continue + resolved[where] = jobs_of(where[0])[where[1]] + assert not unresolved, ( + f"required context(s) resolve to no job: {unresolved}. Fix that first — a required check that " + "never reports blocks every PR forever, and this module cannot assess a job it cannot find." + ) + return resolved + + +def test_every_security_job_is_classified() -> None: + """A new job in security.yml must be declared blocking or advisory HERE before it can land. + + Without this, adding a scanner would leave it outside every assertion below and this module would + quietly stop covering the file it is named for. + """ + actual = set(jobs_of(_SECURITY)) + classified = _BLOCKING_SECURITY_JOBS | _ADVISORY_SECURITY_JOBS + print(f"[security-posture] classified {len(classified)} of {len(actual)} jobs in {_SECURITY}") + assert actual == classified, ( + f"security.yml jobs are not all classified.\n" + f" unclassified (add to _BLOCKING_SECURITY_JOBS or _ADVISORY_SECURITY_JOBS): " + f"{sorted(actual - classified)}\n" + f" named here but gone from the workflow: {sorted(classified - actual)}" + ) + + +def test_blocking_security_jobs_are_in_the_required_set() -> None: + """ "Blocking" means "in branch protection". A job that fails but is not required is decoration.""" + required = set(required_contexts()) + jobs = jobs_of(_SECURITY) + missing = sorted( + context_of(k, jobs[k]) + for k in _BLOCKING_SECURITY_JOBS + if context_of(k, jobs[k]) not in required + ) + assert not missing, ( + f"these security.yml jobs are declared BLOCKING but are absent from " + f".github/required-contexts.txt: {missing}. A hard-failing job that is not a required context " + "does not stop auto-merge — it only looks like it does." + ) + + +def test_advisory_security_jobs_are_not_required() -> None: + required = set(required_contexts()) + jobs = jobs_of(_SECURITY) + promoted = sorted( + context_of(k, jobs[k]) + for k in _ADVISORY_SECURITY_JOBS + if context_of(k, jobs[k]) in required + ) + assert not promoted, ( + f"{promoted} is advisory in security.yml but present in the required set. Both advisory jobs " + "are cron/dispatch-only, so as a required context they would never report on a PR and would " + "block every merge (docs/CI.md, 'the required-but-absent trap'). Remove the `if:` gate first." + ) + + +def test_advisory_security_jobs_keep_continue_on_error() -> None: + """The mirror of the blocking assertion: an accidental promotion must also be a deliberate edit.""" + jobs = jobs_of(_SECURITY) + for key in sorted(_ADVISORY_SECURITY_JOBS): + assert jobs[key].get("continue-on-error") is True, ( + f"security.yml job {key!r} is advisory by design but no longer declares " + "`continue-on-error: true`. If this is a deliberate promotion, move it to " + "_BLOCKING_SECURITY_JOBS, add its context to .github/required-contexts.txt and branch " + "protection, and remove the schedule/dispatch `if:` gate so it reports on PRs." + ) + + +def test_required_jobs_carry_no_continue_on_error() -> None: + """Job-level AND step-level. Either one turns a red gate green before protection sees it.""" + offenders: list[str] = [] + examined = 0 + for (wf, key), job in _required_jobs().items(): + examined += 1 + if job.get("continue-on-error") not in (None, False): + offenders.append(f"{wf}:{key} — job-level continue-on-error") + for step in job.get("steps") or []: + if (step or {}).get("continue-on-error") not in (None, False): + name = (step or {}).get("name") or (step or {}).get("uses") or "" + offenders.append(f"{wf}:{key} — step {name!r} has continue-on-error") + # Liveness receipt. NOT `examined == len(required_contexts())`: the three `test (, py3.14)` + # contexts are ONE matrix job, so 12 contexts collapse to 10 jobs. Pinned rather than derived so + # that a change in the collapse — a matrix split, or a context that quietly stops resolving — + # forces a look here instead of passing on a self-consistent count. + print( + f"[security-posture] examined {examined} distinct jobs backing " + f"{len(required_contexts())} required contexts" + ) + assert examined == 10, ( + f"expected the 12 required contexts to resolve to 10 distinct jobs (the 3 `test` legs share one " + f"matrix job); got {examined}. If the workflow layout genuinely changed, update this count." + ) + assert not offenders, ( + "a REQUIRED status check cannot fail, so it gates nothing:\n " + + "\n ".join(offenders) + + "\nGitHub reports a continue-on-error job as SUCCESS, so branch protection stays green while " + "the scanner's findings are discarded. To take a gate off the merge path, remove its context " + "from branch protection and .github/required-contexts.txt — do not neuter it in place." + ) + + +def test_required_jobs_have_no_neutered_steps() -> None: + """`|| true` / `--exit-zero` discard the exit code without touching continue-on-error. + + This is how mutation testing spent months reporting success in 37 seconds while measuring nothing + (docs/CI.md, 'Gate liveness'). The same one-token edit inside a required security job would be + invisible. + """ + offenders: list[str] = [] + scanned_steps = 0 + for (wf, key), job in _required_jobs().items(): + for step in job.get("steps") or []: + run = str((step or {}).get("run") or "") + if not run: + continue + scanned_steps += 1 + for pattern, label in _NEUTERING: + if pattern.search(run): + name = (step or {}).get("name") or "" + offenders.append(f"{wf}:{key} — step {name!r} contains {label}") + print(f"[security-posture] scanned {scanned_steps} run steps across required jobs") + assert scanned_steps > 0, "scanned ZERO run steps — the parser stopped seeing steps, not a pass" + assert not offenders, ( + "a required job discards a non-zero exit code:\n " + + "\n ".join(offenders) + + "\nIf a specific command legitimately tolerates failure (log tailing, best-effort cleanup), " + "confine it to its own step so the GATING command's exit code is still the step's." + ) + + +def test_required_jobs_declare_no_skippable_job_level_if() -> None: + """A job-level `if:` is the other way a required context silently never reports.""" + offenders: list[str] = [] + for (wf, key), job in _required_jobs().items(): + expr = job.get("if") + if expr is None: + continue + allowed = _JOB_IF_ALLOWLIST.get((wf, key)) + if allowed is None or str(expr).strip() != allowed: + offenders.append(f"{wf}:{key} — if: {str(expr).strip()!r} (allowlisted: {allowed!r})") + assert not offenders, ( + "a required job carries an unreviewed job-level `if:`:\n " + + "\n ".join(offenders) + + "\nAn `if:` that evaluates false SKIPS the job, and a required context that does not report " + "blocks the PR forever. Gate the expensive STEPS instead — that is what ci.yml's `test` leg " + "does with `needs: changes`, keeping the context present and green on a docs-only PR. If the " + "expression genuinely cannot skip a pull_request run, add it to _JOB_IF_ALLOWLIST with the " + "reason." + ) + + +def test_the_downgrade_note_points_at_this_guard() -> None: + """security.yml documents the downgrade. It must also say what will now refuse it. + + The note is accurate and worth keeping — but read alone it presents the edit as a supported + operation, which is precisely how it would come to be applied to a required gate. + """ + text = (WORKFLOWS / _SECURITY).read_text(encoding="utf-8") + if "continue-on-error: true` back to its job" in text: + assert "test_security_posture" in text, ( + "security.yml still describes adding `continue-on-error: true` to downgrade a gate without " + "noting that tests/test_security_posture.py refuses it for any job in the required set. " + "Point the reader at the guard, so the documented remedy and the enforced rule agree." + ) From 7b224fa93e0e1f8eda5459f2bc0d8f058a9932f8 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 16:02:20 -0500 Subject: [PATCH 2/5] feat(security): reject a dependency name that PyPI cannot vouch for pip-audit answers "does this pinned version have a known CVE". It cannot answer "is this package the one I meant". A freshly registered hallucinated name has no advisory, so it resolves through `uv lock`, hashes into requirements.lock and installs under --require-hashes -- every DEP-1 control passes it clean. The only thing in its way was that `uv lock` cannot resolve a name PyPI does not serve, which is a resolver side effect rather than a control, and is exactly the case slopsquatting is built to defeat. That matters here specifically because this project's dependencies are chosen by an AI. Spracklen et al. (USENIX Security 2025) measured 19.7% hallucinated package references, 58% of invented names recurring across runs -- predictable, therefore registrable. docs/Secure_AI_Development_Standards.md has carried this as the "highest-priority deferred gate" while recording that verify-before-add is "enforced only by the human remembering", and its own specified interim control -- a verify-before-add line in the PR template -- had never been added either. scripts/security/new_dependency_check.py sweeps every distribution declared in pyproject.toml, core AND every extra (an extra is where a niche, plausible-sounding name lands), and fails on a name that does not exist, publishes no files, was first released inside a 90-day floor, or is served under another project's canonical name. Age is taken from the EARLIEST release across all versions, not the current one: otherwise a squatter who registers a name and publishes later looks established, and a decade-old project looks fresh the day it ships. It runs as a step inside the already-REQUIRED pip-audit job rather than as a new context, so it blocks today with no branch-protection change -- the same reasoning as the ledger-gate backstop in ci.yml, and the lesson from this branch's first commit: a hard-failing job that is not a required context does not stop auto-merge, it only looks like it does. Fail-closed throughout. Unreachable PyPI exits 2 rather than passing, because a supply-chain gate that goes green while blind is worse than no gate. Examining zero distributions also exits 2, so a pyproject schema change cannot read as a clean sweep. The transport contract is explicit -- only a 404 is "absent", everything else raises -- because mapping a timeout onto None would turn a PyPI outage into a wall of false "hallucinated dependency" failures. Verified rather than assumed: 41 distributions clean against live PyPI, and an injected `hl7-dicom-bridge>=1.0` rejected end-to-end. 29 offline tests drive the shipped functions through the fetch seam, never a local copy of the rule. Its blind spot is documented rather than glossed. The gate cannot tell that a real package is the WRONG one. The counter-example is in this tree: pyproject warns that py-webauthn is AS207960's project while the intended name is exactly webauthn -- and measured against live PyPI today, py-webauthn exists, publishes, is years old and is canonically named, so every check passes it. That class stays on human verify-before-add plus the dated vet note, the module docstring says so, and a named test pins it -- following tests/test_gate_liveness.py, which likewise documents its own blindness instead of hiding it. The standard's deferred-gate entry now records what is built and what remains (no check yet REQUIRES the vet note on a new dependency; that needs a merge-base diff that fails loudly rather than silently finding no base). --- .github/PULL_REQUEST_TEMPLATE.md | 7 + .github/workflows/security.yml | 18 ++ docs/Secure_AI_Development_Standards.md | 5 +- scripts/security/new_dependency_check.py | 341 +++++++++++++++++++++++ tests/test_new_dependency_check.py | 338 ++++++++++++++++++++++ 5 files changed, 707 insertions(+), 2 deletions(-) create mode 100644 scripts/security/new_dependency_check.py create mode 100644 tests/test_new_dependency_check.py diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index bad33b7d..380b7595 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -29,6 +29,13 @@ Implements: BACKLOG # - [ ] **No real PHI or customer data** anywhere in the diff, tests, fixtures, screenshots, or commit messages — synthetic HL7 only (`python -m messagefoundry generate`). - [ ] Tests added/updated for new behavior. +- [ ] **Any new dependency was verified before adding it** — real, reputable, and the *exact* intended + distribution — then re-locked (`uv lock` / `uv export`), with a dated vet note beside it in + `pyproject.toml`. AI-suggested package names are frequently hallucinated or typosquatted. + CI (`scripts/security/new_dependency_check.py`, in the required `pip-audit` job) rejects a name + that does not exist, publishes nothing, was registered in the last 90 days, or is served under + another project's canonical name — but it **cannot** tell that a real package is the wrong one + (see the `[webauthn]` extra's note in `pyproject.toml`). That judgement is this checkbox. - [ ] Gates pass locally: `ruff check .`, `ruff format --check .`, `mypy messagefoundry`, and `pytest -q` (`QT_QPA_PLATFORM=offscreen` for console tests). `python -m messagefoundry check` is green. - [ ] Uses **Connection / Router / Handler** vocabulary; no new declarative "channel" element; no diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 9195a622..5e2179d4 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -89,6 +89,24 @@ jobs: pip install pip-audit # Audit the committed, pinned set — reproducible (vs auditing a fresh latest-resolve). pip-audit -r requirements.lock --desc + # ANTI-SLOPSQUAT. Deliberately a STEP in this already-REQUIRED job rather than a new context: it + # blocks today with no branch-protection change, the same reasoning as the ledger-gate backstop + # in ci.yml. (A hard-failing job that is NOT a required context does not stop auto-merge — it + # only looks like it does.) + # + # This answers the question pip-audit above structurally cannot. pip-audit asks "does this pinned + # version have a known CVE"; a freshly registered hallucinated name has no advisory, so it + # resolves, locks, hashes and installs through every DEP-1 control clean. This project's + # dependencies are chosen by an AI, and docs/Secure_AI_Development_Standards.md has carried the + # check as the "highest-priority deferred gate" while verify-before-add stayed "enforced only by + # the human remembering". + # + # Stdlib-only (no install). Fails CLOSED: if PyPI is unreachable it exits 2 rather than passing, + # and it exits 2 if it examined zero distributions, so a schema change cannot read as a clean + # sweep. Its known blind spot — a real package that is not the INTENDED one — is documented in + # the script and pinned by tests/test_new_dependency_check.py. + - name: Verify every declared dependency is a real, established distribution + run: python scripts/security/new_dependency_check.py npm-audit: name: npm-audit (ide dependency vulnerabilities) diff --git a/docs/Secure_AI_Development_Standards.md b/docs/Secure_AI_Development_Standards.md index e864f867..0e38618e 100644 --- a/docs/Secure_AI_Development_Standards.md +++ b/docs/Secure_AI_Development_Standards.md @@ -434,7 +434,8 @@ The repo's tiered-honesty taxonomy, applied to the **dev-process tooling itself* - PreToolUse [`block-blanket-git-stage.ps1`](../scripts/hooks/block-blanket-git-stage.ps1) (Bash + PowerShell, fail-open). - [`.claude/settings.json`](../.claude/settings.json) secrets/keys/`*.db` **path-based** deny-list + destructive-command denies. - Blocking security CI: bandit, semgrep ([`.semgrep/messagefoundry.yml`](../.semgrep/messagefoundry.yml)), pip-audit, gitleaks, crypto-inventory, forbidden-content ([`scripts/security/scan_forbidden.py`](../scripts/security/scan_forbidden.py)). The CycloneDX **SBOM** job is **advisory** (`continue-on-error`), not blocking. -- **Dependency-CVE fast response (SSDF RV.2 evidence):** dependency-vulnerability metrics ([`vuln-metrics.yml`](../.github/workflows/vuln-metrics.yml)), scoped Dependabot auto-merge + supply-chain cooldown ([`dependabot-auto-merge.yml`](../.github/workflows/dependabot-auto-merge.yml)), auto lock-resync ([`dependabot-lock-resync.yml`](../.github/workflows/dependabot-lock-resync.yml)), and the adopter vulnerable-pin CI tripwire. The **`CI gate` roll-up** required check gates the conditional/matrix legs in [`ci.yml`](../.github/workflows/ci.yml). *(This is the **audit/known-CVE** posture — distinct from the still-deferred hallucinated/typosquatted **new-dependency-introduction** check below.)* +- **Dependency-CVE fast response (SSDF RV.2 evidence):** dependency-vulnerability metrics ([`vuln-metrics.yml`](../.github/workflows/vuln-metrics.yml)), scoped Dependabot auto-merge + supply-chain cooldown ([`dependabot-auto-merge.yml`](../.github/workflows/dependabot-auto-merge.yml)), auto lock-resync ([`dependabot-lock-resync.yml`](../.github/workflows/dependabot-lock-resync.yml)), and the adopter vulnerable-pin CI tripwire. The **`CI gate` roll-up** required check gates the conditional/matrix legs in [`ci.yml`](../.github/workflows/ci.yml). *(This is the **audit/known-CVE** posture. The distinct hallucinated/typosquatted **new-dependency-introduction** check is now built alongside it — [`new_dependency_check.py`](../scripts/security/new_dependency_check.py), a step in the same required `pip-audit` job — with a documented residual; see below.)* +- **Required-merge-gate integrity:** the required status-check set is recorded in [`.github/required-contexts.txt`](../.github/required-contexts.txt) and every in-repo claim about it is reconciled by [`tests/test_required_contexts.py`](../tests/test_required_contexts.py) (five statements disagreed with the live API, one naming a context string that matches no job). [`tests/test_security_posture.py`](../tests/test_security_posture.py) refuses `continue-on-error`, `|| true`/`--exit-zero`, and a skippable job-level `if:` on any job backing a required context — the one-line edit that leaves a required check green while its scanner's findings are discarded — and pins the advisory jobs the other way so an accidental promotion is equally deliberate. `actionlint` (pre-commit hook + a checksum-verified step in [`zizmor.yml`](../.github/workflows/zizmor.yml)) catches the invalid-`${{ }}` class that aborts workflow compilation so required contexts silently never appear. - [`messagefoundry check`](../messagefoundry/checks.py) exit-coded validate + dryrun gate. - SessionStart worktree-context hook; worktree scripts ([`new.ps1`/`remove.ps1`](../scripts/worktree/)); shared AI project memory (facts only); synthetic generators; the dependency-boundary test ([`tests/test_dependency_boundaries.py`](../tests/test_dependency_boundaries.py)). - `Co-Authored-By` provenance trailer — **by convention** (model/version, commit granularity). @@ -443,7 +444,7 @@ The repo's tiered-honesty taxonomy, applied to the **dev-process tooling itself* **Designed but deferred (Build trigger + Design record):** - **A dedicated SPDX-header CI test — *highest-priority deferred gate*.** SPDX-header presence is today enforced only by **convention** — there is no test asserting it. ("AI memory" is **not** a control: it fails silently the moment context rots, §6.3.) *Interim compensating control:* a manual checklist item in the [PR template](../.github/PULL_REQUEST_TEMPLATE.md) (add if absent) — a deterministic-ish artifact, **not** "AI memory" — plus the leak/forbidden scan. *Build trigger (cheap — do this first):* a trivial pytest walking first-party `.py` files. *Design record:* Appendix A.6. -- **A new-dependency-introduction check — *highest-priority deferred gate*.** `pip-audit` finds CVEs in *already-pinned* packages — it does **not** flag a freshly **hallucinated/typosquatted** name with no advisory. Verify-before-add (§6.4) is today enforced only by the human remembering — exactly the cheap deterministic gate that should stand in for the absent second reviewer. *Interim compensating control:* a manual verify-before-add line in the [PR template](../.github/PULL_REQUEST_TEMPLATE.md) (add if absent). *Build trigger (cheap — do this first):* a pytest/CI step diffing `pyproject` deps against the prior commit and requiring a recorded justification. *Design record:* Appendix A.6. +- **A new-dependency-introduction check — *partly BUILT 2026-07-29, residual deferred*.** `pip-audit` finds CVEs in *already-pinned* packages — it does **not** flag a freshly **hallucinated/typosquatted** name with no advisory. **Now built:** [`scripts/security/new_dependency_check.py`](../scripts/security/new_dependency_check.py) sweeps every distribution declared in `pyproject.toml` (core **and** every extra) against PyPI and fails on a name that does not exist, publishes no files, was first released inside a 90-day floor, or is served under another project's canonical name. It runs as a step inside the **already-required `pip-audit` job**, so it is blocking today without a new branch-protection context; it is stdlib-only and **fails closed** (unreachable PyPI ⇒ exit 2; zero distributions examined ⇒ exit 2, so a schema change cannot read as a clean sweep). Verified end-to-end against live PyPI: 41 distributions clean, and an injected `hl7-dicom-bridge>=1.0` rejected. Tests: [`tests/test_new_dependency_check.py`](../tests/test_new_dependency_check.py). *Interim compensating control (also now added):* the verify-before-add line in the [PR template](../.github/PULL_REQUEST_TEMPLATE.md). **Residual deferred —** two things, stated because the gate must not be read as broader than it is: (1) it cannot detect a **real package that is not the intended one** (measured 2026-07-29: `py-webauthn` exists, publishes, is years old and is canonically named, yet is AS207960's project rather than the intended `webauthn` — every check passes it; the blind spot is pinned by a named test), which leaves that class on human verify-before-add plus the dated vet note in `pyproject.toml`; and (2) no automated check yet **requires** that vet note on a newly-introduced dependency — the original "diff `pyproject` against the prior commit and require a recorded justification" trigger. *Build trigger for the residual:* a merge-base `pyproject` diff (needs a non-shallow base fetch that fails loudly rather than silently finding no base). *Design record:* Appendix A.6. - **A `Co-Authored-By` trailer-format CI check** (presence/format) **and** a **line/hunk-level** AI-authorship record. *Build trigger:* a commit-msg lint; a hunk-attribution mechanism. *Design record:* Appendix A.6. - **A retained-transcript provenance store** (PHI/secret-free, access-controlled). - **Code-quality measurement gates — specified by the [Code Quality & Anti-Slop Standards](Code_Quality_Standards.md) §5.** Five *quality* (not security) gates, none built today, that measure the shallow-test and copy-instead-of-abstract slop modes (§3): **mutation testing on changed code** (the highest-leverage — it adversarially checks whether tests assert anything, the one control that most directly compensates for the solo-maintainer review deviation, [A.6](#a6-documented-deviations)), **clone-detection on the diff**, **diff-coverage visibility**, an **advisory `C901` complexity triage**, and an **expanded ruff ruleset** (`B`/`C4`/`SIM`/`UP`/`I`). *Build trigger (cheapest first):* expand ruff `select` + advisory `C901` (local + CI) → diff-coverage + clone-detection (CI) → mutation-on-diff (CI-first, advisory). *Anti-metric rule (binds here):* never gate on line-coverage % or raw complexity alone — measure structure/behavior, not a scoreboard. *Design record:* [Code Quality & Anti-Slop Standards](Code_Quality_Standards.md) §5 + Appendix A.3. diff --git a/scripts/security/new_dependency_check.py b/scripts/security/new_dependency_check.py new file mode 100644 index 00000000..f5971269 --- /dev/null +++ b/scripts/security/new_dependency_check.py @@ -0,0 +1,341 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Verify every dependency this project declares is a REAL, established distribution. + +THE GAP THIS CLOSES. ``pip-audit`` answers "does this pinned version have a known CVE". It cannot +answer "is this package the one I meant, or a name a model invented". A freshly registered +hallucinated name has no advisory, so it audits clean, resolves through ``uv lock``, hashes into +``requirements.lock`` and installs under ``--require-hashes`` — every DEP-1 control passes it. The only +thing standing in the way today is that ``uv lock`` cannot resolve a name PyPI does not serve, which is +a resolver side effect rather than a control, and is exactly the case slopsquatting is built to defeat. + +WHY IT MATTERS HERE SPECIFICALLY. This project's dependencies are chosen by an AI. Spracklen et al. +(USENIX Security 2025) measured 19.7% hallucinated package references across 2.23M suggestions, with +58% of invented names recurring across runs — i.e. predictable, therefore registrable. A 2026 +frontier-model replication found the rate compressed to ~4.6-6.1% but identified 127 names that five +independent models invent IDENTICALLY. ``docs/Secure_AI_Development_Standards.md`` already names this +the "highest-priority deferred gate" and records that verify-before-add "is today enforced only by the +human remembering". CLAUDE.md section 5 states the rule in prose, and prose is what a session +rationalizes past at 2am. + +WHAT IT CHECKS, per declared distribution: + * EXISTS -- PyPI serves a project page for the name at all. + * HAS RELEASES -- at least one release with files. A registered-but-empty placeholder is the shape + a squatter parks on a predicted name. + * ESTABLISHED -- its FIRST release is older than ``--min-age-days``. Every real dependency here is + years old, so this is quiet in practice and fires precisely on a name registered + to catch a hallucination. + * CANONICAL -- the name PyPI considers canonical normalizes to the declared name, so a spec + that is being served under some OTHER project's canonical name (an alias or a + redirect) is flagged rather than silently accepted. + +WHAT THIS CANNOT CATCH -- stated plainly, because a supply-chain control that overstates its reach is +worse than a modest one. This gate answers "is this name a real, established, self-consistent +distribution". It cannot answer "is it the one you MEANT". The live counter-example is in this very +tree: ``pyproject.toml`` warns that "py_webauthn"/"py-webauthn" resolve to AS207960's project while the +intended distribution is exactly "webauthn". Verified 2026-07-29 -- ``py-webauthn`` exists, publishes, +is years old, and PyPI serves it under precisely that canonical name, so EVERY check above passes it. +It is simply a different maintainer's WebAuthn library. + +So the taxonomy is: this gate covers the invented name, the parked name, the freshly registered name +and the aliased name. The plausible-but-wrong name -- a real package that is not your intended one -- +is covered only by human verify-before-add (CLAUDE.md section 5) plus the dated vet note beside each +dependency in ``pyproject.toml``. ``tests/test_new_dependency_check.py`` pins that blind spot with a +named test rather than leaving it to be discovered, following the precedent in +``tests/test_gate_liveness.py``, which documents its own reconciliation sum's blindness instead of +hiding it. + +FAIL-CLOSED. A network error, a rate limit or an unparsable response is a FAILURE, never a pass. A +supply-chain gate that degrades to green when it cannot see is the failure mode this repo has already +catalogued three times (docs/CI.md, "Gate liveness"): a check that reports success while measuring +nothing. For the same reason the summary prints the number of distributions actually EXAMINED, and +exits non-zero if that number is zero. + +USAGE + python scripts/security/new_dependency_check.py # sweep pyproject.toml + python scripts/security/new_dependency_check.py --min-age-days 180 + python scripts/security/new_dependency_check.py --pyproject path/to/pyproject.toml +""" + +from __future__ import annotations + +import argparse +import json +import re +import sys +import tomllib +import urllib.error +import urllib.parse +import urllib.request +from collections.abc import Callable +from dataclasses import dataclass +from datetime import UTC, datetime +from pathlib import Path + +_ROOT = Path(__file__).resolve().parents[2] + +# A distribution registered less recently than this is treated as established. 90 days is chosen +# against the attack, not arbitrarily: slopsquatting requires the attacker to register a PREDICTED +# name and wait for a victim, so the window between registration and first use is what this closes. It +# is also comfortably below the age of every dependency in this tree, so it costs nothing today. +DEFAULT_MIN_AGE_DAYS = 90 + +_PYPI_JSON = "https://pypi.org/pypi/{name}/json" + +# PEP 508 name grammar. Anything outside this never reaches a URL. +_SAFE_NAME = re.compile(r"^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$") + +# Reviewed exceptions: distribution -> reason. Deliberately a code constant rather than a file, so +# adding one shows up in a diff with its justification attached. +ALLOWLIST: dict[str, str] = {} + + +def normalize(name: str) -> str: + """PEP 503 normalization -- the identity PyPI actually compares names under.""" + return re.sub(r"[-_.]+", "-", name).lower() + + +def requirement_name(spec: str) -> str | None: + """The distribution name from a PEP 508 requirement string, or None if there isn't one. + + ``"pynetdicom>=3.0.4,<4"`` -> ``pynetdicom``; ``"fhir.resources>=7.1.0"`` -> ``fhir.resources``. + Extras and environment markers are stripped: only the name is being vetted. + """ + text = spec.strip() + if not text or text.startswith("#"): + return None + text = text.split(";", 1)[0] # environment marker + text = text.split("[", 1)[0] # extras + match = re.match(r"^[A-Za-z0-9][A-Za-z0-9._-]*", text) + return match.group(0) if match else None + + +def declared_distributions(pyproject_text: str) -> dict[str, str]: + """Every distribution this project declares -> the spec it was declared with. + + Covers ``[project].dependencies`` AND every ``[project.optional-dependencies]`` extra, because an + extra is exactly where a niche, plausible-sounding, hallucination-prone name lands. + """ + data = tomllib.loads(pyproject_text) + project = data.get("project") or {} + specs: list[str] = list(project.get("dependencies") or []) + for extra_specs in (project.get("optional-dependencies") or {}).values(): + specs.extend(extra_specs or []) + + found: dict[str, str] = {} + for spec in specs: + name = requirement_name(spec) + if name is not None: + found.setdefault(name, spec) + return found + + +@dataclass(frozen=True) +class Finding: + distribution: str + problem: str + detail: str + + +PyPIFetch = Callable[[str], dict | None] +"""Fetch the PyPI JSON payload for a name. Returns None for "no such project" (404). + +Anything else -- a timeout, a 5xx, a rate limit, malformed JSON -- must RAISE. Returning None on a +transport error would turn every dependency into a "does not exist" failure, and swallowing it would +turn the gate green while blind; neither is acceptable, so the contract is explicit. +""" + + +def _first_release_date(payload: dict) -> datetime | None: + """The earliest upload time across all releases, or None if no release has files. + + Earliest across ALL releases, not the current version's: a squatter who registers a name and later + publishes a version would otherwise look established, and a legitimate project's newest release + says nothing about how long the NAME has existed. + """ + earliest: datetime | None = None + for files in (payload.get("releases") or {}).values(): + for file_info in files or []: + raw = file_info.get("upload_time_iso_8601") or file_info.get("upload_time") + if not raw: + continue + try: + # fromisoformat parses a trailing "Z" natively on 3.11+, so no pre-substitution is + # needed; the project floor is 3.14. PyPI serves both `upload_time_iso_8601` + # (Z-suffixed, UTC) and the naive `upload_time`, which is why tzinfo is normalized + # below rather than assumed. + stamp = datetime.fromisoformat(raw) + except ValueError: + continue + if stamp.tzinfo is None: + stamp = stamp.replace(tzinfo=UTC) + if earliest is None or stamp < earliest: + earliest = stamp + return earliest + + +def vet( + declared: dict[str, str], + fetch: PyPIFetch, + *, + now: datetime, + min_age_days: int = DEFAULT_MIN_AGE_DAYS, + allowlist: dict[str, str] | None = None, +) -> tuple[list[Finding], int]: + """Vet every declared distribution. Returns (findings, number examined). + + The count is returned rather than inferred so the caller can prove the sweep was not empty -- + "zero findings" and "nothing was checked" are indistinguishable from the exit code alone. + """ + allowed = allowlist if allowlist is not None else ALLOWLIST + findings: list[Finding] = [] + examined = 0 + + for name in sorted(declared, key=normalize): + if normalize(name) in {normalize(k) for k in allowed}: + continue + examined += 1 + + if not _SAFE_NAME.match(name): + findings.append( + Finding( + name, "malformed name", f"{name!r} is not a valid PEP 508 distribution name" + ) + ) + continue + + payload = fetch(name) + if payload is None: + findings.append( + Finding( + name, + "does not exist on PyPI", + "no project is published under this name. This is the signature of a hallucinated " + "or mistyped dependency -- verify the INTENDED name before changing anything.", + ) + ) + continue + + info = payload.get("info") or {} + canonical = str(info.get("name") or "") + if canonical and normalize(canonical) != normalize(name): + findings.append( + Finding( + name, + "resolves to a different project", + f"PyPI serves this name under the canonical name {canonical!r}. An alias or " + "redirect landing on another project is worth a deliberate look -- confirm which " + "distribution you actually meant. (Note this does NOT catch a real package that " + "is simply the wrong one: see the module docstring's limitations.)", + ) + ) + continue + + first = _first_release_date(payload) + if first is None: + findings.append( + Finding( + name, + "no release history", + "the name is registered but publishes no files. A registered-but-empty project is " + "what a squatter parks on a predicted name.", + ) + ) + continue + + age_days = (now - first).days + if age_days < min_age_days: + findings.append( + Finding( + name, + "freshly registered", + f"first release {first.date().isoformat()} ({age_days}d ago) is younger than the " + f"{min_age_days}d floor. A brand-new distribution appearing as a dependency is the " + "slopsquat shape; if it is genuinely new and genuinely intended, add it to " + "ALLOWLIST with a reason.", + ) + ) + + return findings, examined + + +def _http_fetch(name: str, *, timeout: float = 20.0) -> dict | None: + """Fetch PyPI's JSON for ``name``. None means 404; every other error raises (fail-closed).""" + url = _PYPI_JSON.format(name=urllib.parse.quote(name, safe="")) + # bandit owns this one. Older urllib call sites here carry a ruff `S310` suppression alongside the + # nosec, but flake8-bandit's `S` family is not in pyproject's ruff extend-select, so that half is + # inert -- and ruff's own RUF100 strips it on sight. Only the nosec is load-bearing. + # + # Why it is safe: the scheme is the module-level https://pypi.org literal and the only interpolated + # part is a percent-encoded path segment whose value already passed _SAFE_NAME above. B310's + # non-http(s)-scheme concern therefore cannot arise -- no scheme, host, or path traversal is + # reachable from a dependency name. + request = urllib.request.Request( # nosec B310 — fixed https://pypi.org literal + url, + headers={"Accept": "application/json", "User-Agent": "messagefoundry-dep-vet"}, + ) + try: + with urllib.request.urlopen(request, timeout=timeout) as response: # nosec B310 + return json.loads(response.read().decode("utf-8")) + except urllib.error.HTTPError as exc: + if exc.code == 404: + return None + raise + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter + ) + parser.add_argument("--pyproject", type=Path, default=_ROOT / "pyproject.toml") + parser.add_argument("--min-age-days", type=int, default=DEFAULT_MIN_AGE_DAYS) + args = parser.parse_args(argv) + + declared = declared_distributions(args.pyproject.read_text(encoding="utf-8")) + try: + findings, examined = vet( + declared, _http_fetch, now=datetime.now(UTC), min_age_days=args.min_age_days + ) + except (urllib.error.URLError, TimeoutError, json.JSONDecodeError, OSError) as exc: + # FAIL CLOSED. Passing here would mean "PyPI was unreachable, so every dependency is fine". + print( + f"::error::dependency vetting could not reach PyPI ({exc!r}). Treating this as a FAILURE: " + "a supply-chain gate that goes green while blind is worse than no gate.", + file=sys.stderr, + ) + return 2 + + # Liveness receipt: report the number EXAMINED, not the number of problems. "0 findings" and "the + # parser stopped seeing dependencies" produce the same exit code otherwise. + print(f"new_dependency_check: examined {examined} distributions from {args.pyproject}") + if examined == 0: + print( + "::error::examined ZERO distributions. pyproject.toml parsed but yielded no dependency " + "names -- the schema moved under the parser. This is a broken gate, not a clean sweep.", + file=sys.stderr, + ) + return 2 + + if findings: + for finding in findings: + print( + f"::error::{finding.distribution}: {finding.problem} -- {finding.detail}", + file=sys.stderr, + ) + print( + f"\n{len(findings)} dependency name(s) failed vetting. Verify the INTENDED distribution " + "(real, reputable, exact name) before touching pyproject.toml, then re-lock " + "(CLAUDE.md section 5).", + file=sys.stderr, + ) + return 1 + + print( + f"new_dependency_check: all {examined} distributions exist, publish, and are established." + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_new_dependency_check.py b/tests/test_new_dependency_check.py new file mode 100644 index 00000000..117718a3 --- /dev/null +++ b/tests/test_new_dependency_check.py @@ -0,0 +1,338 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Tests for the new-dependency (anti-slopsquat) gate. + +Every test drives the SHIPPED functions from ``scripts/security/new_dependency_check.py`` through an +injected ``fetch``, never a local reimplementation of the rule. A guard whose test asserts its own copy +of the logic is a guard nobody has ever run. + +The fetch seam is also what makes these offline and deterministic: the real gate talks to PyPI, and a +unit test that did the same would be a network flake in the required leg. +""" + +from __future__ import annotations + +import json +import urllib.error +from datetime import UTC, datetime, timedelta +from pathlib import Path + +import pytest + +from scripts.security.new_dependency_check import ( + Finding, + declared_distributions, + main, + normalize, + requirement_name, + vet, +) + +_ROOT = Path(__file__).resolve().parents[1] +_NOW = datetime(2026, 7, 29, tzinfo=UTC) + + +def _payload(name: str, *, first_release: datetime | None, canonical: str | None = None) -> dict: + releases: dict[str, list[dict]] = {} + if first_release is not None: + releases["1.0.0"] = [{"upload_time_iso_8601": first_release.isoformat()}] + return {"info": {"name": canonical or name}, "releases": releases} + + +def _fetch_all_good(name: str) -> dict: + return _payload(name, first_release=_NOW - timedelta(days=2000)) + + +# --- the parsing layer ------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("spec", "expected"), + [ + ("pynetdicom>=3.0.4,<4", "pynetdicom"), + ("fhir.resources>=7.1.0", "fhir.resources"), + ("webauthn>=3.0.0,<4", "webauthn"), + ("annotated-types<0.8", "annotated-types"), + ("uvicorn[standard]>=0.30", "uvicorn"), + ("tomli; python_version < '3.11'", "tomli"), + (" hvac>=2.3.0 ", "hvac"), + ("", None), + ], +) +def test_requirement_name_extracts_the_distribution(spec: str, expected: str | None) -> None: + assert requirement_name(spec) == expected + + +def test_normalize_matches_pep503() -> None: + """The identity PyPI compares under — and the one that bit this repo already. + + pyproject records that "py_webauthn"/"py-webauthn" normalize to an UNRELATED project while the + intended distribution is exactly "webauthn". Normalization is why those are the same name to PyPI. + """ + assert normalize("py_webauthn") == normalize("py-webauthn") == "py-webauthn" + assert normalize("webauthn") == "webauthn" + assert normalize("fhir.resources") == "fhir-resources" + assert normalize("Annotated_Types") == "annotated-types" + + +def test_declared_distributions_covers_extras_not_just_core() -> None: + """An extra is exactly where a niche, plausible-sounding, hallucination-prone name lands.""" + found = declared_distributions( + """ + [project] + dependencies = ["fastapi>=0.100", "hl7>=0.4"] + + [project.optional-dependencies] + dicom = ["pynetdicom>=3.0.4,<4", "pydicom>=3.0.2,<4"] + vault = ["hvac>=2.3.0"] + """ + ) + assert set(found) == {"fastapi", "hl7", "pynetdicom", "pydicom", "hvac"} + + +# --- the vetting rules: each failure mode proven, not assumed ---------------------------------- + + +def test_a_clean_tree_produces_no_findings_and_a_nonzero_count() -> None: + findings, examined = vet({"fastapi": "fastapi>=0.100"}, _fetch_all_good, now=_NOW) + assert findings == [] + assert examined == 1, "a clean sweep must still report what it examined" + + +def test_a_nonexistent_name_is_caught() -> None: + """The hallucinated-dependency case: PyPI serves no project under the name.""" + findings, examined = vet( + {"hl7-fhir-toolkit": "hl7-fhir-toolkit>=1.0"}, lambda _: None, now=_NOW + ) + assert examined == 1 + assert [f.problem for f in findings] == ["does not exist on PyPI"] + + +def test_a_registered_but_empty_project_is_caught() -> None: + """A parked name with no files — the shape a squatter leaves on a predicted name.""" + findings, _ = vet( + {"messagefoundry-helpers": "messagefoundry-helpers>=1.0"}, + lambda name: _payload(name, first_release=None), + now=_NOW, + ) + assert [f.problem for f in findings] == ["no release history"] + + +def test_a_freshly_registered_distribution_is_caught() -> None: + """The actual slopsquat: a real, published package registered days ago.""" + findings, _ = vet( + {"hl7-parse-utils": "hl7-parse-utils>=0.1"}, + lambda name: _payload(name, first_release=_NOW - timedelta(days=9)), + now=_NOW, + ) + assert [f.problem for f in findings] == ["freshly registered"] + assert "9d ago" in findings[0].detail + + +def test_age_is_measured_from_the_earliest_release_not_the_newest() -> None: + """A squatter who registers a name then publishes later must not look established. + + Taking the CURRENT version's upload date would call a name registered yesterday "established" the + moment it cut a second release, and would call a decade-old project "fresh" the day it ships. + """ + old_name_new_release = { + "info": {"name": "settled"}, + "releases": { + "0.1.0": [{"upload_time_iso_8601": (_NOW - timedelta(days=3000)).isoformat()}], + "9.9.9": [{"upload_time_iso_8601": (_NOW - timedelta(days=1)).isoformat()}], + }, + } + findings, _ = vet({"settled": "settled>=1"}, lambda _: old_name_new_release, now=_NOW) + assert findings == [], "a long-established project that just released must not be flagged" + + +def test_a_name_served_under_a_different_canonical_name_is_caught() -> None: + """An alias/redirect landing on another project. + + This is the aliased-name case, NOT the wrong-package case — see the blind-spot test below. + """ + findings, _ = vet( + {"some-alias": "some-alias>=3.0.0"}, + lambda name: _payload( + name, first_release=_NOW - timedelta(days=2000), canonical="other-project" + ), + now=_NOW, + ) + assert [f.problem for f in findings] == ["resolves to a different project"] + + +def test_documented_blind_spot_a_real_but_wrong_package_passes_every_check() -> None: + """THE LIMIT OF THIS GATE, pinned so it is known rather than discovered. + + ``pyproject.toml`` warns that "py_webauthn"/"py-webauthn" resolve to AS207960's project while the + intended distribution is exactly "webauthn". Verified against live PyPI on 2026-07-29: + ``py-webauthn`` exists, publishes files, is years old, and is served under precisely that canonical + name. So every check here passes it — it is simply a different maintainer's WebAuthn library. + + A registry cannot answer "is this the package you meant"; that needs intent. The compensating + controls are human verify-before-add (CLAUDE.md section 5) and the dated vet note beside each + dependency. This test exists so nobody reads the gate as covering that class, following + tests/test_gate_liveness.py, which likewise documents its own blindness instead of hiding it. + """ + findings, examined = vet( + {"py-webauthn": "py-webauthn>=3.0.0"}, + # Exactly what PyPI really returns for this name: canonical, established, publishing. + lambda name: _payload( + name, first_release=_NOW - timedelta(days=2000), canonical="py-webauthn" + ), + now=_NOW, + ) + assert examined == 1 + assert findings == [], ( + "this gate does NOT detect a real-but-wrong package. If a future change makes it do so, that is " + "a genuine improvement — update this test and the module docstring's limitations section " + "together, rather than deleting the assertion." + ) + + +def test_a_malformed_name_never_reaches_the_network() -> None: + """Validated before it is interpolated into a URL, so no scheme or host can be injected.""" + + def exploding_fetch(_: str) -> dict: + raise AssertionError("fetch must not be called for a malformed name") + + findings, _ = vet({"evil name/../x": "evil name/../x"}, exploding_fetch, now=_NOW) + assert [f.problem for f in findings] == ["malformed name"] + + +def test_the_allowlist_skips_a_name_and_does_not_count_it() -> None: + findings, examined = vet( + {"brand-new-thing": "brand-new-thing>=0.1"}, + lambda name: _payload(name, first_release=_NOW - timedelta(days=1)), + now=_NOW, + allowlist={"brand_new_thing": "reviewed 2026-07-29 — deliberately new, vendor-published"}, + ) + assert findings == [] + assert examined == 0, "an allowlisted name is skipped, and must not inflate the examined count" + + +def test_findings_are_reported_for_every_bad_name_not_just_the_first() -> None: + """A gate that stops at the first problem hides the rest of the diff's problems.""" + bad = {"ghost-one": "ghost-one>=1", "ghost-two": "ghost-two>=1", "fastapi": "fastapi>=0.1"} + + def fetch(name: str) -> dict | None: + return None if name.startswith("ghost") else _fetch_all_good(name) + + findings, examined = vet(bad, fetch, now=_NOW) + assert examined == 3 + assert sorted(f.distribution for f in findings) == ["ghost-one", "ghost-two"] + + +# --- fail-closed behaviour --------------------------------------------------------------------- + + +def test_a_transport_error_propagates_rather_than_reading_as_absent() -> None: + """A 500 or a timeout must NOT be reported as "the package does not exist". + + Mapping a transport error onto None would turn a PyPI outage into a wall of false + "hallucinated dependency" failures; swallowing it would turn the gate green while blind. The + contract is that only a 404 is None and everything else raises. + """ + + def flaky_fetch(_: str) -> dict: + raise urllib.error.URLError("connection reset") + + with pytest.raises(urllib.error.URLError): + vet({"fastapi": "fastapi>=0.1"}, flaky_fetch, now=_NOW) + + +def test_main_exits_2_when_pypi_is_unreachable(monkeypatch: pytest.MonkeyPatch) -> None: + """Fail CLOSED at the CLI boundary: unreachable is a failure, never a pass.""" + import scripts.security.new_dependency_check as mod + + def unreachable(_name: str, **_kw: object) -> dict: + raise urllib.error.URLError("no route to host") + + monkeypatch.setattr(mod, "_http_fetch", unreachable) + assert main([]) == 2 + + +def test_main_exits_2_when_the_sweep_examines_nothing( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """ "Zero findings" must never be reachable by "zero dependencies parsed".""" + import scripts.security.new_dependency_check as mod + + monkeypatch.setattr(mod, "_http_fetch", lambda name, **_kw: _fetch_all_good(name)) + empty = tmp_path / "pyproject.toml" + empty.write_text('[project]\nname = "x"\n', encoding="utf-8") + assert main(["--pyproject", str(empty)]) == 2 + + +def test_main_exits_1_on_a_hallucinated_name( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + import scripts.security.new_dependency_check as mod + + monkeypatch.setattr(mod, "_http_fetch", lambda _name, **_kw: None) + project = tmp_path / "pyproject.toml" + project.write_text( + '[project]\nname = "x"\ndependencies = ["totally-invented-pkg>=1"]\n', encoding="utf-8" + ) + assert main(["--pyproject", str(project)]) == 1 + + +def test_main_exits_0_on_the_real_pyproject_with_a_stubbed_registry( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The real file must parse and sweep cleanly — a schema change here is a broken gate.""" + import scripts.security.new_dependency_check as mod + + monkeypatch.setattr(mod, "_http_fetch", lambda name, **_kw: _fetch_all_good(name)) + assert main([]) == 0 + + +# --- the real tree, offline -------------------------------------------------------------------- + + +def test_the_real_pyproject_declares_a_plausible_number_of_distributions() -> None: + """Liveness against the actual file: if this collapses, the gate is sweeping almost nothing. + + A floor, not an exact count — adding a dependency must not require editing this test, but the + parser silently losing the optional-dependencies table must. + """ + declared = declared_distributions((_ROOT / "pyproject.toml").read_text(encoding="utf-8")) + assert len(declared) >= 25, ( + f"only {len(declared)} distributions parsed out of pyproject.toml: {sorted(declared)}. The " + "[project.optional-dependencies] table carries most of them, so a low count means the parser " + "stopped seeing the extras." + ) + # Spot-check one core dep and one extra, so a table being dropped entirely is caught by name. + assert "fastapi" in declared + assert "pynetdicom" in declared, "the [dicom] extra is not being read" + + +def test_every_finding_message_names_the_remedy() -> None: + """A failure a reader cannot act on gets suppressed rather than fixed.""" + findings, _ = vet( + {"ghost": "ghost>=1", "fresh": "fresh>=1"}, + lambda name: None if name == "ghost" else _payload(name, first_release=_NOW), + now=_NOW, + ) + assert len(findings) == 2 + for finding in findings: + assert len(finding.detail) > 40, f"{finding.problem} has no actionable detail" + + +def test_finding_is_hashable_so_callers_can_dedupe() -> None: + a = Finding("x", "p", "d") + assert {a, Finding("x", "p", "d")} == {a} + + +def test_the_payload_helper_shape_matches_what_pypi_returns() -> None: + """Guards the FIXTURE, not the code. + + If this helper drifts from PyPI's real schema, every test above passes against a shape the gate + will never see in production — the quietest way for a green suite to mean nothing. `upload_time` + and `upload_time_iso_8601` are both real PyPI fields; the code reads either. + """ + payload = json.loads(json.dumps(_payload("fastapi", first_release=_NOW))) + assert set(payload) == {"info", "releases"} + assert "name" in payload["info"] + entry = next(iter(payload["releases"].values()))[0] + assert "upload_time_iso_8601" in entry From cb9e45971d922aadb3ce9a140215aed4812f07e1 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 16:11:30 -0500 Subject: [PATCH 3/5] fix(provenance,supply-chain): stop claiming a trailer we do not write; pin the gates' own toolchain TWO UNRELATED-LOOKING FIXES, ONE THEME: a control is only worth what it actually does, and both of these were being scored on their description. 1. THE PROVENANCE TRAILER IS NOT IN USE, AND CANNOT BE. docs/Secure_AI_Development_Standards.md prescribed a `Co-Authored-By` + `Tier:` commit trailer, listed it under "Built (in code today)", put it in the section-11 retained auditable evidence set, and cited it as evidence for two claims in the A.4 register -- a register whose stated audience is "adopters and auditors". Measured: ZERO. `git log -n 300` contains 0 `Co-Authored-By` trailers and 0 `Tier:` lines, while 81 tracked files under docs/ instruct omitting it. And the cause is structural, not slippage: cla.yml allowlists exactly three identities, the CLA bot reads a trailer co-author as a contributor who must sign, and `cla` is a REQUIRED status check -- so adding the trailer reds the merge gate. The prescription and the prohibition were both in the tree, neither referencing the other, so the contradiction was invisible from either side. Fixed by making the documents honest rather than by building a hook. A commit-msg trailer hook would reverse a standing project decision the tree already reflects, and would have a solo author satisfying a ceremony on themselves. So section 6.7 now states the trailer is not in use AND why; it is out of the Built list, out of the evidence set, and the "Provenance recorded at commit granularity by convention" claim is WITHDRAWN with a defensible replacement wording. The A.6 deviation is corrected to the broader truth it always was: there is no per-commit model/version record at any granularity, so the project cannot answer "which model wrote this, under what policy" for any commit in its history. The note that the trailer is not CI-enforced was already there and was fine. What was wrong was calling it Built and citing it as evidence. tests/test_ai_provenance_claims.py stops it reverting. It asserts consistency rather than counting trailers in `git log`, deliberately: CI checks out shallow, so a commit-count guard would measure a different history than a developer sees, and "0 of 0 commits" is a gate measuring nothing. Either resolution of the contradiction passes -- adopt the trailer and clear the omission instructions, or keep omitting it and do not claim it. Proved by reverting the doc: 4 of its 6 assertions catch the pre-fix text. The two that do not read other files and correctly should not. Building it also caught two bugs in itself: `find("**Built")` was matching an inline mention 400 lines above the real list (so the guard checked the wrong section entirely), and the why-is-it-blocked assertion was loose enough to pass on the broken doc. 2. THE BLOCKING GATES INSTALLED THEIR OWN TOOLING UNPINNED. The pip-audit job -- whose entire purpose is proving nothing unpinned enters the tree -- installed its auditor with a bare `pip install pip-audit`. The DEP-1 step bootstrapped `uv`, the resolver that produces every lockfile the job then audits, unpinned. semgrep rode `~=1.90`, so any new 1.x could change taint propagation and red a green PR for reasons unrelated to its diff. gitleaks was `curl | tar` with no integrity check -- while the sbomqs step in the SAME FILE already verified against the release's own checksums file. Unfinished scope, not an accepted risk. All four sit in REQUIRED contexts, and all four are dependency intake no audited lockfile covers. Now: pip-audit==2.10.1, uv==0.12.0, semgrep==1.172.0 (each is what the previous spec already resolved to today, so no behaviour change), and gitleaks verified with sha256sum against gitleaks_8.18.4_checksums.txt, mirroring the sbomqs shape exactly rather than inventing a new one. tests/test_ci_venv_pinning.py grows two guards so it cannot regress. Writing them surfaced the trap they were meant to catch, in themselves: the first flagged `-r requirements.lock` (the most rigorously pinned install in the workflow) and the second matched the words `| tar` inside my own comment EXPLAINING the pipe it had replaced -- a detector counting itself. Both fixed, and the comment-stripping is now the documented reason rather than an incidental. --- .github/workflows/security.yml | 38 ++++- docs/Secure_AI_Development_Standards.md | 50 ++++-- tests/test_ai_provenance_claims.py | 212 ++++++++++++++++++++++++ tests/test_ci_venv_pinning.py | 114 +++++++++++++ 4 files changed, 397 insertions(+), 17 deletions(-) create mode 100644 tests/test_ai_provenance_claims.py diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 5e2179d4..cc0dab52 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -60,7 +60,10 @@ jobs: python-version: "3.14" - name: Check the lockfile is in sync with pyproject (DEP-1) run: | - python -m pip install --upgrade pip uv + # uv is PINNED: it is the resolver that produces every lockfile this job then audits, so an + # unpinned upgrade lets a new resolver silently change the exported set mid-PR and reds the + # `git diff --exit-code` below for a reason unrelated to the change. Bump deliberately. + python -m pip install --upgrade pip "uv==0.12.0" # Fails if pyproject changed without re-running `uv lock` (drift guard). uv lock --check # Re-export to the same path and fail if it differs from what's committed (keeps the @@ -86,7 +89,11 @@ jobs: /tmp/lockcheck/bin/pip install --require-hashes -r requirements.lock - name: Audit the locked dependencies (DEP-1) run: | - pip install pip-audit + # PINNED. This job exists to prove nothing unpinned enters the tree, and it was installing + # its own auditor with a bare `pip install pip-audit` — dependency intake that none of the + # lockfiles it checks actually covers. `==` also makes the audit reproducible: an unpinned + # auditor can change its advisory database handling between two runs of the same commit. + python -m pip install --upgrade pip "pip-audit==2.10.1" # Audit the committed, pinned set — reproducible (vs auditing a fresh latest-resolve). pip-audit -r requirements.lock --desc # ANTI-SLOPSQUAT. Deliberately a STEP in this already-REQUIRED job rather than a new context: it @@ -326,13 +333,25 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 0 # full history so the scan also covers earlier commits, not just the tip - - name: Install gitleaks (pinned) + - name: Install gitleaks (pinned + checksum-verified) run: | # Pin the release; bump deliberately. Verify the tag exists if this step 404s. + # + # VERIFIED, not just version-pinned. This was `curl ... | tar -xz` straight into a pipe: a + # tag pin says WHICH artifact to fetch, not that the bytes received are that artifact, and + # this runs inside a REQUIRED gate. The sbomqs step in this same file already had the answer + # (download the release's own checksums file and sha256sum -c it), so this was unfinished + # scope rather than an accepted risk. Mirroring that shape exactly. VER=8.18.4 - curl -sSfL "https://github.com/gitleaks/gitleaks/releases/download/v${VER}/gitleaks_${VER}_linux_x64.tar.gz" \ - | tar -xz -C /tmp gitleaks - sudo install -m 0755 /tmp/gitleaks /usr/local/bin/gitleaks + asset="gitleaks_${VER}_linux_x64.tar.gz" + base="https://github.com/gitleaks/gitleaks/releases/download/v${VER}" + curl -sSfL "${base}/${asset}" -o "$asset" + curl -sSfL "${base}/gitleaks_${VER}_checksums.txt" -o gitleaks_checksums.txt + # Keep the asset's canonical name: sha256sum -c verifies BY the filename in the line, so a + # renamed download would silently verify nothing. + grep " ${asset}$" gitleaks_checksums.txt | sha256sum -c - + tar -xzf "$asset" gitleaks + sudo install -m 0755 gitleaks /usr/local/bin/gitleaks gitleaks version - name: Scan repository for secrets run: gitleaks detect --config .gitleaks.toml --redact --verbose --no-banner @@ -348,7 +367,12 @@ jobs: python-version: "3.14" - name: Run the MessageFoundry rules run: | - python -m pip install --upgrade pip "semgrep~=1.90" + # PINNED to an exact version, not `~=1.90`. A compatible-release range silently adopts every + # new 1.x, and a new semgrep release can add rules or change taint propagation — which reds a + # green PR for a reason that has nothing to do with its diff. Same posture as bandit==1.9.4 + # above and zizmor==1.5.2: bump deliberately, in a PR that also clears any new findings. + # (1.172.0 is what `~=1.90` resolved to on 2026-07-29, so this pin is a no-op today.) + python -m pip install --upgrade pip "semgrep==1.172.0" # --error: any finding fails the step — and the job, now that this gate is blocking. # Includes `tee/` (in-tree vendored SOUP) so the standalone relay is held to the same # dangerous-sink rules as the engine. diff --git a/docs/Secure_AI_Development_Standards.md b/docs/Secure_AI_Development_Standards.md index 0e38618e..f8bf5da4 100644 --- a/docs/Secure_AI_Development_Standards.md +++ b/docs/Secure_AI_Development_Standards.md @@ -349,7 +349,31 @@ The maintainer reviews **every** diff against the §6.2 plan. **Reject code you > **Dial:** T0 none → **T1 one coherent layer per commit + PR + `Co-Authored-By` + tier-declaration** → T2 + ADR link + Plan artifact + PR-thread reference → T3 + claims-register entry + AI-vs-human provenance + SOUP assessment (by analogy, §10). -Work on a feature branch and open a PR (**direct `main` pushes are blocked**). One coherent layer per commit. Record provenance in the trailer: +Work on a feature branch and open a PR (**direct `main` pushes are blocked**). One coherent layer per commit. + +> ### ⛔ The `Co-Authored-By` trailer is NOT IN USE — and cannot be, as things stand +> +> This section prescribed the trailer block below. **Measured 2026-07-29: the observed rate is zero** — +> `git log -n 300` contains **0** `Co-Authored-By` trailers and **0** `Tier:` lines — and **81 tracked +> files** under `docs/` instruct omitting it. That is not slippage; it is **structurally blocked**: +> [`cla.yml`](../.github/workflows/cla.yml) allowlists exactly three identities +> (`wshallwshall`, `dependabot[bot]`, `github-actions[bot]`), the CLA bot treats a trailer co-author as +> a contributor who must sign, and `cla` is a **required** status check. Adding the trailer therefore +> reds a required context and blocks the merge. +> +> So the trailer is recorded here as **designed, blocked, and not in use** (§9) — *not* as Built, and +> **not** as evidence for any claim in the A.4 register. A control with a measured adoption of zero +> cited as audit evidence is the same integrity failure the doc-drift test family exists to prevent, +> and this standard is published to adopters and auditors. +> +> **If it is ever adopted**, note the shipped Linux-kernel form is **`Assisted-by:`** — the kernel +> explicitly **rejected** `Co-developed-by:` for AI attribution because it requires a paired +> `Signed-off-by:` an AI cannot give. Any adoption needs (a) a CLA-compatible trailer verified against +> `cla.yml`'s allowlist, and (b) enforcement — a `commit-msg` hook plus an ungated CI backstop, the +> two-layer shape the ledger gate already uses to survive `--no-verify`. Until both exist, do not +> reintroduce the prescription. + +For reference, the format that was specified (**not currently used** — see the box above): ``` @@ -359,9 +383,15 @@ Tier: S1×P3 ⇒ T3 — Co-Authored-By: Claude Opus 4.8 (1M context) ``` -The human-gate stack: [CODEOWNERS](../.github/CODEOWNERS) + branch protection + required CI checks + the [PR template](../.github/PULL_REQUEST_TEMPLATE.md) + the CLA bot ([`cla.yml`](../.github/workflows/cla.yml)). For the **tag-time** bar (SBOM / signed tag / SLSA), see [RELEASE-GATE.md](security/RELEASE-GATE.md). +The human-gate stack: [CODEOWNERS](../.github/CODEOWNERS) + branch protection + required CI checks + the [PR template](../.github/PULL_REQUEST_TEMPLATE.md) + the CLA bot ([`cla.yml`](../.github/workflows/cla.yml)). For the **tag-time** bar (SBOM / signed tag / SLSA), see [RELEASE-GATE.md](security/RELEASE-GATE.md). Note `CODEOWNERS` is **documentation, not enforcement** here: `require_code_owner_reviews` is false and the sole owner is the sole author. -> **Provenance honesty (read with §9).** The `Co-Authored-By` trailer is **Built (by convention)** and records model/version at **commit granularity** — it is **not** enforced by CI and does **not** mark which *lines/hunks* are AI- vs human-authored. The line-level distinction a QMS actually wants is **designed-but-deferred** (§9). Do not claim more than the trailer delivers (§8; Appendix A.6 deviation). +> **Provenance honesty (read with §9).** With the trailer unused, the durable provenance record is the +> **PR thread + commit history + this standard**, at commit granularity. There is no model/version +> record in the commit stream today, so the project cannot answer "which model wrote this, under what +> policy" for any commit in its history — and there is **no** retained-transcript store either (§9). +> The line/hunk-level distinction a QMS actually wants is **designed-but-deferred**. Do not claim more +> than this delivers (§8; Appendix A.6 deviation). `tests/test_ai_provenance_claims.py` fails if the +> trailer is re-listed as Built or as evidence while the repo still instructs omitting it. *Maps to:* Principle 5 · SSDF **RV.2** · ASVS V8. @@ -438,14 +468,14 @@ The repo's tiered-honesty taxonomy, applied to the **dev-process tooling itself* - **Required-merge-gate integrity:** the required status-check set is recorded in [`.github/required-contexts.txt`](../.github/required-contexts.txt) and every in-repo claim about it is reconciled by [`tests/test_required_contexts.py`](../tests/test_required_contexts.py) (five statements disagreed with the live API, one naming a context string that matches no job). [`tests/test_security_posture.py`](../tests/test_security_posture.py) refuses `continue-on-error`, `|| true`/`--exit-zero`, and a skippable job-level `if:` on any job backing a required context — the one-line edit that leaves a required check green while its scanner's findings are discarded — and pins the advisory jobs the other way so an accidental promotion is equally deliberate. `actionlint` (pre-commit hook + a checksum-verified step in [`zizmor.yml`](../.github/workflows/zizmor.yml)) catches the invalid-`${{ }}` class that aborts workflow compilation so required contexts silently never appear. - [`messagefoundry check`](../messagefoundry/checks.py) exit-coded validate + dryrun gate. - SessionStart worktree-context hook; worktree scripts ([`new.ps1`/`remove.ps1`](../scripts/worktree/)); shared AI project memory (facts only); synthetic generators; the dependency-boundary test ([`tests/test_dependency_boundaries.py`](../tests/test_dependency_boundaries.py)). -- `Co-Authored-By` provenance trailer — **by convention** (model/version, commit granularity). +- One coherent layer per commit + PR + the PR template's tier/AI declaration. *(The `Co-Authored-By` model/version trailer is **NOT** in this list: measured adoption is zero and the CLA required check structurally blocks it — see §6.7 and the deferred list below.)* - `/code-review` + `/security-review` skills. **Designed but deferred (Build trigger + Design record):** - **A dedicated SPDX-header CI test — *highest-priority deferred gate*.** SPDX-header presence is today enforced only by **convention** — there is no test asserting it. ("AI memory" is **not** a control: it fails silently the moment context rots, §6.3.) *Interim compensating control:* a manual checklist item in the [PR template](../.github/PULL_REQUEST_TEMPLATE.md) (add if absent) — a deterministic-ish artifact, **not** "AI memory" — plus the leak/forbidden scan. *Build trigger (cheap — do this first):* a trivial pytest walking first-party `.py` files. *Design record:* Appendix A.6. - **A new-dependency-introduction check — *partly BUILT 2026-07-29, residual deferred*.** `pip-audit` finds CVEs in *already-pinned* packages — it does **not** flag a freshly **hallucinated/typosquatted** name with no advisory. **Now built:** [`scripts/security/new_dependency_check.py`](../scripts/security/new_dependency_check.py) sweeps every distribution declared in `pyproject.toml` (core **and** every extra) against PyPI and fails on a name that does not exist, publishes no files, was first released inside a 90-day floor, or is served under another project's canonical name. It runs as a step inside the **already-required `pip-audit` job**, so it is blocking today without a new branch-protection context; it is stdlib-only and **fails closed** (unreachable PyPI ⇒ exit 2; zero distributions examined ⇒ exit 2, so a schema change cannot read as a clean sweep). Verified end-to-end against live PyPI: 41 distributions clean, and an injected `hl7-dicom-bridge>=1.0` rejected. Tests: [`tests/test_new_dependency_check.py`](../tests/test_new_dependency_check.py). *Interim compensating control (also now added):* the verify-before-add line in the [PR template](../.github/PULL_REQUEST_TEMPLATE.md). **Residual deferred —** two things, stated because the gate must not be read as broader than it is: (1) it cannot detect a **real package that is not the intended one** (measured 2026-07-29: `py-webauthn` exists, publishes, is years old and is canonically named, yet is AS207960's project rather than the intended `webauthn` — every check passes it; the blind spot is pinned by a named test), which leaves that class on human verify-before-add plus the dated vet note in `pyproject.toml`; and (2) no automated check yet **requires** that vet note on a newly-introduced dependency — the original "diff `pyproject` against the prior commit and require a recorded justification" trigger. *Build trigger for the residual:* a merge-base `pyproject` diff (needs a non-shallow base fetch that fails loudly rather than silently finding no base). *Design record:* Appendix A.6. -- **A `Co-Authored-By` trailer-format CI check** (presence/format) **and** a **line/hunk-level** AI-authorship record. *Build trigger:* a commit-msg lint; a hunk-attribution mechanism. *Design record:* Appendix A.6. +- **A commit-granular AI-provenance trailer AT ALL** — reclassified 2026-07-29 from "built by convention" to **deferred and currently BLOCKED**, because measured adoption is zero (0 trailers and 0 `Tier:` lines in 300 commits) and 81 tracked files instruct omitting it. The blocker is structural, not cultural: `cla.yml` allowlists three identities, a trailer co-author reads as an unsigned contributor, and `cla` is a **required** check — so the trailer reds the merge gate. *Build trigger:* pick a **CLA-compatible** trailer (the kernel's shipped form is `Assisted-by:`; it rejected `Co-developed-by:` because that requires a paired `Signed-off-by:` an AI cannot give), verify it against `cla.yml`'s allowlist, then enforce with a `commit-msg` hook **plus** an ungated CI backstop — the ledger gate's two-layer shape, which survives `--no-verify`. A **line/hunk-level** record remains separately deferred. *Design record:* Appendix A.6. *Drift guard:* `tests/test_ai_provenance_claims.py`. - **A retained-transcript provenance store** (PHI/secret-free, access-controlled). - **Code-quality measurement gates — specified by the [Code Quality & Anti-Slop Standards](Code_Quality_Standards.md) §5.** Five *quality* (not security) gates, none built today, that measure the shallow-test and copy-instead-of-abstract slop modes (§3): **mutation testing on changed code** (the highest-leverage — it adversarially checks whether tests assert anything, the one control that most directly compensates for the solo-maintainer review deviation, [A.6](#a6-documented-deviations)), **clone-detection on the diff**, **diff-coverage visibility**, an **advisory `C901` complexity triage**, and an **expanded ruff ruleset** (`B`/`C4`/`SIM`/`UP`/`I`). *Build trigger (cheapest first):* expand ruff `select` + advisory `C901` (local + CI) → diff-coverage + clone-detection (CI) → mutation-on-diff (CI-first, advisory). *Anti-metric rule (binds here):* never gate on line-coverage % or raw complexity alone — measure structure/behavior, not a scoreboard. *Design record:* [Code Quality & Anti-Slop Standards](Code_Quality_Standards.md) §5 + Appendix A.3. @@ -479,7 +509,7 @@ PHI data detail routes to [PHI.md](PHI.md); the **product** egress policy routes ## 11. Evidence, attestation, and references -**Evidence set.** *Retained, auditable evidence:* this standard + [`../CLAUDE.md`](../CLAUDE.md); the [`.claude/settings.json`](../.claude/settings.json) hook/deny-list config; the security-CI scan history; the `Co-Authored-By` model/version trailers (commit granularity); PR/commit history; the **claims register** (A.4); the **deviations register** (A.6). *Ephemeral (not retained today):* the live session transcript — there is **no** retained-transcript store yet (§9), so it is not a standing archive; the PR thread + commit trailer are the durable provenance until that store is built. +**Evidence set.** *Retained, auditable evidence:* this standard + [`../CLAUDE.md`](../CLAUDE.md); the [`.claude/settings.json`](../.claude/settings.json) hook/deny-list config; the security-CI scan history; PR/commit history; the **claims register** (A.4); the **deviations register** (A.6). *Ephemeral (not retained today):* the live session transcript — there is **no** retained-transcript store yet (§9), so it is not a standing archive; the PR thread + commit history are the durable provenance until that store is built. **Removed from this set 2026-07-29:** the `Co-Authored-By` model/version trailers. They were listed as retained evidence while the measured count was **zero** (§6.7) — evidence that does not exist cannot be produced on request, and citing it is worse for an audit than declaring the gap. > **Provenance retention constraint.** Any **retained transcript or memory** used as a provenance record **MUST be free of PHI and secrets and access-controlled** — it is otherwise a long-lived PHI/secret store and a HIPAA exposure. A retained-transcript store is **designed-but-deferred** (§9); until then, provenance is the trailer + CI history + the PR thread, not a persisted transcript archive. @@ -552,10 +582,10 @@ MEFOR is an open-source HL7 v2.x integration engine (Python; FastAPI; SQLite/WAL | Claim (approved wording) | Evidence | |---|---| -| "Built with AI assistance under the Secure AI-Assisted Development Standards." | This doc; `Co-Authored-By` trailers; CI history. | +| "Built with AI assistance under the Secure AI-Assisted Development Standards." | This doc; PR/commit history; CI history. *(Previously cited `Co-Authored-By` trailers — struck 2026-07-29: measured count zero, §6.7.)* | | "AI-assisted code review as a compensating control for the solo-maintainer review deviation." | A.6; SDS §A.6; `/code-review` + `/security-review`; blocking CI. | | "Gates enforce intent deterministically." | `.claude/settings.json`; PreToolUse hook; `security.yml`; `messagefoundry check`. | -| "Provenance recorded at commit granularity by convention." | `Co-Authored-By` trailer; **not** CI-enforced (A.6). | +| ~~"Provenance recorded at commit granularity by convention."~~ **WITHDRAWN 2026-07-29 — do not use this wording.** | The `Co-Authored-By` trailer this rested on has a measured adoption of **zero** and is blocked by the required `cla` check (§6.7). The defensible replacement claim is: *"AI involvement is disclosed per PR via the PR template; there is no per-commit model/version record."* | | "AI-surfaced security findings are independently (multi-vote) verified before action; the residual risk is control-asymmetry, addressed by a parity review gate." | The 2026-06 audit (44 candidates → 26 confirmed); §6.6 parity gate; the parity guard/tests (A.2). | | "AI-added security controls are validated against the governing CI gate and real environments, not the model's self-selected tests." | §6.5 'scoped-green ≠ the gate'; the crypto-inventory + Windows-leg CI catches. | @@ -566,7 +596,7 @@ MEFOR is an open-source HL7 v2.x integration engine (Python; FastAPI; SQLite/WAL - **Runtime:** **Python 3.14-only** (`requires-python >=3.14`; CI runs a single 3.14 matrix across ubuntu + Windows 2022/2025) — 3.11/3.12/3.13 support dropped 2026-06-24 (PR #515). - **Verification order:** `ruff check` → `ruff format --check` → `mypy` (strict) → `pytest` (`QT_QPA_PLATFORM=offscreen` for console) → `messagefoundry check`. - **Commits:** one coherent layer per commit; branch + PR (direct `main` blocked). -- **Trailer format:** `Co-Authored-By: Claude Opus 4.8 (1M context) ` + the `Tier:` line. +- **Trailer format:** **none in use.** The specified `Co-Authored-By` + `Tier:` block is blocked by the required `cla` check and has a measured adoption of zero — see §6.7 before reinstating it anywhere. - **Clamp posture:** fail-closed to the strictest applicable tier when `phi_touch`/scale is unknown or off-loopback. - **Parallel sessions:** one worktree each; **shared AI memory is single-writer** — coordinate memory writes across sessions ([WORKTREES.md](WORKTREES.md)). - **PHI-to-AI exception (§4.5):** **not enabled** — MEFOR asserts no BAA-covered, zero-data-retention AI connection for the dev tooling today, so the effective posture is **no PHI to the AI**. To enable, an operator records the signed **BAA + zero-data-retention** reference (vendor, tool/endpoint, date) here; routine dev/test stays synthetic-only (PO.5) regardless. @@ -577,7 +607,7 @@ The standard requires deviations be documented with a compensating control (SDS - **Single-maintainer review (PO.2 / PW.7) — the canonical deviation this doc owns.** *First accepted 2026-06-16 in [SDS §A.6](Secure_Development_Standards.md#a6-documented-deviations); expanded here 2026-06-18; owner: project maintainer.* The T3 "human second reviewer" control cannot mean an independent human today. **Compensating controls:** blocking SAST/SCA/secret-scan (bandit/semgrep/pip-audit/gitleaks), AI-assisted review (`/code-review`, `/security-review`), branch protection + required CI checks, no direct `main` pushes. **Build trigger:** *a second human maintainer joins ⇒ T3 review escalates to a true human second reviewer.* **Design record:** [SDS §A.6](Secure_Development_Standards.md#a6-documented-deviations) + this section (+ an ADR if cut). *Honesty:* wording forbids "AI-reviewed = independently audited" (§8). - **"Explain it with AI help" accepted in lieu of unaided comprehension — the anti-SOUP override.** *Risk accepted 2026-06-18; owner: project maintainer.* The floor's *reject code you cannot explain* (§4.4) is satisfied by an explanation reached **with AI assistance** — a deliberate relaxation of the stricter "the human understands it unaided" reading that most directly prevents **SOUP** (Software of Unknown Provenance, §10). **Why override:** the standard governs a solo maintainer who builds with AI; an unaided-comprehension bar would bar that workflow. **Compensating controls:** the maintainer must produce, verify, and **stand behind** the explanation (code opaque *even with AI help* is still rejected; rubber-stamping forbidden); the deterministic gates (tests + blocking SAST/SCA) still apply; at T3 the explanation is captured durably (comment/PR/ADR/test). **Build trigger:** a regulated-device / formal-IEC-62304 deployment requiring documented SOUP assessment or unaided comprehension ⇒ reinstate the strict bar. **Design record:** §4.4, §6.6, §10. -- **AI-authorship recorded at commit granularity by convention.** *Risk accepted 2026-06-18; owner: project maintainer.* The AI-vs-human distinction is commit-level via `Co-Authored-By`, not line/hunk-level, and not CI-enforced (MEFOR has no QMS of its own — §10). **Compensating controls:** the trailer + the PR thread + the PR tier-declaration. **Build trigger:** a trailer-format CI lint and a hunk-attribution mechanism (§9). **Design record:** §6.7, §9. +- **NO per-commit AI-authorship record at all.** *Risk accepted 2026-06-18 as "commit granularity by convention"; **corrected 2026-07-29** — owner: project maintainer.* The earlier wording claimed a commit-level AI-vs-human distinction via `Co-Authored-By`. **Measurement says otherwise:** 0 trailers and 0 `Tier:` lines across 300 commits, 81 tracked files instructing omission, and the required `cla` check structurally blocking the trailer (§6.7). So the honest deviation is broader than first accepted — there is **no** model/version record in the commit stream, at any granularity, and no retained transcript either, which means the project cannot answer "which model wrote this, under what policy" for any commit in its history. **Compensating controls:** the PR thread + the PR template's AI/tier declaration + this standard (a per-PR record, not per-commit). **Build trigger:** a CLA-compatible trailer (e.g. the kernel's `Assisted-by:`) enforced by a `commit-msg` hook **plus** an ungated CI backstop; separately, a hunk-attribution mechanism (§9). **Design record:** §6.7, §9. **Drift guard:** `tests/test_ai_provenance_claims.py` — it fails if the trailer is re-listed as Built or as A.4 evidence while the repo still instructs omitting it. - **Automated requirement→design→test traceability not built (T3).** *Risk accepted 2026-06-18; owner: project maintainer.* **Compensating controls:** ADR ↔ test-name ↔ requirement links + the Plan artifact + the tier-declaration. **Build trigger:** the first regulated-release / adopter audit. **Design record:** §10, §9 (+ an ADR if cut). - **SPDX-header enforcement by convention.** *Risk accepted 2026-06-18; owner: project maintainer.* No dedicated test asserts header presence. **Compensating controls:** convention + AI memory + the leak/forbidden scan. **Build trigger:** a trivial pytest walking first-party `.py`. **Design record:** §9. diff --git a/tests/test_ai_provenance_claims.py b/tests/test_ai_provenance_claims.py new file mode 100644 index 00000000..770a9296 --- /dev/null +++ b/tests/test_ai_provenance_claims.py @@ -0,0 +1,212 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The AI-provenance claims in the published standard must match what the repo actually does. + +THE DEFECT THIS EXISTS FOR. ``docs/Secure_AI_Development_Standards.md`` prescribed a +``Co-Authored-By`` + ``Tier:`` commit trailer, listed it under **Built (by convention)**, put it in +§11's *retained, auditable evidence* set, and cited it as evidence for two claims in the A.4 register — +a register whose stated audience is "adopters and auditors". + +Measured 2026-07-29: **zero**. ``git log -n 300`` contains 0 ``Co-Authored-By`` trailers and 0 +``Tier:`` lines, while **81 tracked files** under ``docs/`` instruct omitting the trailer because the +CLA bot fails on it. And the blocker is structural rather than cultural: ``cla.yml`` allowlists exactly +three identities, the CLA bot reads a trailer co-author as a contributor who must sign, and ``cla`` is +a **required** status check — so adding the trailer reds the merge gate. + +Citing a control with a measured adoption of zero as audit evidence is the same integrity failure the +doc-drift test family already guards elsewhere (``test_security_doc_drift.py``, +``test_asvs_file_surface_doc_drift.py``, ``test_threat_model_doc_drift.py``). This module closes the +loop for the provenance claim specifically, so the correction cannot silently revert. + +WHY IT ASSERTS CONSISTENCY, NOT A COMMIT COUNT. Counting trailers in ``git log`` would make the test +depend on clone depth — CI checks out shallow for most jobs, so the count would be measured over a +different history than the one a developer sees, and a shallow fetch returning "0 of 0 commits" is a +gate measuring nothing. Instead this asserts the invariant that actually matters and is fully +determined by the tracked tree: **the standard must not claim the trailer as built/evidence while the +repo's own documents instruct omitting it.** Whichever way that contradiction is resolved — adopt the +trailer and delete the omission instructions, or keep omitting it and don't claim it — this passes. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +_ROOT = Path(__file__).resolve().parents[1] +_STANDARD = _ROOT / "docs" / "Secure_AI_Development_Standards.md" +_CLA = _ROOT / ".github" / "workflows" / "cla.yml" + +_TRAILER = "Co-Authored-By" + +#: The EXACT heading of the "operating today" list — the claim an adopter acts on. Anchored on the full +#: string, not a `**Built` prefix: the doc says "Built / designed-but-deferred / aspirational" hundreds +#: of lines earlier, so a prefix match slices the wrong section and the guard silently checks nothing. +_H_BUILT = "**Built (in code today):**" + + +def _standard() -> str: + return _STANDARD.read_text(encoding="utf-8") + + +def _files_instructing_omission() -> list[Path]: + """Tracked docs that tell a contributor to OMIT the trailer. + + These are the counter-instruction: as long as any exist, the standard cannot honestly present the + trailer as an operating convention. + """ + found: list[Path] = [] + for path in sorted(_ROOT.joinpath("docs").rglob("*.md")): + if path == _STANDARD: + continue + text = path.read_text(encoding="utf-8", errors="replace") + if _TRAILER.lower() not in text.lower(): + continue + for line in text.splitlines(): + low = line.lower() + if _TRAILER.lower() not in low: + continue + if any( + cue in low for cue in ("omit", "do not add", "don't add", "no co-auth", "cla bot") + ): + found.append(path) + break + return found + + +def test_the_omission_instruction_is_still_the_repo_norm() -> None: + """Liveness for every assertion below: they are conditional on this contradiction existing. + + If the project ever adopts the trailer and clears those instructions, this test fails FIRST and + tells the reader to revisit the module rather than letting the rest pass vacuously. + """ + omitting = _files_instructing_omission() + print( + f"[ai-provenance] {len(omitting)} tracked doc(s) instruct omitting the {_TRAILER} trailer" + ) + assert omitting, ( + f"no tracked doc instructs omitting the {_TRAILER} trailer any more. If the trailer has been " + "genuinely adopted (a CLA-compatible form, enforced by a commit-msg hook plus an ungated CI " + "backstop), then this whole module is obsolete — delete it and restore the claims in " + "docs/Secure_AI_Development_Standards.md §6.7/§11/A.4/A.5/A.6 deliberately. Until then it must " + "not pass by finding nothing." + ) + + +def _section(heading: str, *, until: str = "\n**") -> str: + """The standard's text from ``heading`` up to ``until``. + + ``heading`` must be the FULL heading string. A prefix is not safe here: the tooling-honesty + paragraph in §1 uses the words "Built / designed-but-deferred / aspirational", so a `**Built` + prefix search lands ~400 lines above the real list and the guard checks the wrong text. + """ + text = _standard() + start = text.find(heading) + assert start != -1, f"the standard lost its {heading!r} section — re-point this test" + rest = text[start + len(heading) :] + end = rest.find(until) + return rest if end == -1 else rest[:end] + + +def test_the_trailer_is_not_listed_as_a_built_guardrail() -> None: + """ "Built" is what an adopter reads as "this is operating today".""" + built = _section(_H_BUILT) + offending = [ + line.strip() + for line in built.splitlines() + if _TRAILER in line and not re.search(r"\bNOT\b|not in this list|blocked", line) + ] + assert not offending, ( + f"the {_TRAILER} trailer is listed as a BUILT guardrail:\n " + + "\n ".join(offending) + + "\n" + "Measured adoption is zero and the required `cla` check blocks it (§6.7). Move it to the " + "designed-but-deferred list, or actually adopt and enforce it." + ) + + +def test_the_trailer_is_not_cited_as_retained_audit_evidence() -> None: + """§11's evidence set is a promise that the artefact can be produced on request.""" + text = _standard() + match = re.search(r"\*\*Evidence set\.\*\*(.*?)(?:\n\n|\n#)", text, re.DOTALL) + assert match, "the standard lost its '**Evidence set.**' paragraph — re-point this test" + evidence = match.group(1) + # A "removed from this set" note legitimately names the trailer; a live citation does not. + live = re.split(r"\*\*Removed from this set[^*]*\*\*", evidence)[0] + assert _TRAILER not in live, ( + f"§11 still cites the {_TRAILER} trailers as retained, auditable evidence. Evidence with a " + "measured count of zero cannot be produced on request, and citing it is worse for an audit " + "than declaring the gap." + ) + + +def test_the_claims_register_does_not_rest_a_live_claim_on_the_trailer() -> None: + """A.4 is the approved external wording. A claim resting on a zero-adoption control is not usable.""" + text = _standard() + start = text.find("### A.4 Claims register") + assert start != -1, "the standard lost its A.4 claims register — re-point this test" + register = text[start : text.find("### A.5", start)] + + offending: list[str] = [] + for line in register.splitlines(): + if not line.strip().startswith("|") or _TRAILER not in line: + continue + # A withdrawn/struck row may name it; a live row citing it as evidence may not. + if re.search(r"WITHDRAWN|~~|struck|Previously cited", line): + continue + offending.append(line.strip()) + + assert not offending, ( + "an A.4 claim still cites the trailer as evidence:\n " + "\n ".join(offending) + "\n" + "Mark the claim WITHDRAWN or re-evidence it. This register is what gets quoted externally." + ) + + +def test_section_6_7_says_the_trailer_is_not_in_use_and_why() -> None: + """A gap named without its cause gets 'fixed' by reinstating the thing that cannot work. + + §6.7 is where a reader goes to learn the commit convention. If it still presents the trailer as + the convention, the next session re-adds it, watches the required `cla` context go red, and reaches + for the branch-protection settings. Both halves are asserted — the STATUS (not in use) and the + CAUSE (the required CLA check) — because the status alone reads as an oversight to correct. + """ + section = _section("### 6.7 Commit / PR with provenance", until="### 6.8") + low = section.lower() + + assert any(marker in low for marker in ("not in use", "not currently used", "⛔")), ( + "§6.7 does not state that the Co-Authored-By trailer is NOT in use. Measured adoption is zero, " + "so presenting the trailer block as the commit convention makes this section fiction — and it " + "is the section a contributor actually follows." + ) + assert "cla" in low and re.search(r"block|reds\b|red the", low), ( + "§6.7 does not record WHY the trailer is unusable — that `cla` is a required status check and " + "the CLA bot treats a trailer co-author as a contributor who must sign. Without the cause, the " + "recorded gap invites exactly the change that wedges the merge gate for every PR." + ) + + +def test_the_cla_allowlist_is_still_what_makes_the_trailer_unusable() -> None: + """Pins the CAUSE in the workflow, so the doc claim and the mechanism cannot drift apart. + + If the allowlist mechanism changes, the standard's explanation needs revisiting — and this is the + test that says so, rather than the explanation quietly becoming fiction. + """ + yaml = pytest.importorskip("yaml") + wf = yaml.safe_load(_CLA.read_text(encoding="utf-8")) + jobs = wf.get("jobs") or {} + allowlists = [ + str((step.get("with") or {}).get("allowlist", "")) + for job in jobs.values() + for step in (job or {}).get("steps") or [] + if (step.get("with") or {}).get("allowlist") + ] + assert allowlists, ( + "cla.yml no longer declares an `allowlist`. That was the mechanism making a trailer co-author " + "read as an unsigned contributor — re-check whether the trailer is now usable and update " + "docs/Secure_AI_Development_Standards.md §6.7 either way." + ) + assert "wshallwshall" in allowlists[0], ( + f"the CLA allowlist no longer names the maintainer: {allowlists[0]!r}. The trailer-blocking " + "explanation in §6.7 is derived from this list; re-verify it." + ) diff --git a/tests/test_ci_venv_pinning.py b/tests/test_ci_venv_pinning.py index 480b4daf..17b33d2d 100644 --- a/tests/test_ci_venv_pinning.py +++ b/tests/test_ci_venv_pinning.py @@ -95,3 +95,117 @@ def test_scratch_venvs_do_not_hide_an_unpinned_pip_fetch(workflow: str) -> None: f"unpinned exactly like the deleted `pip install --upgrade pip`, but the scanner cannot see it. " f"ADR 0034 requires a visible dismissal over an invisible filter." ) + + +# --- the blocking gates' OWN toolchain ------------------------------------------------------------- +# +# The tests above cover the lock-only scratch venvs. They say nothing about how the blocking jobs +# install the scanners THEMSELVES, and that was its own gap: the pip-audit job — whose entire purpose +# is proving nothing unpinned enters the tree — installed its auditor with a bare `pip install +# pip-audit`, the DEP-1 step bootstrapped the resolver with a bare `uv`, semgrep rode a +# compatible-release range, and gitleaks was a `curl | tar` with no integrity check. All four are +# dependency intake that no audited lockfile covers, inside REQUIRED contexts. + +#: ``pip install`` invocations in these jobs must name an exact version. Interpreter-level installs +#: (not into a lock-fed venv) can never be hash-verified, so `==` is the available control. +_PINNED_INSTALL_JOBS = ("pip-audit", "bandit", "semgrep") + + +def _run_bodies(workflow: str, job_key: str) -> list[str]: + yaml = pytest.importorskip("yaml") + wf = yaml.safe_load((_WORKFLOWS / workflow).read_text(encoding="utf-8")) + job = (wf.get("jobs") or {}).get(job_key) + assert job is not None, ( + f"{workflow} has no job {job_key!r} — re-point this guard at whatever replaced it rather than " + f"letting it pass vacuously" + ) + return [str(step["run"]) for step in (job.get("steps") or []) if "run" in step] + + +@pytest.mark.parametrize("job_key", _PINNED_INSTALL_JOBS) +def test_blocking_jobs_pin_the_tools_they_install(job_key: str) -> None: + """No bare or range-specified `pip install` inside a blocking security job. + + An unpinned scanner changes its own findings between two runs of the SAME commit, which shows up + as a mystery red on an unrelated PR — the failure that got bandit pinned to 1.9.4 after an implicit + upgrade changed `# nosec` parsing and broke a green branch. + """ + bodies = _run_bodies("security.yml", job_key) + assert bodies, f"job {job_key!r} has no run steps" + + unpinned: list[str] = [] + for body in bodies: + for line in body.splitlines(): + code = line.strip() + if code.startswith("#") or "pip install" not in code: + continue + args = code.split("pip install", 1)[1] + # A requirements-file install is pinned BY the file, and the lock-only-venv tests above + # already assert --require-hashes on those. Checking the file's name for `==` here would + # flag `-r requirements.lock` — the most rigorously pinned install in the whole workflow. + if re.search(r"(?:^|\s)(?:-r|--requirement)\s", args) or "--require-hashes" in args: + continue + targets = [token.strip("\"'") for token in args.split() if not token.startswith("-")] + for target in targets: + # `pip` itself is the bootstrap the existing dismissal covers; a path target is a + # local install, not a registry fetch. + if target == "pip" or target.startswith(("/", ".")): + continue + if "==" not in target: + unpinned.append(f"{job_key}: {target!r} in {code!r}") + + assert not unpinned, ( + "a blocking security job installs a tool without an exact `==` pin:\n " + + "\n ".join(unpinned) + + "\nPin it (and bump deliberately, in a PR that also clears any new findings). A range like " + "`semgrep~=1.90` silently adopts every new minor, so the gate's behaviour changes without a " + "diff — and this is dependency intake no lockfile in the repo covers." + ) + + +def test_release_asset_downloads_in_blocking_jobs_are_checksum_verified() -> None: + """A version tag says WHICH artifact to fetch, not that the bytes are that artifact. + + `curl … | tar -xz` inside a required gate executes third-party bytes with no integrity check. The + sbomqs step in this same workflow already verifies against the release's own checksums file, so any + download here must do the same. + """ + yaml = pytest.importorskip("yaml") + wf = yaml.safe_load((_WORKFLOWS / "security.yml").read_text(encoding="utf-8")) + jobs = wf.get("jobs") or {} + + checked = 0 + offenders: list[str] = [] + for job_key, job in jobs.items(): + # Advisory jobs are out of scope here: they cannot turn a required context green while + # compromised. `trivy` is deliberately excluded for that reason and noted in its own comment. + if (job or {}).get("continue-on-error") is True: + continue + for step in (job or {}).get("steps") or []: + raw = str((step or {}).get("run") or "") + # Comments OUT before matching. A rationale comment in these workflows quotes the very + # command being prohibited (the gitleaks step explains the `curl | tar` it replaced), so a + # whole-body match reports the explanation as the offence — a detector counting itself. + body = "\n".join(line for line in raw.splitlines() if not line.strip().startswith("#")) + if "releases/download" not in body: + continue + checked += 1 + name = (step or {}).get("name") or "" + if "sha256sum -c" not in body: + offenders.append(f"security.yml:{job_key} — step {name!r} verifies no checksum") + if re.search(r"\|\s*tar\b", body): + offenders.append( + f"security.yml:{job_key} — step {name!r} pipes the download straight into tar, " + "so there is no file left to verify" + ) + + # Liveness: if no blocking job downloads a release asset any more, say so rather than pass on an + # empty scan. + print(f"[ci-venv-pinning] examined {checked} release-asset download step(s) in blocking jobs") + assert checked > 0, ( + "no blocking job in security.yml downloads a release asset — if that is now true this guard is " + "obsolete, but an empty scan must not read as a pass" + ) + assert not offenders, "unverified release-asset download in a blocking job:\n " + "\n ".join( + offenders + ) From 4b55406c9f0ea0a5234e755c3f5c2c3fdf1f9a5e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 16:57:13 -0500 Subject: [PATCH 4/5] fix(ci,hooks): stop actionlint being a different linter in CI than locally; make the shared pre-commit shim worktree-aware 1. ACTIONLINT WAS TWO DIFFERENT LINTERS. The actionlint step added earlier in this branch passed locally and went RED in CI on ~30 findings in ci.yml and release.yml that predate it. Cause: actionlint shells out to shellcheck for every `run:` body IF shellcheck is on PATH. GitHub runners ship shellcheck; a local actionlint binary does not. So the same command was a workflow-syntax linter on my machine and workflow-syntax + shellcheck in CI -- precisely the environment-dependent reach this repo keeps getting bitten by, and my local green was not evidence of anything. Both sides now pass `-shellcheck=`. The step's purpose is the compile-aborting expression-syntax class docs/CI.md describes; adopting shellcheck over every `run:` body is a real question but a separate PR that also clears or annotates ~30 findings, several of which are shellcheck misparsing a GitHub interpolation inside a shell script (release.yml:392 "'(' is invalid here"). It is not something to smuggle in as a side effect of which tools a runner happens to have installed. 2. THE GATE THEN CAUGHT ME, WITH THE EXACT DEFECT IT EXISTS FOR. Writing the comment explaining that class, I put a literal empty interpolation in it -- inside the `run:` body. actionlint parses expressions in run bodies INCLUDING shell comments, which is the whole reason this step exists, so the illustration WAS the defect: `zizmor.yml:52: unexpected end of input while parsing variable access`. The comment now names the hazard in words and warns the next reader not to illustrate it literally. Worth recording precisely, because the distinction is the bug: an empty interpolation in a YAML-LEVEL comment is harmless -- YAML strips it before anything evaluates expressions -- and five such comments exist in ci.yml and dependabot-lock-resync.yml. Inside a `run:` block a comment is part of the script string, so it is parsed. Only the second kind breaks. 3. THE SHARED PRE-COMMIT SHIM ASSUMED A SINGLE CHECKOUT. `core.hooksPath` points all 10 worktrees at one .git/hooks/pre-commit that pre-commit generated with two assumptions that a multi-worktree checkout breaks: * INSTALL_PYTHON was hardcoded to MessageFoundry-ledger's .venv -- a DISPOSABLE worktree. Remove it and the `command -v pre-commit` fallback finds nothing on a bare PATH, so every commit in every worktree exits 1. Fails closed, but blocks every session at once. * ruff/bandit/ledger-gate/forbidden-content are `language: system` -- deliberately, so pre-commit can never disagree with the CI/dev ruff on a version. But `language: system` resolves from PATH, and PATH holds no venv unless the shell activated one. Measured from a bare PATH: `ruff` not found, hooks fail. install-git-hooks.ps1 now patches the generated shim to resolve BOTH against the worktree being committed in, so `language: system` gets exactly what that worktree's venv holds -- what the config always intended, now true from any shell. Marker- guarded and idempotent, it refuses to splice if pre-commit's `# end templated` anchor is missing (a wrong insertion point would break commits everywhere), backs up to pre-commit.prepatch, and re-running is a no-op. `pre-commit install` regenerates the file and drops the patch, so -Status now reports whether it is applied, prints the hardcoded INSTALL_PYTHON, and says outright when that path no longer exists. -Status also compares this worktree's ruff against constraints.lock. That is not hypothetical: one worktree carried ruff 0.16.0 against pyproject's `<0.16` cap (installed standalone, so nothing capped it), producing ~829 findings CI does not have -- and its `ruff check --fix` hook then STRIPPED `# noqa` directives the pinned 0.15.22 still wants. A worktree that lints with a different linter than CI and rewrites files accordingly is worth one line of output. A bug the splice found in itself, before it ran anywhere real: implemented with PowerShell `-replace`, in whose replacement syntax `$_` means THE ENTIRE INPUT STRING. The patch contains $_mf_root / $_mf_dir / $_mf_py, so it spliced the whole hook file into itself at each one -- 681 bytes to 9556, with `PATH="#!/bin/sh...` written into the middle of the PATH assignment. Now String.Replace, which has no substitution semantics, plus an anchor-count check. Verified on a copy: 2318 bytes, one patch block, every shell variable intact, `sh -n` and `bash -n` clean, and under a bare PATH it takes ruff from not-found to this worktree's 0.15.22. NOT DONE HERE, and it needs the owner: the live .git/hooks/pre-commit is unchanged. The worktree gate blocks writes under the primary checkout, and .git/hooks is inside it. Apply with: pwsh -NoProfile -File scripts\coord\install-git-hooks.ps1 --- .github/workflows/zizmor.yml | 23 ++++- .pre-commit-config.yaml | 9 ++ scripts/coord/install-git-hooks.ps1 | 140 ++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 1 deletion(-) diff --git a/.github/workflows/zizmor.yml b/.github/workflows/zizmor.yml index 3f345f98..807a6e0c 100644 --- a/.github/workflows/zizmor.yml +++ b/.github/workflows/zizmor.yml @@ -63,7 +63,28 @@ jobs: tar -xzf "$asset" actionlint sudo install -m 0755 actionlint /usr/local/bin/actionlint actionlint --version - actionlint + # `-shellcheck=` DISABLES actionlint's shellcheck integration, deliberately. + # + # actionlint shells out to shellcheck for every `run:` body IF shellcheck is on PATH. GitHub + # runners ship it; a local actionlint binary generally does not. So the same command is a + # different linter in the two places -- this step passed locally and failed in CI on ~30 + # PRE-EXISTING findings in ci.yml and release.yml that have nothing to do with the change that + # added it. That asymmetry is the exact defect class this repo keeps hitting: a gate whose + # reach depends on its environment. + # + # Scope is the reason, not the noise. This step exists for the workflow-syntax class in + # docs/CI.md -- an invalid GitHub expression interpolation that aborts workflow COMPILATION so + # required contexts never appear. Adopting shellcheck's whole ruleset over every `run:` body is + # a separate decision, and several of its findings here are it misparsing a GitHub + # interpolation inside a shell script (release.yml:392 "'(' is invalid here"). Turning it on + # repo-wide belongs in its own PR that also clears or annotates the backlog. + # + # NB -- do NOT write a literal empty interpolation (dollar-brace-brace) in this comment to + # illustrate the point: actionlint parses expressions in `run:` bodies INCLUDING comments, so + # the illustration is itself the defect. Writing one here is how this very step first went red. + # + # Now identical locally and in CI, which is what makes a green here mean something. + actionlint -shellcheck= - name: Analyze the workflows env: # Lets zizmor run its online audits (e.g. unpinned / known-problematic action refs). diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6410238c..dd5b0f2d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -82,10 +82,19 @@ repos: # The upstream hook already scopes itself to `types: [yaml]` + `files: ^\.github/workflows/`, so no # `files:` override is needed here — and adding one would be the scope drift # tests/test_lint_scope_parity.py exists to prevent. + # + # `-shellcheck=` disables actionlint's shellcheck integration, matching the CI step in zizmor.yml. + # actionlint shells out to shellcheck for every `run:` body IF shellcheck is on PATH — so without + # this flag the same hook is a DIFFERENT linter on a machine that has shellcheck than on one that + # does not, and different again on a GitHub runner (which ships it). That is how the CI step first + # landed red on ~30 pre-existing findings after passing locally. Whether to adopt shellcheck over + # every `run:` body is a real question, but it is a separate PR that also clears the backlog — not a + # side effect of which tools happen to be installed. Keep both sides carrying this flag. - repo: https://github.com/rhysd/actionlint rev: v1.7.12 hooks: - id: actionlint + args: ["-shellcheck="] # Python SAST — the SAME skips AND the same excluded paths as the CI bandit job (security.yml). # It said "same ... as CI" before and was not: CI scanned `-r messagefoundry tee` while this hook diff --git a/scripts/coord/install-git-hooks.ps1 b/scripts/coord/install-git-hooks.ps1 index d480b3c2..7e25d787 100644 --- a/scripts/coord/install-git-hooks.ps1 +++ b/scripts/coord/install-git-hooks.ps1 @@ -87,6 +87,45 @@ if ($Status) { } $pushInstalled = (Test-Path $prePush) -and ((Get-Content $prePush -Raw -EA SilentlyContinue) -match [regex]::Escape($pushMarker)) Write-Host "pre-push : $(if ($pushInstalled) { 'INSTALLED (push guard)' } elseif (Test-Path $prePush) { 'present, but NOT ours' } else { 'NOT INSTALLED' })" + # Report the worktree-aware patch explicitly. Unpatched is not cosmetic: the shim's INSTALL_PYTHON + # can point at a worktree that no longer exists (blocking every commit everywhere), and the + # `language: system` ruff/bandit hooks only resolve from an activated shell. + if ((Test-Path $preCommit) -and ((Get-Content $preCommit -Raw -EA SilentlyContinue) -match 'MessageFoundry local patch \(worktree-aware toolchain\)')) { + Write-Host " patch : worktree-aware (resolves .venv + INSTALL_PYTHON from the CURRENT worktree)" + } elseif ($pcShim) { + Write-Host " patch : NOT APPLIED -- run this script to fix" -ForegroundColor Yellow + # (?m) so ^ anchors per LINE: Get-Content -Raw yields one string, and piping that to + # Select-String with a ^ anchor silently matches nothing -- the diagnostic would just vanish. + if ((Get-Content $preCommit -Raw -EA SilentlyContinue) -match "(?m)^INSTALL_PYTHON='(.+)'\s*$") { + $pinnedPy = $Matches[1] + Write-Host " INSTALL_PYTHON is hardcoded to $pinnedPy" -ForegroundColor Yellow + if (-not (Test-Path $pinnedPy)) { + Write-Host " ^ THAT PATH DOES NOT EXIST -- commits are relying on the PATH fallback." -ForegroundColor Red + } else { + Write-Host " If that checkout is ever removed, EVERY commit in EVERY worktree fails." -ForegroundColor Yellow + } + } + Write-Host " ruff/bandit are `language: system` -- they need the venv on PATH." -ForegroundColor Yellow + } + # A worktree whose ruff drifts off constraints.lock lints with a DIFFERENT linter than CI, and its + # `ruff check --fix` hook rewrites files accordingly. Compare rather than assume. + $wantRuff = (Select-String -Path (Join-Path $RepoRoot "constraints.lock") -Pattern '^ruff==(\S+)' -EA SilentlyContinue) + if ($wantRuff) { + $want = $wantRuff.Matches[0].Groups[1].Value + $ruffExe = Join-Path $RepoRoot ".venv\Scripts\ruff.exe" + if (-not (Test-Path $ruffExe)) { $ruffExe = Join-Path $RepoRoot ".venv/bin/ruff" } + if (Test-Path $ruffExe) { + $have = ((& $ruffExe --version) -split '\s+')[1] + if ($have -eq $want) { + Write-Host "ruff : $have (matches constraints.lock)" + } else { + Write-Host "ruff : $have but constraints.lock pins $want -- THIS WORKTREE LINTS DIFFERENTLY THAN CI" -ForegroundColor Red + Write-Host " Fix: .venv\Scripts\python.exe -m pip install `"ruff==$want`"" -ForegroundColor Red + } + } else { + Write-Host "ruff : not installed in this worktree's .venv (constraints.lock pins $want)" -ForegroundColor Yellow + } + } Write-Host "worktrees : $(@(& git -C $RepoRoot worktree list).Count) share these hooks" return } @@ -185,6 +224,107 @@ exec "$PY" "$HOOK_DIR/push_guard.py" "$@" [System.IO.File]::WriteAllText($prePush, $pushHook, (New-Object System.Text.UTF8Encoding $false)) +# --- make pre-commit's generated shim WORKTREE-AWARE ---------------------------------------------- +# pre-commit generates ONE .git/hooks/pre-commit, and because it lives in the common git dir every +# worktree runs it. It is generated with two assumptions that a multi-worktree checkout breaks: +# +# 1. INSTALL_PYTHON is hardcoded to whichever checkout happened to run `pre-commit install` -- here it +# was MessageFoundry-ledger's .venv, a DISPOSABLE worktree. Remove that worktree and the fallback +# `command -v pre-commit` finds nothing on a bare PATH, so EVERY commit in EVERY worktree exits 1. +# Fails closed rather than open, but it blocks every session at once. +# 2. The ruff/bandit hooks are `language: system` -- deliberately, so pre-commit can never disagree +# with the CI/dev ruff on a version (.pre-commit-config.yaml says so). But `language: system` +# resolves the entry from PATH, and PATH holds no venv unless the user activated one. Measured +# 2026-07-29 from a bare PATH: `ruff` not found, so the ruff hooks fail. The symptom looks like a +# broken hook; the cause is an unactivated shell. +# +# Worse than either alone: a worktree whose .venv drifts off constraints.lock silently lints with a +# DIFFERENT ruff than CI. That happened -- one worktree carried ruff 0.16.0 against pyproject's +# `<0.16` cap (installed standalone, so nothing capped it), producing ~829 findings CI does not have, +# and its `ruff check --fix` hook then STRIPPED `# noqa` directives the pinned ruff still wants. +# +# So resolve both against the worktree being committed in. `language: system` then gets exactly what +# that worktree's venv holds -- which is what the config always intended. +# +# Idempotent and marker-guarded: safe to re-run, and a no-op once applied. `pre-commit install` +# regenerates the file from scratch and drops this, so re-run this script after doing that. +$pcPatchMarker = "MessageFoundry local patch (worktree-aware toolchain)" +$pcPatch = @' + +# --- MessageFoundry local patch (worktree-aware toolchain) ---------------------------------------- +# Injected by scripts/coord/install-git-hooks.ps1 -- see that script for the full rationale. +# `pre-commit install` REGENERATES this file and removes this block; re-run the script afterwards. +_mf_root="$(git rev-parse --show-toplevel 2>/dev/null)" +if [ -n "$_mf_root" ]; then + # Put THIS worktree's venv first on PATH so `language: system` hooks (ruff, bandit) resolve to the + # same versions its tests and CI use, from any shell, activated or not. + for _mf_dir in "$_mf_root/.venv/Scripts" "$_mf_root/.venv/bin"; do + if [ -d "$_mf_dir" ]; then + # POSIX-ify: a bare `C:/...` entry in a colon-separated PATH is ambiguous. cygpath ships + # with Git for Windows; if it is somehow missing, skip rather than corrupt PATH. + if command -v cygpath >/dev/null 2>&1; then + _mf_dir="$(cygpath -u "$_mf_dir")" + fi + case ":$PATH:" in + *":$_mf_dir:"*) ;; + *) PATH="$_mf_dir:$PATH"; export PATH ;; + esac + break + fi + done + # Run pre-commit itself from this worktree's interpreter when it has one, so the framework is never + # tied to a worktree that can be deleted. The templated value above stays as the last resort. + for _mf_py in "$_mf_root/.venv/Scripts/python.exe" "$_mf_root/.venv/bin/python"; do + if [ -x "$_mf_py" ]; then + INSTALL_PYTHON="$_mf_py" + break + fi + done +fi +# --- end MessageFoundry local patch --------------------------------------------------------------- +'@ -replace "`r`n", "`n" + +$pcIsFramework = (Test-Path $preCommit) -and ((Get-Content $preCommit -Raw -EA SilentlyContinue) -match 'File generated by pre-commit') +if (-not $pcIsFramework) { + # Never fabricate the file: without pre-commit's own shim there is nothing to patch, and the loud + # "LEDGER GATE IS NOT ARMED" warning below is the correct signal. + Write-Host "pre-commit shim absent -- skipping the worktree-aware patch (nothing to patch)." -ForegroundColor Yellow +} else { + $pcBody = Get-Content $preCommit -Raw + if ($pcBody -match [regex]::Escape($pcPatchMarker)) { + Write-Host "pre-commit shim already worktree-aware (patch present) -- unchanged." -ForegroundColor DarkGray + } elseif ($pcBody -notmatch '# end templated') { + # Anchor missing => pre-commit changed its template. Refuse rather than guess at an insertion + # point: a patch spliced into the wrong place could break every commit in every worktree. + Write-Host "!! pre-commit's shim has no '# end templated' anchor -- NOT patching." -ForegroundColor Red + Write-Host " Its template changed. Re-check scripts/coord/install-git-hooks.ps1 against the" -ForegroundColor Red + Write-Host " generated file before relying on worktree-aware tool resolution." -ForegroundColor Red + } else { + # Insert immediately after the templated block, so INSTALL_PYTHON is overridden after pre-commit + # sets it but before the exec that uses it. + # + # String.Replace, NOT -replace. In .NET regex replacement `$_` means THE ENTIRE INPUT STRING, + # and this patch contains `$_mf_dir` / `$_mf_py` / `$_mf_root` -- so `-replace` spliced the whole + # hook file into itself at each one (measured: 681 bytes -> 9556, with `PATH="#!/bin/sh...` + # written into the middle of the PATH assignment). A corrupted pre-commit hook breaks commits in + # all worktrees at once, so this is a literal replace with no substitution semantics. + $anchor = "# end templated" + $anchorCount = ([regex]::Matches($pcBody, [regex]::Escape($anchor))).Count + if ($anchorCount -ne 1) { + Write-Host "!! found $anchorCount '$anchor' anchors (expected 1) -- NOT patching." -ForegroundColor Red + return + } + $patched = $pcBody.Replace($anchor, $anchor + $pcPatch) + if ($patched -eq $pcBody) { + Write-Host "!! could not splice the worktree-aware patch -- pre-commit shim left UNCHANGED." -ForegroundColor Red + } else { + Copy-Item -LiteralPath $preCommit -Destination "$preCommit.prepatch" -Force + [System.IO.File]::WriteAllText($preCommit, ($patched -replace "`r`n", "`n"), (New-Object System.Text.UTF8Encoding $false)) + Write-Host "pre-commit shim PATCHED to be worktree-aware (backup: pre-commit.prepatch)." -ForegroundColor Green + } + } +} + # Git for Windows does not need the exec bit, but a WSL/Linux checkout of the same repo would. if ($IsLinux -or $IsMacOS) { & chmod +x $commitMsg; & chmod +x $prePush } From d9973d3782c390d28195fbf0a517ab4bbc3fd8c1 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 17:18:16 -0500 Subject: [PATCH 5/5] fix(hooks): stop patching pre-commit's shim; diagnose it instead The previous commit taught install-git-hooks.ps1 to splice a worktree-aware block into .git/hooks/pre-commit. tests/test_ledger_check.py ::test_the_installer_no_longer_writes_a_pre_commit_hook caught it, and the test is right: the last time this script owned that file, `pre-commit install` moved it to pre-commit.legacy and chained to it, which FAILS on Windows and blocked every commit in the repo. That invariant is not incidental, it is scar tissue. I read pre-commit's own installer before deciding. `is_our_script()` tests whether the file CONTAINS the template hash on its `# ID:` line, which the patch preserved -- so it would in fact have been recognised as pre-commit's own and overwritten, not moved aside, and the .legacy hazard would not have fired. But that same code settles the question the other way: `_install_hook_script` rewrites the file from the template unconditionally, so ANY third-party block is erased by the next `pre-commit install` -- the command a fresh clone runs. The patch was fragile by construction, test or no test. Dropped, not exempted. The supported fix needs no patching at all: `pre-commit install` bakes INSTALL_PYTHON to whichever interpreter runs it, so running it from the primary checkout re-anchors the shim off a removable worktree. -Status now says so, in place of a fix nobody should apply. What survives is the diagnosis, which is where the durable value was: interp : ...\MessageFoundry-ledger\.venv\Scripts\python.exe ^ NOT the primary checkout, so that worktree is removable. If it goes, every commit in all 10 worktrees fails at once (it fails closed). ruff : 0.15.22 (matches constraints.lock) The ruff line is not decoration. A worktree whose ruff disagrees with constraints.lock lints with a DIFFERENT linter than CI *and* its `ruff check --fix` hook rewrites files to match: measured, one worktree carried ruff 0.16.0 against pyproject's `<0.16` cap (installed standalone, so nothing capped it), reporting ~829 findings CI does not have and STRIPPING `# noqa` directives the pinned 0.15.22 wants. Three bugs in the is-this-the-primary predicate, each found by running it against cases rather than reading it: 1. `"...\MessageFoundry-ledger\...".StartsWith("...\MessageFoundry")` is TRUE, so a bare prefix test reported a SIBLING worktree as the primary -- the exact path this repo actually has. 2. git returns the common dir with FORWARD slashes while INSTALL_PYTHON uses backslashes, so an un-normalised compare never matched at all. 3. Even a boundary-correct prefix test is wrong: a worktree at `\.claude\worktrees\` is INSIDE the primary's path and is still removable. Only the primary's own `.venv` is non-removable, so the predicate is exact equality against that, not containment. Verified on all three: sibling -> not primary, primary -> primary, nested worktree -> not primary. --- scripts/coord/install-git-hooks.ps1 | 178 ++++++++++------------------ 1 file changed, 64 insertions(+), 114 deletions(-) diff --git a/scripts/coord/install-git-hooks.ps1 b/scripts/coord/install-git-hooks.ps1 index 7e25d787..b1007cb8 100644 --- a/scripts/coord/install-git-hooks.ps1 +++ b/scripts/coord/install-git-hooks.ps1 @@ -87,25 +87,48 @@ if ($Status) { } $pushInstalled = (Test-Path $prePush) -and ((Get-Content $prePush -Raw -EA SilentlyContinue) -match [regex]::Escape($pushMarker)) Write-Host "pre-push : $(if ($pushInstalled) { 'INSTALLED (push guard)' } elseif (Test-Path $prePush) { 'present, but NOT ours' } else { 'NOT INSTALLED' })" - # Report the worktree-aware patch explicitly. Unpatched is not cosmetic: the shim's INSTALL_PYTHON - # can point at a worktree that no longer exists (blocking every commit everywhere), and the - # `language: system` ruff/bandit hooks only resolve from an activated shell. - if ((Test-Path $preCommit) -and ((Get-Content $preCommit -Raw -EA SilentlyContinue) -match 'MessageFoundry local patch \(worktree-aware toolchain\)')) { - Write-Host " patch : worktree-aware (resolves .venv + INSTALL_PYTHON from the CURRENT worktree)" - } elseif ($pcShim) { - Write-Host " patch : NOT APPLIED -- run this script to fix" -ForegroundColor Yellow - # (?m) so ^ anchors per LINE: Get-Content -Raw yields one string, and piping that to - # Select-String with a ^ anchor silently matches nothing -- the diagnostic would just vanish. - if ((Get-Content $preCommit -Raw -EA SilentlyContinue) -match "(?m)^INSTALL_PYTHON='(.+)'\s*$") { - $pinnedPy = $Matches[1] - Write-Host " INSTALL_PYTHON is hardcoded to $pinnedPy" -ForegroundColor Yellow - if (-not (Test-Path $pinnedPy)) { - Write-Host " ^ THAT PATH DOES NOT EXIST -- commits are relying on the PATH fallback." -ForegroundColor Red - } else { - Write-Host " If that checkout is ever removed, EVERY commit in EVERY worktree fails." -ForegroundColor Yellow - } + # Report WHERE the shim's interpreter points. This script does not (and must not) rewrite that file + # -- see the "DIAGNOSE, never write" note below -- so this is a diagnostic with a remedy, not a + # pending action. It matters because the path is baked in at `pre-commit install` time and can name + # a worktree that no longer exists, which fails every commit in every worktree at once. + # + # (?m) so ^ anchors per LINE: Get-Content -Raw yields ONE string, and piping that to Select-String + # with a ^ anchor silently matches nothing -- the diagnostic would just vanish (it did). + if ($pcShim -and ((Get-Content $preCommit -Raw -EA SilentlyContinue) -match "(?m)^INSTALL_PYTHON='(.+)'\s*$")) { + $pinnedPy = $Matches[1] + # The PRIMARY checkout is the only non-disposable one: it owns the common git dir, so it cannot + # be `git worktree remove`d. Anything else -- a sibling clone-style worktree like + # MessageFoundry-ledger just as much as one under .claude/worktrees/ -- can vanish. Testing for + # a `.claude\worktrees\` path missed exactly that case, so derive the primary from $common + # instead of pattern-matching a layout convention. + $primaryRoot = (Split-Path -Parent $common) + # EXACT match against the primary's own venv, not a path-prefix test. Three traps, all hit while + # writing this: + # * `"...\MessageFoundry-ledger\...".StartsWith("...\MessageFoundry")` is TRUE, so a bare + # prefix test reports a SIBLING worktree as the primary. + # * git returns the common dir with FORWARD slashes (C:/Users/...) while INSTALL_PYTHON uses + # backslashes, so an un-normalised compare never matches at all. + # * even a boundary-correct prefix test is wrong here: a worktree under + # `\.claude\worktrees\` is INSIDE the primary's path and is still removable. + # Only the primary's own `.venv` is non-removable, so compare against exactly that. + $normPinned = [System.IO.Path]::GetFullPath($pinnedPy) + $primaryVenvPys = @( + (Join-Path $primaryRoot ".venv\Scripts\python.exe"), + (Join-Path $primaryRoot ".venv/bin/python") + ) | ForEach-Object { [System.IO.Path]::GetFullPath($_) } + if (-not (Test-Path $pinnedPy)) { + Write-Host " interp : $pinnedPy" -ForegroundColor Red + Write-Host " ^ DOES NOT EXIST. Commits now depend on ``pre-commit`` being on PATH," -ForegroundColor Red + Write-Host " and on a bare PATH it is not. Fix: run ``pre-commit install`` from" -ForegroundColor Red + Write-Host " $primaryRoot" -ForegroundColor Red + } elseif ($primaryVenvPys -notcontains $normPinned) { + Write-Host " interp : $pinnedPy" -ForegroundColor Yellow + Write-Host " ^ NOT the primary checkout, so that worktree is removable. If it goes," -ForegroundColor Yellow + Write-Host " every commit in all $(@(& git -C $RepoRoot worktree list).Count) worktrees fails at once (it fails closed)." -ForegroundColor Yellow + Write-Host " Re-anchor: run ``pre-commit install`` from $primaryRoot" -ForegroundColor Yellow + } else { + Write-Host " interp : $pinnedPy (primary checkout -- not removable)" } - Write-Host " ruff/bandit are `language: system` -- they need the venv on PATH." -ForegroundColor Yellow } # A worktree whose ruff drifts off constraints.lock lints with a DIFFERENT linter than CI, and its # `ruff check --fix` hook rewrites files accordingly. Compare rather than assume. @@ -224,106 +247,33 @@ exec "$PY" "$HOOK_DIR/push_guard.py" "$@" [System.IO.File]::WriteAllText($prePush, $pushHook, (New-Object System.Text.UTF8Encoding $false)) -# --- make pre-commit's generated shim WORKTREE-AWARE ---------------------------------------------- -# pre-commit generates ONE .git/hooks/pre-commit, and because it lives in the common git dir every -# worktree runs it. It is generated with two assumptions that a multi-worktree checkout breaks: +# --- pre-commit's generated shim: DIAGNOSE, never write ------------------------------------------- +# This script deliberately does NOT touch .git/hooks/pre-commit. pre-commit owns that file alone, and +# tests/test_ledger_check.py::test_the_installer_no_longer_writes_a_pre_commit_hook enforces it, +# because two tools contending for it once blocked EVERY commit in the repo on Windows (see the +# header). A patch here would also be futile: `pre-commit install` rewrites the file from its template, +# so anything spliced in is erased the next time anyone runs it. # -# 1. INSTALL_PYTHON is hardcoded to whichever checkout happened to run `pre-commit install` -- here it -# was MessageFoundry-ledger's .venv, a DISPOSABLE worktree. Remove that worktree and the fallback -# `command -v pre-commit` finds nothing on a bare PATH, so EVERY commit in EVERY worktree exits 1. -# Fails closed rather than open, but it blocks every session at once. -# 2. The ruff/bandit hooks are `language: system` -- deliberately, so pre-commit can never disagree -# with the CI/dev ruff on a version (.pre-commit-config.yaml says so). But `language: system` -# resolves the entry from PATH, and PATH holds no venv unless the user activated one. Measured -# 2026-07-29 from a bare PATH: `ruff` not found, so the ruff hooks fail. The symptom looks like a -# broken hook; the cause is an unactivated shell. +# There are two real problems with the generated shim in a multi-worktree checkout. Both are REPORTED +# by -Status and both have a supported remedy that needs no patching: # -# Worse than either alone: a worktree whose .venv drifts off constraints.lock silently lints with a -# DIFFERENT ruff than CI. That happened -- one worktree carried ruff 0.16.0 against pyproject's -# `<0.16` cap (installed standalone, so nothing capped it), producing ~829 findings CI does not have, -# and its `ruff check --fix` hook then STRIPPED `# noqa` directives the pinned ruff still wants. +# 1. INSTALL_PYTHON is hardcoded to whichever checkout last ran `pre-commit install` -- here it was +# MessageFoundry-ledger's .venv, a DISPOSABLE worktree. Delete that worktree and the fallback +# `command -v pre-commit` finds nothing on a bare PATH, so every commit in every worktree exits 1. +# Fails closed, but blocks every session at once. +# REMEDY: run `pre-commit install` from the PRIMARY checkout, whose .venv is not disposable. That +# is pre-commit's own mechanism for repointing it -- no third-party edit to its file. # -# So resolve both against the worktree being committed in. `language: system` then gets exactly what -# that worktree's venv holds -- which is what the config always intended. +# 2. ruff/bandit/ledger-gate/forbidden-content are `language: system` -- deliberately, so pre-commit +# cannot disagree with the CI/dev ruff on a version. But `language: system` resolves from PATH, so +# the hooks need a venv on PATH. Measured from a bare PATH: `ruff` not found. +# REMEDY: commit from a shell with the worktree's .venv activated (the documented workflow). # -# Idempotent and marker-guarded: safe to re-run, and a no-op once applied. `pre-commit install` -# regenerates the file from scratch and drops this, so re-run this script after doing that. -$pcPatchMarker = "MessageFoundry local patch (worktree-aware toolchain)" -$pcPatch = @' - -# --- MessageFoundry local patch (worktree-aware toolchain) ---------------------------------------- -# Injected by scripts/coord/install-git-hooks.ps1 -- see that script for the full rationale. -# `pre-commit install` REGENERATES this file and removes this block; re-run the script afterwards. -_mf_root="$(git rev-parse --show-toplevel 2>/dev/null)" -if [ -n "$_mf_root" ]; then - # Put THIS worktree's venv first on PATH so `language: system` hooks (ruff, bandit) resolve to the - # same versions its tests and CI use, from any shell, activated or not. - for _mf_dir in "$_mf_root/.venv/Scripts" "$_mf_root/.venv/bin"; do - if [ -d "$_mf_dir" ]; then - # POSIX-ify: a bare `C:/...` entry in a colon-separated PATH is ambiguous. cygpath ships - # with Git for Windows; if it is somehow missing, skip rather than corrupt PATH. - if command -v cygpath >/dev/null 2>&1; then - _mf_dir="$(cygpath -u "$_mf_dir")" - fi - case ":$PATH:" in - *":$_mf_dir:"*) ;; - *) PATH="$_mf_dir:$PATH"; export PATH ;; - esac - break - fi - done - # Run pre-commit itself from this worktree's interpreter when it has one, so the framework is never - # tied to a worktree that can be deleted. The templated value above stays as the last resort. - for _mf_py in "$_mf_root/.venv/Scripts/python.exe" "$_mf_root/.venv/bin/python"; do - if [ -x "$_mf_py" ]; then - INSTALL_PYTHON="$_mf_py" - break - fi - done -fi -# --- end MessageFoundry local patch --------------------------------------------------------------- -'@ -replace "`r`n", "`n" - -$pcIsFramework = (Test-Path $preCommit) -and ((Get-Content $preCommit -Raw -EA SilentlyContinue) -match 'File generated by pre-commit') -if (-not $pcIsFramework) { - # Never fabricate the file: without pre-commit's own shim there is nothing to patch, and the loud - # "LEDGER GATE IS NOT ARMED" warning below is the correct signal. - Write-Host "pre-commit shim absent -- skipping the worktree-aware patch (nothing to patch)." -ForegroundColor Yellow -} else { - $pcBody = Get-Content $preCommit -Raw - if ($pcBody -match [regex]::Escape($pcPatchMarker)) { - Write-Host "pre-commit shim already worktree-aware (patch present) -- unchanged." -ForegroundColor DarkGray - } elseif ($pcBody -notmatch '# end templated') { - # Anchor missing => pre-commit changed its template. Refuse rather than guess at an insertion - # point: a patch spliced into the wrong place could break every commit in every worktree. - Write-Host "!! pre-commit's shim has no '# end templated' anchor -- NOT patching." -ForegroundColor Red - Write-Host " Its template changed. Re-check scripts/coord/install-git-hooks.ps1 against the" -ForegroundColor Red - Write-Host " generated file before relying on worktree-aware tool resolution." -ForegroundColor Red - } else { - # Insert immediately after the templated block, so INSTALL_PYTHON is overridden after pre-commit - # sets it but before the exec that uses it. - # - # String.Replace, NOT -replace. In .NET regex replacement `$_` means THE ENTIRE INPUT STRING, - # and this patch contains `$_mf_dir` / `$_mf_py` / `$_mf_root` -- so `-replace` spliced the whole - # hook file into itself at each one (measured: 681 bytes -> 9556, with `PATH="#!/bin/sh...` - # written into the middle of the PATH assignment). A corrupted pre-commit hook breaks commits in - # all worktrees at once, so this is a literal replace with no substitution semantics. - $anchor = "# end templated" - $anchorCount = ([regex]::Matches($pcBody, [regex]::Escape($anchor))).Count - if ($anchorCount -ne 1) { - Write-Host "!! found $anchorCount '$anchor' anchors (expected 1) -- NOT patching." -ForegroundColor Red - return - } - $patched = $pcBody.Replace($anchor, $anchor + $pcPatch) - if ($patched -eq $pcBody) { - Write-Host "!! could not splice the worktree-aware patch -- pre-commit shim left UNCHANGED." -ForegroundColor Red - } else { - Copy-Item -LiteralPath $preCommit -Destination "$preCommit.prepatch" -Force - [System.IO.File]::WriteAllText($preCommit, ($patched -replace "`r`n", "`n"), (New-Object System.Text.UTF8Encoding $false)) - Write-Host "pre-commit shim PATCHED to be worktree-aware (backup: pre-commit.prepatch)." -ForegroundColor Green - } - } -} +# The sharper half of (2) is version drift, not absence, and -Status checks it: a worktree whose ruff +# disagrees with constraints.lock lints with a DIFFERENT linter than CI *and* its `ruff check --fix` +# hook rewrites files to match. Measured 2026-07-29: one worktree carried ruff 0.16.0 against +# pyproject's `<0.16` cap (installed standalone, so nothing capped it), producing ~829 findings CI does +# not have, and stripping `# noqa` directives the pinned 0.15.22 still wants. # Git for Windows does not need the exec bit, but a WSL/Linux checkout of the same repo would. if ($IsLinux -or $IsMacOS) { & chmod +x $commitMsg; & chmod +x $prePush }