feat(target-config): add release identity contract - #124
Conversation
📝 WalkthroughWalkthroughThe change adds shared release identity utilities, extends release manifests with optional identity data, integrates identity creation and validation into manifest generation and merging, updates final SemVer checks, and documents the release versioning model. ChangesRelease identity and manifest integration
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74839cd0c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const semVerPattern = new RegExp( | ||
| `^${semVerCore}(?:-${preReleaseIdentifier}(?:\\.${preReleaseIdentifier})*)?(?:\\+[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?$`, | ||
| 'u', |
There was a problem hiding this comment.
When releaseRevision is omitted, this pattern accepts a valid version such as 1.2.3+sha, and createMpgdReleaseIdentity uses it verbatim as the label. generate-release-manifest.ts then passes that label to formatMpgdReleaseId, whose component validation rejects every +, so manifest generation now fails for APP_VERSION or package.json.version values containing SemVer build metadata that worked before this commit. The accepted version grammar and release-ID encoding need to compose, or metadata must be rejected explicitly at the identity boundary.
Useful? React with 👍 / 👎.
| if (manifest.gameVersion !== releaseIdentity.gameVersion) { | ||
| throw new TypeError('Release manifest gameVersion must match releaseIdentity.gameVersion.'); |
There was a problem hiding this comment.
If a schema-valid or hand-edited manifest contains releaseIdentity.gameVersion: " 1.0.0 " while the top-level version, label, and release ID are canonical, createMpgdReleaseIdentity trims the nested value and this comparison succeeds because it checks only the top-level value against the normalized result. The validator therefore returns a noncanonical identity; when another target is generated with "1.0.0", hasMatchingReleaseIdentity raw-compares the nested strings and resets the manifest instead of preserving the earlier targets. Check the original nested value against the normalized value, analogous to the label check above.
Useful? React with 👍 / 👎.
| if (previous === undefined || next === undefined) { | ||
| return true; |
There was a problem hiding this comment.
When the previous file is a legacy manifest for game version 1.0.0-v42, its old release ID is identical to the new release ID for { gameVersion: "1.0.0", releaseRevision: 42 } when the build ID is reused. If the source and other contract fields also match—for example, APP_VERSION is changed between builds from the same checkout while the default BUILD_ID=local is retained—this unconditional match preserves targets built for the prerelease inside the final release manifest. Compare the legacy top-level game version as well, or otherwise disambiguate legacy prerelease labels before merging.
Useful? React with 👍 / 👎.
![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
🤖 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 `@packages/target-config/src/releaseIdentity.ts`:
- Around line 90-136: Update normalizeMpgdSemVer to reject SemVer values
containing build metadata (+ suffix), so gameVersion cannot produce a label with
the reserved release-ID separator. Ensure createMpgdReleaseIdentity fails at
normalization time with a clear TypeError, while preserving acceptance of valid
versions without build metadata.
🪄 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 Plus
Run ID: 985fbbe8-86d1-485f-a9b9-a56cc79d7e89
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.sampo/changesets/jovial-warden-akka.mddocs/MULTI_TARGET_RELEASE_VERSIONING.mdpackages/release-manifest/package.jsonpackages/release-manifest/src/index.tspackages/target-config/src/index.tspackages/target-config/src/releaseIdentity.tspackages/target-config/test/target-config-smoke.tsrelease.manifest.schema.jsontools/smoke/release-manifest-merge.tstools/target/generate-release-manifest.tstools/target/native-release-identity.ts
| export function formatMpgdReleaseId(label: string, buildId: string): string { | ||
| assertReleaseIdComponent(label, 'label'); | ||
| assertReleaseIdComponent(buildId, 'buildId'); | ||
|
|
||
| return `mpgd-${label}+${buildId}`; | ||
| } | ||
|
|
||
| /** Parse an optional release revision without making empty values meaningful. */ | ||
| export function parseMpgdReleaseRevision(value: string | undefined): number | undefined { | ||
| if (value === undefined || value.trim().length === 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const normalized = value.trim(); | ||
|
|
||
| if (!/^[1-9]\d*$/u.test(normalized)) { | ||
| throw new TypeError('Release revision must be a positive safe integer.'); | ||
| } | ||
|
|
||
| const releaseRevision = Number.parseInt(normalized, 10); | ||
|
|
||
| if (!Number.isSafeInteger(releaseRevision)) { | ||
| throw new TypeError('Release revision must be a positive safe integer.'); | ||
| } | ||
|
|
||
| return releaseRevision; | ||
| } | ||
|
|
||
| function normalizeMpgdSemVer(value: string): string { | ||
| const normalized = value.trim(); | ||
|
|
||
| if (!semVerPattern.test(normalized)) { | ||
| throw new TypeError('Release identity gameVersion must be a SemVer value.'); | ||
| } | ||
|
|
||
| return normalized; | ||
| } | ||
|
|
||
| function assertReleaseIdComponent(value: string, label: string): void { | ||
| if (value.length === 0 || /\s/u.test(value)) { | ||
| throw new TypeError(`Release identifier ${label} must be a non-empty canonical token.`); | ||
| } | ||
|
|
||
| if (value.includes('+')) { | ||
| throw new TypeError(`Release identifier ${label} must not contain "+".`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Build-metadata gameVersion silently breaks formatMpgdReleaseId.
normalizeMpgdSemVer (Lines 118-126) accepts SemVer build metadata (the +... suffix) as part of a valid gameVersion. When releaseRevision is not set, formatNormalizedMpgdReleaseLabel returns that gameVersion unchanged as the label (Lines 64-66), so the label can contain +. formatMpgdReleaseId and assertReleaseIdComponent then reject any label containing +, because + is the reserved separator between label and buildId.
A gameVersion such as 0.3.30+001 with no release revision passes createMpgdReleaseIdentity, but then throws later when formatMpgdReleaseId(releaseIdentity.label, buildId) runs in tools/target/generate-release-manifest.ts (Line 103) or packages/release-manifest/src/index.ts (Line 83). The failure surfaces far from its cause and with a confusing message.
Reject build metadata in normalizeMpgdSemVer, or strip it before it becomes part of the label, so invalid input is caught where it originates.
🐛 Proposed fix: reject build metadata in gameVersion at the source
-const semVerPattern = new RegExp(
- `^${semVerCore}(?:-${preReleaseIdentifier}(?:\\.${preReleaseIdentifier})*)?(?:\\+[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?$`,
- 'u',
-);
+// Build metadata is intentionally unsupported: `+` is reserved as the
+// releaseId separator between the label and the build identifier.
+const semVerPattern = new RegExp(
+ `^${semVerCore}(?:-${preReleaseIdentifier}(?:\\.${preReleaseIdentifier})*)?$`,
+ 'u',
+);🤖 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 `@packages/target-config/src/releaseIdentity.ts` around lines 90 - 136, Update
normalizeMpgdSemVer to reject SemVer values containing build metadata (+
suffix), so gameVersion cannot produce a label with the reserved release-ID
separator. Ensure createMpgdReleaseIdentity fails at normalization time with a
clear TypeError, while preserving acceptance of valid versions without build
metadata.
1 participant
Summary
Validation
pnpm checkpnpm smoke:release-manifest-mergeSummary by CodeRabbit
New Features
Documentation
Bug Fixes