diff --git a/.github/dependabot.yml b/.github/dependabot.yml index fd436e2b..fb6eeb3d 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -1,5 +1,6 @@ # Dependabot: surface vulnerable / outdated dependencies and CI actions as PRs (CI-1 / DEP-1). -# Until a committed lockfile lands, this is the primary signal for a known-CVE dependency. +# Dependabot's job here is REMEDIATION — it opens the PR that moves the pin. Detection does not rest +# on it: security.yml's pip-audit audits the committed locks directly (see the DEP-1 note below). version: 2 updates: # Python deps via the native "uv" ecosystem (was "pip"). The uv ecosystem resolves against @@ -10,10 +11,13 @@ updates: # .github/workflows/dependabot-lock-resync.yml re-exports those on the Dependabot branch so the # gate stays green. # - # UNVERIFIED, and it cannot be verified before the next weekly run: whether this ecosystem enumerates - # PEP 735 `[dependency-groups]` (the hash-pinned CI toolchain, ADR 0034 §3) at all. If it does not, - # those pins go stale silently — which is why security.yml's pip-audit step audits ci/locks/*.lock - # directly, turning "silently stale" into a red gate within ~24h via the daily cron. + # VERIFIED by the weekly run of 2026-07-30 (PR #66, Dependabot commit f066c2b8): this ecosystem DOES + # reach PEP 735 `[dependency-groups]` (the hash-pinned CI toolchain, ADR 0034 §3) — it bumped + # `zizmor==1.5.2` -> `1.28.0` inside `ci-scanners`, touching pyproject.toml + uv.lock only; that + # group's exported lock came from the resync workflow named above, exactly the split described. So + # at least `ci-scanners` does not go stale silently (`ci-quality` had no candidate in that run, so it + # is unobserved, not disproven), and only on the ROUTINE version track. security.yml's pip-audit + # still audits ci/locks/*.lock directly, for the reason recorded at that step. - package-ecosystem: "uv" directory: "/" schedule: @@ -29,6 +33,31 @@ updates: # design), now backstopped by the published-GHSA gate in dependabot-auto-merge.yml (SEC-007 #2). default-days: 5 semver-major-days: 7 + # Dependabot WIDENS a declared cap instead of respecting it, so a load-bearing upper bound in + # pyproject.toml must be restated here as a version range or the cap is decorative. PR #66 rewrote + # `annotated-types<0.8` -> `<0.9` (BOTH declaration sites) and `ruff>=0.4,<0.16` -> `<0.17`; the + # resync then propagated 0.8.0/0.16.0 into constraints.lock, so ci.yml's `--constraint` pinned CI + # *to* the broken versions. `python-deps` groups on `*`, so those two red the WHOLE batch and hold + # every benign bump in it hostage. WHY each is capped lives beside the cap in pyproject.toml + # (annotated-types in [project.dependencies] and in the [fhir] extra; ruff in [dev]) — do not + # restate it here; lift a cap and delete its entry in the SAME PR. ONE entry covers BOTH + # annotated-types declarations: `ignore` filters a candidate VERSION by package name within this + # manifest, not by declaration site, so a second entry would describe nothing. + # TRADE-OFF, accepted: `ignore` suppresses the SECURITY track for the named RANGE as well as the + # routine one (`update-types` is the version-only knob and cannot express a range). A 0.7.x / + # 0.15.x advisory fix still flows. Detection is untouched — security.yml's REQUIRED + # `pip-audit -r requirements.lock` covers both on every PR and on the daily cron, so a withheld fix + # reds a merge-blocking gate within ~24h and a human lifts the cap. Note the ceiling is the + # pyproject CAP, not this entry: a human would have to lift it either way. + # NOT IGNORED, deliberately: the exact `==` pins in `[dependency-groups]`, and no blanket + # `versioning-strategy: lockfile-only`. Both would freeze the hash-pinned CI toolchain, which ADR + # 0034 §3 wants moving THROUGH this machinery — a pinned toolchain outside it rots into + # "pinned, stale, unpatched — worse posture than floating". + ignore: + - dependency-name: "annotated-types" + versions: [">=0.8.0"] + - dependency-name: "ruff" + versions: [">=0.16.0"] groups: # Version-update grouping (applies-to defaults to version-updates). python-deps: @@ -43,6 +72,16 @@ updates: directory: "/" schedule: interval: "weekly" + # The uv/npm blocks' fresh-publish dodge, for the ecosystem where auto-merge actually fires: #75 + # took two of its five bumps to main under 24h from upstream publish (codeql-action v4.37.4 was 7h + # old), and a compromised action runs INSIDE CI with whatever token the job holds. SHA-pinning + # stops tag mutation, not a malicious new release — Dependabot just bumps the pin to the new SHA. + # Only default-days is honored here (this ecosystem does not support the semver-*-days knobs) and + # it ages off the tag's commit date rather than the release date, so treat 5 as approximate. + # Security updates bypass cooldown, so an advisory fix is unaffected. Watch codeql-action — a + # frequent releaser — for stalls after this lands. + cooldown: + default-days: 5 groups: # One weekly PR for all action bumps instead of one PR per action (CI cost: every extra PR # buys a full required-check pass + a push-to-main run when it auto-merges). Mirrors the diff --git a/.github/required-contexts.txt b/.github/required-contexts.txt index a7d68b8d..c158e301 100644 --- a/.github/required-contexts.txt +++ b/.github/required-contexts.txt @@ -108,7 +108,7 @@ a PR that implements BACKLOG #N must update BACKLOG.md # 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. +# zizmor.yml -- paths-filtered, 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. # dast.yml -- nightly/tag/dispatch only, with no pull_request trigger, so it can diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index fbe0f639..e04a09ee 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -32,10 +32,15 @@ jobs: uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: "3.14" + # --constraint constraints.lock for the reason spelled out in ci.yml's "Set up uv" step: a bare + # `-e ".[extras]"` re-resolves from pyproject's `>=` floors and adopts whatever upstream shipped + # since. A benchmark leg is the worst place for that — a silent dependency change reads as a + # performance regression. Every install in EVERY workflow is constrained except freethread-smoke, + # which is deliberately unconstrained (see the note at its install step). - name: Install project (dev) run: | python -m pip install --upgrade pip - pip install -e ".[dev]" + pip install --constraint constraints.lock -e ".[dev]" - name: Environment stamp run: | mkdir -p out/load @@ -96,7 +101,7 @@ jobs: - name: Install project (dev + postgres) run: | python -m pip install --upgrade pip - pip install -e ".[dev,postgres]" + pip install --constraint constraints.lock -e ".[dev,postgres]" - name: Environment stamp run: | mkdir -p out/load @@ -180,7 +185,7 @@ jobs: - name: Install project (dev + sqlserver) run: | python -m pip install --upgrade pip - pip install -e ".[dev,sqlserver]" + pip install --constraint constraints.lock -e ".[dev,sqlserver]" - name: Environment stamp run: | mkdir -p out/load diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 57a5ab04..c28844ef 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -129,10 +129,11 @@ jobs: # # EVERY install below passes `--constraint constraints.lock` — the HASHLESS export of uv.lock, kept # in sync by the DEP-1 gate. Without it, `uv pip install -e ".[extras]"` RE-RESOLVES from - # pyproject's `>=` floors and silently adopts whatever upstream published since: ruff 0.16.0 (626 - # lint errors) and annotated-types 0.8.0 (dropped `SLOTS`, broke fhir-core) each reddened EVERY - # open PR on the same day. The constraint pins every version to the lock while each job still - # installs only ITS OWN extras (so the narrow image-smoke set stays narrow). + # pyproject's `>=` floors and silently adopts whatever upstream published since: ruff 0.16.0 and + # annotated-types 0.8.0 (dropped `SLOTS`, broke fhir-core) each reddened EVERY open PR on the same + # day — see each cap's rationale in pyproject.toml. The constraint pins every version to the lock + # while each job still installs only ITS OWN extras (so windows-service-smoke's deliberately + # narrow set stays narrow; the docker-smoke job builds from docker/locks/* and installs nothing). - name: Set up uv if: needs.changes.outputs.code == 'true' || github.event_name == 'push' || github.event_name == 'workflow_dispatch' uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index 29bb64e0..ccbc85dd 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -12,6 +12,12 @@ name: Dependabot auto-merge # - MINOR updates of DEV-only dependencies # OUT OF SCOPE (left for human review, surfaced same-day by the daily security cron + alerts): # - MINOR/MAJOR updates of runtime deps, and ALL MAJOR updates +# NOTE the uv ecosystem is review-by-default in practice, not by exception: `python-deps` is +# `patterns: ["*"]`, so one weekly batch carries everything, and (a) a grouped update-type is the +# MAX across the batch (fetch-metadata's UPDATE_TYPES_PRIORITY) — one minor demotes every patch +# riding with it — while (b) the deny-list below names at least eight of this project's directly +# declared runtime deps and denies the WHOLE group on any hit. #66 was denied on `fastapi`. +# Auto-merge does fire for github-actions (#75, all-patch batch) and npm (#64). # # Fresh-release supply-chain poisoning is handled upstream by the dependabot.yml `cooldown` # (routine updates age before a PR opens); SECURITY updates bypass cooldown by design, so a real @@ -100,9 +106,8 @@ jobs: - name: Verify a published advisory backs the security track id: ghsa env: - DEP_NAMES: ${{ steps.meta.outputs.dependency-names }} DEP_GROUP: ${{ steps.meta.outputs.dependency-group }} - PREV_VERSION: ${{ steps.meta.outputs.previous-version }} + DEPS_JSON: ${{ steps.meta.outputs.updated-dependencies-json }} GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -u @@ -124,14 +129,34 @@ jobs: # Require at least one matching published (non-withdrawn) advisory for EVERY named dep at # the previous version. Query the global advisories endpoint (ecosystem pip — Python # advisories are pip-keyed). Fail closed on any error. + # fetch-metadata's scalar `previous-version` is ONE dependency's even on a grouped PR (#66: + # five deps, previous-version=0.7.0 — annotated-types'). Keying every name to it asks about + # versions the PR does not touch, and because most advisory ranges are open at the bottom, a + # low first-dep version makes almost every other name match: false confirmation in the + # PERMISSIVE direction (measured against the live endpoint at @0.7.0: aiohttp 30, pillow 29, + # urllib3 9, requests 6). `updated-dependencies-json` carries a per-dependency prevVersion. + pairs="$(printf '%s' "$DEPS_JSON" | jq -r '.[] | [.dependencyName, .prevVersion] | @tsv' 2>/dev/null || echo "ERR")" + if [ "$pairs" = "ERR" ] || [ -z "$pairs" ]; then + echo "::warning::no per-dependency metadata — failing closed (manual review)." + echo "advisory_ok=false" >> "$GITHUB_OUTPUT" + exit 0 + fi advisory_ok=true - IFS=',' read -ra names <<< "$DEP_NAMES" - for raw in "${names[@]}"; do + while IFS="$(printf '\t')" read -r raw prev; do name="$(echo "$raw" | tr '[:upper:]' '[:lower:]' | xargs)" - [ -z "$name" ] && continue + if [ -z "$name" ]; then continue; fi + # A non-first dependency can carry an EMPTY prevVersion — update_metadata.ts:101 falls back + # to '' for index > 0 when the PR body yields no "from" version. Unknown previous version + # means the advisory question cannot be asked, so DENY; skipping it would wave the dep + # through on a gate whose whole contract is to fail closed. + if [ -z "$prev" ]; then + echo "::warning::no previous version for '$name' — failing closed (manual review)." + advisory_ok=false + break + fi count="$(gh api -X GET /advisories \ -f ecosystem=pip \ - -f affects="${name}@${PREV_VERSION}" \ + -f affects="${name}@${prev}" \ --jq '[.[] | select(.withdrawn_at == null)] | length' 2>/dev/null || echo "ERR")" if [ "$count" = "ERR" ] || [ -z "$count" ]; then echo "::warning::advisory lookup failed for '$name' — failing closed (manual review)." @@ -139,12 +164,12 @@ jobs: break fi if [ "$count" -lt 1 ]; then - echo "::warning::no published advisory covers '${name}@${PREV_VERSION}' — failing closed (manual review)." + echo "::warning::no published advisory covers '${name}@${prev}' — failing closed (manual review)." advisory_ok=false break fi - echo "::notice::published advisory confirmed for '${name}@${PREV_VERSION}'." - done + echo "::notice::published advisory confirmed for '${name}@${prev}'." + done <<< "$pairs" echo "advisory_ok=$advisory_ok" >> "$GITHUB_OUTPUT" - name: Enable auto-merge for in-scope updates diff --git a/.github/workflows/freethread-smoke.yml b/.github/workflows/freethread-smoke.yml index fcd5d535..a29f6968 100644 --- a/.github/workflows/freethread-smoke.yml +++ b/.github/workflows/freethread-smoke.yml @@ -74,6 +74,13 @@ jobs: # free-threaded-relevant compiled surface: pydantic-core, cryptography, and the argon2-cffi + cffi # chain. --prerelease handling: pip resolves the cp314t wheels; the cffi 2.0 chain may need a # prerelease allowance depending on the index state at run time. + # + # DELIBERATELY UNCONSTRAINED — the ONE install in any workflow without `--constraint + # constraints.lock`. The lock records what resolved for the GIL build; pinning those versions here + # can leave a dependency with no cp314t wheel at that exact version and send pip to a source + # build, which either fails or quietly produces a differently-built extension. Either way the + # canary stops measuring free-threading and starts measuring the lock. It soft-fails + # (continue-on-error) precisely because this leg is allowed to be ahead of the pinned world. - name: Install the core engine (dev extra; no compiled-heavy extras) id: install if: steps.setup.outcome == 'success' diff --git a/.github/workflows/selfhosted-win2025-sql.yml b/.github/workflows/selfhosted-win2025-sql.yml index 24147c06..8e7c2c6d 100644 --- a/.github/workflows/selfhosted-win2025-sql.yml +++ b/.github/workflows/selfhosted-win2025-sql.yml @@ -66,7 +66,7 @@ jobs: shell: powershell run: | python -m pip install --upgrade pip - python -m pip install -e ".[dev,sqlserver]" + python -m pip install --constraint constraints.lock -e ".[dev,sqlserver]" - name: SQL Server store / coordinator / connector suites (against the real DB) shell: powershell diff --git a/.github/workflows/zizmor.yml b/.github/workflows/zizmor.yml index 99a00a02..4b7a6a95 100644 --- a/.github/workflows/zizmor.yml +++ b/.github/workflows/zizmor.yml @@ -1,16 +1,26 @@ name: zizmor # GitHub Actions static analysis (Trail of Bits), split out of security.yml so it runs only when the -# surface it lints actually changes: zizmor reads .github/** (workflows + its config), so a Python / -# docs / test PR cannot change its result and running it there was pure cost (~1 billed min on every -# one of ~900 monthly PR/push scans). The daily cron re-runs it against the UNCHANGED tree so a -# newly-added zizmor rule or newly-flagged action ref still surfaces within ~24h, and +# surface it lints actually changes. Its result moves with at least two inputs — the .github/** tree +# it lints, and its own pinned version, which arrives from ci/locks/ci-scanners.lock (see the install +# step) — so both are in the paths filter, and a PR touching neither was pure cost (~1 billed min on +# every one of ~900 monthly PR/push scans). The daily cron re-runs it against the UNCHANGED tree so a +# newly-added zizmor rule, or a newly-flagged action ref from the ONLINE audits that no paths filter +# can see, still surfaces within ~24h, and # workflow_dispatch covers on-demand runs. NOT a branch-protection required check (verified against # the live protection rules), so a paths-filtered skip can never wedge a PR or auto-merge. on: pull_request: paths: - ".github/**" + # zizmor's own VERSION lives OUTSIDE .github/ — pyproject.toml's [dependency-groups].ci-scanners, + # hash-pinned through this export, which the install step below reads. A bump PR touching only + # those two never ran this gate (PR #66, zizmor 1.5.2 -> 1.28.0: 33 contexts, not one of them + # zizmor), so a jump against a deliberately CLEAN baseline was first adjudicated by the 06:00 + # cron, against main, after merge. Unlike bandit and pip-audit — whose gates sit in security.yml + # on an unfiltered pull_request trigger and so DO re-run at the bumped version on the bump PR — + # this workflow is the only place the scanner it pins is exercised pre-merge. + - "ci/locks/ci-scanners.lock" schedule: - cron: "0 6 * * *" workflow_dispatch: diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index dd5b0f2d..a01ece4c 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -76,7 +76,7 @@ repos: # 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 + # NOT a required check (it is paths-filtered, 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 diff --git a/docs/CI.md b/docs/CI.md index 165c1194..5fc95d04 100644 --- a/docs/CI.md +++ b/docs/CI.md @@ -21,7 +21,7 @@ claims move with it. | `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), 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. | +| `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, 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. | | `dast.yml` | Authenticated authorization sweep against a live loopback listener in front of a real engine. **Not a required check** — nightly / release-tag / manual dispatch only, with no `pull_request` trigger, so it never reports on a PR and cannot wedge one. It is NOT `continue-on-error`: it goes red on a finding. See [ADR 0155](adr/0155-dast-dynamic-security-testing-of-the-running-engine.md). | | `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. | @@ -144,7 +144,7 @@ an unrelated PR without turning the gate red. 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` (which is already paths-filtered). 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 diff --git a/pyproject.toml b/pyproject.toml index d37f34f2..847114b1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -48,9 +48,13 @@ dependencies = [ "hl7>=0.4.5", # python-hl7: fast, tolerant parsing for routing "pydantic>=2.6", # channel config models / validation # CONSTRAINT PIN (transitive, via pydantic/fhir-core): annotated-types 0.8.0 dropped `SLOTS`, - # which fhir-core/fhir.resources import — every FHIR test ImportErrors on it. CI installs with a - # FRESH resolve (uv pip install -e ".[dev,...]"), so it ignores the lock's annotated-types==0.7.0. - # Drop this cap once fhir-core supports 0.8.x, or once CI installs from the hashed lock. + # which fhir-core/fhir.resources import — every FHIR test ImportErrors on it. The cap bounds the + # RESOLVER, the only place it CAN be bounded: `uv lock` would otherwise choose 0.8.0 and every + # downstream artifact inherits it (uv.lock -> constraints.lock -> ci.yml's `--constraint`), which + # is exactly how PR #66 went red. It also binds every resolve CI does not constrain — an end user's + # `pip install messagefoundry[fhir]`, the local dev venv behind pre-commit, the dispatch-only + # workflow legs. Drop it once fhir-core ships a 0.8-compatible release — NOT once CI installs from + # the hashed lock; the lock carries whatever the resolver already chose. "annotated-types<0.8", "aiosqlite>=0.20", # async SQLite for the message store/queue "fastapi>=0.137.1", # localhost engine API @@ -118,8 +122,10 @@ otel = ["opentelemetry-sdk>=1.20", "opentelemetry-exporter-otlp>=1.20"] # optional lxml extra and is deferred (JSON-only MVP) — never bare-parsed (ADR 0022 §6, Options #5). # annotated-types is CAPPED <0.8: fhir-core (dragged in by fhir.resources) imports the `SLOTS` # constant that annotated-types 0.8.0 REMOVED, so any fhir.resources parse dies with -# "ImportError: cannot import name 'SLOTS' from 'annotated_types'". It is a transitive dep, so the cap -# lives here rather than in [project.dependencies] (pydantic, the other consumer, is fine on 0.8). +# "ImportError: cannot import name 'SLOTS' from 'annotated_types'". Declared BOTH here and in +# [project.dependencies]: this copy documents the consumer, the base copy is what bounds a plain +# `pip install messagefoundry`. One Dependabot ignore entry covers both (it matches by package name, +# not declaration site) — lift the ignore and both caps together. # Drop the bound once fhir-core ships a 0.8-compatible release. fhir = ["fhir.resources>=7.1.0", "fhirpathpy>=2.2.0", "annotated-types<0.8"] # DICOM (DIMSE C-STORE SCP/SCU) connectors + codec for parsing/dicom/ (ADR 0025, BACKLOG #24). Lazy-imported, @@ -182,10 +188,11 @@ dev = [ "pytest-asyncio>=0.26", # >=0.26 adds asyncio_default_test_loop_scope (BACKLOG #17 shared-loop fix) "pytest-timeout>=2.3", # per-test watchdog: a hung test dumps stacks + fails fast in 60s, never wedges the leg "pytest-rerunfailures>=16.0", # in-run auto-retry for the known harness-monitor timing flake: a single flake occurrence self-heals instead of reding the whole matrix + blocking auto-merge - # Pinned BELOW 0.16: ruff 0.16.0 turned on stricter defaults (RUF022/RUF100/BLE001) that flag ~525 - # findings in existing code. CI installs with a FRESH resolve (uv pip install -e ".[dev,...]"), so an - # unbounded ">=" silently adopts a new major lint baseline and reds every PR. Lift the cap in a - # deliberate PR that also clears the new findings. + # Pinned BELOW 0.16: ruff 0.16.0 turned on stricter defaults (RUF022/RUF100/BLE001) that flag + # hundreds of findings in existing code — at least ~870 on PR #66's ubuntu leg (whole-repo scope, + # 2026-08-01). An unbounded ">=" lets `uv lock` adopt that new baseline, and it flows out through + # uv.lock -> constraints.lock -> ci.yml's `--constraint`, plus every unconstrained resolve. Lift the + # cap in a deliberate PR that also clears the new findings. "ruff>=0.4,<0.16", "mypy>=1.10", # httpx (the ASGI test client for the API + harness/load polling) is now a BASE dependency, so it diff --git a/tests/test_coord_lock.py b/tests/test_coord_lock.py index fd2d6940..6ca1da2d 100644 --- a/tests/test_coord_lock.py +++ b/tests/test_coord_lock.py @@ -6,9 +6,12 @@ once race ``.git/config.lock`` -- reproduced on Windows as "could not lock config file", leaving orphaned branches behind. ``new.ps1`` now serializes that call through this lock. -The load-bearing test is ``test_only_one_of_eight_concurrent_claimants_wins``: it launches eight real -processes at once and asserts exactly one acquires. Anything less than genuine concurrency would pass -against a lock that does not lock at all -- which is the failure mode this file exists to exclude. +The load-bearing test is ``test_eight_concurrent_claimants_never_hold_it_at_once``: it launches eight +real processes at once and asserts that no two are ever inside the critical section together, via a +CreateNew sentinel taken under the lock. Counting winners instead would test the scheduler -- a +straggler that arrives after the winner released wins legitimately, which is what made the earlier +form fail under CI load. Anything less than genuine concurrency would pass against a lock that does +not lock at all -- which is the failure mode this file exists to exclude. The number is not arbitrary: a read-modify-write in this same codebase was measured silently losing 4 of 8 concurrent PowerShell writes, so eight is the shape already known to break the naive approach. """ @@ -56,6 +59,7 @@ def acquire( timeout: int = 2, hold_ms: int = 0, barrier: Path | None = None, + witness: Path | None = None, ) -> subprocess.CompletedProcess[str]: """Take the lock, optionally hold it, release. Prints ACQUIRED on success. @@ -63,8 +67,11 @@ def acquire( the moment *its own* ``pwsh`` has booted, and cold-start times vary by far more than any reasonable hold: a straggler that arrives after the winner has already released then wins legitimately, and the caller sees two winners against a lock that never misbehaved. With it, the - claimant signals readiness *after* start-up and waits for the go file, so the whole cohort - arrives within milliseconds of each other however long any individual process took to start. + claimant signals readiness *after* start-up and waits for the go file, so pwsh cold-start time is + excluded from the race. It does NOT cover Enter-CoordLock's own prologue: the ``git rev-parse`` + spawn at lock.ps1:47 runs after the gate and before the deadline is set at lock.ps1:56, and under + CPU load that alone spread the cohort's deadline start by 645-2134 ms. ``witness`` is what makes + the test sound despite that -- see the test below. """ gate = "" if barrier is not None: @@ -75,12 +82,26 @@ def acquire( f"if ((Get-Date) -gt $bd) {{ throw 'barrier timeout' }}; " f"Start-Sleep -Milliseconds 10 }}; " ) + enter = leave = "" + if witness is not None: + # Asserts the invariant the mutex actually PROMISES -- that two claimants are never inside the + # critical section at once -- with no reference to a clock. CreateNew is the same atomic + # test-and-set the lock itself uses, so a second simultaneous holder throws. NB: at this + # hold/timeout ratio no second LEGITIMATE acquirer exists within a round, so "sentinel already + # exists" can only mean overlap; lowering hold_ms re-arms a false positive in this bare catch. + enter = ( + f"try {{ $w = [IO.File]::Open('{witness}', 'CreateNew', 'Write', 'None') }} " + f"catch {{ Write-Error 'MUTEX-VIOLATED'; exit 9 }}; " + ) + leave = f"$w.Dispose(); Remove-Item -LiteralPath '{witness}' -Force; " script = ( f". '{LOCK}'; " f"{gate}" f"$l = Enter-CoordLock -Name '{name}' -TimeoutSeconds {timeout} -Repo '{repo}'; " + f"{enter}" f"Write-Output 'ACQUIRED'; " f"Start-Sleep -Milliseconds {hold_ms}; " + f"{leave}" f"Exit-CoordLock $l" ) return subprocess.run( @@ -100,27 +121,37 @@ def test_lock_can_be_taken_and_released(repo: Path) -> None: assert "ACQUIRED" in second.stdout, second.stderr -def test_only_one_of_eight_concurrent_claimants_wins(repo: Path, tmp_path: Path) -> None: +def test_eight_concurrent_claimants_never_hold_it_at_once(repo: Path, tmp_path: Path) -> None: """Eight real processes, one lock, all reaching for it at the same instant. - The barrier is load-bearing, not ceremony. ``ThreadPoolExecutor`` only starts the eight processes - together; it says nothing about when each one, after ``pwsh`` start-up, actually reaches - ``Enter-CoordLock``. On an idle machine that spread is tens of milliseconds and the test looks - fine; on a loaded CI runner cold-starting eight shells it can exceed the 1500 ms hold, and then a - straggler finds the lock already released and wins on its own merits. The result is two winners - and an accusation against a mutex that did nothing wrong — measured, not theorised: delaying one - claimant by 1.9 s reproduces exactly that against the unmodified lock. - - Releasing the cohort only once all eight are parked on the gate also makes the file's headline - claim true rather than hopeful: the overlap is now guaranteed, so a lock that did not lock could - not pass this by getting lucky with scheduling. + Asserts OCCUPANCY, not winner count. The barrier releases the cohort together, but each process + then runs Enter-CoordLock's own prologue (a ``git rev-parse`` spawn, lock.ps1:47) before its deadline + is set (lock.ps1:56), and nothing synchronizes that. A winner-count assertion tolerates only + (hold - timeout) of that skew; under CPU load the skew was measured at 645-2134 ms, so counting + winners tests the scheduler, not the mutex. Measured directly: in five multi-winner rounds the hold + intervals were strictly DISJOINT -- the extra winners acquired legitimately, after release. + + The witness sentinel is what makes this sound. hold_ms is sized so that a lock which did not lock + would put all eight inside the critical section together and trip it; the margin over the measured + skew is (hold - timeout) = 7 s, ~3.3x. Validated both ways per the project's make-it-fail-first + rule: with Enter-CoordLock stubbed to a no-op it FAILS 3/3 (7 violations each); against the real + lock it PASSES 8/8 at 40 CPU burners -- the exact load at which the old assertion failed 8/12. + Cost: the winner now holds for 8 s, so this test's floor wall-time is ~8-9 s on every run. """ barrier = tmp_path / "barrier" barrier.mkdir() with ThreadPoolExecutor(max_workers=8) as pool: claims = [ - pool.submit(acquire, repo, timeout=1, hold_ms=1500, barrier=barrier) for _ in range(8) + pool.submit( + acquire, + repo, + timeout=1, + hold_ms=8000, + barrier=barrier, + witness=tmp_path / "holder.sentinel", + ) + for _ in range(8) ] deadline = time.monotonic() + 120 while len(list(barrier.glob("ready-*"))) < 8: @@ -131,10 +162,16 @@ def test_only_one_of_eight_concurrent_claimants_wins(repo: Path, tmp_path: Path) (barrier / "go").write_text("", encoding="utf-8") results = [c.result() for c in claims] + violations = [r for r in results if "MUTEX-VIOLATED" in r.stderr] + assert not violations, f"{len(violations)} claimants held the lock simultaneously" winners = [r for r in results if "ACQUIRED" in r.stdout] - assert len(winners) == 1, f"expected exactly 1 winner, got {len(winners)}" + assert winners, "nobody acquired -- the lock is wedged" # Everyone else must FAIL, not silently proceed. Silent success is the bug. - assert all("Timed out" in r.stderr for r in results if "ACQUIRED" not in r.stdout) + assert all( + "Timed out" in r.stderr + for r in results + if "ACQUIRED" not in r.stdout and "MUTEX-VIOLATED" not in r.stderr + ) def test_timeout_refuses_to_steal_and_names_the_holder(repo: Path) -> None: