build(packaging): pin and hash the bundled setuptools wheel, with one source of truth - #306
Conversation
… source of truth `pip download "setuptools<81"` floated. Three OS runners resolved independently, so one release could bundle different builds across platforms, and a rebuild of the same tag could bundle a different one again - with nothing written down to say which. Every other component of this installer is pin-and-held through a committed lock; this was the one exception, and it looked deliberate while being merely unpinned. `packaging/setuptools-compatibility.txt` is now the single source: `setuptools==80.9.0` with its sha256, verified against PyPI (1201486 bytes, `py3-none-any`, so ONE artifact serves all four platforms). Both workflows download it with `--require-hashes`, stage it by exact name rather than globbing `setuptools-*.whl`, and re-hash the staged bytes - `pip download` succeeds on a cache hit too, so a stale `packaging/staging/` from an earlier unpinned run would otherwise ship unnoticed. The pin had FOUR homes, not three. Beyond the two download commands and this script, each build env carried its own `setuptools<81` in `create-args` - and since tMAVEN is built with `--no-build-isolation`, that bound decided what tMAVEN built against. It is gone: the same hashed wheel is installed into the build env, so the build-time and shipped setuptools are one artifact by construction. The contract test found it, after two narrowings that are themselves the point - the first flagged the docstring's deprecation history, the second flagged the module's own parser. The install is two pip commands because it has to be: hash-checking mode is all-or-nothing and the tMAVEN spec is a git URL with no hash. Order is load-bearing - setuptools lands first, or tMAVEN builds against the lock's 82.x, the release that removed `pkg_resources`. Documented as a temporary runtime-only compatibility exception with its removal trigger stated (tMAVEN stops importing `pkg_resources`), and explicitly not waived from dependency auditing. Closes: #218
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Diff declared final at Risk is @codex review The thing I most want attacked is what this PR cannot verify. Neither packaging workflow runs on
Two smaller ones:
The digest was verified against PyPI rather than taken from the grooming note: |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR metadata and changed files =='
gh pr view 306 --repo bioedca/tether --json headRefOid,baseRefOid,files,title,url
gh pr diff 306 --repo bioedca/tether --name-only
echo '== Candidate files =='
fd -a -t f '^(packaging\.yml|.*\.ya?ml|setup_sidecar\.py|setuptools-compatibility\.txt|.*setuptools.*|.*sidecar.*)$' . \
-E .git -E node_modules -E dist -E build | sort
echo '== Python source outlines =='
for f in $(fd -a -t f 'setup_sidecar\.py$' . -E .git); do
echo "-- $f"
ast-grep outline "$f" --items all || true
done
echo '== Workflow and lock-file excerpts =='
for f in $(fd -a -t f '.*\.ya?ml$' .github packaging -E .git 2>/dev/null; fd -a -t f 'setuptools-compatibility\.txt$' . -E .git); do
echo "-- $f"
rg -n -C 8 --glob '*.yml' --glob '*.yaml' --glob 'setuptools-compatibility.txt' \
'(setuptools|require-hashes|only-binary|setup_sidecar|staging|pip download|pip install|pip wheel|create-args)' "$f" || true
done
echo '== Repository-wide candidate pin/bound references (excluding VCS metadata) =='
rg -n -i -C 2 \
'(setuptools\s*(==|<=|>=|<|>|~=|!=)\s*[0-9]|\b80\.9\.0\b|\b81\b|pkg_resources|setuptools-[^[:space:]]*\.whl)' \
-g '!.git/**' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' .
echo '== Relevant test outlines and assertions =='
fd -a -t f -i '(test|tests).*' . -E .git | while read -r f; do
if rg -q '(setuptools-compatibility|setup_sidecar|print-setuptools-wheel|restates_the_version|require-hashes)' "$f"; then
echo "-- $f"
ast-grep outline "$f" --items all || true
rg -n -C 12 '(setuptools-compatibility|setup_sidecar|print-setuptools-wheel|restates_the_version|require-hashes)' "$f"
fi
doneLength of output: 93212 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Raw release workflow install step (make control characters visible) =='
sed -n '174,188l' .github/workflows/release.yml
echo '== release.yml diff for that step =='
git diff 38b71fed7ddce31e2ec625b1dd32a3ae3e623ae2 4de6d5312e7237bbf3d31bc6ba5e43dca1b8f10d -- .github/workflows/release.yml | sed -n '1,180p'
echo '== setup_sidecar parser and CLI verification implementation =='
sed -n '115,215p' scripts/setup_sidecar.py
echo '== Parser tests, including all rejected fixtures =='
sed -n '80,205p' tests/test_setup_sidecar.py
echo '== Local packaging instructions and installer wheel consumption =='
sed -n '1,100p' packaging/README.md
sed -n '1,120p' packaging/scripts/post_install.sh
sed -n '1,130p' packaging/scripts/post_install.bat
echo '== PR diff for README and installer scripts =='
git diff 38b71fed7ddce31e2ec625b1dd32a3ae3e623ae2 4de6d5312e7237bbf3d31bc6ba5e43dca1b8f10d -- \
packaging/README.md packaging/scripts/post_install.sh packaging/scripts/post_install.bat
echo '== Search for documented executable packaging commands and compatibility-file references =='
rg -n -i -C 3 \
'(micromamba create|pip (download|install).*setuptools|SETUPTOOLS_WHEEL|setuptools-compatibility\.txt)' \
-g '*.md' -g '*.rst' -g '*.txt' -g '*.sh' -g '*.bat' -g '*.yml' -g '*.yaml' \
-g '!packaging/setuptools-compatibility.txt' .Length of output: 39584
The parser itself appears fail-closed for the extra forms called out: its ✅ Action performedReview finished.
|
…h consumer Both from CodeRabbit's round-1 review, and both would have surfaced first as a broken RELEASE - neither packaging workflow runs on a pull request, so nothing here executes the steps this PR changes. **A literal `\n` reached release.yml.** The generated step read `--only-binary=:all: \n -r ...`, so bash would have handed pip an argument `n`. The YAML parses, actionlint sees a valid string, and no CI job runs it. Now a real continuation, and a test rejects the shape - keyed on a FAILED CONTINUATION (`\n` followed by continuation indentation) rather than the two characters, because `printf '### %s\n%s\n\n'` in the same file is correct and flagging it would make the guard a nuisance that gets deleted. **There were FIVE consumers, not four.** `packaging/README.md` documents a runnable local build recipe that still added `"setuptools<81"` to the build env, downloaded it unpinned and unhashed, and exported `staging/setuptools-*.whl`. A documented command is a consumer - it gets copied and run - and that recipe could still produce exactly the unreproducible artifact this change removes. It now installs and downloads from the one source, verifies the staged bytes, and exports the exact name. The contract test covers it, which is what would have caught it. The single-source claim in the PR body was wrong when written: I asserted four homes after the test found the fourth, and stopped looking.
|
Round 1 answered at 1. A literal The test I added is keyed on a failed line continuation — The cause is worth naming: the step was produced by a Python script writing YAML, and one level of 2. There were FIVE consumers, not four. The single-source claim in my PR body was wrong when I wrote it. The contract test found a fourth Thank you also for confirming the two things I asked about and could not test: the parser is No further review is requested. Round 1 of 2 is spent; this is the answer to it. @codex has not reported on this PR. Under the routing rule |
Review FindingP1 — The “fifth consumer” fix leaves the removal contract and public documentation stale. The compatibility file still instructs maintainers to delete the file and “its three consumers.” Following that instruction would leave the newly recognized runnable The standalone handoff repeats the same incorrect count and lists only the script and two workflows, omitting the local packaging recipe. docs/idealize/standalone-tmaven-handoff.mdL35-L40 docs/idealize/standalone-tmaven-handoff.mdL48-L50 Additionally, the packaging README still says the source path uses Please update both removal instructions to include the README recipe—preferably by naming all consumers rather than retaining another fragile count—and replace the obsolete Checks
|
|
Nothing in Filed as #311, with the explicit note that widening the frozen band to make it pass is the one This PR is not blocked on it. It is blocked on |
Summary
Testing
|
Codex's round-1 finding, and its recommendation is better than the fix it asked for: a COUNT goes stale the moment one is added, which is precisely what happened. The requirements file still said "its three consumers" while the contract test had already found a fourth and CodeRabbit a fifth, so following the removal instruction would have left `packaging/README.md` pointing at a deleted file. The consumers are now listed by name, in the file itself, with the test that enforces the list named beside them. It also surfaced four dangling references to `SETUPTOOLS_PIN`, a constant this PR REMOVES: `post_install.sh`, `post_install.bat`, a `packaging.yml` comment, and - worst - `docs/troubleshooting.md`, which told a user debugging a failed sidecar that the script is "the ONLY place the pin lives". Each sent a reader looking for something that is not there. Removing a constant is easy; removing every sentence that explains it is what gets missed, so that is now a test rather than a sweep - repository-wide rather than over a consumer list, because the failure mode is exactly a file nobody thought of as a consumer.
|
Answered at "Name the consumers rather than retaining another fragile count." That is the actual lesson. The The Removing a constant is easy; removing every sentence that explains it is what gets missed. So that is On your third check: Both providers have now answered this round and the findings did not overlap. CodeRabbit found a
No further review is requested. Round 1 of 2 is spent; this is the answer to it. |
|
Acknowledged. The Round 1 findings were addressed at |
…se binary Maintainer resolved the escalation: `gh attestation verify --bundle`, shipped as a pinned+hashed `extra_files` artifact, bundle fetched from the public attestations API, NOT from conda-forge and NOT in the lock. Everything decision 2 requires substantively - attestation verification, checksum necessary-but-not-sufficient, refuse-and-report, no "install anyway" - is unchanged. The call mechanism and the delivery mechanism changed. THE ESCALATION PRESENTED A FALSE BINARY. It framed the choice as "bundle into the base lock" vs "first pip entry in the base lock". A third route was already proven in this repo: construct.yaml `extra_files` with a pinned, hashed, single-source-of-truth artifact - the #306 setuptools-wheel pattern - which ships a third-party binary with full provenance and never re-solves the 1192-entry lock. The re-lock this record insisted must be "a separate, deliberate PR" is not owed at all. That was the single biggest cost the escalation was about. It also decouples the verifier from the conda-forge feedstock, which matters more than it sounds: cosign there is 3.0.4 against upstream 3.1.2, four releases and ~6 months behind, one maintainer. Pin-and-holding a SECURITY tool to a channel that does not patch it promptly inverts the point of pinning. gh OVER cosign, on how much security logic Tether must author. cosign is a signature engine, not a sufficient verifier: it ignores `--type` entirely (a nonsense predicate URI still returned "Verified OK", exit 0), binds the digest but never the subject filename, and makes the caller hand-author the Fulcio identity regex - where a wrong regex silently accepts any Sigstore-signed artifact from any repository. gh encodes GitHub's own policy via --repo/--signer-workflow/--source-ref. AND A CORRECTION THIS RECORD OWED. An earlier draft said gh was disqualified because it demands a token. That is true only of the path where gh fetches the bundle itself. Both verified: the attestations REST endpoint is fully public (HTTP 200, complete sigstore v0.3 bundle, no Authorization header), and `gh attestation verify --bundle` then verifies with no credentials at all - proven end-to-end against the real 624MB v1.0.0-rc1 arm64 installer. No release-pipeline change is needed either; the bundle comes from the API, not from a release asset. Four app-side requirements are now recorded as requirements, not polish, because no tool here is sufficient alone: bind the subject FILENAME as well as the digest (one bundle covers all four installers); read the API response as explicit UTF-8 (cp1252 on Windows corrupts the Rekor checkpoint em-dash and produces a false tamper verdict); pin gh >= 2.67.0 (CVE-2025-25204 made this exact command FAIL OPEN); prefer the inline bundle and tolerate bundle_url. And one honest limit: decision 2 asks that the user be told WHY verification failed. That is not achievable - gh collapses every outcome to exit 1, so a tampered artifact and a dead network are indistinguishable. Recorded as a bounded split instead: ran-and-failed is reported, could-not-run is a SILENT no-op, or the air-gapped promise breaks by a second route. PRD 7.12 amended to match rather than mandating what cannot be built. All six follow-ups are now filed and linked: #330 settings store, #332 gh bundling, #248 release query, #333 verification, #331 hand-off, and the privacy amendment inside #248.
Third PR of the swarm pilot, and the one the plan chose as its stress case — #218 previously ran
17 review rounds. Claimed as
agent/issue-218, generation38511909794, fence held across bothauthoritative writes.
The defect
It floats. Three OS runners resolve independently, so one release can bundle different setuptools
builds across platforms — and a rebuild of the same tag can bundle a different one again, with
nothing written down to say which. Every other component of this installer is pin-and-held through a
committed lock; this was the one exception, and it looked deliberate while being merely unpinned.
The pin had four homes, not three
The issue names three (two workflows +
setup_sidecar.py). The contract test found a fourth: eachbuild env carried its own
setuptools<81increate-args— and because tMAVEN is built with--no-build-isolation, that bound decided what tMAVEN actually built against, independently ofwhat shipped.
It is gone. The same hashed wheel is installed into the build env, so the build-time setuptools and
the shipped setuptools are one artifact by construction rather than by two bounds agreeing.
What lands
packaging/setuptools-compatibility.txt— the single source.setuptools==80.9.0plus its SHA-256,verified against PyPI rather than trusted from the grooming note:
1201486bytes,py3-none-any,so one artifact serves win-64 / osx-64 / osx-arm64 / linux-64.
Both workflows now:
--require-hashes -r packaging/setuptools-compatibility.txt;setup_sidecar.py --print-setuptools-wheel, never asetuptools-*.whlglob;pip downloadsucceeds on a cache hit too, so a stalepackaging/staging/left by an earlier unpinned run would otherwise be staged and shipped withnothing having checked it.
setup_sidecar.pyparses the file instead of restating it, and installs in two pip commands— it has to: hash-checking mode is all-or-nothing and the tMAVEN spec is a git URL with no hash to
give. Order is load-bearing: setuptools lands first, or tMAVEN builds against the lock's 82.x, the
release that removed
pkg_resources.The contract test, and two narrowings that are the point
test_no_consumer_restates_the_version_or_the_boundtook three attempts, and each failure wasinformative rather than noise:
pkg_resourcesby 80.9.0 — deprecation history a reader needs, not a pin a bump must find;setuptools<comparator>flagged the module's own parser and an f-string echoing thevalue it had just parsed;
setuptools=={version}and the regex,rejects
setuptools<81andsetuptools==80.9.0— and that is what surfaced the fourth home.Acceptance criteria
setup_sidecar.pyconsumes the same source, hashed install split from the git-sourced tMAVENstated: tMAVEN no longer imports
pkg_resourcesfrom dependency auditing
packaging.ymlis manual/tag-triggered, sothe changed download and staging steps do not run here. That is stated rather than implied; see
below.
What is verified, and what is not
Verified: the digest against PyPI; the parser against five malformed requirement files; the two-command
split and its order; the absence of any restatement in all three consumers;
mkdocs build --strict.Not verified here: the workflow steps themselves. Neither packaging workflow runs on a pull
request, so
--require-hashes, the exact-name staging and the re-hash step are reviewed as code andnot as a passing run. The lowest-risk way to close that is a manual
packaging.ymldispatch on thisbranch before the next release; it is not a merge blocker for
main, but it is the residual andshould not be discovered later.
Risk is
high:.github/workflows/**, a dependency pin, and a release path.Closes: #218