fix(update-system): defensive VERSION parsing for release-please marker#547
fix(update-system): defensive VERSION parsing for release-please marker#547nhardgrove wants to merge 1 commit intosantifer:mainfrom
Conversation
|
Welcome to career-ops, @nhardgrove! Thanks for your first PR. A few things to know:
We'll review your PR soon. Join our Discord if you have questions. |
📝 WalkthroughWalkthroughThe update script introduces a Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related issues
Possibly related PRs
🚥 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 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: 2
🤖 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 19-20: Remove the unsupported extra-files input from the
release-please workflow (the extra-files: VERSION entry) and instead configure
release-please via the config-file input for release-please-action@v4; add with:
config-file: .release-please-config.json to the action invocation and
create/update .release-please-config.json to list VERSION under the extraFiles
field so the VERSION file gets included in release PRs.
In `@VERSION`:
- Line 1: VERSION now contains an inline release-please marker which breaks the
strict semver check in test-all.mjs; update the validator in test-all.mjs to
ignore an inline marker by splitting the VERSION line at the first '#' (or
trimming any trailing " # x-release-please-version") and validating only the
part before the marker as semver (or update the regex to allow an optional " #
..." suffix). Locate the semver validation code in test-all.mjs (the VERSION
check) and change it to parse/trim off the marker before calling the semver
test.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: abfef407-0ff6-4515-bf6b-4faeb2d643d7
📒 Files selected for processing (3)
.github/workflows/release.ymlVERSIONupdate-system.mjs
| @@ -1 +1 @@ | |||
| 1.3.0 | |||
| 1.6.0 # x-release-please-version | |||
There was a problem hiding this comment.
Update VERSION validator to handle the release-please marker
Line 1 now includes an inline marker, but test-all.mjs (Lines 302-315) still validates the entire line as strict semver. That will fail after this change.
Proposed fix (in test-all.mjs)
- const version = readFile('VERSION').trim();
- if (/^\d+\.\d+\.\d+$/.test(version)) {
+ const versionRaw = readFile('VERSION').trim();
+ const version = versionRaw.split(/\s+/)[0] || '';
+ if (/^\d+\.\d+\.\d+$/.test(version)) {
pass(`VERSION is valid semver: ${version}`);
} else {
- fail(`VERSION is not valid semver: "${version}"`);
+ fail(`VERSION is not valid semver: "${versionRaw}"`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@VERSION` at line 1, VERSION now contains an inline release-please marker
which breaks the strict semver check in test-all.mjs; update the validator in
test-all.mjs to ignore an inline marker by splitting the VERSION line at the
first '#' (or trimming any trailing " # x-release-please-version") and
validating only the part before the marker as semver (or update the regex to
allow an optional " # ..." suffix). Locate the semver validation code in
test-all.mjs (the VERSION check) and change it to parse/trim off the marker
before calling the semver test.
|
Hey @nhardgrove — heads-up: #507 (chorrell) acabó de mergearse y resuelve la parte estructural de este PR usando PERO tu Si quieres, ¿podrías rebasear este PR contra current main manteniendo SOLO el cambio en Si no tienes bandwidth, no problem — cierro este PR en 7 días con thank-you. Door open para futuras contribuciones. La parseVersionFile idea era buena. Closes nothing technical — la mejora estructural ya en main vía #507. Tu defensive helper sigue siendo standalone valioso. |
…marker
The local VERSION file and the upstream raw fetch are both passed
through .trim() and a strict semver regex before this change. If the
project ever adopts the release-please convention of appending an
x-release-please-version marker comment to plain-text version files
(e.g. "1.6.0 # x-release-please-version"), the existing parsers break:
.trim() returns the whole string, the SEMVER_RE no longer matches, and
.split('.').map(Number) yields NaN.
This change extracts a parseVersionFile() helper that strips trailing
whitespace and ignores anything after the first whitespace token, then
routes both the local read (localVersion) and the upstream fetch
(check) through it. Behavior for marker-less files is unchanged --
"1.6.0\n" still parses to "1.6.0". Behavior for marker-bearing files
becomes correct rather than silently broken.
Standalone defensive code; complements the structural release-please
fix in santifer#507. No workflow or VERSION file changes here -- those landed
already via santifer#507.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
nhardgrove
force-pushed
the
fix/release-please-bump-version
branch
from
15302a8 to
b611603
Compare
nhardgrove
changed the title
|
Rebased onto current Verified locally against the marker-less Helper is whitespace-tolerant and falls back cleanly when no marker is present, so it remains a no-op for current main while protecting against any future change to the VERSION format. Updated the PR title to reflect the narrower scope. Thanks for the steer toward #507 — the external |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@update-system.mjs`:
- Around line 105-109: The VERSION validation in test-all.mjs must use the same
normalization as parseVersionFile to accept marker-suffixed VERSION strings;
update test-all.mjs so that when it reads the VERSION content it trims and
splits on whitespace and takes the first token (same logic as parseVersionFile)
before running the semver/regex checks (i.e., normalize the raw VERSION string
to raw.trim().split(/\s+/)[0] and then validate that token instead of the
unnormalized full string).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 613bfc5e-aed2-4fbf-a6b9-bbfb9633186c
📒 Files selected for processing (1)
update-system.mjs
| function parseVersionFile(raw) { | ||
| // VERSION may carry a release-please marker, e.g. "1.6.0 # x-release-please-version". | ||
| // Take the first whitespace-delimited token so the marker doesn't break semver parsing. | ||
| return raw.trim().split(/\s+/)[0] || ''; | ||
| } |
There was a problem hiding this comment.
Align VERSION validation in test-all.mjs with the new normalization
This parser now accepts marker-suffixed VERSION content, but test-all.mjs still requires strict trimmed semver and will fail when the marker is present. Please normalize there too (same first-token parsing) to avoid CI false failures.
Suggested follow-up diff (cross-file)
--- a/test-all.mjs
+++ b/test-all.mjs
@@
if (fileExists('VERSION')) {
- const version = readFile('VERSION').trim();
+ const version = readFile('VERSION').trim().split(/\s+/)[0] || '';
if (/^\d+\.\d+\.\d+$/.test(version)) {
pass(`VERSION is valid semver: ${version}`);
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@update-system.mjs` around lines 105 - 109, The VERSION validation in
test-all.mjs must use the same normalization as parseVersionFile to accept
marker-suffixed VERSION strings; update test-all.mjs so that when it reads the
VERSION content it trims and splits on whitespace and takes the first token
(same logic as parseVersionFile) before running the semver/regex checks (i.e.,
normalize the raw VERSION string to raw.trim().split(/\s+/)[0] and then validate
that token instead of the unnormalized full string).
2 participants
Summary
.release-please-manifest.jsonandCHANGELOG.mdbut leavesVERSIONpinned at1.3.0across every release. The latest tag isv1.6.0, butmain:VERSIONstill reads1.3.0.update-system.mjs: everyapplyends withvX.Y.Z → vX.Y.Zbecause it reads the upstream VERSION file after checkout, and the checker keeps reportingupdate-available 1.3.0 → 1.6.0indefinitely (it falls back to the GitHub release tag at lines 218-222 when VERSION is stale).extra-filesso release-please bumpsVERSIONduring the release PR. The marker comment release-please requires for generic files is added, andupdate-system.mjsparses around it.Repro
Changes
.github/workflows/release.yml: addextra-files: VERSIONso release-please-action@v4 rewrites the version on each release PR.VERSION: bump to1.6.0(backfill) and append thex-release-please-versionmarker required by release-please for plain-text extra files.update-system.mjs: extractparseVersionFile()and use it for both the local file read and the upstream raw fetch, so the marker doesn't break semver parsing.Test plan
node update-system.mjs checkreturnsup-to-dateafter the bumpparseVersionFile("1.6.0 # x-release-please-version\n")→"1.6.0"VERSIONchangeupdate-system.mjsNotes
Existing installs pulling this commit will jump to
1.6.0immediately; subsequent release PRs will bumpVERSIONautomatically. No breaking change for downstream —parseVersionFileis whitespace-tolerant and falls back cleanly when the marker is absent.🤖 Generated with Claude Code
Summary by CodeRabbit