chore: apply npm supply-chain best practices#382
Merged
Conversation
Apply the relevant controls from lirantal/npm-security-best-practices as enforced config plus a vitest gate that fails CI if a practice regresses. Future agents can't silently weaken these settings. Config: - Bump packageManager: pnpm@10.14.0 → 10.33.2 (needed for blockExoticSubdeps). - pnpm-workspace.yaml: minimumReleaseAge: 10080 (7-day cooldown before new package versions are eligible), blockExoticSubdeps: true (forbids git/tarball transitive deps). - .npmrc: pin default + @cipherstash registry to npmjs.org. Auth tokens stay in user-level ~/.npmrc or env vars. - .github/dependabot.yml: 7-day cooldown for minor/patch, 14-day for majors, grouped dev + @types/* updates, covers both npm and github-actions. - .github/CODEOWNERS: @cipherstash/developers owns supply-chain critical paths (workflows, dependabot.yml, pnpm-workspace.yaml, pnpm-lock.yaml, .npmrc, the new skill). - .github/workflows/tests.yml: Node 20 → 22, pnpm install → pnpm install --frozen-lockfile in both jobs. Test gate (e2e/tests/supply-chain.e2e.test.ts, 12 cases): asserts each invariant above plus that every pnpm-lock.yaml resolution points at registry.npmjs.org (substitutes for lockfile-lint, which doesn't support pnpm-lock.yaml). Negative- control verified — breaking each practice produces a clean failure naming the regression. Docs: - New skills/stash-supply-chain-security/SKILL.md with the full guide (enforced / documented / deferred). - AGENTS.md gains a "Supply Chain Security" section pointing at the skill and lists the new skill in Repository Layout. Practices 11 (provenance attestations) and 12 (OIDC trusted publishing) are deferred — they need npmjs.com-side Trusted Publisher configuration before release.yml can drop NPM_TOKEN. Tracked in a separate issue. Source: https://github.com/lirantal/npm-security-best-practices
🦋 Changeset detectedLatest commit: 33e19a6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds repository-level supply-chain security controls: CODEOWNERS, Dependabot config, pnpm workspace policy, .npmrc registry pinning, CI changes (Node 22 + frozen-lockfile), repository packageManager bump, new supply-chain E2E tests, and documentation/skill material describing and enforcing these controls. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Repo as Repository
participant Dependabot as Dependabot
participant CI as CI (GitHub Actions)
participant Registry as npm Registry
Dev->>Repo: Push PR (code / deps)
Repo->>CI: Trigger tests/workflows
CI->>CI: Run `pnpm install --frozen-lockfile` (Node 22)
CI->>Repo: Report pass/fail
Dependabot->>Repo: Open scheduled update PRs (npm / actions)
Repo->>CI: Dependabot PR triggers same CI checks
CI->>Registry: Fetch packages from https://registry.npmjs.org/
CI->>Repo: Fail if lockfile exotic subdeps or CI/frozen-lockfile checks fail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 32 minutes and 53 seconds.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: 3
🧹 Nitpick comments (1)
e2e/tests/supply-chain.e2e.test.ts (1)
91-93: ⚡ Quick winMake install-step detection resilient to pnpm flags before
install.The current matcher only catches
pnpm install. Variants likepnpm --filter ... installwon’t be checked.Suggested refactor
const installSteps = job.steps.filter( - (s) => typeof s.run === 'string' && /\bpnpm\s+install\b/.test(s.run), + (s) => typeof s.run === 'string' && /\bpnpm\b[^\n]*\binstall\b/.test(s.run), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/supply-chain.e2e.test.ts` around lines 91 - 93, The install-step filter only matches the exact "pnpm install" token; update the predicate used in installSteps (the job.steps.filter callback) to use a more flexible regex that allows flags/args before install, e.g. replace /\bpnpm\s+install\b/ with a pattern like /\bpnpm\b(?:\s+\S+)*\s+install\b/ so commands such as "pnpm --filter=... install" or "pnpm -w install" are recognized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/supply-chain.e2e.test.ts`:
- Around line 100-105: The test currently skips jobs missing an
actions/setup-node step; update the test in the "every job runs on Node 22"
block to first detect whether a job runs pnpm (scan job.steps for run commands
containing 'pnpm' or uses of pnpm actions) and if so assert that a setup step
exists (the found variable `setup` must be truthy) and then assert that
setup.with?.['node-version'] === '22'; for jobs that don't run pnpm you may
still skip the Node check as before but do not use a blind continue for
pnpm-using jobs—fail the test when a pnpm job lacks `actions/setup-node` and
ensure the assertion references `workflow.jobs`, `job.steps`, and the `setup`
variable.
- Around line 123-124: Update the test "github-actions ecosystem is also
covered" so it doesn't just assert existence of an update with package-ecosystem
=== 'github-actions' but also asserts that the matched update enforces the
cooldown policy: locate the update via db.updates.find(u =>
u['package-ecosystem'] === 'github-actions') and then assert the cooldown is
applied (for your implementation check the update's cooldown flag or timestamp
field — e.g. expect(u.cooldown).toBe(true) or
expect(u.cooldownUntil).toBeGreaterThan(Date.now()) — whichever field your
cooldown implementation uses).
- Around line 129-140: The test currently checks substring presence in
CODEOWNERS via the `co` variable and `toContain(path)` which can match comments
or unrelated entries; instead parse CODEOWNERS lines (from `co`), locate the
rule line that applies to each path in the array (match exact path or path
prefix like 'path/' rules), extract the owner list from that rule, and assert
that the owner list includes the expected owner string
`@cipherstash/developers`; update the test inside the `it('protects supply-chain
critical paths'...)` block to perform this owner identity check for each `path`
rather than using `toContain(path)`.
---
Nitpick comments:
In `@e2e/tests/supply-chain.e2e.test.ts`:
- Around line 91-93: The install-step filter only matches the exact "pnpm
install" token; update the predicate used in installSteps (the job.steps.filter
callback) to use a more flexible regex that allows flags/args before install,
e.g. replace /\bpnpm\s+install\b/ with a pattern like
/\bpnpm\b(?:\s+\S+)*\s+install\b/ so commands such as "pnpm --filter=...
install" or "pnpm -w install" are recognized.
🪄 Autofix (Beta)
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
Run ID: 17894716-c1cd-4f24-abe9-f2ca57a7c647
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.github/CODEOWNERS.github/dependabot.yml.github/workflows/tests.yml.npmrcAGENTS.mde2e/package.jsone2e/tests/supply-chain.e2e.test.tspackage.jsonpnpm-workspace.yamlskills/stash-supply-chain-security/SKILL.md
Address review feedback on e2e/tests/supply-chain.e2e.test.ts. Each finding passed against the current (correct) config but would let real regressions slip through: - Node 22 check silently skipped jobs without actions/setup-node. Now: detect pnpm-using jobs (uses pnpm/action-setup or runs `pnpm`) and require both setup-node and node-version: '22'. - github-actions ecosystem was only checked for existence in dependabot.yml. Now: mirror the npm assertion and require cooldown.default-days >= 3. - CODEOWNERS substring match could be satisfied by comment-only mentions (e.g. `# pnpm-workspace.yaml is critical`). Now: strip blank/comment lines, find a rule containing each path, and assert the rule's owner column includes @cipherstash/developers. - Install regex /\bpnpm\s+install\b/ missed `pnpm --filter=foo install` and `pnpm -w install`. Now: allow flag tokens between `pnpm` and `install` while avoiding false matches like `pnpm run install-x`. Each tightening was verified by breaking the relevant control and confirming the test fails naming the regression. 12/12 still pass against the unchanged config.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e/tests/supply-chain.e2e.test.ts (1)
151-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCritical CODEOWNERS path coverage is incomplete.
Line 151’s required path list omits
/.github/CODEOWNERSand/skills/stash-supply-chain-security/, so ownership regressions there won’t fail this gate.Suggested patch
for (const path of [ - 'pnpm-workspace.yaml', - 'pnpm-lock.yaml', - 'dependabot.yml', - '.npmrc', - '.github/workflows/', + '/pnpm-workspace.yaml', + '/pnpm-lock.yaml', + '/.github/dependabot.yml', + '/.npmrc', + '/.github/workflows/', + '/.github/CODEOWNERS', + '/skills/stash-supply-chain-security/', ]) {As per coding guidelines, "Validate supply chain controls against the e2e/tests/supply-chain.e2e.test.ts test to prevent silent regressions in CI."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/supply-chain.e2e.test.ts` around lines 151 - 157, The path list in the for loop inside the supply-chain test (for (const path of [...]) in e2e/tests/supply-chain.e2e.test.ts) is missing critical entries; update that array to include '/.github/CODEOWNERS' and '/skills/stash-supply-chain-security/' so the test will fail when ownership changes touch those locations, preserving the existing format and separators used in the list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/supply-chain.e2e.test.ts`:
- Around line 28-31: The test 'pnpm-workspace.yaml sets minimumReleaseAge ≥ 3
days' currently asserts ws.minimumReleaseAge ≥ 4320 (3 days) which is weaker
than the intended 7-day policy; update the expectation in the test (the it block
and its assertion referencing ws.minimumReleaseAge and the numeric literal 4320)
to assert the configured 7-day minimum (10080) so CI enforces the correct
supply-chain control.
---
Duplicate comments:
In `@e2e/tests/supply-chain.e2e.test.ts`:
- Around line 151-157: The path list in the for loop inside the supply-chain
test (for (const path of [...]) in e2e/tests/supply-chain.e2e.test.ts) is
missing critical entries; update that array to include '/.github/CODEOWNERS' and
'/skills/stash-supply-chain-security/' so the test will fail when ownership
changes touch those locations, preserving the existing format and separators
used in the list.
🪄 Autofix (Beta)
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
Run ID: cd5e90a5-4645-4fe6-8c1d-7a2228691c8c
📒 Files selected for processing (1)
e2e/tests/supply-chain.e2e.test.ts
- Bump cooldown threshold from 4320 (3 days) to 10080 (7 days) in test, to match the policy - Add CODEOWNERS and skills to the CODEOWNERS path list, to ensure review coverage
calvinbrewer
approved these changes
May 1, 2026
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
2 participants
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.
Apply the relevant controls from lirantal/npm-security-best-practices as enforced config, plus a vitest gate that fails CI if a practice regresses. Future agents can't silently weaken these settings.
Config:
pnpmfrom10.14.0to10.33.2(needed forblockExoticSubdeps).blockExoticSubdeps)dependabot.yml,pnpm-workspace.yaml,pnpm-lock.yaml,.npmrc, the new skill).Docs:
Tests:
Summary by CodeRabbit
New Features
Chores
Security / Maintenance
Documentation