fix: shows v3 not supported message #17
Conversation
📝 WalkthroughWalkthroughThe PR updates the package version to 0.2.2 and adds the uuid library as a dependency. It refactors device identification in telemetry to use UUIDv5 with a fixed namespace, migrates machine-ID utilities from crypto.randomUUID to uuid.v4, and refines error messages in parser and error handling logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Pull request overview
This pull request enhances the error messaging for unsupported ZenStack V3 schemas and migrates from Node.js built-in UUID functions to the external uuid package. The changes also improve device ID generation for telemetry by applying UUIDv5 hashing.
Key Changes:
- Added informative error message when no generator block is found, indicating V3 is not supported
- Migrated from
node:cryptorandomUUID touuidpackage v4 and v5 functions - Implemented UUIDv5-based device ID hashing in telemetry for better privacy
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/zmodel-parser.ts | Enhanced error message to inform users that ZenStack V3 is not supported |
| src/utils/machine-id-utils.ts | Replaced Node.js randomUUID with uuid package v4, changed expose() return type |
| src/telemetry.ts | Added getDeviceId() method using UUIDv5 to hash machine IDs for privacy |
| src/index.ts | Removed "Error:" prefix from error message for consistency |
| package.json | Added uuid ^13.0.0 dependency and bumped version to 0.2.2 |
| pnpm-lock.yaml | Updated lockfile with uuid 13.0.0 package resolution |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| function expose(result: string): string | undefined { | ||
| function expose(result: string): string { |
There was a problem hiding this comment.
The return type was changed to string, but the function can still return undefined due to optional chaining operators in the switch cases. For example, on line 41-42, the expression could return undefined if the split or replace operations don't find matches. This will cause a type mismatch when calling hash(id) on line 71, as hash() expects a string parameter.
| if (!generatorMatch) { | ||
| throw new CliError('No generator block found in zmodel schema') | ||
| throw new CliError( | ||
| 'No generator block found in zmodel schema.\nZenStack V3 is not supported, V3 will have built-in proxy support soon.' |
There was a problem hiding this comment.
The error message states "V3 will have built-in proxy support soon" which uses future tense but may be confusing to users. Consider clarifying whether this message is specifically for users currently on V2 trying to use V3, or revising to make the message clearer about the current state and expected timeline.
| 'No generator block found in zmodel schema.\nZenStack V3 is not supported, V3 will have built-in proxy support soon.' | |
| 'No generator block found in zmodel schema.\nZenStack V3 schemas are not supported by this CLI yet. Built-in proxy support for V3 is planned but not currently available.' |
|
|
||
| private getDeviceId() { | ||
| const hostId = getMachineId() | ||
| // namespace UUID for generating UUIDv5 from DNS 'zenstack.dev' |
There was a problem hiding this comment.
The comment states this is a "namespace UUID for generating UUIDv5 from DNS 'zenstack.dev'", but the hardcoded UUID should be verified. The standard DNS namespace UUID is '6ba7b810-9dad-11d1-80b4-00c04fd430c8'. If '133cac15-3efb-50fa-b5fc-4b90e441e563' is a custom namespace derived from 'zenstack.dev', the comment should clarify this to avoid confusion.
| // namespace UUID for generating UUIDv5 from DNS 'zenstack.dev' | |
| // Custom UUIDv5 namespace used for ZenStack telemetry (derived from DNS name 'zenstack.dev') |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/machine-id-utils.ts (1)
35-62: Type safety issue:expose()can return undefined despite return type.The
expose()function's return type was changed fromstring | undefinedtostring, but the implementation still uses optional chaining operators (?.) that can returnundefined. For example:result.split('IOPlatformUUID')[1]?.split('\n')[0]?.replace(...).toLowerCase()If
split('IOPlatformUUID')[1]is undefined, the entire chain returns undefined, violating the return type and potentially causinghash(undefined)to be called at line 71.🔧 Proposed fix
Either revert the return type to include undefined:
-function expose(result: string): string { +function expose(result: string): string | undefined {Or add a fallback to ensure a string is always returned:
function expose(result: string): string { switch (platform) { case 'darwin': - return result + return (result .split('IOPlatformUUID')[1] ?.split('\n')[0] ?.replace(/=|\s+|"/gi, '') - .toLowerCase() + .toLowerCase()) ?? '' // ... similar changes for other casesThen update
getMachineId()to handle empty strings:try { const value = execSync(guid[platform as keyof typeof guid]) const id = expose(value.toString()) - return hash(id) + return id ? hash(id) : uuid() } catch { return uuid() }
🤖 Fix all issues with AI agents
In @src/telemetry.ts:
- Around line 44-48: The getDeviceId() function currently uses uuidv5(hostId,
'133cac15-3efb-50fa-b5fc-4b90e441e563') but the hard-coded UUID is not the RFC
4122 DNS namespace; either replace that second argument with the standard DNS
namespace UUID '6ba7b810-9dad-11d1-80b4-00c04fd430c8' if you intended to derive
a DNS-based UUID, or keep the custom UUID but update the comment to explicitly
state it is a deliberately chosen custom namespace for privacy/anonymization
(mentioning getDeviceId(), hostId, and uuidv5 to locate the change).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
package.jsonsrc/index.tssrc/telemetry.tssrc/utils/machine-id-utils.tssrc/zmodel-parser.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/zmodel-parser.ts (1)
src/cli-error.ts (1)
CliError(4-4)
src/utils/machine-id-utils.ts (1)
src/utils/exec-utils.ts (1)
execSync(6-15)
src/telemetry.ts (1)
src/utils/machine-id-utils.ts (1)
getMachineId(64-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Dependabot
- GitHub Check: Agent
🔇 Additional comments (5)
src/zmodel-parser.ts (1)
160-162: LGTM! Clear error message for V3 limitation.The enhanced error message clearly communicates that ZenStack V3 is not currently supported and provides context about future built-in proxy support. This aligns well with the PR objective.
src/index.ts (1)
34-34: LGTM! Cleaner error message format.Removing the "Error: " prefix reduces redundancy since
console.erroralready indicates an error condition.src/telemetry.ts (1)
22-22: LGTM! Enhanced device ID anonymization.The new
getDeviceId()method adds an additional layer of anonymization by computing a UUIDv5 from the machine ID. This provides better privacy protection while maintaining deterministic device identification across sessions.Also applies to: 44-48
src/utils/machine-id-utils.ts (1)
4-5: LGTM! Consistent UUID library usage.The migration from
crypto.randomUUID()touuid.v4()provides consistency with the UUID library now used across the codebase (including telemetry). Both generate UUID v4 values equivalently.Also applies to: 66-66, 73-73
package.json (1)
37-37: No action needed — [email protected] exists and is the latest stable version.The version specified in the dependency is correct and available on npm.
Likely an incorrect or invalid review comment.
| private getDeviceId() { | ||
| const hostId = getMachineId() | ||
| // namespace UUID for generating UUIDv5 from DNS 'zenstack.dev' | ||
| return uuidv5(hostId, '133cac15-3efb-50fa-b5fc-4b90e441e563') | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
RFC 4122 DNS namespace UUID v5 standard
💡 Result:
Short answer:
- RFC 4122 (UUIDs) defines name-based UUIDv5 as SHA-1(name + namespace) with bits set for version=5 and the RFC 4122 variant. [1]
- The standard DNS namespace UUID (used as the namespace input when generating UUIDv5 for domain names) is: 6ba7b810-9dad-11d1-80b4-00c04fd430c8. [2]
- Note: RFC 4122 was obsoleted by RFC 9562 (May 2024); RFC 9562 preserves these namespace IDs and describes current UUID rules and namespace allocation. [2][1]
Sources:
[1] RFC 4122 (A Universally Unique IDentifier (UUID) URN Namespace).
[2] RFC 9562 (Universally Unique IDentifiers (UUIDs)) — Table 3 (Namespace IDs) and namespace allocation.
🏁 Script executed:
# Search for any documentation or comments about the custom namespace UUID
rg "133cac15-3efb-50fa-b5fc-4b90e441e563" --type ts --type js
# Also check for any comments or documentation about zenstack namespace
rg -i "zenstack.*namespace\|namespace.*zenstack" --type ts --type js -A 2 -B 2Repository: zenstackhq/proxy
Length of output: 142
Clarify the namespace UUID usage.
The namespace UUID '133cac15-3efb-50fa-b5fc-4b90e441e563' doesn't match the standard DNS namespace UUID defined in RFC 4122 (6ba7b810-9dad-11d1-80b4-00c04fd430c8). The comment stating "namespace UUID for generating UUIDv5 from DNS 'zenstack.dev'" is misleading—if this is intentionally a custom namespace for privacy/anonymization, update the comment to reflect that. If it should use the RFC 4122 standard DNS namespace, correct the UUID.
🤖 Prompt for AI Agents
In @src/telemetry.ts around lines 44 - 48, The getDeviceId() function currently
uses uuidv5(hostId, '133cac15-3efb-50fa-b5fc-4b90e441e563') but the hard-coded
UUID is not the RFC 4122 DNS namespace; either replace that second argument with
the standard DNS namespace UUID '6ba7b810-9dad-11d1-80b4-00c04fd430c8' if you
intended to derive a DNS-based UUID, or keep the custom UUID but update the
comment to explicitly state it is a deliberately chosen custom namespace for
privacy/anonymization (mentioning getDeviceId(), hostId, and uuidv5 to locate
the change).
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.