chore(ci): adopt node-ci gold-standard reusable#604
Merged
Conversation
Replace lint.and.build.yml + codeql.yml + security.yml with one ci.yml calling the org node-ci reusable (lint/type-check/test/build + node-audit + codeql + gitleaks + dependency-review). Piloting against the reusable's feature branch; flips to @main once netresearch/.github#248 merges. Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned Files
|
Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
|
CybotTM
added a commit
that referenced
this pull request
Jul 23, 2026
…github reusables (#605) Routes two workflows through shared reusables in [`netresearch/.github`](https://github.com/netresearch/.github): | Caller | Reusable | |---|---| | `.github/workflows/pr-quality.yml` | [`pr-quality.yml@main`](https://github.com/netresearch/.github/blob/main/.github/workflows/pr-quality.yml) | | `.github/workflows/auto-merge-deps.yml` | [`auto-merge-deps.yml@main`](https://github.com/netresearch/.github/blob/main/.github/workflows/auto-merge-deps.yml) | Both callers now have **zero step-level `uses:`** — each has a single job that is a `uses:` of its reusable. `.github/workflows/AGENTS.md` is updated for the changed `auto-merge-deps.yml` scope. `release.when-tagged.yml` and `release-please.yml` are deliberately untouched. --- # 1. `pr-quality.yml` ## How the original blocker was proved resolved This migration was **refused once before**: this repo authorizes auto-approval on the author's *repository permission* (`admin`/`write`), while the hub reusable's default test is `author_association` — which is not a permission (`COLLABORATOR` is returned for read/triage collaborators as well). The hub has since gained `auto-approve-require-write`. Proof it actually delivers the property, traced from requirement to API call: 1. **Association gate is bypassed, not merely supplemented.** Reusable [lines 152-159](https://github.com/netresearch/.github/blob/main/.github/workflows/pr-quality.yml#L152-L159): the job `if` ends in `( inputs.auto-approve-require-write || contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.pull_request.author_association) )`. With the input `true` the `||` short-circuits and `author_association` is never evaluated. 2. **The permission lookup is the same call this repo made.** Reusable [lines 172-184](https://github.com/netresearch/.github/blob/main/.github/workflows/pr-quality.yml#L172-L184) runs `gh api "repos/$REPO/collaborators/$PR_AUTHOR/permission" --jq '.permission' 2>/dev/null || echo "none"` — byte-identical to the removed line 44 of this repo's old workflow, including the fail-closed `|| echo "none"`. 3. **Approval is gated on that lookup.** Reusable [lines 194-196](https://github.com/netresearch/.github/blob/main/.github/workflows/pr-quality.yml#L194-L196): `inputs.auto-approve-require-write == false || contains(fromJSON('["admin","write"]'), steps.check-permission.outputs.permission)`. With the input `true` the left operand is `false`, so `admin`/`write` is mandatory — equivalent to the removed line 48. 4. **No second approval path exists in the reusable.** `grep -n "createReview\|pr review\|APPROVE"` over the reusable returns only the one `createReview` block at lines 205-211. Nothing else can widen the gate downstream. 5. **The lookup demonstrably works here at runtime** (not just statically). PRs #604 and #603 (author `CybotTM`, same-repo branches) carry `APPROVED` reviews from `github-actions[bot]`, while `gh run list` shows **Auto-merge bot PRs = skipped** on both of those branches and **PR Quality Gates = success**. Only the old `pr-quality.yml` could have produced those approvals — so `GET /collaborators/{author}/permission` already resolves to `admin`/`write` under `pull_request_target` with `GITHUB_TOKEN` and this repo's `pull-requests: write` grant. The migrated job grants a superset (`contents: read` + `pull-requests: write`). ## Before / after behaviour | Behaviour | Before | After | Delta | |---|---|---|---| | Approve author with `admin`/`write` repo permission | yes (`gh pr review --approve`) | yes (`pulls.createReview`, same lookup) | none — proof above | | Approve author with `read`/`triage`/no permission | no (lookup → `none`) | no (lookup → `none`, fails closed) | none | | Approve on `author_association` alone | never | never (`auto-approve-require-write: true`) | none | | Approve `dependabot[bot]` / `renovate[bot]` | yes, **duplicated** by `auto-merge-deps.yml` | dropped here; `auto-merge-deps.yml` still approves them | **no effective loss** — see below | | Approve `github-actions[bot]` (release-please) | step existed but was a **no-op** | dropped | **no effective loss** — see below | | Re-approve on `synchronize` (needed: `dismiss_stale_reviews: true`) | yes | yes (trigger preserved) | none | | Approval blocked when the size-check job fails | n/a (no such job) | yes — the reusable's `auto-approve` carries `needs: quality-gate` | **new coupling, stated below** | | Approve draft PRs | yes | no (reusable guards `draft == false`) | mitigated by adding `ready_for_review` | | Approve PRs from forks by write-collaborators | yes | no (reusable requires `head.repo.full_name == github.repository`) | narrower, safer; no fork PRs in this repo's recent history | | PR size check / large-PR warning | none | added (`quality-gate` job) | new job, and approval now depends on it | | Approval review body | empty | short explanatory body, **no `{controls}` placeholder** so no dead link | cosmetic | | Top-level `permissions:` | `pull-requests: write` | `{}` + job-level union | tightened | | Runner | `ubuntu-24.04` | `ubuntu-latest` (reusable) | none material | | Required status checks | job names not referenced by branch protection (`ci / …` only) | unchanged | none | ### Approval is now coupled to the quality gate — stated plainly The reusable's `auto-approve` job declares [`needs: quality-gate`](https://github.com/netresearch/.github/blob/main/.github/workflows/pr-quality.yml#L144). Consequences, which an earlier revision of this description glossed as merely "additive": - If `quality-gate` **fails** (e.g. `pulls.listFiles` errors, or the checkout step fails), `auto-approve` is skipped and the PR is **not** auto-approved. That failure mode did not exist before. - `quality-gate` is itself guarded by `draft == false`, so on a draft PR both jobs are skipped — the `ready_for_review` trigger below is what recovers from that. - `core.warning()` for a large PR does **not** fail the job, so an oversized PR still gets approved; only a genuine job failure blocks approval. This is accepted, not refuted: the shared standard runs the size check ahead of approval in every consuming repo, and this repo now gets the same ordering. ### Why dropping the bot branches loses nothing - `auto-merge-deps.yml` also approves `dependabot[bot]` and `renovate[bot]` — before this PR in its own inline `Auto-approve PR` step, after this PR in the reusable's `Approve PR` step, whose job `if` admits exactly those two logins. `gh pr view <n> --json reviews` shows PRs #602, #597 and #587 each carrying **2 `APPROVED` reviews from `github-actions[bot]`**, and #601 carrying **4** — one per workflow, per run. Removing one of two duplicates leaves the approval intact. - The `Auto-approve (bot via APPROVE_TOKEN)` step was **already a no-op**: `APPROVE_TOKEN` exists neither as a repo secret (`gh secret list` → empty) nor among the org secrets visible to this repo, so it fell back to `GITHUB_TOKEN`, which cannot approve its own bot's PRs, and the failure was swallowed by `|| true`. Empirical confirmation: release-please PR #466 (`github-actions[bot]`, label `autorelease: pending`) was approved **manually by `CybotTM`**, not by the workflow. ### Trigger change `ready_for_review` is added to `types`. The reusable guards both jobs with `draft == false`; marking a PR ready fires neither `opened` nor `synchronize`, so without this a draft PR would stay unapproved until its next push — a regression the migration itself would introduce. The spelling is validated by `actionlint`, which rejects an unknown activity type for `pull_request_target` and lists `ready_for_review` among the accepted ones (negative control below). All pre-existing types (`opened`, `synchronize`, `reopened`) are preserved, as are the workflow `name` and the (absent) `concurrency`. ### `actions/checkout` under `pull_request_target` The old file carried a comment forbidding `actions/checkout`; the reusable's `quality-gate` job contains one. The invariant that comment protects — untrusted PR code executing with a write-scoped token — is preserved: - The only step after the checkout is `actions/github-script` with an **inline** script; nothing from the working tree is executed, so the checked-out ref is not a code-execution vector either way. - `actions/checkout` is called without `ref`, so it resolves `github.ref`, which for `pull_request_target` is the **base branch**, not the PR head. - It sets `persist-credentials: false`, so no token is left in `.git/config`. --- # 2. `auto-merge-deps.yml` ## The release-please arm is removed The old job's `if` had three arms; the third matched `github-actions[bot]` carrying the label `autorelease: pending`, i.e. a release-please PR. Its scope, measured rather than assumed (`gh pr list --state all --limit 300`, filtered on head branch): - **Exactly one** genuine release-please PR has ever been opened in this repo — [#466](#466) (`release-please--branches--main--components--node-magento-eqp`, 2026-02-16, merged). The two other "release-please" head-branch matches are Renovate PRs bumping the release-please *action* (#529, #540), which the `dependabot`/`renovate` arms handle anyway. - Nothing since — five months, while `release-please.yml` kept running `success` (most recently 2026-07-20). Releases here are cut by pushing a signed `v*` tag, handled by `release.when-tagged.yml`. - An org-wide code search for `googleapis/release-please-action` matches only this repo and its sibling, out of 191 — nothing else in the org uses it. The arm's **approval half was a no-op by construction**: `Auto-approve PR` was explicitly skipped for `github-actions[bot]`, and the comment deferred approval to "`pr-quality.yml` using `APPROVE_TOKEN`" — a path that does not exist (no such secret, see above) and that the `pr-quality.yml` migration in this same PR removes outright, so that comment pointed at a capability that is gone. On #466 the approval came **manually from `CybotTM`**. Its **merge half did fire, once**: `gh run list` shows `auto-merge-deps.yml` running on that branch on 2026-02-16 (three `success`, one earlier `failure`). So the honest statement is not "this code never ran" but "this code has run for one PR in the repo's history, five months ago, on a release path the repo no longer uses". Removing it is a deliberate owner decision, not an oversight, and it is **not** reimplemented in the shared workflow — [netresearch/.github#261](netresearch/.github#261) ("RECOMMEND CLOSE: allow-release-please arm is unreachable") was closed for that reason. If release-please is ever revived here, `--auto` on its PRs comes back as an explicit change rather than as an unused branch nobody remembers. `release-please.yml` itself is out of scope for this PR and is untouched. With that arm gone the remaining logic — approve `dependabot[bot]`/`renovate[bot]`, then `gh pr merge --auto` — is exactly what the shared reusable does, so the file becomes a caller. ## Merge strategy is pinned, not auto-detected The reusable auto-detects squash-first: ``` if .allow_squash_merge then "--squash" elif .allow_merge_commit then "--merge" elif .allow_rebase_merge then "--rebase" else "--squash" end ``` `gh api repos/netresearch/node-magento-eqp` returns `{"merge":true,"squash":false,"rebase":true}`, so the detector would resolve to `--merge`, whereas the inline job ran `gh pr merge --auto --rebase`. The caller therefore passes `merge-strategy: rebase`, which the reusable turns into `STRATEGY="--rebase"` before the detector branch is reached. ## Before / after behaviour | Behaviour | Before | After | Delta | |---|---|---|---| | Approve + auto-merge `dependabot[bot]` / `renovate[bot]` | yes | yes (reusable job `if` admits exactly those two logins) | none | | Handle `github-actions[bot]` + `autorelease: pending` | matched the job `if`, approval deliberately skipped, then `gh pr merge --auto --rebase` | not matched | **removed** — fired for one PR in repo history (#466, 2026-02-16); owner-decided drop, evidence above | | Merge strategy | `--rebase` (hardcoded) | `--rebase` (`merge-strategy: rebase`) | none | | Merge gating | merge everything the `if` matched | same (`safe-updates-only` left at its default `false`) | none | | `dependabot/fetch-metadata` step | ran for Dependabot; its output `steps.metadata` was **never referenced** | only runs under `safe-updates-only`, i.e. not here | none — the step had no effect | | Trigger | `pull_request_target:` with `types: [opened, synchronize, reopened]` | `pull_request_target:` — and those three types are exactly GitHub's default activity set | none | | Top-level `permissions:` | `contents: write`, `pull-requests: write` | `{}` + job-level `contents: write` + `pull-requests: write` | tightened; the job grant equals the reusable's job permissions exactly | | Runner / timeout | `ubuntu-24.04`, no timeout | `ubuntu-latest`, `timeout-minutes: 5` (reusable) | none material | | Workflow `name` | `Auto-merge bot PRs` | `Auto-merge dependency PRs` (matches the org templates; it no longer handles non-dependency bots) | not a required status check — branch protection requires only `ci / …` contexts | | `actions/checkout` under `pull_request_target` | forbidden by comment, absent | still absent — the reusable has no checkout | none | ## Permissions The reusable's job declares `contents: write` + `pull-requests: write`. A caller granting less startup-fails before any job runs, so the caller job grants exactly that set. --- ## Verification Run in this branch's working tree, and nothing beyond this is claimed: - `actionlint .github/workflows/auto-merge-deps.yml .github/workflows/pr-quality.yml` → exit 0. - Negative control for the trigger fix: the same `actionlint` on a copy with `ready_for_review` misspelled → exit 1, `invalid activity type … available types are … "ready_for_review" …`. - `yq '[.jobs[].steps[]?.uses]' .github/workflows/auto-merge-deps.yml` → `[]` (no step-level `uses`); `yq '.on, .permissions, .jobs'` confirms the single job, its `permissions`, and `with: {merge-strategy: rebase}`. - `gh api repos/netresearch/node-magento-eqp --jq '{merge:.allow_merge_commit,squash:.allow_squash_merge,rebase:.allow_rebase_merge}'` → `{"merge":true,"squash":false,"rebase":true}` — the basis for pinning the strategy. - `gh api repos/netresearch/node-magento-eqp/branches/main/protection` → required contexts are `ci / TS Check / Lint`, `ci / TS Check / Check types`, `ci / Test / test (node 24)`, `ci / Build / build`; `required_approving_review_count: 1`, `dismiss_stale_reviews: true`. No job renamed or removed here is a required context. - `gh pr view 602|601|597|587 --json reviews` → 2, 4, 2, 2 `APPROVED` reviews from `github-actions[bot]` (the duplicate-approval claim above). - `gh pr list --state all --limit 300 --json number,headRefName` filtered on head branch → three matches, of which one (#466) is a genuine release-please branch and two (#529, #540) are Renovate action bumps; `gh run list --workflow=auto-merge-deps.yml` filtered the same way → four runs, all on #466's branch, 2026-02-16. - Both reusables read in full from `netresearch/.github@main` (`84499c7`), plus this repo's `release-please.yml` and `branches/main/protection`. **Correction carried forward:** the first commit message on the `auto-merge-deps.yml` change asserted that no release-please PR had ever been opened here. That is wrong — #466 exists, and the arm's merge half ran on it. The follow-up commit `7da85b4` states the accurate scope in the file itself; the removal decision is unchanged. Not verified: **neither migrated file can be exercised by this PR.** `pull_request_target` always runs the workflow definition from the base branch, so the runs on this PR execute the *old* files from `main`. The new definitions first run after merge. The remote `uses:` references and their inputs were therefore checked by reading `netresearch/.github@main` directly — `auto-approve-require-write` (boolean), `auto-approve-message` (string) and `merge-strategy` (string) all exist with those names and types — not by execution.
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
1 participant
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.
Pilots the org node-ci gold-standard meta-reusable (netresearch/.github#248). Replaces
lint.and.build.yml+codeql.yml+security.ymlwith oneci.ymlcallingnode-ci.yml(lint/type-check/test/build + node-audit + codeql + gitleaks + dependency-review). References the reusable's@feat/node-gold-standardbranch for validation; flips to@mainonce #248 merges. Required checks (🔎 Lint, 🔨 Build) will be updated to the newci / …names once the run reports them.