Repository navigation
ci(gates): run the shell self-tests and a bash 3.2 parse sweep on macOS - #355
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
|
@claude review this |
|
Claude finished @astubbs's task in 3m 7s —— View job Review:
|
| Claim | Verified |
|---|---|
62 tracked *.sh files |
✅ git ls-files -- '*.sh' → 62 |
60/62 use #!/usr/bin/env bash |
✅ confirmed by shebang scan (60 env-bash, 2 #!/bin/bash) |
17 self-test suites (bin/test-*.sh) |
✅ git ls-files -- 'bin/test-*.sh' → 17 |
bin/test-check-branch-self-reference.sh has 31 cases |
✅ 31 assert/assert_ci invocations |
bin/check-docs-data.sh fails closed without PyYAML |
✅ line 32-33 does exactly that, exit 2 |
docs/solutions/workflow-issues/gnu-only-constructs-fail-silently-on-bsd-2026-08-25.md exists |
✅ |
Referenced commit cf0007df1a exists and says what the PR body quotes |
✅ (cf0007d in the branch log) |
I also checked the actual CI run for this exact commit (a730b45, matches current HEAD) via mcp__github_ci: the Repo Hygiene workflow passed, and critically the new shell: macos job ran and succeeded on a real hosted macOS runner — so the PR's "assumed, not verified" caveat about node/gh/python3 availability is now resolved for the suites actually exercised. (highcpu / Chaos Pain Suite failed on this run, but that's an unrelated self-hosted integration lane this PR doesn't touch — not something to fix here. review gate red is expected per this repo's convention until this review lands.)
Workflow (.github/workflows/repo-hygiene.yml)
- The
shell: macosjob is well constructed: it pins PATHbashto/bin/bash(the real BSD 3.2 build Apple ships), then asserts the pin took (BASH_VERSINFO[0] = 3) before trusting anything downstream — this is the right instinct, since a silently-unpinned bash would make the whole lane pass while testing nothing new. - Both the self-test loop and the parse sweep have an explicit "matched nothing → fail" guard, avoiding the classic "green tick over zero tests" trap this repo has been bitten by before (cited in the job's own header comment).
- Job naming (
shell: macos) matches the<area>: <check>convention already used by the siblingshell: sigpipejob, andactions/checkout@v6matches the version used everywhere else in the file (satisfiesbin/check-action-versions.sh's one-version-per-action rule). - Minor, non-blocking observation:
pip install ... pyyamlhas no version pin, so a future PyYAML release could in theory change behavior under this lane silently. Given this only needsimport yamlfor a schema loader, I wouldn't block on it — just flagging it as the one spot without a determinism guarantee in an otherwise very defensive job.
bin/check-branch-self-reference.sh
The header correction is accurate and appropriately humble about the earlier mistake: the old comment claimed the pre-fix mapfile bug "still exited 0," which would have been the worse failure mode (silent success). The new text correctly attributes that wrong reading to checking $? through | tail rather than directly, and cites the AGENTS.md warning about exactly that class of git/pipe status mismeasurement. The new text also correctly forward-references the shell: macos job as the thing that would now catch a regression here.
docs/inflight/ci-bsd-portability-gaps.md
The edit follows this directory's own contract precisely (docs/inflight/AGENTS.md: track only what's open, delete/shrink on resolution, never leave a "will be fixed by PR X" marker). The removed section ("nothing runs on macOS in CI") is exactly the thing this PR closes. The remaining two sections are honestly scoped — each says explicitly whether the new lane helps (it does not help the check-pr-ready.sh stat/mtime gap or the latent source fallback defect, and says why in both cases) rather than overclaiming coverage.
Overriding the inherited decision
The PR body correctly identifies and satisfies the condition cf0007d set: that commit explicitly deferred adding the macOS lane pending "a repo-wide decision," and this PR is that decision being taken as a dedicated, reviewable CI change rather than a side effect of another fix — exactly per AGENTS.md's "read the commits you inherit" rule.
Verdict
No correctness bugs found. The change does what it says, the numbers in the PR description check out against the actual repo, and it's already been proven green on a real hosted macOS runner at this commit. Nothing here blocks merge from a correctness standpoint.
· branch fix/gates-run-on-macos
Leaving it unpinned, for three reasons - recording them here so the next reader does not have to re-derive the call. It would split an existing convention, not establish one. Pinning only the new site leaves the repo with one pinned and one floating install of one dependency, which is the state The failure mode is loud, not silent - which is the distinction this PR is built around. The lane's whole reason to exist is constructs that keep running and report a confident wrong answer. An unpinned PyYAML is not in that class: the very next step asserts importability ( A pin here would be an unwatched version string. Dependabot does not read inline Thanks for the verification pass on the PR body's numbers - that is the part I most wanted a second pair of eyes on. |
Every shell self-test lane in this repo is ubuntu-latest, so the harness was only ever exercised against GNU coreutils and bash 5 while contributors run it on macOS with BSD userland and bash 3.2. Four live portability defects reached master, were found by hand on the only Mac in play, and no CI job went red (#341). The class and its instances are written up in docs/solutions/workflow-issues/gnu-only-constructs-fail-silently-on-bsd-2026-08-25.md, which owns that knowledge. The damaging shape is not a flag that errors on BSD - that exits non-zero and somebody fixes it. It is the construct BSD ACCEPTS with a different meaning, so the script keeps running and reports a confident wrong answer: check-inflight-tags.sh extracted 0 of 17 impacts and printed "valid"; rename-packages.sh printed "already applied, nothing to do" over an untouched tree. **This overrides an inherited decision, recorded rather than done quietly.** Commit cf0007d ends: "Adding the macOS lane is a repo-wide decision rather than a side effect of a portability fix, so it is recorded and not done here." That condition is now met - this is a dedicated CI change, not a side effect. PATH bash is pinned to /bin/bash and the pin is then asserted. Most scripts are `#!/usr/bin/env bash` and the self-tests invoke gates as bare `bash`, so PATH bash decides what is actually under test; pinning makes "this lane exercises bash 3.2" true by construction rather than by hope. Suites are selected by glob, and the job fails if the glob matches nothing. **The lane shipped with the defect it exists to catch, and the fix is folded in here rather than left as a patch on top.** Its loop was `if bash "$t"`, collapsing every non-zero exit into one bucket. bin/test-check-shell-lint.sh exits 2 - "cannot run", no ShellCheck on hosted macOS - so CI reported a BSD defect in a suite that never executed a single case. Exit 2 now has its own counter and its own message naming the remedy, and the job prints "N run, N failed, N could not run". Exit 2 still FAILS the job, deliberately. On a portability lane a suite that could not run is missing coverage, not a tolerable skip; letting it pass is how a lane quietly stops testing anything while staying green. What the change buys is attribution, not leniency. A SKIP IS NOT A PASS is already the house rule in bin/check-all.sh, which gives exit 2 its own column for exactly this reason. Both missing dependencies are installed and then asserted importable/present, so a later exit 2 there means a real defect rather than a runner-image gap: PyYAML for check-docs-data.sh, ShellCheck for check-shell-lint.sh. Master's own repo: hygiene job already carries the identical shellcheck assertion. Proven able to fail, by mutation rather than by a green run: self-tests, unmodified 17 run, 0 failed mapfile reintroduced exit 1, 31/31 cases fail on bash 3.2 - and all 31 pass on bash 5, which is the coverage only this lane adds parse sweep, unmodified 62 scanned, 0 failed parse sweep, apostrophe restored exit 1, names the file version assert, stub bash reporting 5.x exit 1 exit-2 attribution, three-arm fixture 0, 1 and 2 counted and named separately; under real /bin/bash 3.2 exit-2-only still exits 1 Merging master brought one conflict worth recording: master had collapsed Repo Hygiene's nine per-gate jobs into a single job running `bin/check-all.sh --with-tests`. Resolved as master's structure plus this lane, because check-all.sh discovers the ubuntu gates by glob and re-adding them would double-run each. **Folding shell: macos into check-all.sh would not work** and is worth stating because it is the natural-looking simplification: check-all.sh runs on ubuntu-latest, so it exercises none of what this lane exists for. The value is the platform, not the list of scripts. .github/workflows/README.md claims to index every check and did not mention this one; it now does. docs/inflight/ci-bsd-portability-gaps.md is shrunk to what remains open, per that directory's rule against FIXED narratives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
…e header claimed Separate from the macOS lane in the same branch, and kept as its own commit because it is a factual correction to a different script with a different subject. bin/check-branch-self-reference.sh's header claimed the pre-fix gate "still exited 0" on the `mapfile` failure. It exited **127**. `set -euo pipefail` sits above the `mapfile` line, so exiting 0 was not available to it. The wrong figure came from reading `$?` through a pipe - the status belonged to the last command in the pipeline, not to the script being measured. That is the same mistake in miniature that the rest of this branch is about: a number that looks measured, is not, and is then written down as evidence. Swept the tree for the claim: one instance, here. The solutions write-up does not repeat it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
009d9dc to
06050d8
Compare
The reference gate requires any #NNN below the threshold to name its repo, because the fork's numbering sits entirely inside upstream's range and a bare number is a coin flip. It checks added lines only, so it never fired on text nobody was editing - leaving the convention true of the files the original work touched and false of the rest of the tree. Two passes, on the same lines and so landing together: - 366 bare references gain their repo. All 77 distinct numbers were resolved against BOTH repos first: 62 of them exist in each, meaning different things, so the classification is per-occurrence rather than per-number. astubbs#188 and astubbs#195 are fork mirror issues in the release notes while confluentinc#188 and confluentinc#195 are the upstream bugs cited in test comments in the same tree. - 24 `upstream #NNN` uses become the owner form. That form passed the gate but names a relationship rather than a repository, and this fork is itself upstream to anyone who forks it. Three sets are deliberately NOT prefixed, because they are not references: author ordinals ("run astubbs#1", "produce astubbs#1/astubbs#2", "NUDGE astubbs#1/astubbs#2") annotating log excerpts, reworded to plain numbers; the changelog gate's fixture, which asserts that a *bare* #NN is not a citation and would have been destroyed by qualifying it, so it moves above the threshold as a fake #999104; and upstream-pr-analysis.adoc, which is exempt and written entirely in upstream terms. README link text keeps its qualifier even though the URL beside it already names the repo - the gate can see a link target, a reader cannot, and this fork has its own astubbs#12. Quoted upstream titles keep the quotation intact with the number appended rather than having the owner inserted mid-title. README is generated: the edit is in src/docs/README_TEMPLATE.adoc. @tag("astubbs#355") becomes @tag("confluentinc#355"). Verified nothing selects on that tag - no pom, workflow or script filters it - and both classes still collect and pass. The 14 upstream-derived Java files gain the "Modifications Copyright" line the provenance-aware header check requires of any file changed since the fork point. docs/inflight/next-qualify-remaining-refs.md is deleted: this is everything it tracked, and in-flight files do not outlive their work. No behaviour change.
1 participant
Closes the "no CI lane runs any of this on macOS" item in
docs/inflight/ci-bsd-portability-gaps.md.Description
Adds a
shell: macosjob torepo-hygiene.ymlrunning the shell self-tests and a bash 3.2 parsesweep, and corrects a factual error in a script header.
This overrides an explicit inherited decision, recorded rather than done silently. Commit
cf0007df1aends: "Adding the macOS lane is a repo-wide decision rather than a side effect of aportability fix, so it is recorded and not done here." That condition is now met - the repo-wide
decision has been taken, and this is a dedicated CI change rather than a side effect. The commit body
names that commit and quotes the sentence.
Why the lane, in one line: four BSD portability defects reached master, were found by hand on the
only macOS machine in play, and no CI job went red, because every shell self-test lane is
ubuntu-latest.The job pins PATH bash to
/bin/bashbefore running anything and then asserts the pin took. Mostscripts are
#!/usr/bin/env bashand the self-tests invoke gates as barebash, so PATH bash decideswhat is actually under test; pinning makes the claim true by construction rather than by hope. Suites
are selected by glob, not a hand-list, so a new
bin/test-*.shgets covered automatically, and thejob fails if the glob matches nothing.
Two items in the original scope turned out to be already fixed on master by
ffba75936, so theyare not here. They were verified rather than assumed: the gate exits 0, its self-test is 31/31, and
all tracked scripts parse under bash 3.2.
One factual error corrected. The header of
bin/check-branch-self-reference.shclaimed thepre-fix gate "still exited 0". It exited 127 -
set -euo pipefailsits above themapfileline,so it could not exit 0. The wrong figure came from reading
$?through a pipe. Swept the tree: oneinstance, and the solutions write-up does not repeat it.
What the first hosted runs found
The original version of this section said the lane assumed hosted macOS provides
node,ghandpython3, and that the first CI run would be the test of that. It has now run.node,ghandpython3are all present. Two other things were not, and each surfaced as a suite refusing tomeasure rather than as a suite finding a defect:
bin/check-docs-data.shneeds it and fails closed at exit 2 without it. Installed,then asserted importable, so a later exit 2 there means a real defect.
bin/check-shell-lint.shwraps it andbin/test-check-shell-lint.shexits 2without it. Same treatment.
bin/check-all.shhad already named this exact pair as a hole itcannot close on such a machine: "the broken gate exits 2 and the linter exits 2, both land in
cannot, and the sweep reports zero failures - a false green produced by two skips agreeing."
Installing it here is what gives that gate any BSD coverage at all.
The lane had this PR's own bug in it
Worth calling out, because it is the subject matter turning up in the implementation.
The self-test loop was
if bash "$t"; then ok; else "self-test fails on macOS"; fi, which collapsesevery non-zero exit into one bucket. So the missing ShellCheck did not surface as "could not run";
it surfaced as
A suite that never executed a single case, reported as one that ran and found a BSD defect. That is
exactly the class this lane exists to catch, as its own header says two jobs up: not a construct that
errors, but one that keeps running and reports a confident wrong answer.
The loop now gives exit 2 its own counter and its own message naming the remedy, and prints
N run, N failed, N could not run. A SKIP IS NOT A PASS is already the house rule inbin/check-all.sh, which gives exit 2 its own column for this reason; the lane simply did notimplement it.
Exit 2 still fails the job, deliberately. On a dedicated portability lane a suite that could not
run is missing coverage, not a tolerable skip - letting it pass is how the lane quietly stops testing
anything while staying green, the same failure mode the bash 3.2 pin assertion guards against one
step up. What the change buys is attribution, not leniency.
Fixing only the dependency would have gone green and left the conflation in place until the next
exit-2 suite arrived, so both land together.
Proof the lane can fail
Mutation arms, not just a green run. The suite and script counts below are from when the arms were
run; the merge described next has since raised the suite count, which is the point of globbing rather
than hand-listing.
mapfilereintroduced/bin/bash3.2Merging master, and the one conflict worth recording
Master collapsed Repo Hygiene's nine per-gate jobs into a single job running
bin/check-all.sh --with-tests. This branch still had the nine-job shape plus its macOS lane.Resolved as master's structure plus the macOS job.
check-all.shdiscovers the ubuntu gates byglob, so re-adding them would double-run each, while the macOS lane is the one thing master cannot
subsume.
Folding
shell: macosintocheck-all.shwould not work, and is worth stating because it is thenatural-looking simplification:
check-all.shruns onubuntu-latest, so it exercises none of whatthis lane exists for. The lane's value is the platform, not the list of scripts.
.github/workflows/README.mdis the directory that claims to index every check, and itsrepo-hygiene.ymlline did not mention the new job; it now does.Checklist
docs/inflight/ci-bsd-portability-gaps.mdshrunk to what remains open, per thatdirectory's rule against FIXED narratives; the three surviving items each say whether this lane
helps.
.github/workflows/README.mdnow indexes the new job.docs/features/- N/A - CI-only change,no user-facing behaviour.
the mutation arms above demonstrate it can go red, including the new exit-2 attribution arm.