Added CI guard against pnpm patched dependencies#28014
Conversation
`pnpm patch` is incompatible with Ghost's production build: Dockerfile.production installs from the packed ghost/core archive, which does not include the repo-root patches/ directory, so `pnpm install --prod` fails with ENOENT on the patch file. #28009 shipped such a patch and broke Build & Publish on main. Adds a check to the Lint job that fails if `patchedDependencies` is declared in either package.json or pnpm-workspace.yaml — giving fast feedback (~seconds) instead of a ~7-minute Docker build failure, and making "don't use pnpm patch" an enforced rule rather than a convention.
WalkthroughThis PR adds a Node.js CI guard script that fails the pipeline if Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/scripts/check-no-patched-dependencies.js (1)
27-41: 💤 Low valueConsider checking additional lines for more robust block-entry detection.
The current logic checks only
lines[idx + 1]for block entries. This could miss cases where an empty line appears betweenpatchedDependencies:and its first entry:patchedDependencies: foo@1.0.0: patches/foo.patchGiven the intentional trade-off (lightweight text scan vs. YAML parser dependency) and the low likelihood of this formatting in auto-generated patches, this is acceptable. However, if you want to strengthen the detection, consider checking a few lines ahead:
Optional enhancement to detect block entries across empty lines
const workspaceFile = path.join(repoRoot, 'pnpm-workspace.yaml'); if (fs.existsSync(workspaceFile)) { const lines = fs.readFileSync(workspaceFile, 'utf8').split('\n'); const idx = lines.findIndex(line => /^patchedDependencies:/.test(line)); if (idx !== -1) { const inline = lines[idx].slice('patchedDependencies:'.length).trim(); const hasInlineEntries = inline !== '' && inline !== '{}'; - const hasBlockEntries = /^\s+\S/.test(lines[idx + 1] || ''); + // Check next few lines to handle empty lines or comments + const hasBlockEntries = lines.slice(idx + 1, idx + 4).some(line => + /^\s+[^#\s]/.test(line) // Indented non-comment line + ); if (hasInlineEntries || hasBlockEntries) { offenders.push('pnpm-workspace.yaml -> patchedDependencies'); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/check-no-patched-dependencies.js around lines 27 - 41, The block-entry detection for patchedDependencies only inspects lines[idx + 1], so it misses cases with one or more blank lines after the header; update the check around patchedDependencies to scan forward a few lines (e.g., loop from idx+1 to idx+N) skipping blank lines and test each for a non-indented entry, replacing the single-line check that sets hasBlockEntries; refer to the existing variables/workflow (workspaceFile, lines, idx, hasInlineEntries, hasBlockEntries, offenders) and stop the scan early if you hit another top-level key or end of file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/scripts/check-no-patched-dependencies.js:
- Around line 27-41: The block-entry detection for patchedDependencies only
inspects lines[idx + 1], so it misses cases with one or more blank lines after
the header; update the check around patchedDependencies to scan forward a few
lines (e.g., loop from idx+1 to idx+N) skipping blank lines and test each for a
non-indented entry, replacing the single-line check that sets hasBlockEntries;
refer to the existing variables/workflow (workspaceFile, lines, idx,
hasInlineEntries, hasBlockEntries, offenders) and stop the scan early if you hit
another top-level key or end of file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4dca963-1ef2-426b-a359-55eaa6dd1cfc
📒 Files selected for processing (2)
.github/scripts/check-no-patched-dependencies.js.github/workflows/ci.yml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #28014 +/- ##
=======================================
Coverage 73.83% 73.83%
=======================================
Files 1523 1523
Lines 128982 128982
Branches 15483 15483
=======================================
Hits 95233 95233
Misses 32787 32787
Partials 962 962
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
Adds a CI check that fails if the repo declares pnpm
patchedDependencies, preventing a recurrence of the broken production build fixed in #28011 (introduced by #28009).pnpm patchis structurally incompatible with Ghost's production build:Dockerfile.productioninstalls from the packedghost/corearchive, which doesn't include the repo-rootpatches/directory — sopnpm install --prodaborts withENOENTon the patch file. There's no "correct" way to use it, so the check is a flat hard-fail:pnpm patchis not supported in this repo.What it does
.github/scripts/check-no-patched-dependencies.js— fails ifpatchedDependenciesis declared in eitherpackage.json(pnpm.patchedDependencies) orpnpm-workspace.yaml(pnpm 10 allows both).pnpm install, so it fails in seconds rather than after a ~7-minute Docker build.patchedDependencies— that's the only pnpm config field that references external files.overrides,packageExtensions, andonlyBuiltDependenciesare pure in-package.jsonconfig and install fine in the prod image.Complementary action (not code)
The other half of preventing this — making Build & Publish Artifacts a required status check on
main— is a branch-protection setting, not a code change. #28009 merged red because that job's failure only caused its dependent E2E jobs to skip, and skipped required checks count as passing. That has to be set in repo Settings → Branches.