deps: update pnpm to 10.16.0 for minimumReleaseAge enforcement - #280
Conversation
* update pnpm@10 to pnpm@10.34.3 in Dockerfiles to match packageManager * bump documented pnpm requirement in AGENTS.md to 10.16.0+ (minimum for minimumReleaseAge)
There was a problem hiding this comment.
Pull request overview
Updates the Docker build environment and contributor documentation to align with the repository’s pinned pnpm version and to ensure pnpm’s dependency age-gating (minimumReleaseAge) can be enforced as intended (Fixes #176).
Changes:
- Pin pnpm to
10.34.3in Docker build images (matchingpackage.json#packageManager). - Update
AGENTS.mdto document a pnpm minimum version that supportsminimumReleaseAge.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Dockerfile.rootless | Pins globally installed pnpm to 10.34.3 during image build. |
| Dockerfile | Pins globally installed pnpm to 10.34.3 during image build. |
| docker/forkana/Dockerfile | Pins globally installed pnpm to 10.34.3 during image build. |
| AGENTS.md | Updates documented pnpm prerequisite to reflect minimum version needed for dependency age gating. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/review AI review started. |
Advanced AI Review
Click to expand reviewI'll investigate the context around this PR to provide a thorough review.Let me verify the pnpm configuration for the minimum release age feature and check the packageManager alignment.The Context verified
1. 🟡 DRY violation — pnpm version now hardcoded in 3 Dockerfiles, diverging from CI's single-source pattern
Pinning to an exact version is good for reproducibility, but it creates a new sync hazard: the literal If someone bumps Notably, CI already avoids this duplication: every workflow uses The Dockerfiles should follow the same single-source-of-truth approach via corepack, which ships with Node and reads RUN apk --no-cache add \
build-base \
git \
nodejs \
npm \
&& npm install -g pnpm@10.34.3 \
&& rm -rf /var/cache/apk/*with: RUN apk --no-cache add \
build-base \
git \
nodejs \
npm \
&& corepack enable \
&& rm -rf /var/cache/apk/*This eliminates all three hardcoded versions; the exact pnpm release is then governed solely by If, for some reason, corepack can't be used in these alpine images, a weaker fallback is to at least add a comment next to each pin such as No 🔴 (bugs/security) issues found; the version numbers are internally consistent and the spelling/grammar in the changed strings is correct. Review generated using |
* remove duplicated hardcoded pnpm version from 3 Dockerfiles * keep package.json as the single pnpm version source * preserve npm in Alpine build deps for Corepack bootstrap
|
@taoeffect approved! ✅ Confirmed that the pnpm version is set to |
|
@pedrogaudencio This PR doesn't seem to address my comment in any way as it doesn't touch |
@taoeffect you're right, I missed addressing this but I skipped adding the directive in
|
|
I'm not satisfied with Fable's response. If we have an .npmrc file it should contain the right settings. And otherwise why do we have it at all? It's just there adding confusion and should be deleted. What is its purpose? If it has one, add the delay to it too and also a comment saying that the file is mostly ignored (or only used for XYZ settings) and reference the actual file pnpm uses. If it has none, delete it. If you decide to delete it just make sure that the pnpm file has all the stuff from npmrc like save-exact. |
|
|
I'd do it inside of npmrc, and please add the min age 14 days to it as well |
* added header comment clarifying that pnpm manages project dependencies. * added minimum-release-age=20160 to enforce 14-day delay for new releases
Advanced AI Review
Click to expand reviewI'll investigate the codebase to verify the changes are consistent and correct.The Review:
|
Looks like it didn't. |
* install pnpm@^10.34.3 with --min-release-age=14
* lower pnpm bootstrap version from ^10.34.3 to ^10.16.0 in all three Dockerfiles, keeping the 14-day --min-release-age gate * remove the packageManager pin (pnpm@10.34.3) from package.json, relying on the existing engines constraint (>= 10.16.0)
* add version "^10.16.0" to all 9 pnpm/action-setup@v4 steps * required after packageManager removal from package.json * keeps CI aligned with Dockerfile bootstrap and engines.pnpm constraint
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
package.json:6
- The PR title/description indicate pnpm should be pinned (e.g. pnpm@10.34.3) via the packageManager field, but this change removes it entirely. That makes the pnpm version less reproducible and no longer aligns with the stated intent to “match packageManager”.
{
"type": "module",
"engines": {
"node": ">= 22.6.0",
"pnpm": ">= 10.16.0"
},
|
/review AI review started. |
Advanced AI Review
Click to expand reviewI'll investigate the codebase to verify the claims in the changes and find issues.Let me research the critical technical claims about npm's Review: deps: update pnpm to 10.16.0 for minimumReleaseAge enforcementI verified the core technical claims: Alpine 3.23 ships npm 11.11.0 (✓ supports 1. 🟡
|
| Location | Key | Value | Unit |
|---|---|---|---|
.npmrc:16 |
min-release-age |
14 |
days |
pnpm-workspace.yaml:2 |
minimumReleaseAge |
20160 |
minutes |
Dockerfile:26 |
--min-release-age |
14 |
days |
Dockerfile.rootless:26 |
--min-release-age |
14 |
days |
docker/forkana/Dockerfile:25 |
--min-release-age |
14 |
days |
The npm/pnpm unit difference (days vs. minutes) is a well-known footgun. If the policy changes to, say, 21 days, all five locations must be updated, and pnpm-workspace.yaml requires a non-trivial conversion (21 * 24 * 60 = 30240). The Dockerfile comments already cross-reference pnpm-workspace.yaml, but .npmrc and pnpm-workspace.yaml themselves do not cross-reference each other.
Suggestion: add a cross-reference comment in .npmrc and ensure the pnpm-workspace.yaml comment (which already shows the math) is the canonical source:
# .npmrc — keep in sync with pnpm-workspace.yaml:minimumReleaseAge (which uses
# minutes; npm uses days). The canonical policy value is 14 days.
min-release-age=143. 🟡 Removing packageManager from package.json reduces build reproducibility
- Addressed
- Dismissed
The diff removes:
"packageManager": "pnpm@10.34.3",This was the single source of truth for the exact pnpm version. Its removal means:
- Corepack users lose automatic provisioning —
corepack enableno longer knows which pnpm to install. - CI reproducibility degrades:
version: "^10.16.0"in the workflows (e.g.,.github/workflows/pull-compliance.yml:40) is a floating range that resolves to whatever the latest 10.x is on the day CI runs. A lockfile generated with pnpm 10.34.3 today could be installed with pnpm 10.40.0 next month. engines.pnpm: ">= 10.16.0"is a minimum constraint, not a pin — it allows pnpm 11.x, yet the CI range^10.16.0caps at< 11.0.0, creating a silent mismatch.
The packageManager field and engines.pnpm serve complementary purposes (exact pin vs. compatibility range) and are not mutually exclusive. Consider restoring it:
"packageManager": "pnpm@10.34.3",
"engines": {
"node": ">= 22.6.0",
"pnpm": ">= 10.16.0"
},If the intent is to avoid pinning a version newer than the 14-day age gate allows, pin a version that is already > 14 days old (10.34.3 qualifies).
4. ⚪️ The crush workflow's npm install -g @charmland/crush may silently bypass min-release-age
- Addressed
- Dismissed
The .npmrc:6-7 comment cites @charmland/crush as a beneficiary of this file's npm settings. However, .github/workflows/pull-review-crush.yml does not use actions/setup-node — it relies on whatever Node.js/npm ubuntu-latest ships on PATH (.github/workflows/pull-review-crush.yml:424):
npm install -g @charmland/crushIf the runner image's default npm is < 11.10.0 (e.g., Node 22.x bundles npm 10.x), min-release-age=14 is silently ignored, and the crush install receives no age-gating — contradicting the security intent. The current ubuntu-latest likely ships Node 24.x with npm ≥ 11.10.0, but this is implicit and can change without notice.
Suggestion: add an explicit setup-node step to guarantee npm ≥ 11.10.0:
- uses: actions/setup-node@v5
with:
node-version: 24
- run: npm install -g @charmland/crush5. ⚪️ .npmrc comment lists Dockerfile pnpm bootstrap as a consumer of this file, but it isn't
- Addressed
- Dismissed
.npmrc:5 states:
# - Bootstrapping pnpm itself (e.g. `npm install -g --min-release-age=14 "pnpm@^10.16.0"` in the Dockerfile)
But the Dockerfile comment itself (Dockerfile:18-20) explains that the repo .npmrc is not yet copied at that point, which is exactly why the --min-release-age=14 flag is passed explicitly. Listing the Dockerfile bootstrap under "this file contains npm settings used for..." is misleading — that particular npm invocation does not read this .npmrc. Consider moving the Dockerfile example to a separate note clarifying it uses the CLI flag instead of this file.
Review generated using glm-5.2 via Z.AI. Comment /review to re-run.
Advanced AI Review
Click to expand review1. 🟡
|
Fixes #176
AI Disclosure
Co-authored with: Fable 5