test(manifest): cover the installer's integrity surface (backend#1729 sweep 6) - #709
Conversation
… sweep 6)
Sweep 6 is "mutation-check the existing suites". Measuring first: of the 24
shell scripts under scripts/ (excluding tests), exactly TWO are referenced
by no bats suite -- check-style.sh and gen-manifest.sh. Client is in good
shape; this closes the one that matters most.
gen-manifest.sh produces scripts/manifest.sha256, which install.sh
verifies every fetched sub-script against BEFORE running privileged steps.
It is the installer's integrity surface, and nothing proved its guards
fire.
It carries three claims in its own comments. All three are TRUE -- worth
stating, since this epic is largely a record of such claims being false:
1. gen-manifest's FILES must match install.sh's FILES, "or vice versa"
2. the same for install.ps1's $Files
3. --check is non-zero on drift
What was missing is any test that would notice them ceasing to be true.
Both cross-checks are awk extractions of another file's array literal --
precisely the parser that goes quietly stale when the parsed file is
reformatted. 12 cases now pin them, including both stale-parser cases:
rename install.sh's array and the guard must REFUSE, not compare nothing
to nothing and agree.
ONE REAL GAP FOUND AND FIXED. Emptying FILES makes the manifest cover
nothing, and --check then compared an empty manifest to an empty
regeneration. It did fail -- on `set -u`'s "FILES[@]: unbound variable".
A failure by accident, whose message says nothing about the integrity
surface just lost. Now refused explicitly, naming the consequence: "an
empty manifest verifies nothing".
The first version of that guard used ${#FILES[@]}, which is ITSELF an
unbound-variable error on an empty array under bash 3.2 -- what macOS
ships and what this repo deliberately targets. So the guard reproduced,
by a different route, the exact unintelligible failure it was written to
replace. It now uses ${arr[*]-}, which expands safely whether the array
is unset, empty or populated. Caught by running it on bash 3.2, not by
reading it.
Also in the suite: a case asserting the COMMITTED manifest lists every
file install.sh fetches. That guards the premise rather than the
mechanism -- if the manifest were current AND short, every other test
here could pass while a fetched script had no digest at all.
12/12 pass; bats-hygiene passes, so every assertion is load-bearing
rather than advisory.
Remaining from the measurement: check-style.sh has no suite. Filed rather
than bundled -- one self-contained change per PR.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ndows The "committed manifest lists every file both bootstraps fetch" case could pass vacuously: missing starts at 0 and an empty awk/grep extract never entered the loop, so the assertion held having verified nothing. It also only scraped install.sh's FILES array despite claiming both bootstraps, so a Windows-only fetch absent from manifest.sha256 went unnoticed. Now scrape BOTH bootstraps -- install.sh's FILES and install.ps1's $Files, using the same awk/sed extractions gen-manifest.sh itself uses -- and fail loudly when either extract is empty (a broken scrape means the parser went stale, not that coverage is complete), before the missing-count loop runs. Bugbot, client#709. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b9176c1. Configure here.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Approve — reviewed for correctness at head b9176c19.
What this does
Adds scripts/tests/gen-manifest.bats (12 cases) covering gen-manifest.sh --check, the installer's integrity surface, and adds an explicit empty-FILES/WINDOWS_FILES guard to gen-manifest.sh that fails with a diagnostic naming the lost integrity guarantee instead of an accidental set -u "unbound variable".
Correctness — verified, not asserted
I ran the suite locally on macOS system bash 3.2 (the platform this repo deliberately targets):
- All 12/12 pass on bash 3.2 — confirming the guard's
${arr[*]-}form is safe on an empty array, where the first-attempt${#FILES[@]}would have re-created the exact unbound-variable failure it replaces. - bats-hygiene passes (18/18) → every assertion here is
|| return 1hardened (load-bearing). - The empty-surface guard is correctly placed before
ALL_FILES=("${FILES[@]}" "${WINDOWS_FILES[@]}"), whose expansion would itself tripset -uon an empty array — so the guard's diagnostic wins the race. - Mutation tests genuinely fail-closed:
_check_bootstrap_in_sync/_check_windows_bootstrap_in_syncrun before the--checkdiff, so a renamed/reformatted array yields an empty awk extract → "differ", not a vacuous nothing-vs-nothing agreement. - The coverage test now scrapes both bootstraps (
install.shFILES +install.ps1$Files) and guards each extract with[ -n ... ], so a broken parser fails loudly rather than sailing through withmissing=0— this directly addresses the earlier Cursor Bugbot "vacuous pass" note, which is resolved in the head commit.
Notes (non-blocking)
- This suite introduces
perl -0pimutation edits; existing suites usesed -i. Perl is preinstalled onubuntu-latestand macOS, so this is a style deviation, not a portability risk. - The
changing a covered file/$Files cannot be parsedcases assert only on exit status, not message — they still fail for the right reason (verified by running them), but a message assertion would harden them against a future unrelated non-zero exit.
Production runtime paths and the committed manifest.sha256 are unchanged; CI is green across all required checks.
Sweep 6 is "mutation-check the existing suites". Measuring first: of the 24
shell scripts under scripts/ (excluding tests), exactly TWO are referenced
by no bats suite -- check-style.sh and gen-manifest.sh. Client is in good
shape; this closes the one that matters most.
gen-manifest.sh produces scripts/manifest.sha256, which install.sh
verifies every fetched sub-script against BEFORE running privileged steps.
It is the installer's integrity surface, and nothing proved its guards
fire.
It carries three claims in its own comments. All three are TRUE -- worth
stating, since this epic is largely a record of such claims being false:
What was missing is any test that would notice them ceasing to be true.
Both cross-checks are awk extractions of another file's array literal --
precisely the parser that goes quietly stale when the parsed file is
reformatted. 12 cases now pin them, including both stale-parser cases:
rename install.sh's array and the guard must REFUSE, not compare nothing
to nothing and agree.
ONE REAL GAP FOUND AND FIXED. Emptying FILES makes the manifest cover
nothing, and --check then compared an empty manifest to an empty
regeneration. It did fail -- on
set -u's "FILES[@]: unbound variable".A failure by accident, whose message says nothing about the integrity
surface just lost. Now refused explicitly, naming the consequence: "an
empty manifest verifies nothing".
The first version of that guard used ${#FILES[@]}, which is ITSELF an
unbound-variable error on an empty array under bash 3.2 -- what macOS
ships and what this repo deliberately targets. So the guard reproduced,
by a different route, the exact unintelligible failure it was written to
replace. It now uses ${arr[*]-}, which expands safely whether the array
is unset, empty or populated. Caught by running it on bash 3.2, not by
reading it.
Also in the suite: a case asserting the COMMITTED manifest lists every
file install.sh fetches. That guards the premise rather than the
mechanism -- if the manifest were current AND short, every other test
here could pass while a fetched script had no digest at all.
12/12 pass; bats-hygiene passes, so every assertion is load-bearing
rather than advisory.
Remaining from the measurement: check-style.sh has no suite. Filed rather
than bundled -- one self-contained change per PR.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Note
Low Risk
Changes strengthen supply-chain CI checks and installer manifest generation; no runtime auth or data-path behavior changes beyond clearer failure on misconfiguration.
Overview
Adds
scripts/tests/gen-manifest.batsto exercisegen-manifest.sh --checkagainst copiedscripts/trees: lockstep withinstall.shFILESandinstall.ps1$Files, non-zero drift when hashes or files are wrong, and fail-closed behavior when those arrays can’t be parsed.gen-manifest.shnow refuses an emptyFILESorWINDOWS_FILESsurface with a clear integrity message instead of failing indirectly underset -u(or agreeing on an empty manifest during--check). A final test asserts the committedmanifest.sha256lists every path both bootstraps fetch.Reviewed by Cursor Bugbot for commit b9176c1. Bugbot is set up for automated code reviews on this repo. Configure here.