Repository navigation
feat(target-config): add release identity contract - #124
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 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
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseManifestMergeSmoke
participant generate-release-manifest
participant target-config
participant ReleaseManifest
ReleaseManifestMergeSmoke->>generate-release-manifest: provide release revision and expected label
generate-release-manifest->>target-config: create canonical release identity
target-config-->>generate-release-manifest: return release label and revision
generate-release-manifest->>ReleaseManifest: emit releaseIdentity and releaseId
ReleaseManifest->>ReleaseManifest: validate identity and releaseId
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
|
리뷰 피드백을
검증:
@codex review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Commit summary and diff ==='
git show --stat --oneline --decorate --no-renames 063ddb0
git diff --no-ext-diff --unified=80 063ddb0^ 063ddb0 -- \
packages/target-config/src/releaseIdentity.ts \
packages/target-config/test/target-config-smoke.ts \
packages/release-manifest/src/index.ts \
tools/target/generate-release-manifest.ts \
tools/smoke/release-manifest-merge.ts
echo '=== Relevant file outlines ==='
ast-grep outline packages/target-config/src/releaseIdentity.ts --view expanded
ast-grep outline packages/release-manifest/src/index.ts --view expanded
ast-grep outline tools/target/generate-release-manifest.ts --view expanded
ast-grep outline tools/smoke/release-manifest-merge.ts --view expanded
echo '=== Release identity and merge call sites ==='
rg -n -C 4 --glob '*.ts' \
'(createMpgdReleaseIdentity|formatMpgdReleaseId|assertReleaseManifest|releaseIdentity|gameVersion)' \
packages toolsLength of output: 88522
추가적인 차단 문제를 발견하지 못했습니다.
보고한 검증 명령은 이 환경에서 독립 실행하지 않았습니다. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/smoke/release-manifest-merge.ts (1)
122-142: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the migrated release identity, not only the target key.
The assertion at Line 139 verifies only that
microsoft-storereplaced the old target. It does not verify the mergedgameVersion,releaseIdentity, orreleaseId. The validator inpackages/release-manifest/src/index.tsaccepts manifests withoutreleaseIdentity, so an incomplete merge can still pass. Store the merged manifest and assert the expected revision-42 identity and derived release ID.🤖 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 `@tools/smoke/release-manifest-merge.ts` around lines 122 - 142, Update the second runManifest verification in the release-manifest merge test to store the resulting manifest, then assert the revision-42 merged gameVersion, releaseIdentity, and derived releaseId in addition to the microsoft-store target key. Use the expected identity values produced by the releaseRevision: 42 input and preserve the existing target assertion.
🤖 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.
Nitpick comments:
In `@tools/smoke/release-manifest-merge.ts`:
- Around line 122-142: Update the second runManifest verification in the
release-manifest merge test to store the resulting manifest, then assert the
revision-42 merged gameVersion, releaseIdentity, and derived releaseId in
addition to the microsoft-store target key. Use the expected identity values
produced by the releaseRevision: 42 input and preserve the existing target
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 70388571-afc3-47f1-a484-ef4bf1a60fba
📒 Files selected for processing (5)
packages/release-manifest/src/index.tspackages/target-config/src/releaseIdentity.tspackages/target-config/test/target-config-smoke.tstools/smoke/release-manifest-merge.tstools/target/generate-release-manifest.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/target-config/test/target-config-smoke.ts
- packages/target-config/src/releaseIdentity.ts
- packages/release-manifest/src/index.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 063ddb0848
ℹ️ 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".
| "required": ["gameVersion", "label"], | ||
| "properties": { | ||
| "gameVersion": { "type": "string", "minLength": 1 }, | ||
| "releaseRevision": { "type": "integer", "minimum": 1 }, |
There was a problem hiding this comment.
When a manifest is validated only against this JSON Schema, a revision such as 9007199254740992 is accepted because it is an integer above the minimum, but assertReleaseManifest later passes it to createMpgdReleaseIdentity, which rejects every value outside Number.isSafeInteger. This makes schema-valid manifests unusable by the runtime validator; add a maximum of 9007199254740991 so both contracts enforce the same boundary.
Useful? React with 👍 / 👎.
| gitSha: provenance.sourceGitSha, | ||
| kitGitSha: provenance.kitGitSha, | ||
| gameVersion, | ||
| gameVersion: releaseIdentity.gameVersion, |
There was a problem hiding this comment.
When a target is built directly with an APP_VERSION such as " 1.0.0 ", build-target.ts passes that raw value to Vite, whose starter config embeds it as __APP_VERSION__, while createMpgdReleaseIdentity trims it and this line records "1.0.0" in the release manifest. The shipped artifact therefore reports a different version from its provenance manifest (and the Microsoft Store artifact verification explicitly rejects that mismatch); either reject noncanonical input before building or use the normalized value for both the build and manifest.
Useful? React with 👍 / 👎.
|
Addressed the latest review feedback in
Validation:
@codex review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Commit and changed files ==='
git show --stat --oneline --no-renames 77a1084
git diff --name-only 77a1084^ 77a1084
echo '=== Commit diff ==='
git diff --no-ext-diff --unified=100 77a1084^ 77a1084 -- \
release.manifest.schema.json \
tools/target/generate-release-manifest.ts \
tools/smoke/release-manifest-merge.ts \
packages/target-config/src/releaseIdentity.ts \
packages/release-manifest/src/index.ts \
packages/target-config/test/target-config-smoke.ts
echo '=== File outlines ==='
ast-grep outline tools/target/generate-release-manifest.ts --view expanded
ast-grep outline tools/smoke/release-manifest-merge.ts --view expanded
ast-grep outline packages/target-config/src/releaseIdentity.ts --view expanded
echo '=== APP_VERSION, releaseRevision, and schema call sites ==='
rg -n -C 5 --glob '*.ts' --glob '*.json' \
'(APP_VERSION|releaseRevision|MAX_SAFE_INTEGER|createMpgdReleaseIdentity|assertReleaseManifest)' \
packages tools release.manifest.schema.jsonLength of output: 91582 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== build-target imports and execution order ==='
sed -n '1,180p' tools/target/build-target.ts
echo '=== APP_VERSION helper ==='
cat -n tools/target/app-version.ts
echo '=== Build-target test candidates ==='
fd -t f -E node_modules -E dist 'build-target|app-version' tools packages
echo '=== Build target execution and test references ==='
rg -n -C 5 --glob '*.ts' --glob '*.json' --glob '*.yml' --glob '*.yaml' \
'(build-target\.ts|requireCanonicalAppVersion|resolveReleaseProvenance|pnpm.*build:target)' \
tools packages .github package.json pnpm-workspace.yaml 2>/dev/null || true
echo '=== Changed-file complete diff ==='
git diff --check 77a1084^ 77a1084Length of output: 26372
추가 차단 문제를 발견하지 못했습니다.
보고한 ✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Validation
pnpm checkpnpm smoke:release-manifest-mergeSummary by CodeRabbit
New Features
Bug Fixes
Documentation