fix(release): stamp .version at publish time so CLI reports the real tag (#1239)#1560
fix(release): stamp .version at publish time so CLI reports the real tag (#1239)#1560ColinM-sys wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPrepublish now writes a Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/CI
participant Git as Git (tags)
participant Prepub as prepublishOnly script
participant NPM as npm publish
participant Install as Installed package
participant Runtime as getVersion()
Dev->>Git: trigger publish
Git-->>Prepub: git describe --tags --match 'v*'
Prepub-->>Prepub: strip leading "v", write `.version`, test -s .version
Prepub->>NPM: build & publish (includes `.version`)
NPM-->>Install: package tarball (contains `.version`)
Install->>Runtime: getVersion()
alt `.version` exists
Runtime-->>Install: return contents of `.version`
else no `.version`
Runtime-->>Install: fall back to `package.json` semver
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@package.json`:
- Line 19: The prepublishOnly script can create an empty .version when `git
describe` fails, causing getVersion() in src/lib/version.ts to fall back to
package.json and produce stale published versions; update the prepublishOnly
pipeline so the publish fails if tag resolution fails by ensuring the git
describe output is non-empty (for example capture output, test it with a shell
conditional and exit non-zero on empty, or append `|| exit 1` after git
describe), so .version is only written when a valid tag is obtained before
continuing with the nemoclaw install and tsc steps.
In `@src/lib/version.test.ts`:
- Line 37: Replace the GitHub URL in the inline comment within
src/lib/version.test.ts (the semver comment) with a plain issue reference like
"See issue `#1239`"; specifically edit the comment that currently reads "//
semver. See https://github.com/NVIDIA/NemoClaw/issues/1239" to remove the
external link and use "See issue `#1239`" (or equivalent plain text) so the test
file complies with the docs-link policy.
🪄 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: CHILL
Plan: Pro
Run ID: 9ac3e067-e314-472c-b045-0510dabd7e4e
📒 Files selected for processing (2)
package.jsonsrc/lib/version.test.ts
…real tag The CLI's getVersion() chain already prefers a .version file over the hardcoded package.json semver, but nothing in the publish pipeline ever writes that file. As a result, npm-installed copies always fall through to package.json's version (currently 0.1.0) regardless of which tag was actually published, causing 'nemoclaw --version' to lie. Stamp .version from 'git describe --tags' inside prepublishOnly and add the file to the npm tarball via the 'files' field. No source changes — the existing version.ts fallback chain already handles the file. Adds a regression test that mirrors the issue scenario (stale package.json + correct .version) and asserts the .version value wins. Closes NVIDIA#1239
a99f7d8 to
b57ce3f
Compare
… test comment Address CodeRabbit review on NVIDIA#1560: - The previous git describe | sed pipeline could write an empty .version if the repo had no matching tag, since pipes mask the upstream exit code. Add 'test -s .version' so prepublishOnly aborts when tag resolution fails rather than shipping a stale package.json version. - Replace the third-party URL in the regression test comment with a plain issue reference to comply with the docs-link policy.
|
✨ Thanks for submitting this fix, which proposes a way to ensure the CLI reports the correct version by stamping it during publication. |
Summary
Closes #1239.
nemoclaw --versioncurrently prints a stale hardcoded value (0.1.0) on every npm-installed copy, regardless of which tag was actually published.Root cause
getVersion()insrc/lib/version.tstries three sources in order:git describe --tags --match 'v*'— works in dev clones, fails inside an npm install (no.git).versionfile at repo root — meant to be stamped at publish time, but nothing ever writes itpackage.jsonversion — hardcoded to0.1.0and not bumped per-releaseSo in production, steps 1 and 2 always fail and the CLI always reports
0.1.0.Fix
Two minimal changes to
package.json, no source changes:prepublishOnlynow stamps.versionfromgit describe --tags --match 'v*'beforetscruns..versionis added to thefilesarray so it ships in the npm tarball.The existing
getVersion()chain already prefers.versionoverpackage.json(proven by the existing "prefers .version file over package.json" test), so no runtime code needs to change.Test plan
src/lib/version.test.tsmirroring the issue scenario: stalepackage.json(0.1.0) + correct.version(0.0.2) → assertsgetVersion()returns0.0.2.vitest run src/lib/version.test.tspasses (4/4)..versionpresent,getVersion()returns its contents; with it absent, falls through to git describe / package.json as before.Why this approach over bumping package.json
Stamping a separate
.versionfile:package.jsonbumpsversion.ts— leverages the fallback chain that already existsSummary by CodeRabbit
Chores
Tests