ci: use a maintained commitlint action fork - #54
Conversation
There was a problem hiding this comment.
Replaces wagoid/commitlint-github-action (pins @commitlint/ensure ^19, missing the v20 URL long-line exemption) with a version-pinned direct commitlint run using @commitlint/cli@21.2.1 + @commitlint/config-conventional@21.2.0, matching what the pre-commit hook resolves. The workflow logic checks out cleanly: fetch-depth: 0 plus --from/--to BASE_SHA/HEAD_SHA gives the commit range the old action derived from the API, pull-requests: read is correctly dropped since nothing calls the API anymore, and the setup-node pin matches the SHA already used elsewhere in this file. The failing check (node 22/24) job on this head is task dev:fmt-check, which lints source formatting — this PR touches only .github/workflows/ci.yaml, so that failure is unrelated to this diff. No Critical or Important issues found; approving.
168fc0e to
ecd601b
Compare
There was a problem hiding this comment.
Re-review: a prior yo61-lastlight approval on this same content exists but was dismissed after a cosmetic amend (whitespace-only, comment alignment on the setup-node line) — that approval raised no findings, so there is nothing from it to re-verify as fixed/open. Independently re-checking the change: it replaces the wagoid commitlint action with a direct, version-pinned npx commitlint run to fix a real version-skew bug (@commitlint/ensure v19 in the action lacks the v20 URL long-line exemption, so Dependabot bodies fail body-max-line-length in CI). The workflow logic is correct — fetch-depth: 0 plus --from/--to BASE_SHA/HEAD_SHA reproduces the commit range the old action derived from the API, and dropping pull-requests: read is right since nothing calls the GitHub API anymore. One new concern not raised before: the dependency-pinning discipline this file otherwise enforces (every action pinned by commit SHA, the gitleaks image pinned by digest) is dropped for this step, which resolves commitlint's own transitive dependencies fresh from npm on every run with no lockfile.
| set -euo pipefail | ||
| npx --yes \ | ||
| --package @commitlint/cli@21.2.1 \ | ||
| --package @commitlint/config-conventional@21.2.0 \ |
There was a problem hiding this comment.
[Important] commitlint deps aren't reproducibly pinned like everything else in this file
npx --yes --package @commitlint/cli@21.2.1 --package @commitlint/config-conventional@21.2.0 pins only the two top-level packages; their transitive dependencies (including @commitlint/ensure, the package this whole PR is about) are resolved fresh from the npm registry on every run with no lockfile, since these packages aren't in package.json/pnpm-lock.yaml. That reintroduces exactly the mutable-dependency risk the SHA-pinning on every uses: in this file (and the digest-pinned gitleaks image) exists to prevent: a compromised or re-tagged transitive dependency published after this PR merges runs in CI without the workflow file changing at all. The check job's pnpm install --frozen-lockfile shows the project already has a pattern for reproducible installs; this step bypasses it. Worth adding commitlint as a devDependency (pnpm-lock-pinned) and invoking it via pnpm exec/pnpm dlx --package-lock instead of an unlocked npx --yes.
|
|
There was a problem hiding this comment.
Re-review after the pinning fixup commit (ci: pin commitlint in a tracked manifest). Prior findings ledger: the 12:35 review's Important finding — commitlint's transitive deps resolved unlocked via npx --yes — is fixed: commitlint is now a devDependency of .github/commitlint/package.json with a committed package-lock.json, installed via npm ci, and tracked by a new Dependabot entry. The earlier 12:23 approval raised no findings, so there's nothing to re-verify from it. However, the fixup commit also folded the commitlint steps into the check job's node-version matrix, and in doing so silently deleted the check job's actual purpose: go-task/setup-task, pnpm install --frozen-lockfile and task dev:check are all gone, and node-version: ${{ matrix.node }} was hardcoded to 22. As it stands, the check (node 22/24) matrix no longer builds, type-checks, or tests the project on either Node version — it only lints commit messages, and does so identically (and, on push, brokenly) on both matrix legs. This is a regression well outside the PR's stated scope and blocks merge.
| set -euo pipefail | ||
| ./.github/commitlint/node_modules/.bin/commitlint \ | ||
| --config commitlint.config.mjs \ | ||
| --from "$BASE_SHA" --to "$HEAD_SHA" --verbose |
There was a problem hiding this comment.
[Critical] check job no longer runs the project's build/test suite
The check matrix job's real steps — go-task/setup-task, pnpm install --frozen-lockfile, task dev:check — were deleted in this diff and replaced by the commitlint install/lint steps shown here. Both check (node 22) and check (node 24) matrix legs now do nothing but lint commit messages (identically, since node-version is hardcoded to 22 rather than matrix.node). CI no longer typechecks, builds, or runs the test suite on any Node version, and a PR that breaks task dev:check will show green. This is unrelated to the PR's stated goal (fix commitlint version skew) and needs the original check steps restored, with commitlint added as its own step/job rather than replacing them.
| run: npm ci | ||
| - name: Lint commit messages | ||
| env: | ||
| BASE_SHA: ${{ github.event.pull_request.base.sha }} |
There was a problem hiding this comment.
[Critical] Commitlint step now runs unconditionally on push, where github.event.pull_request is unset
The old commitlint job had if: github.event_name == 'pull_request'; that guard was dropped when these steps moved into check, which also triggers on push to main (line 4). On a push event github.event.pull_request.base.sha/.head.sha are both empty, so this becomes commitlint --from "" --to "" --verbose, which fails the step (and therefore every push-triggered check run, i.e. every merge to main). Separately, the checkout step feeding this job (no fetch-depth: 0, unlike the old commitlint job) only fetches a shallow history, so even in the PR case BASE_SHA is likely absent from local history and --from will fail to resolve.
3b0c94e to
ecd601b
Compare
wagoid/commitlint-github-action pins @commitlint/ensure ^19. The exemption for long lines containing a URL landed in v20, so CI enforced a stricter rule than the config intended and failed Dependabot commits on body-max-line-length, while the pre-commit hook resolving v21 passed the same commit. Upstream is unmaintained: master has had no commit since 2025-01-14 and the v20 bump has sat open since July. Point at yo61/commitlint-github-action instead, which runs commitlint v21 on node 24 and is published to ghcr.io.
ecd601b to
5c7055d
Compare
|
|
There was a problem hiding this comment.
Re-review on a rewritten diff: the branch was force-pushed (168fc0e -> 5c7055d) back to the PR's originally-described approach, discarding the intermediate npx-based rewrite that the three prior yo61-lastlight reviews assessed (the 12:23 approval, the 12:35 'unlocked transitive deps' finding, and the 12:58 CHANGES_REQUESTED for a deleted check job and an unconditional-on-push commitlint step). None of those findings apply to the current diff since the code they were about no longer exists. Current change is a single-line action swap in the existing commitlint job: wagoid/commitlint-github-action@v6.2.1 -> yo61/commitlint-github-action@v6.3.0, with a comment explaining the @commitlint/ensure v19->v21 URL-exemption gap. Verified: the pinned SHA (31e5e0454fc0709625e6dcfb502ea18978f45f15) correctly dereferences tag v6.3.0 on the fork; the if: github.event_name == 'pull_request' guard, fetch-depth: 0, and the job's permissions block are all left untouched; CI is green on the current head SHA (run 31497361998). No Critical or Important issues found in the diff itself.
What
Points the commitlint job at
yo61/commitlint-github-action— a maintained fork running commitlint v21 on Node 24.Why
Dependabot PRs fail the commitlint job on
body-max-line-length, while the pre-commitlintjob passes the same commit. Same commit, samecommitlint.config.mjs, different verdict.@commitlint/ensurev20 added an exemption for long lines containing a URL:wagoid/commitlint-github-action@v6.2.1is the latest release and pins@commitlint/ensure: ^19.0.3, so CI enforced a stricter rule than the config intends. Dependabot bodies are markdown link lists — one real example is 314 characters on a single line.Upstream is unmaintained: no commit to
mastersince 2025-01-14, and the v20 bump (PR #837) has been open and mergeable since July with no response.The fork
yo61/commitlint-github-action@v6.3.0— all@commitlint/*at^21, Node 24.19.0 LTS (v21 needsnode >= 22.12),commitlint-plugin-function-rulesat^5(v4's peer range caps at<20and breaksnpm ci), published toghcr.iofrom a pushed tag. Actions SHA-pinned, least-privilege permissions,actionlintandzizmorclean.Verified against the action's own load+lint path with a real
commitlint.config.mjs:subject-case: [0].mjsoverride is honoredCommit subjects stay validated for Dependabot as well as humans, rather than exempting bot commits.