ci(security): reusable pnpm-overrides sanity check (pnpm 10 silently ignores package.json overrides) - #9
Conversation
pnpm 10 no longer reads the `pnpm` field from package.json. It warns "The pnpm field in package.json is no longer read by pnpm" and then IGNORES pnpm.overrides / pnpm.patchedDependencies. The failure mode is silent and security-relevant: a security override declared in package.json under pnpm 10 looks committed and reviewed, resolves nothing, and the advisory stays open. That is exactly how critical tar GHSA-23hp-3jrh-7fpw survived repeated dependency bumps in hypest-intelligence-hub and hypest-dashboard (found 2026-08-12, both had tar 7.5.1 pinned "to >=7.5.19"). This reusable workflow: 1. fails when packageManager pins pnpm >= 10 while package.json still declares overrides / patchedDependencies / peerDependencyRules / packageExtensions, naming the exact dead keys and where to move them; 2. parses the overrides block in pnpm-workspace.yaml and asserts each one is actually reflected in pnpm-lock.yaml, so a declared-but-unapplied override (stale lockfile) also fails. Verified both directions before commit: - positive: real hypest-intelligence-hub lockfile -> tar 7.5.22 vs override >=7.5.19 and tailwindcss>nanoid 3.3.11 vs 3.3.7 both read correctly - negative control: synthetic workspace pinning >=7.5.19 against a lockfile holding tar@7.5.1 -> checker correctly fails (it is not a no-op that always passes)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a reusable GitHub Actions workflow with a configurable working directory. The workflow validates pnpm configuration and verifies workspace overrides against lockfile versions. Changespnpm override validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant Repository
participant OverrideValidator
GitHubActions->>Repository: Check out repository
GitHubActions->>Repository: Read package.json and pnpm-workspace.yaml
GitHubActions->>OverrideValidator: Parse overrides and pnpm-lock.yaml
OverrideValidator->>Repository: Resolve override targets
OverrideValidator->>GitHubActions: Report validation result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@workflow-templates/pnpm-overrides-sanity.yml`:
- Around line 128-138: Update the override-validation logic around the visible
spec parsing and comparison so every unsupported override form—including exact,
caret, tilde, alias, catalog, and removal specifications—is recorded as an
explicit failure rather than treated as valid. Preserve the existing version
comparison for supported >= specifications, and append a clear
unsupported-specification error for all other non-evaluable values.
- Around line 119-124: The scoped override validation around target extraction
must verify the direct parent-to-child dependency edge rather than searching for
the child as a top-level key. Update the lockfile parsing used by the sanity
check to handle quoted scoped package keys, locate the specified parent’s
dependency data, and fail the check when the selected edge cannot be confirmed
instead of continuing as optional.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b510d93-6cb4-4fe3-9476-558c3e318090
📒 Files selected for processing (1)
workflow-templates/pnpm-overrides-sanity.yml
Viktor Didkovskyi (viktor958)
left a comment
Viktor Didkovskyi (viktor958)
left a comment
There was a problem hiding this comment.
Full review at 2d3ac905. The idea is right and the org needs this gate, but as written it reports success without checking the only override that exists in the org, so I would not merge it yet.
Both open threads are real. I verified each by executing the patterns rather than reading them, and replied inline with the evidence.
The more serious one is the version comparison. re.match(r">=\s*([0-9][0-9.]*)", spec) is the only check, and a non-match falls off the end of the loop — nothing appended to failures, nothing logged. So every spec shape other than >= passes silently: bare pins, carets, tildes, npm: aliases, catalog:, file:.
That is not hypothetical. healify-web is the only repo in the org with a pnpm overrides block, and it holds exactly one entry, "tailwindcss>nanoid": 3.3.7. An exact pin. The step finds nanoid in the lockfile, prints the resolved version, skips the comparison, and prints OK: all declared overrides are reflected in pnpm-lock.yaml. Today this workflow would go green having verified nothing on its only consumer — and an exact pin that silently failed to apply is precisely what a supply-chain override guard is for. Pinned-for-security overrides are usually exact or caret, not >=, so I would treat those as the primary shape.
The second thread is narrower but the same failure philosophy. The lockfile lookup requires the two-space indent to be followed immediately by the package name, but pnpm-lock v9 quotes scoped keys ( '@babel/core@7.24.0':), so scoped targets never match, land on the continue branch, and print "may be optional". No scoped override exists in the org right now, so this is latent rather than live — but onlyBuiltDependencies in that same file is mostly scoped packages, so the first scoped override anyone adds will be silently unverified. Worth fixing while it is cheap. (One correction to the bot's analysis: parent selectors are fine — tailwindcss>nanoid reduces to the unquoted key nanoid and matches. The break is specifically quoted scoped keys, including a scoped child in a parent selector.)
Underneath both is one design decision I would change: "target not found" and "target found and verified" both produce a green, silent pass. A guard whose miss path is indistinguishable from its success path will eventually be trusted for something it never did. Printing an explicit skip line for each unverifiable entry — and considering a non-zero exit under strict when nothing could be checked — would make the log honest about coverage.
Worth noting the contrast with #10 in this same repo, which gets this right: its test harness raises AssertionError if the named workflow step is missing, so a rename fails loudly instead of quietly testing nothing. Same instinct applied here would resolve most of the above.
Everything else about the PR is fine — the overrides parser handles quoting and comments, the early exits for a missing or empty block are sensible, and the remediation hint (pnpm install --lockfile-only) is the right one.
Do not merge as it stands. Fix the spec comparison so exact pins and carets are actually compared, and the gate becomes worth having; the scoped-key fix and an explicit skip line can ride along. Happy to re-review whichever head comes out of it.
… scoped lockfile keys, fail when nothing verifiable
|
Pushed
Verified by extracting the step's python and running it against 13 synthetic workspace/lockfile pairs (exact/caret/tilde/gte satisfied and violated, quoted scoped key, scoped child selector, npm alias, catalog, all-unverifiable) — all 13 matched expected exit codes. Ready for re-review. |
2 participants
Why
pnpm 10 no longer reads the
pnpmfield in package.json. It printsThe pnpm field in package.json is no longer read by pnpmand then ignorespnpm.overridesandpnpm.patchedDependencies.The failure is silent and security-relevant: a security override declared there
under pnpm 10 looks committed and reviewed, resolves nothing, and the advisory
stays open.
That is exactly how critical
tarGHSA-23hp-3jrh-7fpw survived repeateddependency bumps in
hypest-intelligence-hubandhypest-dashboard— both hadtarpinned">=7.5.19"while the lockfile happily kept resolving 7.5.1.carrier.llcis onpnpm@10.33.0and still carries apnpmfield, so this isnot a hypest-only footgun.
How
Reusable workflow with two independent checks:
packageManagerpins pnpm ≥ 10 whilepackage.jsonstill declaresoverrides,patchedDependencies,peerDependencyRulesorpackageExtensions. Error names the exact dead keysand where to move them.
overrides:block inpnpm-workspace.yamland asserts each entry is actually reflected inpnpm-lock.yaml. Catches the other half of the problem: override declared inthe right place but lockfile never re-resolved.
Handles scoped selectors (
tailwindcss>nanoidverifiesnanoid), skipsnon-pnpm repos, and no-ops cleanly when the files are absent.
How tested
Both directions, before commit:
hypest-intelligence-hublockfile:tar→7.5.22against override
>=7.5.19, andtailwindcss>nanoid→3.3.11against3.3.7, both parsed correctly>=7.5.19against alockfile holding
tar@7.5.1→ checker correctly failsThe negative control matters: it proves this is not a checker that always passes.
Not validated
with
carrier.llc, which is pnpm 10 with a livepnpmfield).here; an exotic anchored/multi-doc workspace file would need a real parser.
Summary by CodeRabbit
New Features
Tests
Chores