Skip to content

ci: test changed formulas on pull requests#10

Merged
luoliwoshang merged 3 commits into
xgo-dev:mainfrom
MeteorsLiu:ci/formula-test
Jul 24, 2026
Merged

ci: test changed formulas on pull requests#10
luoliwoshang merged 3 commits into
xgo-dev:mainfrom
MeteorsLiu:ci/formula-test

Conversation

@MeteorsLiu

Copy link
Copy Markdown
Collaborator

Add pull-request validation for changed LLAR formula modules.

The implementation includes:

  • Detect changed <owner>/<repo> module roots that contain versions.json.
  • Run llar test on native Linux and Darwin amd64 and arm64 runners.
  • Install llar from goplus/llar and pass the selected OS and architecture to each test.
  • Skip test jobs when a pull request changes no formula module.
  • Leave a focused TODO for option-combination coverage once llar test can enumerate it.

This gives formula changes target-platform validation without running unrelated modules.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FennoAI Review — PR #10: CI to test changed formulas

Solid workflow overall: permissions: contents: read, no secrets, pull_request (not pull_request_target), concurrency with cancel-in-progress, base/head-SHA-pinned diff, NUL-safe path parsing, and fail-fast: false. The empty-module path is handled correctly (jq select(length > 0) filters the empty line → has_modules=false). Findings below are hardening and maintainability suggestions; inline comments carry the specifics.

Key points

  • Fork-PR untrusted execution (inherent): on pull_request, llar test runs fork-controlled formula recipes. The read-only, secret-free posture is correct; residual risk is runner abuse. Recommend enabling "Require approval for all outside collaborators" in repo settings. (see inline on the llar checkout / test steps)
  • Supply-chain / reproducibility: goplus/llar is pinned to mutable main. (inline, line 93)
  • Performance: Go cache disabled + llar rebuilt on every matrix job; matrix can fan out to 4 × N jobs (incl. scarce macOS runners) with no cap. (inline, lines 101, 57)
  • Injection hygiene: ${{ matrix.module }} is interpolated unquoted into the run: script. (inline, line 116)

Cannot be placed inline (file not in this diff)

  • .claude/skills/write-formula/SKILL.md:638 tells authors to validate locally with llar make, but CI gates with llar test --os/--arch (test.yml:116). Contributors following the guide run a different command than the PR gate. Recommend reconciling the guide with the CI command.
  • The README does not mention the new PR gate. A brief "Contributing / CI" note (detects changed owner/repo modules, runs llar test across linux/darwin × amd64/arm64) would help contributors.

Comment thread .github/workflows/test.yml
Comment thread .github/workflows/test.yml
Comment thread .github/workflows/test.yml
Comment thread .github/workflows/test.yml
Comment thread .github/workflows/test.yml
@luoliwoshang
luoliwoshang merged commit 331eeee into xgo-dev:main Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants