Mutation-check hardening + dependency lock (GAP-016)#66
Open
Bobcatsfan33 wants to merge 6 commits into
Open
Conversation
Four review notes, all small, all the kind this repo wins by not letting slide. 1. My report claimed "#65 mutation-verified — kills exactly the 4 the review named". Nothing in the repo backed that: the CI mutation check was #64's, and it mutates the SDK fail-closed default, not the exporter gate. The #65 run was local and one-time — a claim about a moment on my laptop, which is exactly what the house rule forbids. Generalises the script (now scripts/mutation_check.sh, since it is no longer SDK-only) with selectable targets, and adds the exporter gate as the second: it reintroduces the blanket `enabled:false` carve-out and fails the build if test_siem_write_path_gating stays green. CI runs `backend` in the backend job and `sdk` in the SDK job, because the deps differ. 2. Adds a dirty-tree guard. The byte-copy restore fixed what the script puts back; this fixes what a local user can LOSE if it dies at the wrong moment. My own footgun — the first version restored via `git checkout --` and ate the uncommitted change it existed to protect — is the test case. Guarded per-file, because refusing to run over an unrelated dirty doc just teaches people to skip the check. MUTATION_CHECK_ALLOW_DIRTY=1 overrides. 3. _validate_exporter_tier_on_update sent unknown stored types down the gated branch, advising PLATFORM_ENABLE_SIEM_EXTENDED — which can never un-gate a type that does not exist. Unreachable from HTTP (ExporterType is a Literal), but the branch order was the bug, not the reachability. Unknown stored types are now judged on the payload's own merits, with a test. 4. Dedupes routing-cases.json ("explicit false overrides a dev environment" and "explicit false in a dev environment still fails closed" asserted the same env twice), plus a test so the table cannot regrow duplicates — a table read by two languages should not make either wonder if it missed a distinction. Gates: backend 743 passed; python SDK 44; node SDK 41 (one duplicate case retired); both mutation targets verified.
"Green locally" was not "green in CI", and it cost a real break to learn:
local resolved fastapi 0.136.1, CI resolved 0.139.2, and 0.139 had made
include_router lazy. Routing was identical — only introspection changed — so
the app was fine and the TESTS were wrong. CI was right and there was no local
way to discover it. Guardrail 4 ("full suite green before every commit") is
worth very little while the two suites run against different libraries.
backend/requirements.lock: universal, fully hashed (98 packages, 2127 hashes),
generated by scripts/lock.sh via `uv pip compile --universal --generate-hashes`.
CI installs with `pip install --require-hashes -r requirements.lock` and so does
a developer — the same file, the same bytes. That equality IS the fix.
Universal on purpose: one lock serves every interpreter in requires-python
(>=3.11) and both linux CI and macOS dev. A per-platform lock is only true where
it was generated, which would have reproduced the original bug with extra steps.
uv stays a dev-time tool; CI installs with plain pip, so nothing new is trusted
at build time to make trust work.
A lock has two failure modes, so there are two mechanisms:
- drift: a CI step recompiles from pyproject and diffs. A drifted lock is worse
than none — it reports a resolution nobody asked for while looking
authoritative.
- staleness: pinning by hash means a CVE'd dependency stays pinned. Dependabot
covers pip, npm (frontend + Node SDK), gomod, and the Actions themselves — a
compromised action runs with the workflow's credentials. pip-audit remains the
backstop.
Framed as supply chain, not dev hygiene: --require-hashes means every artifact
must match a hash committed here, so a swapped upstream fails the build instead
of executing in it. It is also what makes the existing CycloneDX SBOM and SLSA
provenance describe something reproducible — an SBOM built from a floating
resolution documents one machine's afternoon.
Verified: the full backend suite passes against a scratch venv built only from
the lock (743 passed), and the lock pins fastapi==0.139.2 — the version CI was
already resolving, so local now matches CI rather than the reverse. lock.sh is
idempotent (regenerating produces an empty diff).
…, not the machine CI's drift check caught a real defect on its first run — in the drift check's own premise. The diff was only in "# via" comments: anyio, starlette and pytest-asyncio require typing-extensions on Python 3.12 but not on 3.14, and uv annotates those comments using the markers of whichever interpreter runs the tool. So the same command produced different bytes on macOS/3.14 than in CI/3.12. The RESOLUTION was identical; the file was not — and a byte-diff cannot tell those apart. Which made the lock reproduce, in miniature, the exact bug it exists to fix: output that depends on the machine that produced it. --python-version is now pinned to requires-python's floor (3.11) in both scripts/lock.sh and the CI sync check, so the output is a function of the FLAG. Verified: two runs on 3.14 produce identical bytes; the 3.11-targeted lock installs under 3.14 with --require-hashes; the full suite passes against it (743).
…mmand Second defect in the drift check's premise, same shape as the first. uv embeds the VERBATIM command line in the generated file's header, including the -o argument. The committed lock says "-o requirements.lock"; the check compiled to "-o /tmp/requirements.check". So the two files differed on line 2 by construction, and would have every run forever — the check could never pass. The artifact is a function of the exact invocation, so the only honest way to verify it is to make the exact invocation. CI now runs scripts/lock.sh itself and diffs against a saved copy. The check and the tool can no longer drift apart, because there is only one command. Verified by simulating the CI steps locally byte-for-byte: identical.
…GAP-016)
The image was the one thing GAP-016 didn't pin, and it is the one thing
security.yml Trivy-scans, SBOM-attests and cosign-signs. backend/Dockerfile
did `pip install .` — a floating resolution — so the pinning claim was loudest
about the artifact it did not cover. No number of signatures makes an SBOM
reproducible when the contents were decided by whatever PyPI served that
afternoon.
The Dockerfile now installs from a hashed lock with --require-hashes, then the
project with --no-deps.
A SECOND lock, not the existing one: the runtime stage does
`COPY --from=builder /install /usr/local`, so anything in the builder ships. The
all-extras lock would put pytest/ruff/mypy/bandit in production (100 pkgs vs
75), inflating what A-2 shrank and adding 25 packages of Trivy surface. So:
requirements.lock --all-extras (100) CI + dev
requirements-runtime.lock no extras ( 75) the image
The runtime lock is --constraint'ed to the full lock so the two agree VERSION
for version. Without that they drift: the first cut had CI testing
huggingface-hub 1.23.0 while the image shipped 1.24.0 (a dev extra held the
transitive back) — GAP-016 reintroduced one level down. My own guard test
caught it before it left the branch.
test_dependency_locks.py guards the properties: both locks hashed; runtime
carries no dev tooling; runtime is a strict subset of full at identical
versions; and the Dockerfile actually installs from the runtime lock (the lock
protects the image only if the image uses it).
Review follow-ups, same PR:
- lock.sh now ENFORCES its uv pin instead of naming it — uv's output format is
version-dependent, so a newer uv gives a deterministic but baffling CI
rejection. Verified it refuses a mismatched uv.
- re-added the dev+explicit-false routing case: the dedupe removed a pair that
was behaviourally but not MECHANICALLY duplicate ("dev" vs "development"), so
the dedupe guard would not have caught it and the shorthand lost coverage.
The guard's docstring now states that honest scope.
GAPS.md rewritten to match reality: the four things the lock does NOT cover
(Dependabot files nothing for transitive CVEs — pip-audit is the sole signal;
build-backend deps unpinned under PEP 517 isolation; uv/pip/pip-audit not
hash-pinned; sdks/python CI unpinned = new GAP-017). The "nothing new is
trusted at build time" overstatement is gone.
Verified: full suite 758 passed on dev venv AND on a venv built only from the
full lock; the app builds and serves 82 paths on the RUNTIME-only lock with
zero dev tools installed; both locks idempotent. NOT verified locally: the
Docker build itself (no local Docker) — CI's build-scan-sign job is the check.
Same bug class as the one it guards, one layer up. lock.sh regenerates both locks in the tree, but the sync step diffed only requirements.lock — and it runs before pytest, so test_dependency_locks.py validated the FRESHLY REGENERATED runtime lock, never the committed one. A committed requirements-runtime.lock that was stale, hand-edited, or even carrying pytest passed CI entirely green — and that committed file is exactly what the Docker image installs and cosign signs. "The thing we test is not the thing we ship", reintroduced inside the drift check itself. Fixed with `git diff --exit-code -- backend/` after running lock.sh: it fails if lock.sh changed anything at all, so it covers both locks and any future one for free, without naming files. Verified locally that a runtime lock hand-edited to carry pytest is caught (the previous per-file check let it pass).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits: the four review notes, then the lock file.
The four notes
1. A claim of mine had no in-repo backing. I reported "#65 mutation-verified — kills exactly the 4 the review named." The CI mutation check that existed was #64's, and it mutates the SDK fail-closed default, not the exporter gate. My #65 run was local and one-time — a claim about a moment on my laptop, which is precisely what the house rule forbids. The reviewer was right that this is small and right that it matters.
Fixed by generalising the script (now
scripts/mutation_check.sh, since it is no longer SDK-only) with selectable targets, and adding the exporter gate as the second — the highest-value mutation target in the repo, as suggested. It reintroduces the blanketenabled:falsecarve-out and fails the build iftest_siem_write_path_gatingstays green. CI runsbackendin the backend job andsdkin the SDK job, because the dependencies differ.2. Dirty-tree guard. The byte-copy restore fixed what the script puts back; this fixes what a local user can lose. My own footgun is the test case — the first version restored via
git checkout --and ate the uncommitted change it existed to protect. Guarded per-file rather than whole-tree: refusing to run because an unrelated doc is dirty just teaches people to skip the check.MUTATION_CHECK_ALLOW_DIRTY=1overrides. Verified both branches.3. Unknown stored exporter types no longer fall into the gated branch and advise
PLATFORM_ENABLE_SIEM_EXTENDED— advice the flag can never honour. Unreachable from HTTP, but the branch order was the bug, not the reachability. Test added.4. Deduped
routing-cases.json, plus a test so the table cannot regrow duplicates. A table read by two languages should not make either wonder if it missed a distinction. (Node drops 42 → 41 tests accordingly.)GAP-016 — the lock
backend/requirements.lock: universal, fully hashed (98 packages, 2127 hashes), generated byscripts/lock.sh. CI installs withpip install --require-hashes -r requirements.lock; so does a developer — the same file, the same bytes. That equality is the fix.It pins
fastapi==0.139.2— the version CI was already resolving. So local now matches CI, rather than the reverse.requires-python(>=3.11) and both linux CI and macOS dev. A per-platform lock is only true where it was generated — it would have reproduced the original bug with extra steps.--require-hashesmeans a swapped upstream fails the build instead of executing in it. It's also what makes the existing CycloneDX SBOM and SLSA provenance describe something reproducible — an SBOM built from a floating resolution documents one machine's afternoon.Test plan
pytest tests/— 743 passed on the dev venvpytest tests/— 743 passed on a scratch venv built only from the lock with--require-hashes, exactly as CI installsscripts/lock.shis idempotent (regenerating produces an empty diff — the CI sync check will not flap)dependabot.ymlandci.ymlparseNext: the GAP-001 mounts (aibom, siem) with full HTTP + tenant-isolation treatment — the ratchet's first live test, and the first time the de-aliased exemption lists shrink.
🤖 Generated with Claude Code
Update (post-review): the drift check diffed only
requirements.lockwhilelock.shregenerates both — so a stale committed runtime lock (the file the signed image installs) could pass green. Fixed: the sync step now runsgit diff --exit-code -- backend/, which covers both locks and any future one. Verified locally that a runtime lock hand-edited to carry pytest is caught.