fix: add CLI version output#65
Conversation
WalkthroughThis PR adds a shared ChangesPackage Version Centralization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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)
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 |
|
Thanks for the PR, @xianzuyang9-blip! A quick note on our workflow: we ask contributors to open an issue first, get it assigned, then submit a PR that links it (e.g. "Closes #123"). This PR isn't linked to any issue. It'll still be reviewed, but please open an issue and claim it (comment that you'd like it assigned to you) so the work is tracked. |
There was a problem hiding this comment.
Code Review
What this PR does
Centralises the server version into src/shared/version.ts (read from package.json), replaces five hardcoded '1.0.0' strings, and wires up -v, --version CLI flags on both the HTTP and stdio entry points. Direction is correct; the consolidation is welcome.
Findings
1. ⚠️ Important — JSON import may crash at startup in ESM mode
File: src/shared/version.ts, line 1
import packageJson from '../../package.json';Every other import in this repo uses an explicit .js extension, which is the canonical signal for a "module": "NodeNext" / ESM output configuration. Under Node.js 18 – 22.11, a bare JSON import in an ESM module throws at startup:
TypeError: Unknown file extension ".json" for .../package.json
Node.js requires an import attribute for JSON in ESM mode. The fix is one word:
import packageJson from '../../package.json' with { type: 'json' };with syntax is available from TypeScript 5.3 + Node.js 22.10. If the project targets an older stack, the safer fallback is:
import { createRequire } from 'module';
const require = createRequire(import.meta.url);
export const PACKAGE_VERSION: string =
(require('../../package.json') as { version: string }).version;If --experimental-json-modules is already set in the launch script or CI, please document that; otherwise this flag is easy to miss when running locally.
2. Low — packageJson.version is unguarded
File: src/shared/version.ts, line 3
TypeScript infers the type of packageJson.version from the literal content of package.json at compile time, but there is no runtime guard. If a future refactor strips the version key (or the wrong package.json is resolved), all four call sites silently receive undefined. A one-line guard prevents a confusing bug later:
export const PACKAGE_VERSION: string = packageJson.version ?? '0.0.0';3. Minor — -v short flag conflicts with the --verbose convention
Files: src/http/config.ts line 10, src/stdio/index.ts line 8
Commander's own default short flag for version is -V (uppercase). Lowercase -v is near-universally understood as --verbose across CLI tools. While both CLIs currently have no verbose flag, using lowercase increases the chance of a future conflict and surprises users who reach for -v to enable verbose output.
.version(PACKAGE_VERSION, '-V, --version')4. Nit — minor simplification in version.ts
The default-import + property access can be collapsed to a named import:
import { version as PACKAGE_VERSION } from '../../package.json' with { type: 'json' };
export { PACKAGE_VERSION };(Contingent on finding 1 being resolved first.)
Summary
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | src/shared/version.ts:1 |
JSON import without with { type: 'json' } — ESM startup crash |
|
| 2 | Low | src/shared/version.ts:3 |
version may be undefined at runtime, no fallback |
| 3 | Minor | config.ts:10, stdio/index.ts:8 |
-v collides with --verbose convention; prefer -V |
| 4 | Nit | src/shared/version.ts |
Named import is cleaner |
Finding 1 is the one worth resolving before merge; the rest are low-risk polish.
|
Thanks for the workflow note. I opened this from the Charles microbounty claim, but I understand the project preference for tracking work through issues first. I also addressed the ESM/package version loading risk identified by cubic in f301317. The shared version module now uses a Node-compatible Validation run:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/shared/version.ts`:
- Around line 8-13: The loadPackageJson function can throw if both
require('../../package.json') and require('../package.json') fail during module
initialization; update loadPackageJson to catch errors on the fallback require
and return a safe empty object ({}) on failure so code using const packageJson =
loadPackageJson() (and PACKAGE_VERSION = packageJson.version ?? '0.0.0') never
throws. Specifically, inside loadPackageJson wrap the second require call in a
try/catch (or add a final return {}) and ensure the function always returns an
object with an optional version property when both loads fail.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24bc17d9-8a07-42ef-8461-4bd8c67a191d
📒 Files selected for processing (1)
src/shared/version.ts
|
Thanks, addressed the fallback concern in
Validation run:
|
Closes #66.
Manual validation after the latest update:
npm run buildnpm testnpm run lintnode dist/index.js --version->1.2.10node dist/http-server.js --version->1.2.10Summary by cubic
Adds a
-v, --versionflag to both CLIs and replaces hardcoded versions with the package version for consistent reporting across the app.New Features
-v, --versionforinsforge-mcp-serverandinsforge-mcpCLIs.Refactors
1.0.0withPACKAGE_VERSIONin MCP server init and the HTTP health endpoint.src/shared/version.tsusingcreateRequire, with a safe fallback to0.0.0ifpackage.jsonis unavailable.Written for commit c998eda. Summary will update on new commits.
Summary by CodeRabbit