Reorder check.sh: run source-only checks before the build - #128
Merged
Conversation
Audits all 125 closed PRs (127 pushed branches) for commit churn and repeated red CI runs, since comment counts are near-zero here and discussion isn't where the struggle shows up. Sorts the churn into six classes, each cited to the commits or CI runs it came from, and ranks nine fixes by rework removed per unit of work. The finding the rest follows from: CI is the only compiler and the only formatter, and check.sh runs its cheap source-only checks last, so a compile error hides the mechanical findings until the next 11-minute run. PR #80's own commit message names it -- "the three SwiftLint violations behind the earlier failures". Lands at the root beside the other tracked guides; docs/ is gitignored for local notes, so a tracked deliverable can't live there.
…ity gate Implements fixes 1-5 from DX.md. Together they target the two rework classes the closed-PR audit found most expensive -- Swift/test compile errors and formatting findings, both of which only CI can see, and both of which used to arrive one red run at a time. check.sh: source-only checks now run before anything that builds. swift-format lint, swiftlint lint and every portable linter move above swift test; only swiftlint analyze (reads the app build's compiler log) and periphery (runs its own xcodebuild) stay behind it. A green run does identical total work either way; a red one now reports everything mechanical in the first run instead of the third. Verified a faithful move: every pre-existing line survives, the analyze block only changed indentation on its way out of the nested if. check.yml: two new advisory jobs, neither required, both able to fail only where check already would. `compile` builds the engine and its tests and nothing else, turning a broken test build into ~2 minutes of feedback instead of ~11; dev-build now needs it, so a PR that doesn't compile stops paying for an app build too. `format-patch` runs swift-format for real and publishes the diff as a job summary and artifact, so the fix is `git apply` rather than a blind reflow from a machine with no formatter on it. It holds no write token and never pushes -- the auto-push variant races the author and is left as a deliberate open decision in DX.md. scripts/check-portability.sh: new gate for GNU-only shell idioms in scripts/ and .claude/hooks/, which run on BSD userland on a Mac and on CI but GNU in a web sandbox. shellcheck reads both as correct shell, which is how PR #116's BSD-sed sitemap strip shipped. Twelve rules, a `# portable-ok:` escape, and a --self-test that asserts each rule still matches its own probe -- added because the first draft split "pattern|advice" on the first |, truncating every pattern containing an alternation, and eight of twelve rules silently passed everything. AGENTS.md: records the four Swift Testing traps CI has caught more than once (#expect and mutating methods, #expect around rethrows needing try, per-file Foundation imports, private(set) across a file split), and renumbers the documented check.sh step order. The check skill and CLAUDE.md follow. Verified with scripts/check.sh --portable (green) plus shellcheck, shfmt, actionlint, zizmor, prettier and markdownlint. The Swift side is unverifiable here -- no macOS toolchain -- so CI on macos-26 is the authority on the reordered script and the two new jobs.
Contributor
Dev buildDownload Blurt.app — built from Installing itcd ~/Downloads
unzip -o blurt-dev-build-pr-128.zip # GitHub wraps every artifact in a zip
unzip -o Blurt-dev-7db5b10.zip
find Blurt.app -exec xattr -c {} + # clear quarantine: xattr lost -r in macOS 12.3
rm -rf /Applications/Blurt.app && cp -R Blurt.app /Applications/
open -a BlurtIt is ad-hoc signed and not notarized: Gatekeeper refuses to open it until Expect that re-grant once per dev build, including a second build of this tccutil reset Accessibility dev.alex.blurt |
CI caught what the local run could not: the gate scans `git ls-files`, so while check-portability.sh was itself untracked it never scanned itself. The commit made it visible and its twelve deliberately-bad probe lines became twelve violations. Marks the probes with the `# portable-ok:` escape the file already documents, rather than exempting the whole file — a real GNU idiom in this script should still fail, and it now would. Two things the failure also exposed: The three bare-command rules (timeout / md5sum / tac) required whitespace or line start before the name, so CI flagged nine probes rather than twelve and `$(md5sum f)` or `|tac` would have slipped past. Boundary widened to any non-identifier character. An untracked script is invisible to git ls-files, and a scan that skips the file you just wrote reads exactly like a scan that approved it — the same shape as the truncated-regex bug this file already carries a self-test for. The gate now names untracked shell scripts as a note, so the empty input set announces itself instead of passing quietly. Verified with the file tracked this time: --self-test reports all 12 rules live, the real scan covers 23 scripts clean, and check.sh --portable is green.
alexkroman
enabled auto-merge
August 12, 2026 13:55
The audit's conclusions are already carried by the code and docs it produced — the check.sh ordering rationale, the Swift Testing traps in AGENTS.md, the two CI jobs, and the portability gate all explain themselves at the point of use. The standalone report was the vehicle, not the deliverable. Rewrites the three references so nothing points at a deleted file: the ordering rationale in check.sh, the same in AGENTS.md's "What check.sh runs", and the one in the check skill. Each now states the reason inline rather than deferring to the report.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Reorders
scripts/check.shto run all source-only checks (formatting, linting, portability) before the multi-minute build steps (swift test, sanitizers, app build, UI tests, leaks). This is a pure reordering with no logic changes.Why it matters: When a compile error occurs, the cheap checks are now reached and reported in the first red run instead of the second or third — and they arrive ~10 minutes sooner. A green run does identical total work either way; what changes is how much a red run tells you at once.
Evidence from the closed-PR audit in
DX.md: PR #80 spent multiple runs on "three SwiftLint violations behind the earlier failures"; PR #124 burned three consecutive red runs on one compile error at a time. This reordering collapses those sequences.Related changes:
scripts/check-portability.shto flag GNU-only shell idioms (e.g.,sed -iwithout suffix,grep -P,readlink -f) that work on Linux but fail on BSD (macOS, CI). Catches the class of bug that PR Gate the Pages site's deployability in check.sh #116 paid for ("Fix the sitemap<loc>strip on BSD sed").DX.md: audit of 125 closed PRs identifying six rework classes and nine fixes ranked by impact. Documents the rationale for this and other improvements.AGENTS.mdand.claude/skills/check/SKILL.mdto reflect the new step order and explain why it matters.compilejob tocheck.ymlfor fast-fail typecheck (~2 min vs ~11 min for fullcheck), andformat-patchjob to publish the exactswift-formatreflow as an artifact so authors don't have to reproduce it by hand.--self-testmode tocheck-portability.shto verify every rule still matches its own probe (prevents silent rule death).How it was tested
scripts/check.shpasses (macOS)scripts/check.sh --portablepasses (verified on Linux sandbox)scripts/check-portability.sh --self-testpasses (all 12 rules live)check.ymljobs (compile,format-patch,check) pass on a test PRAGENTS.md,DX.md(new),.claude/skills/check/SKILL.md,CLAUDE.mdhttps://claude.ai/code/session_01LNQ3Jmz46Kjj7DaRJnNx5n