Conversation
…ating GitHub Release
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRelease workflow now scopes npm package names to Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant Git as Git
participant WF as Workflow script
participant NPM as npm registry
participant GHRel as GitHub Releases
Dev->>GH: push / tag triggers workflow
GH->>Git: fetch --tags
GH->>WF: read package.json version
WF->>Git: check for tag vX.Y.Z
alt tag exists
WF->>WF: run napi prepublish flow
WF->>GHRel: skip creating GitHub Release (release may already exist)
else no tag
WF->>WF: run prepublishOnly flow
WF->>GHRel: create GitHub Release after publish
end
GH->>WF: rewrite npm/* package names to `@rush-fs/` (pre-publish)
WF->>NPM: npm publish
GH->>WF: rescan & fix package names to `@rush-fs/` (post-prepublish)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/Release.yml (1)
158-173:⚠️ Potential issue | 🟠 Major
workflow_dispatchpath self-triggers a second run that will always fail to publish.When the workflow is triggered by
workflow_dispatchand the tag does not yet exist, theelsebranch callspnpm prepublishOnly(napi prepublish -t npm).napi prepublishcreates a GitHub Release, which in turn creates thev0.0.4git tag on the remote. That tag push matches thepush: tags: 'v*'trigger and immediately starts a second run of this same workflow.The second run:
git rev-parse "v0.0.4"succeeds → executes theifbranch → reachesnpm publish --access public→ fails with"You cannot publish over the previously published versions rush-fs@0.0.4".The two runs have different
github.refvalues (refs/heads/mainvsrefs/tags/v0.0.4), so the concurrency group does not cancel the second run.The simplest mitigation is to guard the final
npm publishwith an npm-registry check, or to skip the publish step entirely when the event is a tag push that matches a version already present on npm:💡 Proposed fix: skip publish if already on npm
+ VERSION=$(node -p "require('./package.json').version") + TAG="v$VERSION" if git rev-parse "$TAG" >/dev/null 2>&1; then echo "Tag $TAG already exists, skipping GitHub Release creation" pnpm napi prepublish -t npm --no-gh-release else echo "Tag $TAG does not exist, will create GitHub Release" pnpm prepublishOnly fi - npm publish --access public + if npm view "rush-fs@$VERSION" version >/dev/null 2>&1; then + echo "Version $VERSION already published to npm, skipping" + else + npm publish --access public + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml around lines 158 - 173, The publish step can run twice due to the workflow self-triggering via tag creation; modify the script around VERSION/TAG and the final npm publish to skip publishing when the package version is already published to npm (or when the event is a tag push whose version exists on npm). Concretely, before running npm publish --access public, query the registry for the package@${VERSION} (e.g., npm view <package-name>@$VERSION version or curl the registry) and if that version exists exit/skip publish; keep the existing git rev-parse TAG logic but add this npm-registry guard so the second tag-triggered run will not attempt to republish an already published version.
🧹 Nitpick comments (2)
.github/workflows/Release.yml (2)
150-157:optionalDependencieslist is hardcoded — will silently drift if platform matrix changes.The four scoped package names are hardcoded rather than derived from the
npm/*/package.jsonfiles that were just renamed. If a target is added or removed from thebuild.strategy.matrix(Lines 27–38), this block must also be updated manually or the root package will declare incorrect optional deps.♻️ Proposed fix: derive from the renamed npm dirs
- node -e " - const fs = require('fs'); - const p = JSON.parse(fs.readFileSync('package.json', 'utf8')); - const v = p.version; - p.optionalDependencies = { - '@rush-fs/rush-fs-win32-x64-msvc': v, - '@rush-fs/rush-fs-darwin-x64': v, - '@rush-fs/rush-fs-linux-x64-gnu': v, - '@rush-fs/rush-fs-darwin-arm64': v - }; - fs.writeFileSync('package.json', JSON.stringify(p, null, 2)); - " + node << 'EOF' + const fs = require('fs'); + const p = JSON.parse(fs.readFileSync('package.json', 'utf8')); + const v = p.version; + const dirs = fs.readdirSync('npm').filter(d => + fs.existsSync(`npm/${d}/package.json`) + ); + p.optionalDependencies = Object.fromEntries( + dirs.map(d => { + const name = JSON.parse(fs.readFileSync(`npm/${d}/package.json`, 'utf8')).name; + return [name, v]; + }) + ); + fs.writeFileSync('package.json', JSON.stringify(p, null, 2) + '\n'); +EOF🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml around lines 150 - 157, The optionalDependencies block is hardcoded (p.optionalDependencies = { ... }) and can drift when build.targets change; instead, read the renamed npm package dirs (the npm/* package.json files that were renamed earlier), extract each package's name and version, build the optionalDependencies object dynamically, assign it to p.optionalDependencies, and then write it out with fs.writeFileSync('package.json', JSON.stringify(p, null, 2)); locate the code around the p.optionalDependencies assignment to replace the static list with logic that enumerates the renamed npm directories, reads their package.json name/version, and populates p.optionalDependencies accordingly.
92-92:actions/upload-artifact@v6is available and recommended if self-hosted runners support it.
v6runs on Node.js 24 (compared to v5's Node.js 20 default). However, upgrading requires Actions Runner >= 2.327.1 for self-hosted runners; standard GitHub-hosted runners are unaffected. Thedownload-artifactaction is not used in this repository, so no complementary update is needed there. Consider upgrading when runner compatibility is confirmed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml at line 92, Update the GitHub Action step that references "actions/upload-artifact@v5" to use "actions/upload-artifact@v6" (i.e., replace the version tag in the uses string) once you have confirmed self-hosted runners meet Actions Runner >= 2.327.1; no changes to other steps are required because download-artifact isn't used in this repo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/Release.yml:
- Around line 120-134: The inline Node invocation embeds the shell variable $dir
directly into the node -e "...", which can break or be unsafe for directory
names with quotes/backslashes/newlines; change the invocation to pass the
directory path as a CLI argument and read it via process.argv inside the JS code
(e.g., keep the for loop and node -e but call node -e 'const fs=require("fs");
const pkgPath = process.argv[1] + "/package.json"; ...' "$dir"), update
references to pkgPath and ensure all filesystem reads/writes use that
argv-derived path so no shell interpolation occurs inside the JS string.
---
Outside diff comments:
In @.github/workflows/Release.yml:
- Around line 158-173: The publish step can run twice due to the workflow
self-triggering via tag creation; modify the script around VERSION/TAG and the
final npm publish to skip publishing when the package version is already
published to npm (or when the event is a tag push whose version exists on npm).
Concretely, before running npm publish --access public, query the registry for
the package@${VERSION} (e.g., npm view <package-name>@$VERSION version or curl
the registry) and if that version exists exit/skip publish; keep the existing
git rev-parse TAG logic but add this npm-registry guard so the second
tag-triggered run will not attempt to republish an already published version.
---
Nitpick comments:
In @.github/workflows/Release.yml:
- Around line 150-157: The optionalDependencies block is hardcoded
(p.optionalDependencies = { ... }) and can drift when build.targets change;
instead, read the renamed npm package dirs (the npm/* package.json files that
were renamed earlier), extract each package's name and version, build the
optionalDependencies object dynamically, assign it to p.optionalDependencies,
and then write it out with fs.writeFileSync('package.json', JSON.stringify(p,
null, 2)); locate the code around the p.optionalDependencies assignment to
replace the static list with logic that enumerates the renamed npm directories,
reads their package.json name/version, and populates p.optionalDependencies
accordingly.
- Line 92: Update the GitHub Action step that references
"actions/upload-artifact@v5" to use "actions/upload-artifact@v6" (i.e., replace
the version tag in the uses string) once you have confirmed self-hosted runners
meet Actions Runner >= 2.327.1; no changes to other steps are required because
download-artifact isn't used in this repo.
CoderSerio
force-pushed
the
feat/carbon-dev
branch
2 times, most recently
from
bd753f6 to
c1236ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/Release.yml (1)
161-193:⚠️ Potential issue | 🔴 CriticalCritical runtime breakage:
index.jsrequires unscoped platform package names, but packages are published with@rush-fs/scope.
index.jswas generated by napi-rs without a scoped package name override. It contains 40+require()calls using unscoped names (e.g.,require('rush-fs-darwin-x64')), but the Release.yml workflow explicitly renames all published packages to use the@rush-fs/scope (e.g.,@rush-fs/rush-fs-darwin-x64). When end-users install this package, npm will download optional dependencies under the scoped names, butindex.jswill attempt to require the unscoped names, which won't exist—causing a runtime failure.Add
"packageName": "@rush-fs/rush-fs"to thenapiconfiguration inpackage.jsonso that napi-rs generates the correct scoped require() calls inindex.js. Alternatively, regenerateindex.jsafter updating the package name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml around lines 161 - 193, index.js (generated by napi-rs) contains require() calls for unscoped native packages (e.g., require('rush-fs-darwin-x64')) while the release workflow in Release.yml renames published packages to the `@rush-fs/` scope, causing runtime crashes; fix by adding "packageName": "@rush-fs/rush-fs" to the napi config in package.json so napi-rs generates scoped require() calls, then regenerate index.js (re-run the build/generation step that produces index.js) so its require() targets match the `@rush-fs/`* package names (alternatively ensure post-prepublish step rewrites index.js requires to `@rush-fs/` names if you cannot change napi config).
🧹 Nitpick comments (1)
.github/workflows/Release.yml (1)
143-160: Duplicate package-name-fixing loop — extract to a reusable shell function.The identical
for dir in npm/*/loop withPKG_PATH+ inline Node appears twice (lines 143–160 and lines 177–192). Extract it into a function to avoid drift between the two copies.♻️ Proposed refactor — use a shell function
+ - name: Define package-name fix function and run initial fix + shell: bash + run: | + fix_npm_package_names() { + for dir in npm/*/; do + if [ -f "$dir/package.json" ]; then + PKG_PATH="$dir/package.json" + export PKG_PATH + node -e " + const fs = require('fs'); + const pkgPath = process.env.PKG_PATH; + const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); + if (!pkg.name.startsWith('@rush-fs/')) { + console.log('Fixing package name:', pkg.name, '-> `@rush-fs/`' + pkg.name); + pkg.name = '@rush-fs/' + pkg.name; + fs.writeFileSync(pkgPath, JSON.stringify(pkg, null, 2)); + } + " + fi + done + } + # Export so subsequent steps can source this file, or inline calls here: + fix_npm_package_names - - name: Update platform package names to use scope (`@rush-fs/xxx`) - run: | - for dir in npm/*/; do - ... - done - name: Publish to npm run: | ... - # prepublish 之后再次更新包名 - for dir in npm/*/; do - ... - done + fix_npm_package_names npm publish --access publicAlso applies to: 177-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Release.yml around lines 143 - 160, Extract the duplicated "for dir in npm/*/; do ... node -e ..." block into a reusable shell function (e.g., update_package_names) that encapsulates setting PKG_PATH, reading/writing package.json and the "@rush-fs/" name-prefix logic (preserve use of PKG_PATH, npm/*/ glob and the inline node -e behavior), declare this function once before both usages in the workflow run script, and replace both duplicated loops (the blocks that set PKG_PATH and call node -e) with calls to update_package_names so both places call the single shared implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/Release.yml:
- Around line 143-160: The console.log inside the node -e script logs the
"Updating package name from ... to `@rush-fs/`..." message unconditionally, which
can produce incorrect output when pkg.name is already scoped; move the
console.log so it only runs inside the if guard that checks
!pkg.name.startsWith('@rush-fs/'), ensuring the log and the writeFileSync only
execute when pkg.name is actually being changed (refer to the inline script's
pkg, pkgPath, pkg.name, the if (!pkg.name.startsWith('@rush-fs/')) block, and
the console.log statement).
---
Outside diff comments:
In @.github/workflows/Release.yml:
- Around line 161-193: index.js (generated by napi-rs) contains require() calls
for unscoped native packages (e.g., require('rush-fs-darwin-x64')) while the
release workflow in Release.yml renames published packages to the `@rush-fs/`
scope, causing runtime crashes; fix by adding "packageName": "@rush-fs/rush-fs"
to the napi config in package.json so napi-rs generates scoped require() calls,
then regenerate index.js (re-run the build/generation step that produces
index.js) so its require() targets match the `@rush-fs/`* package names
(alternatively ensure post-prepublish step rewrites index.js requires to
`@rush-fs/` names if you cannot change napi config).
---
Nitpick comments:
In @.github/workflows/Release.yml:
- Around line 143-160: Extract the duplicated "for dir in npm/*/; do ... node -e
..." block into a reusable shell function (e.g., update_package_names) that
encapsulates setting PKG_PATH, reading/writing package.json and the "@rush-fs/"
name-prefix logic (preserve use of PKG_PATH, npm/*/ glob and the inline node -e
behavior), declare this function once before both usages in the workflow run
script, and replace both duplicated loops (the blocks that set PKG_PATH and call
node -e) with calls to update_package_names so both places call the single
shared implementation.
| - name: Update platform package names to use scope (@rush-fs/xxx) | ||
| run: | | ||
| for dir in npm/*/; do | ||
| if [ -f "$dir/package.json" ]; then | ||
| PKG_PATH="$dir/package.json" | ||
| export PKG_PATH | ||
| node -e " | ||
| const fs = require('fs'); | ||
| const pkgPath = process.env.PKG_PATH; | ||
| const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); | ||
| console.log('Updating package name from', pkg.name, 'to @rush-fs/' + pkg.name); | ||
| if (!pkg.name.startsWith('@rush-fs/')) { | ||
| pkg.name = '@rush-fs/' + pkg.name; | ||
| fs.writeFileSync(pkgPath, JSON.stringify(pkg, null, 2)); | ||
| } | ||
| " | ||
| fi | ||
| done |
There was a problem hiding this comment.
Misleading log fires unconditionally before the guard.
Line 153 (console.log('Updating package name from', ...)) runs before the if (!pkg.name.startsWith('@rush-fs/')) check. On the second pass — or if a name was already scoped — it prints an incorrect "to @rush-fs/@rush-fs/rush-fs-..." string.
🛠️ Proposed fix — move the log inside the branch
- console.log('Updating package name from', pkg.name, 'to `@rush-fs/`' + pkg.name);
if (!pkg.name.startsWith('@rush-fs/')) {
+ console.log('Updating package name from', pkg.name, 'to `@rush-fs/`' + pkg.name);
pkg.name = '@rush-fs/' + pkg.name;
fs.writeFileSync(pkgPath, JSON.stringify(pkg, null, 2));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/Release.yml around lines 143 - 160, The console.log inside
the node -e script logs the "Updating package name from ... to `@rush-fs/`..."
message unconditionally, which can produce incorrect output when pkg.name is
already scoped; move the console.log so it only runs inside the if guard that
checks !pkg.name.startsWith('@rush-fs/'), ensuring the log and the writeFileSync
only execute when pkg.name is actually being changed (refer to the inline
script's pkg, pkgPath, pkg.name, the if (!pkg.name.startsWith('@rush-fs/'))
block, and the console.log statement).
…d adjust optional dependencies
CoderSerio
force-pushed
the
feat/carbon-dev
branch
from
c1236ab to
623d32b
Compare
1 participant
Summary by CodeRabbit