ci(workflows): trigger monorepo-build on main and dev - #128
Open
elycruz wants to merge 2 commits into
Open
Conversation
- Point `push` and `pull_request` triggers at `[ main, dev ]`; the workflow still targeted the `monorepo` branch that #76 renamed to `main`, so no build ran for any PR. - Replace the undefined `pnpm link-and-build:packages` step with `pnpm build`. That script was renamed to `link-packages` in 84c0578 and the workflow was never updated. Global `pnpm link` calls are redundant under pnpm workspaces (`pnpm i` already symlinks fjl / fjl-validator / fjl-inputfilter into root `node_modules`), and the script's final action was just `pnpm run build`. - Node matrix 18.x/20.x/22.X -> 20.x/22.x/24.x: drop EOL Node 18, fix the stray capital `X`, add current LTS Node 24 (matches local dev). - Bump actions/checkout v2 -> v4, actions/setup-node v2 -> v4, and pnpm/action-setup v2 -> v4. v4 of action-setup reads the `packageManager` field (pnpm@8.13.1), so the explicit `version: 8` input is dropped (passing both is an error in v4). Reordered pnpm setup before setup-node so `cache: 'pnpm'` works. - Fix `PUPPETEER_EXE_PATH: $(which chrome)`: job-level `env` is not shell interpolated, so that was a literal string. Now sourced from the setup-chrome step's `chrome-path` output and scoped to the test step. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…0 runtime The first green run on this PR annotated every job with "Node.js 20 is deprecated ... actions/checkout@v4, actions/setup-node@v4, pnpm/action-setup@v4 are being forced to run on Node.js 24". Bump to the current majors, all of which declare `using: node24`: checkout v4 -> v7, setup-node v4 -> v7, pnpm/action-setup v4 -> v6. The inputs in use (`node-version`, `cache: 'pnpm'`) are unchanged in these majors, and action-setup v6 still reads the `packageManager` field by default, so no `version` input is needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Summary
.github/workflows/monorepo-build.ymlstill triggered only on themonorepobranch, which #76 renamed tomain— so no build has been running for any PR intomainordev. This points the triggers at[ main, dev ]and fixes the other latent breakage that would have surfaced the moment the workflow started running again.Work unit:
118-ci-branch-hooksCloses #118
Changes
All changes are confined to
.github/workflows/monorepo-build.yml.Triggers —
push.branchesandpull_request.branchesare now[ main, dev ](both branches exist on the remote).Removed a step that could never have succeeded — the workflow ran
pnpm link-and-build:packages, a script that is not defined anywhere: not in rootpackage.json, not in anypackages/*/package.json, not innode_scripts/. It was renamed tolink-packagesin 84c0578 ("renamed 'link-and-build*' script to 'link-packages'") and the workflow was never updated. Even with the triggers fixed, CI would have failed on this step withERR_PNPM_NO_SCRIPT.Fix chosen: call
pnpm builddirectly rather than re-adding the alias.node_scripts/tasks/link-packages.mjsdoes two things — a series of globalpnpm link ./pnpm link <name>calls, and thenpnpm run build. The linking half is dead weight under pnpm workspaces:pnpm ialready creates the workspace symlinks (verified in this worktree —node_modules/fjl -> ../packages/fjl, plusfjl-validatorandfjl-inputfilter), and injecting global links over the workspace links in CI is a source of resolution drift, not a fix for it. So the only part CI actually needed waspnpm run build.link-packagesis left inpackage.jsonuntouched as a local dev convenience.Node matrix:
[18.x, 20.x, 22.X]→[20.x, 22.x, 24.x]— see justification below. Also fixes the stray capitalXon22.X.Action version bumps —
actions/checkoutv2 → v7,actions/setup-nodev2 → v7,pnpm/action-setupv2 → v6. The v2 actions run on a runtime GitHub retired long ago. (These landed in two steps: the first pass went to v4, and the resulting green run annotated every job with "Node.js 20 is deprecated … being forced to run on Node.js 24", so a second commit took them to the current majors, which all declareusing: node24.)pnpm/action-setupreads thepackageManagerfield (pnpm@8.13.1) frompackage.json, so the explicitversion: 8input is dropped — passing both is an error. The pnpm setup step now runs beforesetup-node, which is what makescache: 'pnpm'work (store caching added while there).Fixed
PUPPETEER_EXE_PATH: $(which chrome)— job-levelenv:in YAML is not shell-interpolated, so this set the literal seven-character string$(which chrome), not a path. It is now sourced from thesetup-chromestep's documentedchrome-pathoutput (confirmed against the action'saction.yml) and scoped to thepnpm teststep. The barechrome --versionsmoke step now uses the same output instead of relying onPATH.Testing evidence
The exact command sequence the workflow now runs (
pnpm i→pnpm build→pnpm test) was run locally in the worktree withNODE_ENV=CI/CDandHUSKY=0set, matching the job env:pnpm buildexits 0. (It emits pre-existing rollup.d.tswarnings aboutpackages/fjl/dist/esm/object/setTheory.d.ts; those are present onmainand unrelated to this change.)The workflow was also verified live on this PR — which is itself the proof the trigger fix works, since opening this PR is what caused a build to run at all. All three matrix jobs are green end to end (
pnpm i→pnpm build→pnpm test), with zero annotations after the action bump:The
chrome --versionstep resolves to a real path on CI now(
/opt/hostedtoolcache/setup-chrome/chrome/stable/x64/chrome --version), confirming thechrome-pathoutput wiring.The workflow YAML was parsed to confirm it is well-formed, and
pnpm@8.13.1(the pinnedpackageManager) was confirmed to run on Node 24 — the whole install/build/test run above was done underpnpm 8.13.1onnode v24.16.0, so the new top matrix entry is not speculative.pnpm lintwas not run as a gate: it already fails onmain(21 errors / 62 warnings, in vendoreddocs/*.js,packages/fjl-labs/, and some tests). No JS/TS source files are touched by this PR, so no new lint errors are introduced. Note thelintscript is deliberately not wired into this workflow — making it a gate is a separate cleanup, not something to smuggle into a trigger fix.No git hooks were bypassed;
commit-msg,pre-commit, andpre-push(pnpm test && pnpm build) all ran and passed.Node matrix justification
.nvmrcstill pinsv20and rootengines.nodeis>=16, so 20 is the lowest version the repo actually claims to support. Keeping it is the honest floor until those declarations are bumped.22.Xtypo fixed).This yields a three-entry LTS ladder — floor, current LTS, newest LTS — which is the same shape as before, just shifted forward one release.
Two related inconsistencies were found but deliberately left alone as out of scope for this PR (they are package-metadata changes, not CI changes):
engines.nodeis>=16, whilepackages/fjl/package.jsondeclares the nonsensical">=16 || >=18 || >=20 || >=22 || >= 24 || >=26"(an or-chain of lower bounds is just>=16), and the four other packages declare">=16 || >=18". These should be normalized and probably raised to>=20..nvmrcpinsv20while local development is on v24.Once
enginesis raised, the matrix floor should move with it.Manual follow-up required
The third acceptance criterion — make the build a required status check on
main— is a repo-settings mutation on a public repo and has intentionally not been applied here. A repo admin should run it after this PR merges and the workflow has reported once.mainis already protected (verified:required_pull_request_reviews,block_creations, andrequired_conversation_resolutionare on) butrequired_status_checks.checksis empty. A targetedPATCHon just that sub-resource avoids disturbing the existing protection settings:Notes for whoever runs it:
build-and-testalone will never match and would silently never block anything."strict": trueinstead if you also want branches required to be up to date withmainbefore merging. Current protection hasstrict: false; this command preserves that unless you change it.devhas no branch protection at all. Worth mirroring the same required checks there ifdevis meant to be an integration branch:gh api -X PUT repos/functional-jslib/fjl/branches/dev/protection --input …Verify afterwards with:
Note on
publish.yml.github/workflows/publish.ymlcarries the same brokenPUPPETEER_EXE_PATH: $(which chrome)in two places (job-level and step-level). It is outside the scope of this work unit and is left untouched here, but it should get the same treatment.🤖 Generated with Claude Code