From 93bf6dd72ed38e8f6be480609c65c2bd5958ae8c Mon Sep 17 00:00:00 2001 From: Alex Karpov Date: Fri, 7 Aug 2026 11:48:26 +0300 Subject: [PATCH 1/6] Mark the job name as API and fix the usage ref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reusable call reports as " / ", so consumers put that whole string in branch protection. Renaming the job here stops their required check reporting, with nothing in their repo to explain it. The usage example still showed @v1. Consumers track master — the tag was retired rather than kept moving, since a v1 re-pointed at every merge is master with a force-push in front of it. --- .github/workflows/check-ticket.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/check-ticket.yml b/.github/workflows/check-ticket.yml index 4077e65..279700d 100644 --- a/.github/workflows/check-ticket.yml +++ b/.github/workflows/check-ticket.yml @@ -14,7 +14,7 @@ name: Check Ticket (shared) # # jobs: # check-ticket: -# uses: iXsystems/ux-github-workflows/.github/workflows/check-ticket.yml@v1 +# uses: iXsystems/ux-github-workflows/.github/workflows/check-ticket.yml@master # with: # ticket-prefixes: TNC # optional; defaults to NAS @@ -37,6 +37,9 @@ concurrency: jobs: check-ticket: + # API. A reusable call reports as " / ", so consumers + # match this string in branch protection. Renaming it stops their required check + # reporting, silently, with no PR in their repo to explain it. name: Check PR references a ticket runs-on: ubuntu-latest steps: From 6089904f40f24d0f0c52b61293da488c2c708c2e Mon Sep 17 00:00:00 2001 From: Alex Karpov Date: Thu, 6 Aug 2026 16:28:29 +0300 Subject: [PATCH 2/6] NAS-142094: Add shared claude-review reusable workflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collapses the near-identical claude.yml carried by all three consumers. They had drifted: claude-code-action v1.0.7 / v1.0.134 / v1.0.182, models claude-opus-4-8 / claude-opus-5 / action default, and a write-access gate present in two of the three. Notes on two design choices: - The action version is hardcoded rather than an input. `uses:` does not evaluate expressions, and a configurable version is how the three repos drifted in the first place. Bump here, re-tag, all callers move. - The API key is an explicit named secret rather than `secrets: inherit`, because the repos name it differently (CLAUDE_API_KEY vs CLAUDE_TOKEN). The member gate is inlined as a job instead of a second reusable workflow: relative `uses:` paths inside a reusable workflow resolve against the caller's repository, which is a footgun with no upside here. webui's check-ux-team.yml and ui-components' check-member.yml turned out to be byte-identical apart from `name:` — neither checked a team, both checked repo write access — so the gate needs no parameterisation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MMo51bM6QV14CK3Kgyp3DQ --- .github/workflows/claude-review.yml | 146 ++++++++++++++++++++++++++++ README.md | 43 +++++++- 2 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 .github/workflows/claude-review.yml diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml new file mode 100644 index 0000000..a6eda90 --- /dev/null +++ b/.github/workflows/claude-review.yml @@ -0,0 +1,146 @@ +name: Claude Review (shared) + +# Shared automatic-PR-review workflow for the TrueNAS Angular repos +# (truenas/webui, iXsystems/truenas-ui-components, truenas-connect/ui). +# +# Callers own their `on:` trigger — branch filters and paths-ignore differ per +# repo and cannot be passed as inputs, since `workflow_call` has no say in what +# triggers the caller. Everything else lives here. +# +# Usage: +# jobs: +# claude-review: +# uses: iXsystems/ux-github-workflows/.github/workflows/claude-review.yml@v1 +# permissions: +# contents: read +# issues: write +# pull-requests: write +# id-token: write +# secrets: +# anthropic-api-key: ${{ secrets.CLAUDE_API_KEY }} + +on: + workflow_call: + inputs: + model: + description: 'Model passed via claude_args.' + type: string + default: 'claude-opus-5' + prompt-file: + description: 'Repo-relative path to the review guidelines appended to the prompt.' + type: string + default: '.claude/review-prompt.md' + require-write-access: + description: 'Gate the review on the PR author having write/admin access. Keep true on public repos — it is what stops drive-by PRs from spending tokens.' + type: boolean + default: true + skip-label: + description: 'PR label that suppresses the review.' + type: string + default: 'skip-claude' + timeout-minutes: + description: 'Hard cap on the review job.' + type: number + default: 20 + fetch-depth: + description: 'Checkout depth. Needs to cover the PR range for the diff.' + type: number + default: 10 + secrets: + anthropic-api-key: + description: 'Anthropic API key. Mapped by the caller, since the secret name differs per repo.' + required: true + +jobs: + # Gate: does the PR author have write access to the calling repo? Inlined + # rather than split into its own reusable workflow so there is one file to + # reason about — relative `uses:` paths inside a reusable workflow resolve + # against the caller's repo, which is a footgun we do not need here. + check-member: + name: Check member access + runs-on: ubuntu-latest + if: inputs.require-write-access + permissions: + contents: read + outputs: + is_member: ${{ steps.check.outputs.result }} + steps: + - name: Check membership + id: check + uses: actions/github-script@v7 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + result-encoding: string + script: | + try { + const username = context.payload.pull_request.user.login; + console.log(`Checking repository access for user: ${username}`); + + const { data: permissionLevel } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: username + }); + + console.log(`User ${username} has permission: ${permissionLevel.permission}`); + + const hasWriteAccess = ['write', 'admin'].includes(permissionLevel.permission); + console.log(`Has write access: ${hasWriteAccess}`); + + return hasWriteAccess ? 'true' : 'false'; + } catch (error) { + console.log(`Error checking permissions: ${error.message}`); + + // Fall back to the PR author association when the permission + // lookup fails (e.g. the token can't read org membership). + const association = context.payload.pull_request.author_association; + console.log(`PR author association: ${association}`); + + const isTeamMember = ['MEMBER', 'OWNER', 'COLLABORATOR'].includes(association); + console.log(`Is team member based on association: ${isTeamMember}`); + + return isTeamMember ? 'true' : 'false'; + } + + review: + name: Automatic PR review + runs-on: ubuntu-latest + timeout-minutes: ${{ inputs.timeout-minutes }} + needs: [check-member] + # `always()` is required because check-member is skipped when the gate is + # off; without it this job would inherit the skip. The membership condition + # then only has to hold when the gate actually ran. + if: | + always() && + (inputs.require-write-access == false || needs.check-member.outputs.is_member == 'true') && + !contains(github.event.pull_request.labels.*.name, inputs.skip-label) + permissions: + contents: read + issues: write + pull-requests: write + id-token: write + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: ${{ inputs.fetch-depth }} + + # The action version is deliberately NOT an input: `uses:` does not + # evaluate expressions, and making it configurable would recreate the + # drift this workflow exists to remove (the three repos were on v1.0.7, + # v1.0.134 and v1.0.182). Bump it here and re-tag to upgrade all callers. + - name: Automatic PR Review + uses: anthropics/claude-code-action@v1.0.182 + with: + anthropic_api_key: ${{ secrets.anthropic-api-key }} + claude_args: "--model ${{ inputs.model }}" + track_progress: true + use_sticky_comment: true + prompt: | + REPO: ${{ github.repository }} + PR NUMBER: ${{ github.event.pull_request.number }} + + Please review this pull request using the guidelines below. + It should be already checked out in the current directory. + + {{file:${{ inputs.prompt-file }}}} diff --git a/README.md b/README.md index 58fd580..a32e0c3 100644 --- a/README.md +++ b/README.md @@ -56,13 +56,46 @@ today; `iXsystems/truenas-ui-components` deliberately treats the ticket prefix as optional (see its `pr-title.yml`), and `truenas-connect/ui` has no PR-title check at all. Adopt it only where the team has agreed to require tickets. +### `claude-review.yml` + +Automatic Claude PR review, gated on the PR author having write access. + +```yaml +jobs: + claude-review: + uses: iXsystems/ux-github-workflows/.github/workflows/claude-review.yml@v1 + permissions: + contents: read + issues: write + pull-requests: write + id-token: write + secrets: + anthropic-api-key: ${{ secrets.CLAUDE_API_KEY }} +``` + +| Input | Default | Notes | +|---|---|---| +| `model` | `claude-opus-5` | | +| `prompt-file` | `.claude/review-prompt.md` | Repo-relative; the file stays in the consumer repo | +| `require-write-access` | `true` | Keep on for public repos | +| `skip-label` | `skip-claude` | | +| `timeout-minutes` | `20` | | +| `fetch-depth` | `10` | | + +The API key is an explicit named secret rather than `secrets: inherit`, because +consumers name it differently (`CLAUDE_API_KEY` vs `CLAUDE_TOKEN`). + +The `anthropics/claude-code-action` version is **hardcoded**, not an input: +`uses:` does not evaluate expressions, and making it configurable is what let +the consumers drift to v1.0.7 / v1.0.134 / v1.0.182 in the first place. + ## Adoption status -| Repo | `check-ticket.yml` | -|---|---| -| `truenas/webui` | migrating (first adopter) | -| `iXsystems/truenas-ui-components` | n/a — tickets optional there | -| `truenas-connect/ui` | n/a — no PR-title check | +| Repo | `check-ticket.yml` | `claude-review.yml` | +|---|---|---| +| `truenas/webui` | migrating (first adopter) | not yet | +| `iXsystems/truenas-ui-components` | n/a — tickets optional there | migrating | +| `truenas-connect/ui` | n/a — no PR-title check | not yet | ## Releasing From 172593958567e08eb8315a8e386c5242836571ee Mon Sep 17 00:00:00 2001 From: Alex Karpov Date: Fri, 7 Aug 2026 16:46:53 +0300 Subject: [PATCH 3/6] NAS-142094: Add additional-permissions input; reference shared workflows at @master --- .github/workflows/claude-review.yml | 15 +++++++-- README.md | 51 ++++++++++++++++++----------- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index a6eda90..2b14d2c 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -10,7 +10,7 @@ name: Claude Review (shared) # Usage: # jobs: # claude-review: -# uses: iXsystems/ux-github-workflows/.github/workflows/claude-review.yml@v1 +# uses: iXsystems/ux-github-workflows/.github/workflows/claude-review.yml@master # permissions: # contents: read # issues: write @@ -46,6 +46,14 @@ on: description: 'Checkout depth. Needs to cover the PR range for the diff.' type: number default: 10 + additional-permissions: + description: >- + Extra capabilities granted to the review, as understood by + claude-code-action, e.g. "gh pr list, gh pr view, gh api --method GET". + Empty by default: this widens what the reviewer can do, so a repo opts + in rather than inheriting it from the other consumers. + type: string + default: '' secrets: anthropic-api-key: description: 'Anthropic API key. Mapped by the caller, since the secret name differs per repo.' @@ -127,13 +135,14 @@ jobs: # The action version is deliberately NOT an input: `uses:` does not # evaluate expressions, and making it configurable would recreate the - # drift this workflow exists to remove (the three repos were on v1.0.7, - # v1.0.134 and v1.0.182). Bump it here and re-tag to upgrade all callers. + # drift this workflow exists to remove (the three repos were on v1.0.182, + # v1.0.154 and v1.0.134). Bump it here to upgrade every caller at once. - name: Automatic PR Review uses: anthropics/claude-code-action@v1.0.182 with: anthropic_api_key: ${{ secrets.anthropic-api-key }} claude_args: "--model ${{ inputs.model }}" + additional_permissions: ${{ inputs.additional-permissions }} track_progress: true use_sticky_comment: true prompt: | diff --git a/README.md b/README.md index a32e0c3..7d1b269 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ on: jobs: check-ticket: - uses: iXsystems/ux-github-workflows/.github/workflows/check-ticket.yml@v1 + uses: iXsystems/ux-github-workflows/.github/workflows/check-ticket.yml@master with: ticket-prefixes: TNC # optional; defaults to NAS ``` @@ -51,10 +51,11 @@ uppercase key, so `nas-12345` fails with a message saying so. Callers own their `on:` trigger — a reusable workflow has no say in what triggers its caller. -This one is **policy, not just plumbing.** Only `truenas/webui` requires tickets -today; `iXsystems/truenas-ui-components` deliberately treats the ticket prefix -as optional (see its `pr-title.yml`), and `truenas-connect/ui` has no PR-title -check at all. Adopt it only where the team has agreed to require tickets. +This one is **policy, not just plumbing** — it makes a ticket mandatory. All +three consumers have since agreed to that, but a fourth repo should adopt it +only once its team has. Note that `iXsystems/truenas-ui-components` requires a +ticket *and* a Conventional Commits title; the latter stays in its own local +`pr-title.yml`, since it is the only repo running semantic-release. ### `claude-review.yml` @@ -63,7 +64,7 @@ Automatic Claude PR review, gated on the PR author having write access. ```yaml jobs: claude-review: - uses: iXsystems/ux-github-workflows/.github/workflows/claude-review.yml@v1 + uses: iXsystems/ux-github-workflows/.github/workflows/claude-review.yml@master permissions: contents: read issues: write @@ -81,33 +82,45 @@ jobs: | `skip-label` | `skip-claude` | | | `timeout-minutes` | `20` | | | `fetch-depth` | `10` | | +| `additional-permissions` | `''` | Extra reviewer capabilities, e.g. `gh pr list, gh pr view, gh api --method GET`. Opt-in per repo | The API key is an explicit named secret rather than `secrets: inherit`, because consumers name it differently (`CLAUDE_API_KEY` vs `CLAUDE_TOKEN`). The `anthropics/claude-code-action` version is **hardcoded**, not an input: `uses:` does not evaluate expressions, and making it configurable is what let -the consumers drift to v1.0.7 / v1.0.134 / v1.0.182 in the first place. +the consumers drift to v1.0.182 / v1.0.154 / v1.0.134 in the first place. + +The member gate is inlined here rather than kept as its own reusable workflow. +A relative `uses:` inside a reusable workflow resolves against the *caller's* +repo, not this one, so splitting it would mean every consumer either hosting a +copy of the gate or this file hard-coding its own `iXsystems/…@ref` — one file +is simpler than either. ## Adoption status | Repo | `check-ticket.yml` | `claude-review.yml` | |---|---|---| -| `truenas/webui` | migrating (first adopter) | not yet | -| `iXsystems/truenas-ui-components` | n/a — tickets optional there | migrating | -| `truenas-connect/ui` | n/a — no PR-title check | not yet | +| `truenas/webui` | adopted | migrating | +| `iXsystems/truenas-ui-components` | adopted | migrating | +| `truenas-connect/ui` | migrating | migrating | ## Releasing -Callers pin `@v1`, so a change reaches them only when the tag moves: +Callers reference `@master`, so **anything landing on `master` is live in every +consumer immediately** — there is no per-repo review gate between a change here +and three repos' CI running it. -```bash -git tag -f v1 && git push -f origin v1 -``` +That puts the whole burden on the PR into this repo: -Land the change on `master`, verify it against the first adopter's next PR, then -move the tag. For a breaking input change, cut `v2` and migrate callers one at a -time instead. +- Treat a change to a job `name:` as breaking. Consumers match + `" / "` in branch protection, so a rename silently + stops a required check reporting, with no PR in their repo to explain it. +- Same for removing or renaming an input, or tightening a default. +- Verify against one consumer's next real PR before assuming it is fine + everywhere; the consumers differ in trigger, secret names and permissions. -Tags are the release surface here, not branches — a caller pinned to `@master` -would pick up unreviewed changes on every push to every consumer at once. +If that becomes too sharp an edge, the alternative is tagging: cut `v1`, move +callers to `@v1`, and release with `git tag -f v1 && git push -f origin v1`. +That was the original intent, but with only three consumers and one team it was +judged more ceremony than it buys. From 6d0e0f016f6a2556b88fde62fe23d1c7fbb50c7d Mon Sep 17 00:00:00 2001 From: Alex Karpov Date: Fri, 7 Aug 2026 16:50:29 +0300 Subject: [PATCH 4/6] NAS-142094: Split the member gate into a shared check-member workflow --- .github/workflows/check-member.yml | 85 +++++++++++++++++++++++++++++ .github/workflows/claude-review.yml | 55 ++++--------------- README.md | 63 +++++++++++++++++---- 3 files changed, 148 insertions(+), 55 deletions(-) create mode 100644 .github/workflows/check-member.yml diff --git a/.github/workflows/check-member.yml b/.github/workflows/check-member.yml new file mode 100644 index 0000000..5121bb0 --- /dev/null +++ b/.github/workflows/check-member.yml @@ -0,0 +1,85 @@ +name: Check Member Access (shared) + +# Reports whether the PR author has write access to the calling repo, as an +# `is_member` output. Consumers use it two ways: +# +# - to gate spend (claude-review.yml calls this before reviewing), and +# - to route work (main.yml sends team PRs to the self-hosted test runner and +# everyone else to ubuntu-latest). +# +# Usage: +# jobs: +# check-member: +# if: github.event_name == 'pull_request' +# permissions: +# contents: read +# uses: iXsystems/ux-github-workflows/.github/workflows/check-member.yml@master +# +# something: +# needs: [check-member] +# if: needs.check-member.outputs.is_member == 'true' +# +# Only meaningful on `pull_request` events — it reads +# `context.payload.pull_request`. Callers that also run on push must guard the +# job with `if: github.event_name == 'pull_request'`, and then use `always()` +# plus an explicit `!= 'true'` on the downstream job so the skip does not +# cascade. See truenas/webui's main.yml for the worked example. + +on: + workflow_call: + outputs: + is_member: + description: "'true' if the PR author has write or admin access to the calling repo." + value: ${{ jobs.check.outputs.is_member }} + +permissions: + contents: read + +jobs: + check: + # API. A reusable call reports as " / ", so consumers + # match this string in branch protection. Renaming it stops their required check + # reporting, silently, with no PR in their repo to explain it. + name: Check member access + runs-on: ubuntu-latest + outputs: + is_member: ${{ steps.check.outputs.result }} + steps: + - name: Check membership + id: check + uses: actions/github-script@v7 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + result-encoding: string + script: | + try { + const username = context.payload.pull_request.user.login; + console.log(`Checking repository access for user: ${username}`); + + const { data: permissionLevel } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: username + }); + + console.log(`User ${username} has permission: ${permissionLevel.permission}`); + + const hasWriteAccess = ['write', 'admin'].includes(permissionLevel.permission); + console.log(`Has write access: ${hasWriteAccess}`); + + return hasWriteAccess ? 'true' : 'false'; + } catch (error) { + console.log(`Error checking permissions: ${error.message}`); + + // Fall back to the PR author association when the permission + // lookup fails (e.g. the token cannot read org membership). + // Deliberately permissive: this decides where tests run and + // whether a review happens, not whether anything merges. + const association = context.payload.pull_request.author_association; + console.log(`PR author association: ${association}`); + + const isTeamMember = ['MEMBER', 'OWNER', 'COLLABORATOR'].includes(association); + console.log(`Is team member based on association: ${isTeamMember}`); + + return isTeamMember ? 'true' : 'false'; + } diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 2b14d2c..960e8e7 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -60,55 +60,20 @@ on: required: true jobs: - # Gate: does the PR author have write access to the calling repo? Inlined - # rather than split into its own reusable workflow so there is one file to - # reason about — relative `uses:` paths inside a reusable workflow resolve - # against the caller's repo, which is a footgun we do not need here. + # Gate: does the PR author have write access to the calling repo? + # + # Referenced by its full `iXsystems/...@ref` path, not a relative one: inside a + # reusable workflow a relative `uses:` resolves against the *caller's* repo, so + # `./.github/workflows/check-member.yml` would look for the file in webui. + # + # It is a separate file rather than inlined here because main.yml in webui and + # truenas-connect/ui needs the same answer to pick a test runner — inlining + # would put a second copy of the script in the repo that exists to remove them. check-member: - name: Check member access - runs-on: ubuntu-latest if: inputs.require-write-access permissions: contents: read - outputs: - is_member: ${{ steps.check.outputs.result }} - steps: - - name: Check membership - id: check - uses: actions/github-script@v7 - with: - github-token: ${{ secrets.GITHUB_TOKEN }} - result-encoding: string - script: | - try { - const username = context.payload.pull_request.user.login; - console.log(`Checking repository access for user: ${username}`); - - const { data: permissionLevel } = await github.rest.repos.getCollaboratorPermissionLevel({ - owner: context.repo.owner, - repo: context.repo.repo, - username: username - }); - - console.log(`User ${username} has permission: ${permissionLevel.permission}`); - - const hasWriteAccess = ['write', 'admin'].includes(permissionLevel.permission); - console.log(`Has write access: ${hasWriteAccess}`); - - return hasWriteAccess ? 'true' : 'false'; - } catch (error) { - console.log(`Error checking permissions: ${error.message}`); - - // Fall back to the PR author association when the permission - // lookup fails (e.g. the token can't read org membership). - const association = context.payload.pull_request.author_association; - console.log(`PR author association: ${association}`); - - const isTeamMember = ['MEMBER', 'OWNER', 'COLLABORATOR'].includes(association); - console.log(`Is team member based on association: ${isTeamMember}`); - - return isTeamMember ? 'true' : 'false'; - } + uses: iXsystems/ux-github-workflows/.github/workflows/check-member.yml@master review: name: Automatic PR review diff --git a/README.md b/README.md index 7d1b269..5055836 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,45 @@ only once its team has. Note that `iXsystems/truenas-ui-components` requires a ticket *and* a Conventional Commits title; the latter stays in its own local `pr-title.yml`, since it is the only repo running semantic-release. +### `check-member.yml` + +Reports whether the PR author has write access to the calling repo, as an +`is_member` output. Two distinct uses, which is why it is its own file: + +```yaml +jobs: + check-member: + if: github.event_name == 'pull_request' + permissions: + contents: read + uses: iXsystems/ux-github-workflows/.github/workflows/check-member.yml@master + + test-ux-team: + needs: [check-member] + if: needs.check-member.outputs.is_member == 'true' + runs-on: self-hosted + # ... +``` + +| Output | Notes | +|---|---| +| `is_member` | `'true'` / `'false'` — a string, not a boolean. Compare with `== 'true'` | + +`claude-review.yml` calls it to gate spend; `main.yml` in `truenas/webui` and +`truenas-connect/ui` calls it to route tests to the self-hosted runner. Those +were three separate copies of the same script before this existed — two +workflow files plus one inlined directly in `truenas-connect/ui`'s `main.yaml`. + +Only meaningful on `pull_request` events: it reads +`context.payload.pull_request`. A caller whose workflow also runs on `push` +must guard the job with `if: github.event_name == 'pull_request'`, and then use +`always()` plus an explicit `!= 'true'` on the downstream job so the skip does +not cascade — see `truenas/webui`'s `main.yml`. + +If the permission lookup fails it falls back to `author_association`, which is +deliberately permissive. It decides where tests run and whether a review +happens; it must not be load-bearing for anything that gates a merge. + ### `claude-review.yml` Automatic Claude PR review, gated on the PR author having write access. @@ -91,19 +130,23 @@ The `anthropics/claude-code-action` version is **hardcoded**, not an input: `uses:` does not evaluate expressions, and making it configurable is what let the consumers drift to v1.0.182 / v1.0.154 / v1.0.134 in the first place. -The member gate is inlined here rather than kept as its own reusable workflow. -A relative `uses:` inside a reusable workflow resolves against the *caller's* -repo, not this one, so splitting it would mean every consumer either hosting a -copy of the gate or this file hard-coding its own `iXsystems/…@ref` — one file -is simpler than either. +The member gate is `check-member.yml`, called by full `iXsystems/…@master` path. +It has to be the full path: inside a reusable workflow a relative `uses:` +resolves against the *caller's* repo, so `./.github/workflows/check-member.yml` +would be looked for in webui. This nests two levels deep (caller → +`claude-review` → `check-member`), well inside GitHub's limit of four. ## Adoption status -| Repo | `check-ticket.yml` | `claude-review.yml` | -|---|---|---| -| `truenas/webui` | adopted | migrating | -| `iXsystems/truenas-ui-components` | adopted | migrating | -| `truenas-connect/ui` | migrating | migrating | +| Repo | `check-ticket.yml` | `check-member.yml` | `claude-review.yml` | +|---|---|---|---| +| `truenas/webui` | adopted | migrating (`main.yml`) | migrating | +| `iXsystems/truenas-ui-components` | adopted | n/a — no self-hosted runner | migrating | +| `truenas-connect/ui` | adopted | migrating (`main.yaml`) | migrating | + +`claude-review.yml` pulls in `check-member.yml` on its own, so a repo using only +the review does not call it directly — the `check-member.yml` column tracks +`main.yml`-style direct callers. ## Releasing From acfa165c5f22357ec250777d4bc2451ecc9283fc Mon Sep 17 00:00:00 2001 From: Alex Karpov Date: Fri, 7 Aug 2026 18:10:44 +0300 Subject: [PATCH 5/6] NAS-142094: Add shared prepare composite action --- .github/actions/prepare/action.yml | 84 ++++++++++++++++++++++++++++++ README.md | 48 +++++++++++++++-- 2 files changed, 127 insertions(+), 5 deletions(-) create mode 100644 .github/actions/prepare/action.yml diff --git a/.github/actions/prepare/action.yml b/.github/actions/prepare/action.yml new file mode 100644 index 0000000..8334900 --- /dev/null +++ b/.github/actions/prepare/action.yml @@ -0,0 +1,84 @@ +name: 'Prepare: Node and Yarn' +description: 'Sets up Node, enables Corepack for Yarn 4, restores caches and installs dependencies.' + +# Composite action, not a reusable workflow: this runs as a *step* inside an +# existing job, so the caller keeps its own runs-on, permissions and checkout. +# +# The caller must `actions/checkout` first — this installs into whatever is +# already in the workspace. +# +# Usage: +# steps: +# - uses: actions/checkout@v4 +# - uses: iXsystems/ux-github-workflows/.github/actions/prepare@master +# with: +# cache-jest: 'true' # optional +# +# Inputs are strings, as all composite-action inputs are — compare with +# `== 'true'`, not as booleans. + +inputs: + node-version: + description: >- + Exact Node version. Pinned rather than floating on purpose: the library + and the apps that consume it should build on the same Node. + required: false + default: '24.13.1' + cache-jest: + description: "Cache .jest/cache, keyed on yarn.lock. Only useful in repos that run Jest." + required: false + default: 'false' + yarn-cache: + description: "Cache Yarn's global cache folder, keyed on yarn.lock." + required: false + default: 'false' + install: + description: "Run `yarn install --immutable`. Set false to set up the toolchain without installing." + required: false + default: 'true' + +runs: + using: 'composite' + steps: + # Order matters: setup-node must come before `corepack enable`. Corepack + # writes its shims into the active Node installation's bin directory, so + # enabling it first and then letting setup-node swap in a different Node + # leaves `yarn` missing. This is also why setup-node's own `cache: 'yarn'` + # is not used — it shells out to `yarn` before Corepack has run. + - name: Setup Node + uses: actions/setup-node@v4 + with: + node-version: ${{ inputs.node-version }} + + - name: Enable Corepack for Yarn 4 + shell: bash + run: corepack enable + + - name: Resolve Yarn cache folder + if: inputs.yarn-cache == 'true' + id: yarn-cache-dir + shell: bash + run: echo "dir=$(yarn config get cacheFolder)" >> "$GITHUB_OUTPUT" + + - name: Cache Yarn packages + if: inputs.yarn-cache == 'true' + uses: actions/cache@v4 + with: + path: ${{ steps.yarn-cache-dir.outputs.dir }} + key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }} + restore-keys: | + ${{ runner.os }}-yarn- + + - name: Cache Jest cache + if: inputs.cache-jest == 'true' + uses: actions/cache@v4 + with: + path: .jest/cache + key: ${{ runner.os }}-jest-${{ hashFiles('**/yarn.lock') }} + restore-keys: | + ${{ runner.os }}-jest- + + - name: Install packages + if: inputs.install == 'true' + shell: bash + run: yarn install --immutable diff --git a/README.md b/README.md index 5055836..016e042 100644 --- a/README.md +++ b/README.md @@ -136,13 +136,51 @@ resolves against the *caller's* repo, so `./.github/workflows/check-member.yml` would be looked for in webui. This nests two levels deep (caller → `claude-review` → `check-member`), well inside GitHub's limit of four. +## Actions + +### `.github/actions/prepare` + +A **composite action**, not a reusable workflow: it runs as a step inside an +existing job, so the caller keeps its own `runs-on`, `permissions` and checkout. +Reusable workflows cannot do that — they bring their own job. + +```yaml +steps: + - uses: actions/checkout@v4 # required first; this installs into the workspace + - uses: iXsystems/ux-github-workflows/.github/actions/prepare@master + with: + cache-jest: 'true' # optional +``` + +| Input | Default | Notes | +|---|---|---| +| `node-version` | `24.13.1` | Pinned, not floating | +| `cache-jest` | `'false'` | Caches `.jest/cache`; only useful where Jest runs | +| `yarn-cache` | `'false'` | Caches Yarn's global cache folder | +| `install` | `'true'` | Set `'false'` for the toolchain without `yarn install` | + +Inputs are strings — every composite-action input is. Compare with `== 'true'`. + +**Step order is load-bearing.** `actions/setup-node` runs *before* +`corepack enable`, because Corepack writes its shims into the active Node +installation's bin directory: enable it first and then let setup-node swap in a +different Node, and `yarn` goes missing. That is also why setup-node's own +`cache: 'yarn'` is not used — it shells out to `yarn` before Corepack has run, +and would either fail or silently cache Yarn 1's directory for a Yarn 4 repo. +The `yarn-cache` input resolves the folder with `yarn config get cacheFolder` +after Corepack instead. + +This replaced identical local copies in `truenas/webui` and `truenas-connect/ui` +and six inline repetitions in `iXsystems/truenas-ui-components`'s `ci-cd.yml`, +which had drifted to a floating `'24'` against the others' pinned `24.13.1`. + ## Adoption status -| Repo | `check-ticket.yml` | `check-member.yml` | `claude-review.yml` | -|---|---|---|---| -| `truenas/webui` | adopted | migrating (`main.yml`) | migrating | -| `iXsystems/truenas-ui-components` | adopted | n/a — no self-hosted runner | migrating | -| `truenas-connect/ui` | adopted | migrating (`main.yaml`) | migrating | +| Repo | `check-ticket` | `check-member` | `claude-review` | `prepare` | +|---|---|---|---|---| +| `truenas/webui` | adopted | migrating (`main.yml`) | migrating | migrating | +| `iXsystems/truenas-ui-components` | adopted | n/a — no self-hosted runner | migrating | migrating | +| `truenas-connect/ui` | adopted | migrating (`main.yaml`) | migrating | migrating | `claude-review.yml` pulls in `check-member.yml` on its own, so a repo using only the review does not call it directly — the `check-member.yml` column tracks From ceae200368d5d394f38ebfaf050fda1a3075bffc Mon Sep 17 00:00:00 2001 From: Alex Karpov Date: Fri, 7 Aug 2026 18:33:14 +0300 Subject: [PATCH 6/6] =?UTF-8?q?NAS-142094:=20Address=20review=20=E2=80=94?= =?UTF-8?q?=20fix=20member-check=20crash,=20scope=20review=20concurrency?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/actions/prepare/action.yml | 14 +++++++++----- .github/workflows/check-member.yml | 15 +++++++++++++-- .github/workflows/claude-review.yml | 17 +++++++++++++---- README.md | 11 ++++++----- 4 files changed, 41 insertions(+), 16 deletions(-) diff --git a/.github/actions/prepare/action.yml b/.github/actions/prepare/action.yml index 8334900..c48d3d2 100644 --- a/.github/actions/prepare/action.yml +++ b/.github/actions/prepare/action.yml @@ -32,10 +32,6 @@ inputs: description: "Cache Yarn's global cache folder, keyed on yarn.lock." required: false default: 'false' - install: - description: "Run `yarn install --immutable`. Set false to set up the toolchain without installing." - required: false - default: 'true' runs: using: 'composite' @@ -58,7 +54,15 @@ runs: if: inputs.yarn-cache == 'true' id: yarn-cache-dir shell: bash - run: echo "dir=$(yarn config get cacheFolder)" >> "$GITHUB_OUTPUT" + run: | + dir="$(yarn config get cacheFolder)" + # An empty value would reach actions/cache as `path: ''` and fail there + # with a Path Validation Error that says nothing about Yarn. Fail here. + if [ -z "$dir" ]; then + echo "::error::Could not resolve the Yarn cache folder. Is this a Yarn 4 project with a packageManager field?" + exit 1 + fi + echo "dir=$dir" >> "$GITHUB_OUTPUT" - name: Cache Yarn packages if: inputs.yarn-cache == 'true' diff --git a/.github/workflows/check-member.yml b/.github/workflows/check-member.yml index 5121bb0..e2e5bfd 100644 --- a/.github/workflows/check-member.yml +++ b/.github/workflows/check-member.yml @@ -52,8 +52,19 @@ jobs: github-token: ${{ secrets.GITHUB_TOKEN }} result-encoding: string script: | + // Guard first. Both the happy path and the fallback below read + // `pull_request`, so on any other event the fallback used to throw + // a second TypeError *inside* the catch — uncaught, failing the job + // rather than answering 'false'. Returning here keeps the job green + // and the `is_member` output defined for downstream `needs`. + const pullRequest = context.payload.pull_request; + if (!pullRequest) { + core.info(`No pull_request payload on a '${context.eventName}' event — reporting not-a-member.`); + return 'false'; + } + try { - const username = context.payload.pull_request.user.login; + const username = pullRequest.user.login; console.log(`Checking repository access for user: ${username}`); const { data: permissionLevel } = await github.rest.repos.getCollaboratorPermissionLevel({ @@ -75,7 +86,7 @@ jobs: // lookup fails (e.g. the token cannot read org membership). // Deliberately permissive: this decides where tests run and // whether a review happens, not whether anything merges. - const association = context.payload.pull_request.author_association; + const association = pullRequest.author_association; console.log(`PR author association: ${association}`); const isTeamMember = ['MEMBER', 'OWNER', 'COLLABORATOR'].includes(association); diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 960e8e7..a7d647c 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -59,6 +59,13 @@ on: description: 'Anthropic API key. Mapped by the caller, since the secret name differs per repo.' required: true +# One review per PR. Rapid pushes previously started overlapping reviews that +# raced to overwrite the same sticky comment, and paid for every superseded run. +# Groups are scoped to the calling repository, so the PR number alone is enough. +concurrency: + group: claude-review-${{ github.event.pull_request.number }} + cancel-in-progress: true + jobs: # Gate: does the PR author have write access to the calling repo? # @@ -80,11 +87,13 @@ jobs: runs-on: ubuntu-latest timeout-minutes: ${{ inputs.timeout-minutes }} needs: [check-member] - # `always()` is required because check-member is skipped when the gate is - # off; without it this job would inherit the skip. The membership condition - # then only has to hold when the gate actually ran. + # `!cancelled()` rather than a bare `always()`: the job still has to run when + # check-member is *skipped* (gate off) instead of inheriting that skip, but + # `always()` would also push a review through after the run was cancelled — + # spending tokens on work someone explicitly stopped. A failed check-member + # leaves is_member empty, so the gate stays fail-closed either way. if: | - always() && + !cancelled() && (inputs.require-write-access == false || needs.check-member.outputs.is_member == 'true') && !contains(github.event.pull_request.labels.*.name, inputs.skip-label) permissions: diff --git a/README.md b/README.md index 016e042..2597482 100644 --- a/README.md +++ b/README.md @@ -87,10 +87,12 @@ were three separate copies of the same script before this existed — two workflow files plus one inlined directly in `truenas-connect/ui`'s `main.yaml`. Only meaningful on `pull_request` events: it reads -`context.payload.pull_request`. A caller whose workflow also runs on `push` -must guard the job with `if: github.event_name == 'pull_request'`, and then use -`always()` plus an explicit `!= 'true'` on the downstream job so the skip does -not cascade — see `truenas/webui`'s `main.yml`. +`context.payload.pull_request`, and reports `'false'` on any event that has no +PR payload rather than failing. Guarding the job with +`if: github.event_name == 'pull_request'` is still worth doing to skip a +pointless runner — but then the downstream job needs `always()` (or +`!cancelled()`) plus an explicit `!= 'true'`, so the skip does not cascade into +it. See `truenas/webui`'s `main.yml` for the worked example. If the permission lookup fails it falls back to `author_association`, which is deliberately permissive. It decides where tests run and whether a review @@ -157,7 +159,6 @@ steps: | `node-version` | `24.13.1` | Pinned, not floating | | `cache-jest` | `'false'` | Caches `.jest/cache`; only useful where Jest runs | | `yarn-cache` | `'false'` | Caches Yarn's global cache folder | -| `install` | `'true'` | Set `'false'` for the toolchain without `yarn install` | Inputs are strings — every composite-action input is. Compare with `== 'true'`.