feat: add shared composite-action layer (setup-project + lint/test mix-ins)#5
Conversation
|
Claude finished @RobEasthope's task in 2m 32s —— View job Code Review — PR #5
Verified fixes (all three prior blocking issues resolved correctly)
Remaining observations (non-blocking)
OverallThe three major findings from the prior review round are all correctly fixed. The security posture (no eval, array-based file collection, pnpm script names not shell commands) is sound. Documentation is in sync with implementation. No blocking issues remain. |
|
Warning Review limit reached
More reviews will be available in 9 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds a Layer 1 shared-CI action catalogue, new composite actions for setup, linting, typechecking, and tests, plus repository guidance covering pinning, inputs, and the Layer 1/Layer 2 split. ChangesLayer 1 composite actions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/lint-markdown/action.yml:
- Around line 11-17: The `globs` input in the `lint-markdown` action is being
treated as shell code instead of plain data. Update the action’s Markdown lint
step to pass `inputs.globs` directly to `markdownlint-cli2` without `eval`, and
keep the input as a plain structured list/value in `action.yml`. If any
repo-specific glob defaults are needed, move them to each consumer’s
`repo-config.yaml` rather than hardcoding or re-parsing them inside the
composite action.
In @.github/actions/setup-project/action.yml:
- Around line 45-50: The setup in the reusable action currently uses
actions/setup-node with cache: pnpm, which performs both restore and save rather
than restore-only. Update the setup-project action to use an explicit
actions/cache/restore step for the pnpm store cache and keep actions/setup-node
focused on Node version setup only. Use the existing setup-node step and the
pnpm cache configuration in this action to locate the change, and remove the
built-in cache flow so the job does not save cache state here.
In @.github/actions/shellcheck/action.yml:
- Around line 27-42: The ShellCheck target collection in the action run script
is still using text-based `ls` and `echo | xargs`, which can mis-handle
directory inputs and filenames with spaces. Update the `run` block to build the
target list as an array in the shellcheck action, using the existing `SC_PATHS`
handling and `git ls-files` output directly without flattening it back into
whitespace-delimited text. Keep the logic in the same shell script section and
ensure the final `shellcheck` invocation consumes the array safely with the
existing `SEVERITY` setting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1f88061-5b9d-4997-b343-9ffde06d1ef4
📒 Files selected for processing (11)
.github/actions/README.md.github/actions/changelog-validate/action.yml.github/actions/eslint/action.yml.github/actions/lint-markdown/action.yml.github/actions/lint-yaml/action.yml.github/actions/setup-project/action.yml.github/actions/shellcheck/action.yml.github/actions/test-bats/action.yml.github/actions/test-vitest/action.yml.github/actions/typecheck/action.ymlCLAUDE.md
| - name: 🟢 Setup Node.js | ||
| uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 | ||
| with: | ||
| node-version-file: ${{ inputs.node-version-file }} | ||
| # Restore-only pnpm store cache, keyed on the lockfile hash. | ||
| cache: pnpm |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
Does the official actions/setup-nodedocumentation say thatcache: pnpm is restore-only, or does it also save the cache in a post-job step? Please cite the relevant docs.
💡 Result:
The official documentation for actions/setup-node does not state that the built-in cache: pnpm functionality is restore-only [1][2]. Instead, when cache: 'pnpm' is configured, the action automatically handles both restoring the cache at the start of the job and saving the cache in a post-job step [1][2]. The actions/setup-node documentation notes that it uses actions/cache under the hood to provide this built-in functionality [1][2]. Because it manages both the restore and save operations automatically, it does not support "restore-only" behavior by default when using the cache input [3]. To achieve a restore-only workflow, the documentation directs users to use actions/cache/restore directly instead of the built-in cache input of actions/setup-node [4][5].
Citations:
- 1: https://github.com/actions/setup-node/blob/v6/README.md
- 2: https://github.com/actions/setup-node/blob/main/README.md
- 3: https://daksh.be/blog/2026/05/03/why-my-github-actions-cache-was-always-cold/
- 4: https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md
- 5: Add example for restore-only cache in documentation actions/setup-node#1419
actions/setup-node cache is not restore-only. At .github/actions/setup-project/action.yml:45-50, cache: pnpm uses the action’s built-in cache flow, which restores at the start of the job and saves in a post-job step. If this action is meant to be restore-only, switch to an explicit actions/cache/restore step instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/setup-project/action.yml around lines 45 - 50, The setup in
the reusable action currently uses actions/setup-node with cache: pnpm, which
performs both restore and save rather than restore-only. Update the
setup-project action to use an explicit actions/cache/restore step for the pnpm
store cache and keep actions/setup-node focused on Node version setup only. Use
the existing setup-node step and the pnpm cache configuration in this action to
locate the change, and remove the built-in cache flow so the job does not save
cache state here.
…x-ins) Author Layer 1 of ADR 0001 (§5.7): the granular composite actions that the coarse reusable workflows (SK-415/416) will compose. Splitting at the action layer is free (steps in the calling job), so pick-and-mix lives here whilst the reusable workflows stay coarse and pay setup once. Actions under .github/actions/: - setup-project: pnpm + Node-from-.nvmrc + restore-only store cache, then install - eslint, lint-markdown, lint-yaml: lint mix-ins - typecheck, test-vitest, test-bats: build/test mix-ins - shellcheck, changelog-validate: infra/changelog mix-ins lint-yaml implements the SK-438 config injection: yamllint runs against this repo's own .yamllint.yml (resolved via github.action_path), so consumers carry no local copy. Tool pins (pnpm, Node, yamllint 1.37.1, actionlint 1.7.5) mirror ci.yml so the dogfooded self-CI and the shipped actions never drift. Author-only: this repo does not dogfood the actions via uses: (same sha_pinning_required reason as the inline workflows). Adds the action catalogue README and documents the layer in CLAUDE.md. Refs SK-440.
3e9861b to
54752a9
Compare
✅ Full
|
Add --break-system-packages to ci.yml's inline yamllint step so it keeps working once ubuntu-latest moves to a distro Python that marks itself externally-managed (Ubuntu 24.04). No-op on the current runner. Mirrors the lint-yaml composite action added in this PR so the inline copy and the action never drift. Refs SK-440.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| CHANGELOG_COMMAND: ${{ inputs.command }} | ||
| run: | | ||
| set -euo pipefail | ||
| eval "$CHANGELOG_COMMAND" |
There was a problem hiding this comment.
Security: eval on a caller-controlled input.
eval "$CHANGELOG_COMMAND" gives any consumer of this action arbitrary shell execution in the calling job — shell metacharacters, &&/; chains, variable expansion, subshells. This goes beyond the "trust boundary" that SK-422 is meant to enforce.
For a fixed command with optional extra flags, separate the concerns:
| eval "$CHANGELOG_COMMAND" | |
| bash -c "$CHANGELOG_COMMAND" |
Actually even bash -c has the same injection surface. The cleanest fix is to restrict what this input can do. If the command is always pnpm run <script>, model it that way:
inputs:
script:
description: pnpm script name to run (e.g. `changelog:validate`).
default: changelog:validatepnpm run "$CHANGELOG_SCRIPT"If you genuinely need an arbitrary shell command (e.g. npx tool --flag), document that callers are responsible for sanitising the input and add a prominent warning in the action description. The current description gives no indication that the input is shell-executed. Fix this →
| set -euo pipefail | ||
| # Word-splitting on $MD_GLOBS is intentional — it carries multiple globs. | ||
| # shellcheck disable=SC2086 | ||
| eval "pnpm exec markdownlint-cli2 $MD_GLOBS" |
There was a problem hiding this comment.
Security: eval on caller-controlled glob input.
The default value '"**/*.md" "!**/node_modules/**"' smuggles shell quoting into the input so that eval splits it correctly — a strong sign this is the wrong abstraction. Any consumer overriding globs can inject arbitrary shell commands.
mapfile cleanly replaces the eval and keeps the input as plain data:
| eval "pnpm exec markdownlint-cli2 $MD_GLOBS" | |
| mapfile -t md_globs <<<"$MD_GLOBS" | |
| pnpm exec markdownlint-cli2 "${md_globs[@]}" |
Change the default to newline-separated plain globs (no shell quoting needed):
default: |
**/*.md
!**/node_modules/**mapfile -t splits on newlines, so each glob becomes a separate array element. No eval, no injection surface, and callers supply clean patterns. Fix this →
| with: | ||
| node-version-file: ${{ inputs.node-version-file }} | ||
| # Restore-only pnpm store cache, keyed on the lockfile hash. | ||
| cache: pnpm |
There was a problem hiding this comment.
cache: pnpm is not restore-only — it also saves.
The action description, the inline comment ("Restore-only pnpm store cache"), the README catalogue entry, and the CLAUDE.md section all describe this as restore-only. But cache: pnpm on actions/setup-node uses actions/cache under the hood, which registers a post-job step that saves the cache after the job completes. It is not restore-only.
Consequences:
- The "dedicated save job" contention dance is still avoided at single-package scale (fine), but the stated architecture is wrong.
- If the estate later adds a dedicated cache-save job or a consumer tries to control save separately, they'll hit unexpected double-save behaviour.
Option A — accept save-too and update every doc string that says "restore-only":
Update description, comment on line 49, README, and CLAUDE.md.
Option B — make it truly restore-only with actions/cache/restore:
- name: 🟢 Setup Node.js
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: ${{ inputs.node-version-file }}
# No cache: here — handled explicitly below.
- name: ♻️ Restore pnpm store cache
uses: actions/cache/restore@<sha> # vX.Y.Z
with:
path: ~/.local/share/pnpm/store
key: pnpm-store-${{ hashFiles('**/pnpm-lock.yaml') }}Option A is simpler and likely sufficient at single-package scale; Option B matches the documented intent. Either way, the docs and the implementation need to agree. Fix this →
| run: | | ||
| set -euo pipefail | ||
| shellcheck --version | ||
| if [ -n "$SC_PATHS" ]; then | ||
| # Word-splitting on $SC_PATHS is intentional (multiple paths). | ||
| # shellcheck disable=SC2086 | ||
| files=$(ls -1 $SC_PATHS) | ||
| else | ||
| # Discover tracked shell scripts; -z/xargs -0 is robust to odd names. | ||
| files=$(git ls-files -z '*.sh' '*.bash' | tr '\0' '\n') | ||
| fi | ||
| if [ -z "$files" ]; then | ||
| echo "No shell scripts found to check." | ||
| exit 0 | ||
| fi | ||
| echo "$files" | xargs shellcheck --severity="$SEVERITY" |
There was a problem hiding this comment.
Unsafe file list building — breaks on filenames with spaces.
Two problems:
ls -1 $SC_PATHS(line 33):lson a directory input expands to the directory's children, not the directory path itself, and produces unusable output. It also splits on whitespace, soinfrastructure scriptswould be two arguments.echo "$files" | xargs shellcheck(line 42):xargssplits on whitespace, which re-breaks any paths that survived to this point.
Build an array instead, matching the pattern used in the git ls-files -z path:
| run: | | |
| set -euo pipefail | |
| shellcheck --version | |
| if [ -n "$SC_PATHS" ]; then | |
| # Word-splitting on $SC_PATHS is intentional (multiple paths). | |
| # shellcheck disable=SC2086 | |
| files=$(ls -1 $SC_PATHS) | |
| else | |
| # Discover tracked shell scripts; -z/xargs -0 is robust to odd names. | |
| files=$(git ls-files -z '*.sh' '*.bash' | tr '\0' '\n') | |
| fi | |
| if [ -z "$files" ]; then | |
| echo "No shell scripts found to check." | |
| exit 0 | |
| fi | |
| echo "$files" | xargs shellcheck --severity="$SEVERITY" | |
| set -euo pipefail | |
| shellcheck --version | |
| files=() | |
| if [ -n "$SC_PATHS" ]; then | |
| # Word-splitting on $SC_PATHS is intentional (space-separated paths). | |
| # shellcheck disable=SC2206 | |
| paths=($SC_PATHS) | |
| for p in "${paths[@]}"; do | |
| while IFS= read -r -d '' f; do | |
| files+=("$f") | |
| done < <(find "$p" -type f \( -name '*.sh' -o -name '*.bash' \) -print0 2>/dev/null || true) | |
| done | |
| else | |
| while IFS= read -r -d '' f; do | |
| files+=("$f") | |
| done < <(git ls-files -z '*.sh' '*.bash') | |
| fi | |
| if [ "${#files[@]}" -eq 0 ]; then | |
| echo "No shell scripts found to check." | |
| exit 0 | |
| fi | |
| shellcheck --severity="$SEVERITY" "${files[@]}" |
| set -euo pipefail | ||
| base="https://raw.githubusercontent.com/rhysd/actionlint" | ||
| url="${base}/${ACTIONLINT_INSTALLER_SHA}/scripts/download-actionlint.bash" | ||
| bash <(curl -fsSL "$url") "$ACTIONLINT_VERSION" |
There was a problem hiding this comment.
Remote script execution with a mutable version input.
bash <(curl -fsSL "$url") "$ACTIONLINT_VERSION"The installer URL is SHA-pinned (good — ACTIONLINT_INSTALLER_SHA is a commit hash, not a tag), and the installer itself checksum-verifies the binary it downloads. However, ACTIONLINT_VERSION comes from an action input and is passed to the installer as the version to fetch. An attacker who can influence that input could request an older, vulnerable actionlint binary.
The risk is low given this is an internal estate action, but worth documenting: the comment above could note that actionlint-version should only be changed via a Dependabot bump, not overridden ad-hoc by consumers. A default + note in the input description ("bump via Dependabot only") would suffice.
Triage of CodeRabbit + Claude review on PR #5: - lint-markdown / changelog-validate: drop `eval` on caller-supplied inputs (shell-injection surface in the SK-422 trust boundary). markdownlint globs are now newline-separated plain data read into an array; the changelog validator is a `script` input run as `pnpm run <script>` (single argument, never reparsed as shell). - shellcheck: build the target list as an array. The default discovery uses NUL-delimited `git ls-files -z` (robust to spaces); `ls`/`echo | xargs` removed. - setup-project: correct the "restore-only" wording in the action, README and CLAUDE.md — `cache: pnpm` restores at job start and saves in a post-job step (the ADR's intent is avoiding the multi-package dedicated *save job*, which is what stays omitted). Behaviour unchanged. - lint-yaml: note that the yamllint/actionlint version inputs are pinned to ci.yml and should be bumped in lockstep, not overridden per consumer (SK-422). Re-verified with actionlint + yamllint + shellcheck and a full `act` run (setup-project + lint-yaml + lint-markdown green). Refs SK-440.
Triage of review feedback — all addressed in b45ca81Worked through every CodeRabbit + Claude finding:
Not changed (deliberately): the Re-verified: actionlint + yamllint + shellcheck clean; the rewritten |
`cache: pnpm` restores at job start and saves in a post-job step — it is not restore-only. The architecture's actual intent is avoiding the multi-package *dedicated save job*, which stays omitted at single-package scale. Aligns the ADR with the setup-project action's corrected docs (PR #5). Refs SK-440.
## What Reconstructs `shared-workflows`' **entire release history** and graduates the current code to a stable **`v1.0.0`** (A-585). The repo publishes no npm package, so a "release" here is a **git tag (`vX.Y.Z`) + GitHub release** over the reusable workflows and composite actions. ## Versioning methodology The merged PR **titles are not reliable** — Conventional-Commit discipline was loose when the repo was stood up (the GO/NO GO gate shipped as `ci:`, the versioned ruleset as `chore:`). So every PR was re-classified by **what it actually changed to the product surface** (the `reusable-*.yml` workflows + `.github/actions/*` consumers use, and their reference docs), read from the diff. Pre-1.0 rules: `feature` → minor, `fix` → patch, breaking → minor (stay in `0.x`), and `1.0.0` is a deliberate "graduate to stable" milestone. | Version | PR | True category | Headline | |---|---|---|---| | 0.1.0 | #1 | feature | bootstrap: 3 reusable workflows | | 0.2.0 | #5 | feature | Layer-1 composite-action layer | | 0.3.0 | #6 | feature | reusable-lint bundle | | 0.4.0 | #7 | feature | reusable build-test bundle + `build` action | | 0.4.1 | #8 | fix | repoint build-test pins so the build lane resolves | | 0.5.0 | #9 | feature | reusable pkg-release workflow | | 0.6.0 | #15 | refactor **(breaking)** | rename release → pkg-release | | 0.7.0 | #16 | **feature** *(titled `ci:`)* | GO/NO GO aggregator gate + docs | | 0.8.0 | #17 | **feature** *(titled `chore:`)* | versioned canonical estate ruleset | | 1.0.0 | HEAD | milestone | graduate to stable public surface | Non-release PRs (self-host CI, ADR, dep bump, SK→A rename, skill adoptions/re-syncs, check-run rename) are folded into the body of the release window they merged in — the way a release-please changelog aggregates. ## Changes - **`changelog/`** — hand-authored dated entries (one per release) + the schema doc (`changelog/README.md`), adapted from the estate's `eslint-config`/`agent-skills` convention. - **`package.json`** — `0.0.0` → `1.0.0` (stays `private`; the version is the release anchor, not an npm publish). - **`reusable-lint.yml`** — pin the five sibling Layer-1 actions to the **`v0.8.0`** tag (`@7f543be`) instead of the bare pre-release commit, so Dependabot can track them (`v0.8.0` is the latest backfilled tag that carries every action and predates this commit, avoiding a self-reference to `v1.0.0`). - **`CLAUDE.md` / `README`** — replace the "no release tag exists yet" wording. ## After merge The git tags + GitHub releases are cut once this lands: `v0.1.0`–`v0.8.0` at their historical merge commits, `v1.0.0` at this PR's squash-merge commit, with notes sourced from each `changelog/` entry. This unblocks consumers pinning a tag (+ Dependabot) instead of a moving SHA (A-420 / A-446). ## Deferred (follow-up) - **release-please automation** + a tag-only release workflow — the forward release process is a separate decision now the backfill exists. ## Note for review > Backfilling git tags at historical commits is a deliberate divergence from the estate precedent (`eslint-config`/`agent-skills` backfilled changelog *entries* but tagged only going forward) — chosen here because A-585 explicitly wants the *full tag history* reconstructed. The pre-existing Prettier drift on the vendored `.claude`/`.agents` skill bundles is untouched (out of scope). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Marked the reusable workflows and composite actions as stable at **v1.0.0**. * Updated workflow references to use the newer version tags. * **Documentation** * Clarified release and versioning guidance across the repository. * Added and expanded changelog documentation and release notes structure. * **Chores** * Updated internal workflow pins to the latest approved release. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
What
Authors Layer 1 of ADR 0001 (§5.1–§5.3, §5.7): the granular composite actions under
.github/actions/that the coarse reusable workflows (SK-415lint.yml, SK-416build-test.yml) will compose. Splitting at the action layer is free (steps in the calling job — no extra runner/install), so pick-and-mix lives here whilst the reusable workflows stay coarse and pay setup once.Closes the full SK-440 checklist:
setup-project.nvmrc+ restore-only pnpm store cache, then install (the keystone)eslint/lint-markdown/lint-yamltypecheck/test-vitest/test-batsshellcheck/changelog-validateNotable design points
lint-yamlruns yamllint with-cpointed at this repo's own.yamllint.yml, resolved relative togithub.action_path($GITHUB_ACTION_PATH/../../../.yamllint.yml). Consumers carry no local copy; the root file stays the single source of truth.ci.ymlexactly.uses:(samesha_pinning_requiredreason as the inline workflows). They're consumed cross-repo by@<sha>; documented in.github/actions/README.mdandCLAUDE.md.uses:/installer SHA-pinned,set -euo pipefail, composite actions inherit the job's permissions.Verification
prettier --check,yamllint,markdownlint-cli2,actionlintall green.lint-yaml's injected-config path resolution + run, thelint-markdownglob, andshellcheck's graceful no-scripts skip locally.actintegration smoke-test (thesetup-projectinstall path) was not run — Docker wasn't available in the authoring environment.eslint/typecheck/test-vitest/test-bats/changelog-validatehave no local fixtures in this repo (it ships workflows, not a package), so they're authored to house style; real validation lands when SK-415/416 wire them into consumers.changelog-validate's default command (pnpm run changelog:validate) should be confirmed against the estate's actual dated-changelog tool.Refs SK-440.
Summary by CodeRabbit