ci: fix changed formula discovery - #123
Conversation
There was a problem hiding this comment.
Review summary
This PR replaces the inline bash "find changed modules" step with a typed XGo script (find_changed_modules.gsh) run via ixgo, and simplifies the zlib formula. The rewrite is a clear readability win and the script correctly avoids shell-injection in its own git calls (env vars bound to XGo variables, NUL-delimited diff parsing, json.marshal for outputs). Docs (.claude vs .agents copies) are in sync and the new Zlib_llar.gox matches the documented formula API.
Main items below. The most impactful is a first-push regression on the default branch.
Findings (see inline comments)
find_changed_modules.gsh:49— default-branch first-push all-zerobeforeSHA is no longer handled (regression).find_changed_modules.gsh:73— deletion handling (ACMRT→ACDMRT) relies on re-probing the filesystem; fragile.test.yml:43—ixgobuilt from source uncached on the critical path of every run.
Additional note (no reliable inline location)
- Pre-existing shell-injection surface,
test.yml:100-101,112(not introduced by this PR, but this PR reworks the discovery that feeds it):${{ matrix.module }}is interpolated directly intorun:shell scripts, andmatrix.moduleis derived entirely from PR-controlled file paths (parts[0]+"/"+parts[1]). A PR adding a path like`evil`/name/versions.jsoncould inject shell into thetestjob (public repo,pull_requestevent,contents: readlimits blast radius but arbitrary code execution on the runner remains). Recommend passingmatrix.moduleviaenv:and referencing a quoted"$MODULE"inrun:. Since this PR restructures module discovery, it's a good moment to also constrain/validate discovered module names.
Minor / optional
find_changed_modules.gsh:64— the trailing empty token from-zNUL split is harmlessly skipped by theparts.len < 3check today; an explicit empty-changedPathskip would be clearer.find_changed_modules.gsh:71—os.stat(versions.json)runs per changed path rather than per module; harmless at current diff sizes, could dedupe by module.test.yml:43,105—ixgo@v1.1.1/llarare pinned to mutable tags and installed with-checklinkname=0; consider pinning to a commit and documenting why the linkname check is disabled.
| versionsPath := filepath.join(module, "versions.json") | ||
| _, err := os.stat(versionsPath) | ||
| if os.isNotExist(err) { | ||
| has, walkErr := hasFormula(module) |
There was a problem hiding this comment.
Deletion handling relies on re-probing the filesystem (ACMRT → ACDMRT added D).
Including deletions means changedPath can reference paths no longer present at HEAD. To distinguish "module deleted" from "module misconfigured (missing versions.json)", the code stats versions.json and then walks the module dir, treating os.isNotExist(walkErr) as skip. That discrimination depends on filesystem state rather than the diff status of the path: a deleted module is silently dropped, while a module whose dir still exists but lost its versions.json panics. This is fragile — branching on the git delete status (e.g. git diff --name-status / a D marker) would be more robust and intentional than re-probing with os.stat + walkDir + os.isNotExist. Worth an explicit decision/comment on whether deletions should be in scope at all.
| echo "has_modules=true" >> "$GITHUB_OUTPUT" | ||
| fi | ||
| echo "Changed modules: $modules_json" >> "$GITHUB_STEP_SUMMARY" | ||
| go install -tags=linknamefix -ldflags="-checklinkname=0" github.com/goplus/ixgo/cmd/ixgo@v1.1.1 |
There was a problem hiding this comment.
ixgo is built from source, uncached, on the critical path of every run.
setup-go here uses cache: false (line 32) and this step does go install ...ixgo@v1.1.1 from source on every trigger. Since the test matrix has needs: changes, this uncached compile of the ixgo interpreter is added serially to the front of every PR/push. Enabling Go build/module caching (drop cache: false) or using a prebuilt/pinned binary would remove this repeated work from the hot path.
Fix formula CI so it tests only modules changed on the current branch.
The implementation includes:
.github/scripts/find_changed_modules.gsh, executed through ixgov1.1.1.before..afterbehavior for default-branch pushes.llar testsupports version ranges.versions.json, while allowing complete module removals.google/crc32cwas scheduled.This prevents unrelated formulas from entering the CI matrix and turns invalid module metadata into an explicit failure.