Add npm release publishing flow#3
Conversation
|
Warning Review limit reached
More reviews will be available in 14 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds a complete npm publishing pipeline for distributing the Firefox CLI as an npm package with per-platform native binary sub-packages. It introduces platform target infrastructure, multi-platform binary building, npm package assembly with validation, publishing orchestration, and GitHub Actions CI/CD automation. ChangesNPM Publishing Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. 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
🧹 Nitpick comments (3)
scripts/platform-targets.ts (1)
19-24: 💤 Low valueConsider caching
getPlatformKey(target)to avoid duplicate calls.Line 21 and line 23 both call
getPlatformKey(target). While this is not a performance concern in practice (the array is small and computed once at module load), storing the result would be more efficient.♻️ Optional refactor
export const supportedBinaryTargets: readonly SupportedBinaryTarget[] = targetInputs.map((target) => { + const platformKey = getPlatformKey(target); return { ...target, - platformKey: getPlatformKey(target), + platformKey, binaryName: getBinaryName(target), - npmPackageName: `@firefox-cli/native-${getPlatformKey(target)}`, + npmPackageName: `@firefox-cli/native-${platformKey}`, }; });🤖 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 `@scripts/platform-targets.ts` around lines 19 - 24, The map callback for supportedBinaryTargets calls getPlatformKey(target) twice; cache the result in a local variable (e.g., const platformKey = getPlatformKey(target)) inside the map function and reuse that variable for both platformKey and the npmPackageName construction, leaving getBinaryName(target) as-is; update the object properties in the supportedBinaryTargets initializer to reference the cached platformKey..github/workflows/release.yml (1)
189-216: ⚖️ Poor tradeoffReview static analysis warnings for the npm-publish job.
Several static analysis warnings were flagged:
Unpinned action references (lines 201, 202, 205, 212): Actions use version tags rather than commit SHAs. While this is common practice and acceptable for many projects, consider pinning to SHAs if your security policy requires it.
Cache poisoning (lines 202, 205):
setup-bunandsetup-nodeenable caching by default. This is generally safe for official actions from trusted organizations (oven-sh, actions), but be aware of the risk if your threat model requires cache isolation.Credential persistence (line 201): The
artipackedwarning is likely a false positive sinceactions/checkout@v6does not persist credentials to downloaded artifacts.These are policy decisions rather than code defects. If your project's security requirements mandate commit-SHA pinning and cache isolation, address these warnings; otherwise they can be accepted as known trade-offs.
🤖 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/workflows/release.yml around lines 189 - 216, The pipeline job npm-publish triggers static-analysis flags about unpinned action versions, caching, and credential persistence; decide and implement your security policy: if you require immutability, pin each action in the npm-publish steps (actions/checkout, oven-sh/setup-bun, actions/setup-node, actions/download-artifact) to specific commit SHAs instead of tags, and disable cache options for setup-bun and setup-node (or add explicit cache-isolation settings) to avoid cache poisoning; if the warnings are acceptable, add a short justification comment in the workflow near the job explaining why tag versions and caching are permitted and note that actions/checkout does not persist credentials to downloaded artifacts.scripts/npm-publish.ts (1)
80-94: 💤 Low valueConsider extracting
readOptionto a shared utility.This function is duplicated between
build-binary.tsandnpm-publish.ts. While the duplication is not a serious issue, extracting it to a shared module would improve maintainability.🤖 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 `@scripts/npm-publish.ts` around lines 80 - 94, The readOption function is duplicated; extract it to a shared utility and import it from both scripts. Create a new module (e.g., export function readOption(args: readonly string[], name: string): string | undefined) and move the implementation there, then replace the local readOption definitions in npm-publish.ts and build-binary.ts with imports of the shared readOption to avoid duplication and keep behavior identical.
🤖 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 @.github/workflows/release.yml:
- Around line 218-229: The gh release edit step in the publish-github-release
job uses an unquoted template expansion ${ { github.ref_name } } in a shell
command; to fix, ensure the value is properly quoted or passed via an
environment variable: update the gh release edit invocation in the
publish-github-release step to quote the template expansion (e.g., use "${{
github.ref_name }}") or set GITHUB_REF_NAME=${{ github.ref_name }} in env and
reference that variable, keeping GH_TOKEN handling the same so shell injection
is prevented.
In `@packages/cli/src/npm-platform-binary-runtime.js`:
- Around line 22-30: resolvePackagedBinary currently ignores the packageRoot
argument which can cause resolution to happen from the wrong module tree; change
resolution to use a createRequire rooted at path.join(packageRoot,
'package.json') (import createRequire from 'module') and call
rootRequire.resolve instead of the global require.resolve when locating
`${packageName}/package.json`; keep getPlatformKey usage and set packageJsonPath
from rootRequire.resolve, falling back to global require only if packageRoot is
not provided.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 189-216: The pipeline job npm-publish triggers static-analysis
flags about unpinned action versions, caching, and credential persistence;
decide and implement your security policy: if you require immutability, pin each
action in the npm-publish steps (actions/checkout, oven-sh/setup-bun,
actions/setup-node, actions/download-artifact) to specific commit SHAs instead
of tags, and disable cache options for setup-bun and setup-node (or add explicit
cache-isolation settings) to avoid cache poisoning; if the warnings are
acceptable, add a short justification comment in the workflow near the job
explaining why tag versions and caching are permitted and note that
actions/checkout does not persist credentials to downloaded artifacts.
In `@scripts/npm-publish.ts`:
- Around line 80-94: The readOption function is duplicated; extract it to a
shared utility and import it from both scripts. Create a new module (e.g.,
export function readOption(args: readonly string[], name: string): string |
undefined) and move the implementation there, then replace the local readOption
definitions in npm-publish.ts and build-binary.ts with imports of the shared
readOption to avoid duplication and keep behavior identical.
In `@scripts/platform-targets.ts`:
- Around line 19-24: The map callback for supportedBinaryTargets calls
getPlatformKey(target) twice; cache the result in a local variable (e.g., const
platformKey = getPlatformKey(target)) inside the map function and reuse that
variable for both platformKey and the npmPackageName construction, leaving
getBinaryName(target) as-is; update the object properties in the
supportedBinaryTargets initializer to reference the cached platformKey.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f31c8dd6-d768-435a-af8c-edf65321acec
📒 Files selected for processing (15)
.github/workflows/release.ymldocs/development.mdpackage.jsonpackages/cli/src/npm-platform-binary-runtime.jsscripts/build-all-binaries.tsscripts/build-binary.tsscripts/check-firefox-architecture-policy.test.mjsscripts/npm-package-check.tsscripts/npm-package.tsscripts/npm-publish.tsscripts/package-check.tsscripts/package.tsscripts/platform-targets.tsscripts/test/npm-publish.test.tsscripts/test/package-check-test-utils.ts
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Documentation
Tests
Chores