fix(release): shape the ios-spm manifest per channel and verify the real manifest#405
Open
NathanWalker wants to merge 3 commits into
Open
fix(release): shape the ios-spm manifest per channel and verify the real manifest#405NathanWalker wants to merge 3 commits into
NathanWalker wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR moves iOS and visionOS runtime delivery to SwiftPM-backed artifacts, updates project templates and packaging scripts, and adds target-aware release, publication, ios-spm update, and verification stages. ChangesSwiftPM release pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Build as Matrix build
participant Release as GitHub release
participant SPM as ios-spm repository
participant NPM as npm registry
participant Verify as SwiftPM probe
Build->>Release: Upload xcframework and dSYM assets
Release->>SPM: Generate and tag Package.swift
SPM->>NPM: Publish target package
NPM->>Verify: Resolve exact release version
Verify->>Verify: Validate checksums and package resolution
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/npm_release.yml (2)
149-168: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueTemplate-injection pattern: matrix values interpolated directly into
run:.
npm run ${{ matrix.script }}(line 152) embeds a workflow expression straight into a shell command. The values are currently hardcoded insetup'sBUILD_MATRIX, so the practical risk is nil today, but the pattern is fragile against future matrix changes and is what zizmor'stemplate-injectionaudit flags. Prefer routing throughenv:so the value is passed as data, not shell source.♻️ Proposed fix
- name: Build (${{ matrix.target }}) - run: npm run ${{ matrix.script }} + env: + BUILD_SCRIPT: ${{ matrix.script }} + run: npm run "$BUILD_SCRIPT"🤖 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 @.github/workflows/npm_release.yml around lines 149 - 168, Update the “Build (${{ matrix.target }})” step to pass matrix.script through the step’s env configuration, then invoke npm using the environment variable rather than interpolating matrix.script directly in run. Preserve the existing build behavior while ensuring the matrix value is treated as shell data.Source: Linters/SAST tools
4-22: 🩺 Stability & Availability | 🔵 TrivialConsider a
concurrencygroup for the release pipeline.Multiple overlapping triggers (push to
main, tag push, manual dispatch) can start concurrent runs that both attempt to update/force-pushios-spmand publish npm packages for the same or racing versions. Aconcurrency:group keyed on something likerelease-${{ github.ref }}would prevent two release runs from clobbering each other'sios-spmtag/commit.🤖 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 @.github/workflows/npm_release.yml around lines 4 - 22, Add a workflow-level concurrency configuration for the release pipeline, using a stable group such as release-${{ github.ref }} and disabling cancellation of in-progress runs. Place it near the top-level workflow settings so push, tag, and manual triggers are serialized per reference and cannot race while updating ios-spm or publishing packages.build_npm_ios.sh (1)
70-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnquoted glob in
mv *.tgz ../.Shellcheck SC2035: if a filename produced by
npm packever starts with-(unlikely but possible with certain scoped/prerelease names),mvcould misinterpret it as an option.🧹 Proposed fix
pushd "$OUTPUT_DIR" npm pack -mv *.tgz ../ +mv ./*.tgz ../ popd🤖 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 `@build_npm_ios.sh` around lines 70 - 75, In the npm packaging block after npm pack, update the mv command to safely handle generated archive names by preventing option parsing and avoiding the unquoted glob; use an explicit end-of-options marker and quote the resolved package path while preserving the move to the parent directory.Source: Linters/SAST tools
🤖 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 @.github/workflows/npm_release.yml:
- Around line 54-56: Disable implicit package-manager caching on all four
flagged setup-node steps by adding package-manager-cache: false to the with
configuration for the setup, github-release, spm-update, and verify-spm jobs.
Keep the existing pinned action versions and node-version settings unchanged.
- Around line 241-297: Remove the `|| true` from the “Zip dSYMs” step so any
`zip` failure causes the workflow to fail. Update the command to safely handle
filenames and ensure failures from individual dSYM archives propagate instead of
allowing release publication with incomplete symbol assets.
---
Nitpick comments:
In @.github/workflows/npm_release.yml:
- Around line 149-168: Update the “Build (${{ matrix.target }})” step to pass
matrix.script through the step’s env configuration, then invoke npm using the
environment variable rather than interpolating matrix.script directly in run.
Preserve the existing build behavior while ensuring the matrix value is treated
as shell data.
- Around line 4-22: Add a workflow-level concurrency configuration for the
release pipeline, using a stable group such as release-${{ github.ref }} and
disabling cancellation of in-progress runs. Place it near the top-level workflow
settings so push, tag, and manual triggers are serialized per reference and
cannot race while updating ios-spm or publishing packages.
In `@build_npm_ios.sh`:
- Around line 70-75: In the npm packaging block after npm pack, update the mv
command to safely handle generated archive names by preventing option parsing
and avoiding the unquoted glob; use an explicit end-of-options marker and quote
the resolved package path while preserving the move to the parent directory.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 32cac31c-8d89-4311-917e-787c0c7515ec
📒 Files selected for processing (14)
.github/workflows/npm_release.ymlbuild_all_ios.shbuild_all_vision.shbuild_npm_ios.shbuild_npm_vision.shbuild_spm_artifacts.shproject-template-ios/__PROJECT_NAME__.xcodeproj/project.pbxprojproject-template-ios/internal/nativescript-build.xcconfigproject-template-vision/__PROJECT_NAME__.xcodeproj/project.pbxprojproject-template-vision/internal/nativescript-build.xcconfigscripts/generate-spm-manifest.mjsscripts/generate-spm-probe.mjsscripts/stamp-template-local-spm.mjsscripts/stamp-template-version.mjs
setup-node v6 enables dependency caching by default when it detects a package manager, and this workflow runs on push to main — so the cache is written to the default-branch scope every job can read. A poisoned cache is worst in the jobs that mint the ios-spm App token, hold contents:write, or publish with id-token:write, and the build job feeds npm install output into the shipped artifact. Set package-manager-cache: false on every setup-node step in the release workflow (also restores the pre-v6 no-cache behavior).
NathanWalker
force-pushed
the
feat/spm-release-reland
branch
from
5393b34 to
348b4f5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Verify channels per target (ios, visionos) and ensure manifest never contains a target that it was not intended for (next tags, etc.)
Summary by CodeRabbit