fix(package): scope version script git staging to package.json only - #339
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the version script in package.json to stage only the package.json file instead of using git add -A. Feedback indicates that this change will cause an inconsistent repository state because the version stamping script modifies several other files that would no longer be included in the version commit. It is recommended to explicitly stage all files modified by the script.
There was a problem hiding this comment.
Pull request overview
This PR adjusts the npm version lifecycle script to avoid staging unrelated working-tree changes during version bumps, reducing the risk of accidentally committing in-progress or sensitive files.
Changes:
- Replaced
git add -Awithgit add package.jsonin theversionlifecycle script.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks @xiaolai! Took your patch and broadened the allowlist in 30a78e9 to cover every file |
Replaces 'git add -A' in the npm version lifecycle script with an explicit allowlist: - package.json - package-lock.json (npm version writes this) - .claude-plugin/plugin.json - .claude-plugin/marketplace.json - site/content.json Original PR agent-sh#339 from xiaolai (NLPM audit) flagged that 'git add -A' sweeps any unrelated working-tree changes into the version commit. This is the broadened-allowlist version per Copilot's review suggestion - preserves the intent (no working-tree sweep) while keeping all version manifests in sync. Co-authored-by: xiaolai <xiaolai@users.noreply.github.com>
30a78e9 to
bcdff00
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "detect": "node bin/dev-cli.js detect", | ||
| "verify": "node bin/dev-cli.js verify", | ||
| "version": "node scripts/stamp-version.js && git add -A", | ||
| "version": "node scripts/stamp-version.js && git add package.json package-lock.json .claude-plugin/plugin.json .claude-plugin/marketplace.json site/content.json", |
There was a problem hiding this comment.
The PR title/description suggests scoping staging to package.json only, but the updated version script stages an explicit allowlist of 5 files (including package-lock.json, .claude-plugin/*, and site/content.json). Consider updating the PR title/description to reflect the allowlist approach (or, if the intent truly is package.json-only, remove the extra paths).
Bot review caught that CHANGELOG.md is part of the release prep flow (developer manually edits it before running `npm version`), so it must be in the lifecycle script's `git add` allowlist - otherwise `npm version`'s auto-commit drops the changelog entry, leaving the version bump committed but the changelog entry stranded as unstaged. Updates the CHANGELOG #339/#342 description to reflect the new file.
* chore: release v5.8.5 * fix: include CHANGELOG.md in npm version allowlist Bot review caught that CHANGELOG.md is part of the release prep flow (developer manually edits it before running `npm version`), so it must be in the lifecycle script's `git add` allowlist - otherwise `npm version`'s auto-commit drops the changelog entry, leaving the version bump committed but the changelog entry stranded as unstaged. Updates the CHANGELOG #339/#342 description to reflect the new file.
Security Finding (Low)
package.jsonline 40: theversionlifecycle script runsgit add -Aafter stamping the version, which stages all working-tree changes — not just the version bump.If a developer has unrelated uncommitted changes in their working tree at the time they run
npm version, those changes will be silently included in the version bump commit. This can inadvertently publish in-progress work, debug code, or sensitive scratchpad content.Fix
Replace
git add -Awithgit add package.jsonto stage only the file that was actually changed by the version stamp script.If
scripts/stamp-version.jsalso writes to other files (e.g., aVERSIONfile or changelog), those specific files should be added explicitly rather than using the catch-all-A.