Repository navigation
Commit dfcd595
authored
Add bin/ci with gh signoff (#7)
* Add bin/ci with gh signoff, matching our other repos
No cloud CI here, so bin/ci runs the checks locally and signs the commit off
on success, the same shape as bcx and highrise.
Bash rather than their Ruby CI class, deliberately: this repo exists because a
working Ruby is precisely what you don't have yet, so its own CI must not need
one. house-skills/bin/ci sets the in-house precedent.
Only a full run signs off. Passing a platform or version narrows the matrix and
explicitly declines to sign — a green tick covering one platform is worse than
no tick.
The lint pass costs nothing and catches the two mistakes that otherwise surface
ten minutes into a Docker build: a syntax error in a definition, since ruby-build
sources these, and an install_package URL with no #sha256, since ruby-build
silently skips verification when the checksum is absent.
Shellcheck runs when present and says so out loud when it isn't, rather than
passing silently. It's gated at warning severity because the info tier here is
all intentional. Fixing what it did flag: two declare-and-assign warnings, and
a note on the one deliberate unquoted expansion, which holds two words and must
split — an array would be tidier but expanding an empty one under set -u breaks
on macOS's Bash 3.2.
* Handle bin/ci --help before the lint pass, not after
* Fix two silent false-greens in the lint pass, and stop under-scheduling builds
Both lint findings are the same failure mode this repo keeps producing: a check
that reports success because it inspected nothing.
sort -V is GNU-only, and macOS — which we explicitly support — ships BSD sort.
The failure doesn't trip set -e in bin/ci because the call sits in a command
substitution inside a `for` list, so definitions() returned empty and both lint
passes sailed over zero files and printed "CI passed". Reproduced with a stub
sort that rejects -V: six definitions silently unchecked, exit 0. Plain sort
here, since ordering is cosmetic when linting, plus a hard guard so an empty
list can never be a pass whatever the cause — wrong directory, failed glob,
broken sort.
test/build had the same -V dependency but failed loudly instead, because a
failing command substitution in a direct assignment *does* trip set -e. Still
broken on macOS, just noisily, so it now detects -V support and falls back.
The checksum lint only matched double-quoted URLs, so a definition written with
single quotes — valid shell — would extract nothing and pass with no digest at
all. Widening the pattern alone would have flagged 1.8.7's `curl '<savannah>'`
calls, which fetch config.guess and have no checksum to carry, so extraction is
now scoped to install_package lines and accepts either quote style. Verified
both directions: single-quoted without a digest fails, with one passes, and the
savannah URLs stay unflagged. Also fails when a definition yields no
install_package URLs at all, since that means the extractor stopped matching
rather than that the file is clean.
Separately, JOBS was budgeting MAKE_JOBS cores per container. Measured against a
real run, each container averages ~1.0 core: these builds are mostly
single-threaded — miniruby bootstrapping and generating exts.mk, the
dependency-serialized tail of make, gem install bundler — with brief parallel
bursts, and 1.9.3 forces make -j1 in its own definition to dodge a race. The old
default left ~75% of the machine idle and could queue the long pole (1.9.3,
~190s vs ~110s) behind short builds. Default to cores/2.
* Inspect line-continued install_package calls too
Scoping checksum extraction to lines starting with install_package missed a
call spelled across continued lines: only the first line matched, so a URL on a
continuation went uninspected. Latent rather than live — no definition uses
continuations today — but it's the same vacuous-pass hole one level down, and
the "found no URLs" guard wouldn't have caught it either, since another
well-formed call in the same file keeps the count nonzero.
Fold continuations before matching. Pure bash rather than sed or awk: the usual
line-joining one-liners differ between BSD and GNU, and macOS portability is
what this whole section is about.
* Match package URLs regardless of quoting
`install_package "x" https://…` is valid shell, and requiring quotes meant the
extractor found nothing there — passing a definition with no digest at all
rather than complaining. The "found no URLs" guard doesn't help when another
quoted call in the same file keeps the count nonzero.
This is the third input shape to slip past this check, after single quotes and
line continuations, so stop enumerating quote styles: match the URL itself and
let whitespace or either quote terminate it. One pattern now covers
double-quoted, single-quoted and bare, and the `tr -d` goes away with it.
Scoping to install_package lines stays, and stays load-bearing — over the whole
file this pattern would flag 1.8.7's `curl '<savannah url>'` calls, which fetch
config.guess and carry no checksum by nature.
Verified all four forms with digests pass, each without one fails, and the
savannah URLs stay unflagged.
* Check every URL for a digest, not just ones that look like install_package
Fifth report of the same defect, so stop fixing shapes. Matching invocations
means matching shell syntax, and shell spells the same call unboundedly many
ways — quoted, unquoted, line-continued, after `then` or `;` or `&&`, inside a
function. Each form the pattern doesn't know is a download that goes
uninspected, and it fails silently: reports success having looked at nothing.
Four such forms turned up in a row, each fix addressing the shape rather than
the class, so the next one was always waiting.
Inverted: every URL in a definition must carry a digest. That fails closed. A
download written in a syntax nobody anticipated is flagged rather than skipped,
and the only way to exempt one is to say so explicitly — right friction, given
an unverified download is what this exists to prevent.
One exemption, GNU's git web view for config.guess/config.sub: a moving HEAD
with no release tarball and no published digest, fetched only to teach ancient
configure scripts about modern architectures, never linked into the built Ruby.
Comment lines are skipped so a URL in prose isn't treated as a download.
Verified against six forms with no digest — including `&&` chaining and a curl
inside a function body, neither of which review had raised — all caught. Real
definitions still pass, savannah URLs stay exempt, commented URLs ignored.
* Exempt the two config files by name, not the whole savannah domain
The comment claimed a config.guess/config.sub exemption but the pattern was a
domain wildcard, so any other unverified download from that host would have been
skipped — with the checksummed Ruby URL keeping the count nonzero, silently.
Intent and implementation disagreed, and the implementation was the permissive
one.
Narrowing it first required fixing an extraction bug underneath. Excluding shell
metacharacters from the URL pattern also truncated URLs that legitimately
contain them: both savannah links were being cut at the first semicolon, down to
`?p=config.git`, discarding the `f=config.guess` / `f=config.sub` that says
which file is fetched. Nothing to match on. Metacharacters are now allowed
inside the match and trimmed from the end instead, which is where they actually
signal shell syntax rather than URL content.
Verified: real definitions still pass, a different artifact from the same host
is now caught, so is the same gitweb config.git path requesting another file,
and unquoted URLs trailed by `;` or `&&` are trimmed and still checked.
* Match the exempted config URLs as exact literals
`*f=config.guess*` also matches `f=config.guess.backdoor`, so the exemption
still skipped verification on unrelated downloads. Second time a wildcard in
this exemption has been wider than intended — a domain glob before, a filename
prefix now — so drop wildcards entirely and match the two URLs as literals.
There is nothing left to widen: the definitions reference exactly these two
fixed strings, anything else is checked. Quoted so `?` and `;` are matched
literally rather than as glob and case-clause syntax. If the URLs ever change
shape, this list has to be updated by hand, which is the intended cost of
skipping verification on a download.
Verified the two real URLs still pass and four bypass shapes are caught:
config.guess.backdoor, config.subversion, an altered hb parameter, and the same
path on another host.
* Join continued lines with nothing, as shell does
A backslash-newline is removed entirely in shell; it does not become
whitespace. Joining with a space split tokens that bash keeps together, so
`"https\` + `://host/pkg.tar.gz"` — one URL to bash, confirmed by sourcing it —
arrived at the extractor as `https ://host/pkg.tar.gz` and matched nothing.
Another download skipped silently.
Contrived to write by hand, but the joiner was approximating shell semantics
rather than following them, and that gap is what the check keeps getting caught
by. Ordinary continuations already carry whitespace around the backslash, so
nothing else changes.
Verified: real definitions pass, the split-scheme URL is now caught, and
continuations with digests still pass in all three forms.
* Ignore URLs in shell comments
Only full-line comments were stripped, so a reference link in a trailing comment
was treated as a download and failed for lacking a digest — blocking CI and
signoff over a URL that is never fetched. First false positive here rather than
a false negative, and the more disruptive direction: it stops work rather than
letting something through.
Stripping comments has to leave `pkg.tar.gz#<sha256>` alone, since cutting at
the first `#` would turn every checksummed URL into a failure. The `#` opening a
comment is always preceded by whitespace or starts the line; the `#` before a
digest never is, it follows the last character of the URL. Keying on that
separates them without parsing quotes.
Verified: a reference URL in a trailing comment is ignored, the digest on the
same line still registers, a full-line comment URL is ignored, and a genuinely
undigested download on a commented line is still caught.
* Recognise comments that open straight after a shell operator
`install_package ...;# note` is a comment to bash — verified, not assumed:
`bash -c 'echo one;# echo two'` prints only "one". The previous rule required
whitespace or line start before the `#`, so the trailing reference URL was read
as a download and failed the lint despite the real package URL being
checksummed. Same disruptive direction as the last one: it blocks CI over a URL
nothing fetches.
A `#` opens a comment when it starts a word, which includes straight after an
operator that terminated the previous one. Digests stay safe because a digest's
`#` never starts a word — it follows the last character of the URL, and no URL
character is in the operator set.
Verified against `;#`, `&&#` and a spaced `#`, all ignored, with the digests on
those same lines still registering and a genuinely undigested download after a
`;#` still caught.
* Track quote state when stripping comments
A regex over a line cannot know whether a `#` is inside quotes, so the previous
rule fired on the literal hash in `"download #1"` and truncated the rest of the
line — dropping a real install_package and its unverified URL. Successive
regexes here were each wrong in a new way, in both directions: one blocks CI
over a URL nothing fetches, the other skips a genuine unverified download.
So scan the line and track quoting instead of guessing. A `#` opens a comment
only when it starts a word and is unquoted. Digests are untouched because a
digest's `#` follows the last character of the URL, so it never starts a word.
Worth keeping this check despite the churn: ruby-build's verify_checksum returns
success for an empty checksum, so a missing digest means the download is never
verified and the build still passes. Nothing else catches that — not the build
matrix, not ruby-build itself.
Also splits comment_index's `local` in two. `local line="$1" n=${#line}` leaves
n at 0 in bash; it only scanned correctly because dynamic scoping resolved
${#line} to the caller's identically-named variable. Renaming either would have
silently disabled stripping. Verified by renaming the caller's variable and
confirming comments are still stripped.
* Fail safe on quoting forms the scanner doesn't model
$'...' has its own escape rules, so an escaped apostrophe inside it looked like
the end of a quoted string and the following `#` like a comment — truncating a
real install_package off the line and passing its unverified URL.
Modelling $'...', then $"...", then heredocs, is writing a shell lexer in bash,
and every incomplete version of one is wrong in some new way. This is the fourth
report in that sequence, so stop extending the model and bound it instead: the
scanner handles '...' and "..." and, on encountering anything else, declines to
strip that line at all.
The failure then lands in the safe direction by construction. Comments on such a
line get scanned for URLs, which can only produce a false positive — a loud
complaint about a URL that needed no digest. Guessing risks the silent
direction, dropping a real download and passing it unverified. No definition
here uses these forms, so the cost is theoretical while the safety isn't.
Verified the reported case now fails as it should, and ordinary quoted hashes
and trailing comments still strip with no false positives.
* Carry quote state between lines
Quoting is not a per-line property — a string can contain a literal newline, and
everything up to the closing quote is data. Scanning each line from a clean
state made a `#` inside such a string look like a comment, truncating the rest
of the line and taking any real download on it along too.
Unlike the last few reports this isn't an unmodelled construct; it's ordinary
`"..."` quoting that the scanner claimed to handle and got wrong by chunking the
file into lines. Thread the state through instead. When a comment is found the
line necessarily ends unquoted, since a comment can only open outside quotes.
Verified the reported case now fails as it should, with no false positives on
trailing comments, `;#` comments, or a quoted hash on the same line as a
checksummed download.
* Latch closed when quote state becomes unknowable
Interaction between my own last two commits: the fallback for unmodelled quoting
returned before updating the quote state that cross-line tracking had just
started depending on, so state could carry forward desynced and drop a real
download on a later line.
Fixing the interaction directly would leave the same shape of bug available
again. Instead, once a construct we can't track appears, stop stripping for the
rest of that file. State can then never be wrong, only absent, and the failure
is confined to over-reporting: URLs in comments get flagged loudly rather than
real ones dropped quietly. The latch is per file, so one awkward definition
can't affect the others.
Verified the reported case is caught and a clean definition in the same run
still strips its comments normally.
* Skip heredoc bodies instead of scanning them as shell
The comment listed heredocs as unmodelled but nothing acted on that, so their
bodies were scanned as code: a `#` in one looked like a comment, a quote in one
desynced quote state for the rest of the file.
Not hypothetical — 1.8.7-p374 contains a heredoc, and further down has a URL
inside a comment that depends on stripping still working. Latching off on `<<`
would therefore have failed our own definition, so this needs real handling
rather than another fail-closed shortcut: track the delimiter and skip the body.
Anything in it is patch content, not a download.
Openers are looked for in the code part of a line only, so one mentioned in a
comment doesn't start swallowing lines. `<<<` is excluded as a herestring with no
body, and arithmetic `<< 2` can't match since a delimiter must start with a
letter or underscore.
Verified: real definitions pass, a heredoc body containing a quote no longer
hides a later undigested download, `<<-` with an indented terminator ends
correctly, and neither `<<<` nor arithmetic shifts swallow what follows.
* Identify downloads by shape, deleting the shell scanner
Both reports are interactions with the heredoc handling added two commits ago:
an opener detected inside a quoted string, and join_continuations merging a
delimiter line because it runs before heredocs are recognised. Fixing them means
more lexer, and more lexer has produced more findings every time — quoting,
continuations, `;#`, ANSI-C strings, cross-line state, heredocs, now these.
The premise was wrong. All that machinery existed to answer "is this URL an
argument, a comment, or data", which needs a shell tokeniser. But every download
here is a release tarball and every reference URL is an issue or a repo, so the
question answers itself from the URL: check the ones ending in an archive
extension, ignore the rest. Position stops mattering, and 113 lines of scanner
go with it — comment_index, strip_comments, the unknown-state latch and the
heredoc tracking, along with the bug surface that kept generating reports.
The savannah config.guess/config.sub fetches no longer need an exemption list
either; they aren't archives, so they fall outside the rule naturally.
Cost, documented in place: a tarball URL written in a comment is flagged. Rare,
self-explanatory, and loud rather than silent. No definition has one.
Verified every undigested form from the whole review sequence is still caught —
double and single quoted, unquoted, continued, split mid-scheme, after `if`,
after `&&`, before `;`, after ANSI-C quoting, after a string spanning a newline,
after a heredoc, and after a quoted `<<HIDE` — with real definitions passing and
no false positives on their reference links.
* Cut unquoted URLs at the first operator, not just trailing ones
grep stops at whitespace, so `…tar.gz;echo done` arrived with `;echo` attached.
Trimming only trailing punctuation left it there, the archive extension went
unrecognised, and the download was skipped — silently, the direction that
matters.
Cutting at the first operator rather than the last is safe now that archive
shape decides what gets checked. The savannah gitweb links were the reason for
preserving mid-string semicolons, and they aren't archives, so nothing depends
on that any more. Deleting the exemption made this fix a one-liner.
Verified `;`, `&&`, `|` and `)` attached with no space are all caught, real
definitions still pass, and every form from the earlier rounds still fails as
it should.
* Classify on the URL path, ignoring the query string
`https://host/pkg.tar.gz?download=1` didn't end in an archive extension, so it
was skipped and its missing digest went unreported — silently. Classification
now uses the path with the query removed.
Fixing that surfaced a hazard in the previous commit: trimming at the first
shell operator cuts a legitimate `?a=1&b=2` query too, taking the digest after
it and reporting a checksummed URL as unchecksummed. `&` is both a shell
operator and query syntax, and no amount of trimming distinguishes them.
So the two questions are now asked of different strings. Whether a digest is
present is asked of the untrimmed URL, where `&` is harmless. What kind of URL
it is gets asked of the trimmed, query-stripped path, where an attached command
or query can't hide the extension. Neither answer depends on the other.
Verified a query-with-ampersand URL carrying a digest passes, query-string and
attached-command URLs without one fail, and every form from the earlier rounds
still fails as it should.1 parent fb80a75 commit dfcd595
3 files changed
Lines changed: 312 additions & 8 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
153 | 153 | | |
154 | 154 | | |
155 | 155 | | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
156 | 182 | | |
157 | 183 | | |
158 | 184 | | |
159 | | - | |
| 185 | + | |
| 186 | + | |
160 | 187 | | |
161 | 188 | | |
162 | 189 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
7 | | - | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
8 | 16 | | |
9 | 17 | | |
10 | | - | |
11 | | - | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
12 | 28 | | |
13 | | - | |
| 29 | + | |
14 | 30 | | |
15 | | - | |
| 31 | + | |
16 | 32 | | |
17 | 33 | | |
18 | 34 | | |
| |||
58 | 74 | | |
59 | 75 | | |
60 | 76 | | |
61 | | - | |
| 77 | + | |
| 78 | + | |
62 | 79 | | |
63 | 80 | | |
64 | 81 | | |
| |||
109 | 126 | | |
110 | 127 | | |
111 | 128 | | |
112 | | - | |
| 129 | + | |
| 130 | + | |
113 | 131 | | |
114 | 132 | | |
115 | 133 | | |
| |||
140 | 158 | | |
141 | 159 | | |
142 | 160 | | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
143 | 166 | | |
144 | 167 | | |
145 | 168 | | |
| |||
0 commit comments