fix(release): bump package-lock.json alongside package.json - #290
Open
EtienneLescot wants to merge 1 commit into
Open
fix(release): bump package-lock.json alongside package.json#290EtienneLescot wants to merge 1 commit into
EtienneLescot wants to merge 1 commit into
Conversation
prerelease.yml and promote.yml rewrote the version with a sed over package.json alone, so every release shipped a lockfile whose root version disagreed with the package it locks: v1.7.0 package.json=1.7.0 lock=1.6.0 v1.8.0 package.json=1.8.0 lock=1.8.0-rc.4 v1.9.0 package.json=1.9.0 lock=1.8.0 It went unnoticed for three releases because npm ci only fails on dependency drift, never on this field. The mismatch is inert until someone reads the diff, which is how it finally surfaced. Both workflows now call one script that writes package.json and both root version fields of the lockfile (lockfileVersion 3 repeats it under packages[""]). A plain sed cannot do this: the lockfile has a "version" key per dependency, so a naive substitution would rewrite the whole tree. The files are tab-indented JSON that JSON.stringify round-trips byte for byte, so rewriting them whole still yields a three-line diff. That is load-bearing rather than incidental, and the test pins it: if npm ever changes its lockfile formatting, the test fails instead of a release commit silently becoming a 40k-line reformat. The script refuses to write when packages[""] is absent rather than skipping it through optional chaining, since a silent half-bump is the exact failure being fixed.
📝 WalkthroughWalkthroughThe change adds a shared release-version utility, tests its manifest updates and validation, and uses it in prerelease and promotion workflows to update both package manifests. ChangesRelease version synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 @.github/scripts/set-release-version.mjs:
- Around line 39-53: Update the release-version flow around the manifest edits
to parse and validate both package.json and package-lock.json, including
lock.packages[""], before writing either file. Preserve the existing error for
an invalid lockfile, then apply the version updates only after validation
succeeds. Extend the invalid-lockfile test to verify package.json remains at
1.8.0.
- Line 58: Normalize the direct-invocation comparison around
import.meta.filename and argv[1] in the script’s entry-point guard so equivalent
absolute and repository-relative paths match and the version-update flow runs
when invoked from the repository root. Add a CLI test that executes the script
with version 1.9.0 from the repository root and verifies both package.json and
package-lock.json are updated.
🪄 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: facd82fd-3454-49a2-ab4e-116a2b31f23a
📒 Files selected for processing (4)
.github/scripts/set-release-version.mjs.github/scripts/set-release-version.test.mjs.github/workflows/prerelease.yml.github/workflows/promote.yml
Comment on lines
+39
to
+53
| edit("package.json", (pkg) => { | ||
| pkg.version = version; | ||
| }); | ||
|
|
||
| edit("package-lock.json", (lock) => { | ||
| lock.version = version; | ||
| // lockfileVersion 3 repeats the root version inside packages[""]. Optional | ||
| // chaining would quietly skip it if the shape ever changed — the same | ||
| // silent half-bump this script exists to end — so demand it instead. | ||
| if (!lock.packages?.[""]) { | ||
| throw new Error( | ||
| 'package-lock.json has no packages[""] entry; the lockfile format changed and this script needs updating', | ||
| ); | ||
| } | ||
| lock.packages[""].version = version; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the lockfile before writing either manifest.
When package-lock.json lacks packages[""], Line 40 writes package.json and Lines 48-51 then throw. This leaves the manifests out of sync on the error path that must prevent partial updates.
Parse and validate both files first. Write either file only after validation succeeds. Extend the invalid-lockfile test to assert that package.json remains at 1.8.0.
🤖 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/scripts/set-release-version.mjs around lines 39 - 53, Update the
release-version flow around the manifest edits to parse and validate both
package.json and package-lock.json, including lock.packages[""], before writing
either file. Preserve the existing error for an invalid lockfile, then apply the
version updates only after validation succeeds. Extend the invalid-lockfile test
to verify package.json remains at 1.8.0.
| } | ||
|
|
||
| // Only run when invoked directly, so the test can import the function. | ||
| if (import.meta.filename === argv[1]) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files matching script:"
fd -a 'set-release-version\.mjs$' . || true
echo
echo "Script size:"
wc -l .github/scripts/set-release-version.mjs 2>/dev/null || true
echo
echo "Relevant script lines:"
sed -n '1,120p' .github/scripts/set-release-version.mjs 2>/dev/null || true
echo
echo "Usages of script:"
rg -n "set-release-version\.mjs|release-version|releaseVersion|manifest" .github package.json . 2>/dev/null | head -200Repository: getopenscreen/openscreen
Length of output: 9633
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, sys, pathlib, tempfile, os
script = pathlib.Path(".github/scripts/set-release-version.mjs")
if not script.exists():
print("missing script")
sys.exit(0)
source = script.read_text()
for needle in ["import.meta.filename", "argv[1]", "resolve(", "join("]:
print(f"{needle!r}: {source.count(needle)}")
print("contains current comparison:")
print("if (import.meta.filename === argv[1])" in source)
with tempfile.TemporaryDirectory() as d:
dpath = pathlib.Path(d)
(dpath / "package.json").write_text("{}")
os.chdir(dpath)
os.environ["GITHUB_REPOSITORY"] = "owner/repo"
os.environ["GITHUB_WORKSPACE"] = str(dpath)
os.environ["GITHUB_REF_NAME"] = "main"
os.environ["GITHUB_REF"] = "refs/heads/main"
full = script.resolve()
rel = pathlib.Path(".github/scripts/set-release-version.mjs")
print("\nPaths in temporary repo at root:")
print("GITHUB_WORKSPACE:", dpath)
print("script resolve():", full)
print("cli argv[1] =", str(rel))
print("absolute argv:", os.path.abspath(str(rel)))
print("current equality:", str(full) == str(rel))
print("current equality with resolved argv:", str(full) == os.path.abspath(str(rel)))
PYRepository: getopenscreen/openscreen
Length of output: 596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Lines 95-117 of prerelease.yml:"
sed -n '95,117p' .github/workflows/prerelease.yml 2>/dev/null || true
echo
echo "Lines 65-83 of promote.yml:"
sed -n '65,83p' .github/workflows/promote.yml 2>/dev/null || true
echo
echo "Relevant tests:"
sed -n '1,140p' .github/scripts/set-release-version.test.mjs 2>/dev/null || trueRepository: getopenscreen/openscreen
Length of output: 6268
Normalize the direct-invocation path check.
import.meta.filename is absolute, but the workflow commands pass .github/scripts/set-release-version.mjs. The current equality is false, so node .github/scripts/set-release-version.mjs exits without writing package.json or package-lock.json.
Proposed fix
-import { join } from "node:path";
+import { join, resolve } from "node:path";
-if (import.meta.filename === argv[1]) {
+if (argv[1] && import.meta.filename === resolve(argv[1])) {Add a CLI test that invokes node .github/scripts/set-release-version.mjs 1.9.0 from the repository root.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (import.meta.filename === argv[1]) { | |
| if (argv[1] && import.meta.filename === resolve(argv[1])) { |
🤖 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/scripts/set-release-version.mjs at line 58, Normalize the
direct-invocation comparison around import.meta.filename and argv[1] in the
script’s entry-point guard so equivalent absolute and repository-relative paths
match and the version-update flow runs when invoked from the repository root.
Add a CLI test that executes the script with version 1.9.0 from the repository
root and verifies both package.json and package-lock.json are updated.
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
1 participant
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.
The bug
prerelease.ymlandpromote.ymlrewrote the version with asedoverpackage.jsonalone. Every release therefore shipped a lockfile whose root version disagreed with the package it locks:package.jsonpackage-lock.jsonThree releases, three mismatches. Nothing caught it because
npm cionly fails on dependency drift, never on this field — the mismatch is inert until someone reads the diff, which is exactly how it finally surfaced (CodeRabbit, on the 1.9.0 release-sync PR).The fix
Both workflows now call
.github/scripts/set-release-version.mjs, which writespackage.jsonand both root version fields of the lockfile —lockfileVersion3 repeats the version underpackages[""].A plain
sedcannot do this, which is presumably why it was never extended: the lockfile carries a"version"key for every dependency, so a naive substitution would rewrite the entire tree.Why rewriting whole JSON files is safe here
Both files are tab-indented JSON that
JSON.stringify(…, null, "\t")round-trips byte for byte, so rewriting them whole still produces a three-line diff:That property is load-bearing, not incidental, so the test pins it rather than trusting it. If npm ever changes its lockfile formatting, the test fails — instead of a release commit silently becoming a 40 000-line reformat that nobody reviews.
The script also refuses to write when
packages[""]is missing, rather than skipping it via optional chaining. A silent half-bump is the precise failure being fixed here; turning a format change into a loud error is the point.Verification
Six tests, in
.github/scripts/where the other CI scripts are already covered by vitest (scripts/is outside the vitestinclude, which is why the script lives here):Also run end-to-end against the repo's real
package.jsonandpackage-lock.json, then reverted — exactly three changed lines, nothing else touched. Full.github/scriptssuite: 31 tests passing. Both workflows re-parsed to confirmSetup Node.jsstill precedes the step that now invokes node.One transition note
promote.ymlchecks out the frozen release branch before running the script, so the script must exist on that branch. Branches cut after this merges inherit it frommainviaprerelease.yml. A release branch cut before this merges —release/v1.9.0— does not have it, so promoting such a branch would fail on a missing file. 1.9.0 is already promoted, so nothing is currently affected; if an old branch ever needs promoting, cherry-pick the script onto it first.Summary by CodeRabbit
Bug Fixes
Tests