Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77a25ad488
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65e0c116d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Pushed 65e0c11 for both Codex findings. Both were the same failure mode — a check reporting success because it inspected nothing — so both now fail loudly instead, with the empty/unparseable cases guarded rather than just the specific triggers. Also fixed scheduling while in here. Dogfooded end to end on this branch — 12/12 green, signed off. @codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 778e1b1cbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b91da4e923
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Pushed da3df67. Five reports of the same defect meant I was fixing shapes rather than the class — quoted, unquoted, continued, control syntax — so the checksum lint is now inverted: every URL in a definition must carry a digest, not just those on lines that pattern-match an That fails closed. Matching invocations means matching shell syntax, and shell spells the same call unboundedly many ways; each form the pattern doesn't know is a download that goes uninspected, silently reporting success. Checking every URL flags the unanticipated case instead of skipping it. One explicit exemption for GNU's git web view (config.guess/config.sub — moving HEAD, no published digest, never linked into the built Ruby); exempting anything else now requires adding it to that list. Caught six digest-less forms in testing, two of which review hadn't raised. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da3df679a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff1c335cf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/build:29
- This raises the default
JOBSfromCORES / 4toCORES / 2(and the cap from 12 to 16), but the README still documents the old value: line 196 reads "defaults to cores/4". The documented default should be updated to matchCORES / 2so the docs and behavior stay consistent.
JOBS=${JOBS:-$(( CORES / 2 ))}
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c788ff749
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/build:29
- The default
JOBSvalue is changed here fromCORES / 4toCORES / 2, butREADME.mdstill documents the default ascores/4(in the "Testing" section: "Tune the load withJOBS(containers at a time, defaults to cores/4)"). Please update the README so the documented default matches the code.
JOBS=${JOBS:-$(( CORES / 2 ))}
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3345ee0de3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/build:15
- The new "no definition files found" guard can't actually print its message. Under
set -euo pipefail, when the glob[0-9]*matches nothing,lsexits non-zero andpipefailpropagates that through the pipeline, so the bareVERSIONS=$(...)assignment on line 11/13 tripsset -eand the script exits before reaching this guard on line 15. (This is the same trap thebin/civersion deliberately sidesteps by wrapping the check inif [ -z "$(definitions)" ].) Appending|| trueto the two assignments lets an empty result fall through so the intended, friendlier error is shown.
VERSIONS=$(ls -1 [0-9]* 2>/dev/null | sort -rV)
else
VERSIONS=$(ls -1 [0-9]* 2>/dev/null | sort -r)
fi
[ -n "$VERSIONS" ] || { echo "error: no definition files found in $PWD" >&2; exit 1; }
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ede8ff56d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98a62f9dd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/build:29
- The default
JOBSis being changed fromCORES / 4toCORES / 2here, butREADME.md(line 196) still documents the default as "cores/4": "Tune the load withJOBS(containers at a time, defaults to cores/4)". Please update that line in the README so the documented default matches the new code.
JOBS=${JOBS:-$(( CORES / 2 ))}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0314e6db1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| found=$(( found + 1 )) | ||
| # Unanchored, with a boundary, since the digest may be followed by a query | ||
| # remnant or an attached command rather than ending the string. | ||
| [[ $raw =~ \#[0-9a-f]{64}([^0-9a-f]|$) ]] || { fail "$f: no sha256 on $base"; ok=false; } |
There was a problem hiding this comment.
When an unquoted archive URL is immediately followed by ;#<64 hex digits>, Bash ends the URL argument at the semicolon and treats the hash as a comment, so the actual download has no checksum. Classification correctly trims the URL at the operator, but this regex searches the untrimmed raw value and accepts the comment as its digest; I reproduced bin/ci --lint reporting success and exiting 0 for such a definition. Restrict checksum validation to the URL portion rather than any operator-attached suffix.
Useful? React with 👍 / 👎.
| # queued behind short builds instead of starting immediately. | ||
| CORES=$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 4) | ||
| JOBS=${JOBS:-$(( CORES / 4 ))} | ||
| JOBS=${JOBS:-$(( CORES / 2 ))} |
There was a problem hiding this comment.
When JOBS is unset, this now launches one container per two cores (up to 16), while README.md still tells users the default is cores/4. For example, a 16-core machine now starts 8 containers rather than the documented 4, which can materially change the expected resource load; update the documented default alongside this change.
Useful? React with 👍 / 👎.
No cloud CI here, so
bin/ciruns the checks locally and signs the commit off on success — the same shape asbcxandhighrise.Why bash, not the Ruby CI class
The app repos use a small Ruby
CIclass. This repo can't: it exists precisely because a working Ruby is the thing you don't have yet, so its own CI must not require one.house-skills/bin/ciis bash for similar reasons.Usage
Only a full run signs off. Anything narrower reports results and explicitly declines to sign — a green tick that covered one platform is worse than no tick.
The lint pass
Cheap, and it catches the two mistakes that otherwise surface ten minutes into a Docker build:
bash -nover the scripts and every definition. ruby-build sources definitions, so a stray quote is a build failure, not a parse error you'd notice.install_packageURL must carry a#sha256. ruby-build silently skips verification when the checksum is absent, so a missing one downloads and builds whatever it got. Verified this check actually fires by removing a checksum and watching it fail, rather than trusting it to be right.Shellcheck runs if installed and says so out loud if not, rather than passing silently — a skipped check that reads as green is worse than no check. Gated at
--severity=warning: the info tier here is all intentional (lsoverfindon version-numbered filenames, deliberate word splitting), and gating on it would mean either noisy failures or a scattering of disable comments, both of which train people to ignore the step.Also in here
Fixing what shellcheck did flag in
test/build: two declare-and-assign warnings, plus a comment on the one deliberate unquoted expansion. That one holds two words (--platform linux/amd64) and must split; an array would be the tidier idiom, but expanding an empty array underset -uerrors on macOS's Bash 3.2, which this script has to keep working on.Not included
gh signoff install, which sets branch protection onmainand affects everyone on the repo — that's a policy call, not a code change.mainis currently unprotected. Say the word and I'll enable it.