chore(deps): declare pnpm overrides only in pnpm-workspace.yaml - #1816
chore(deps): declare pnpm overrides only in pnpm-workspace.yaml#1816clay-good wants to merge 1 commit into
Conversation
The security overrides were declared twice: in pnpm-workspace.yaml, with the advisory comments explaining each pin, and again under package.json's pnpm.overrides. The copies are not additive — pnpm 10 uses package.json's block instead of the workspace list when both are present — and Dependabot rewrites plain-name entries in package.json whenever it bumps the same package. So a routine bump silently displaces the pins that patch advisories, and fails the equality test that guards them (#1812). Keeps one declaration, in the file that carries the reasoning, and asserts the mirror stays gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change removes duplicate Changespnpm override configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Security overrides are now single-sourced in each workspace configuration, avoiding package-level override precedence and Dependabot drift while preserving the recorded dependency resolutions. The change is ready to merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
Deploying openspec-docs with
|
| Latest commit: |
b0d9c16
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f431b55b.openspec-docs.pages.dev |
| Branch Preview URL: | https://chore-single-source-pnpm-ove.openspec-docs.pages.dev |
No PR-relevant drift confirmed.
|
|
Independently verified both halves of this, with the repo's own pnpm (10.34.5), since the claim is a security one and the PR ships no lockfile change to demonstrate it. 1. Removing the mirrors changes nothing. # pnpm-lock.yaml
overrides:
brace-expansion@<=5.0.8: '>=5.0.9 <6'
postcss@<8.5.23: '>=8.5.23 <9'
js-yaml@>=3.0.0 <3.15.1: '>=3.15.1 <4'
js-yaml@>=4.0.0 <4.3.1: '>=4.3.1 <5'
nanoid@<3.3.17: '>=3.3.17 <4'# website/pnpm-lock.yaml
overrides:
postcss: ^8.5.26
sharp: ^0.35.3
brace-expansion@<=5.0.8: '>=5.0.9 <6'
fast-uri@<3.1.6: ^3.1.6
nanoid@<3.3.17: '>=3.3.17 <4'2. The replace-not-merge behavior is real, and worse than "drift". I reproduced the hazard directly: added a single overrides:
postcss: ^8.5.26Four advisory pins gone from resolution: both The guard is the right shape too: asserting The 3 workspace-config tests pass, and the branch is current with |
alfred-openspec
left a comment
There was a problem hiding this comment.
Approved at exact head b0d9c16a4903835cdcb264c0534b6dc53116a136.
The single-source guard matches pnpm 10 precedence: a package-level pnpm.overrides block would replace, not merge with, the workspace pins. I independently confirmed that lockfile-only installs leave both lockfiles unchanged and that the focused workspace-config suite passes 3/3. Hosted CI and security checks are green.
The adjacent version-qualified allowBuilds/esbuild drift risk is real but separate from this fix.
Status: ready for review. Follow-up to #1815 — removes the cause of the failure that PR patched by hand.
What was wrong: the pnpm security overrides were declared twice — in
pnpm-workspace.yaml, where each pin carries an inline advisory comment, and again underpnpm.overridesinpackage.json(root andwebsite/). Dependabot rewrites plain-name entries inpackage.jsonwhen it bumps the same package. #1812 did exactly that (postcss: ^8.5.26→^8.5.28), leftpnpm-workspace.yamluntouched, and failedTeston all three platforms against the equality assertion intest/pnpm-workspace-config.test.ts.The duplicate is worse than redundant. The two blocks are not additive: when
package.jsondeclarespnpm.overrides, pnpm 10 uses that list instead of the workspace file's. Verified on this repo's manifests with pnpm 10.34.5 — replacing the block with a single unrelated entry produced a lockfile whoseoverrides:contained only that entry, silently dropping every advisory pin (brace-expansion, js-yaml, nanoid, postcss). So a Dependabot rewrite of the mirror does not just drift from the real pins, it displaces them; today only the equality test stands between that and a quietly unpatched dependency graph.How it was fixed: deleted the
pnpm.overridesblock from bothpackage.jsonfiles.pnpm-workspace.yaml— the file that already documents why each pin exists — is now the single declaration, and the test asserts the mirror stays gone instead of asserting the two copies match..github/dependabot.yml's comment about the surfaces Dependabot doesn't manage is corrected: it claimed Dependabot never bumps these, which #1812 disproved.Proof it works:
pnpm install --frozen-lockfileat the root and--dir websiteboth succeed with zero lockfile drift, which is the direct evidence that pnpm was already resolving these overrides from the workspace file and nothing about the installed graph changes.pnpm.overridesblock topackage.jsonfails the root test, and removing it passes (3/3).config-profile,artifact-workflowCursor skills, 2workset10s timeouts) reproduce on unmodifiedmainin this sandbox and touch none of these files.Notes / nits:
packageManagerpinspnpm@10.34.5,pnpm/action-setupreads it, and the Nix build usespnpm_10— so no supported install path loses the pins. Anyone on pnpm 9 would have been getting thepackage.jsoncopy alone, which was already the drifting one.allowBuildsin both workspace files pins an exactesbuild@<version>that the same test derives from the lockfile, so a transitive esbuild bump will break the suite the same way. Worth a separate look — the fix depends on whether pnpm accepts an unversionedallowBuildskey, which I did not want to change blind.🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Documentation
Tests