setup-mistkit: pin to resolved revision#380
Conversation
swift-build@v1's SPM cache key is hashed from `swift package dump-package`'s canonical JSON. With the previous `branch: "<name>"` pin the dependency JSON didn't change when the upstream branch advanced, so a new MistKit commit on the same branch yielded a stale cache hit and consumer code that depended on new MistKit symbols failed to compile against pre-existing cached binaries. Resolve the input branch to its current HEAD commit via `git ls-remote` and write `revision: "<sha>"` into Package.swift instead. The package hash now changes whenever the branch moves, so the cache invalidates correctly. Falls back to the original `branch:` pin if the ref can't be resolved. This is the same shape as the explicit `git ls-remote` + cache-key fix that already exists in CelestraCloud's update-feeds.yml, lifted into the shared action so every consumer gets it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe setup-mistkit GitHub action is updated to resolve branch names to commit SHAs and pin MistKit dependencies by ChangesMistKit Dependency Pinning via Commit SHA
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
Code Review: PR #380 — setup-mistkit: pin to resolved revisionOverviewThis PR fixes a stale SPM build-cache bug in consumer CI (CelestraCloud, BushelCloud). By resolving the branch to its current HEAD SHA and writing The fix is correct and the motivation is well-understood. A few issues worth addressing before merge: Bug: git ls-remote pattern is too broadUnix step:
Recommendation: Scope to branches only: Unix: PowerShell: Minor: sed delimiter collisionThe Observation: silent fallback degradationThe fallback to Positive notes
Summary
The approach is correct. Scoping |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/actions/setup-mistkit/action.yml (1)
22-22: ⚡ Quick winReduce
git ls-remotebranch/tag ambiguity (currently low risk)The MistKit remote currently shows no branch/tag name collisions, and
git ls-remote ... mainreturns onlyrefs/heads/main; so thehead -n1ambiguity concern is unlikely to occur in practice. Usingrefs/heads/$BRANCHis still a small intent-explicit improvement.Suggested refinement for explicit branch matching
- REF=$(git ls-remote https://github.com/brightdigit/MistKit.git "$BRANCH" | head -n1 | cut -f1) + REF=$(git ls-remote https://github.com/brightdigit/MistKit.git "refs/heads/$BRANCH" | head -n1 | cut -f1)🤖 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/actions/setup-mistkit/action.yml at line 22, The git ls-remote invocation that sets REF should explicitly match branch refs to avoid tag/branch ambiguity: update the command that assigns REF (the line using REF=$(git ls-remote ... "$BRANCH" | head -n1 | cut -f1)) to query refs/heads/$BRANCH instead of just "$BRANCH" so it only returns branch refs; keep the rest of the pipeline (head -n1 | cut -f1) intact to extract the commit SHA into REF.
🤖 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 @.github/actions/setup-mistkit/action.yml:
- Line 22: The git ls-remote invocation that sets REF should explicitly match
branch refs to avoid tag/branch ambiguity: update the command that assigns REF
(the line using REF=$(git ls-remote ... "$BRANCH" | head -n1 | cut -f1)) to
query refs/heads/$BRANCH instead of just "$BRANCH" so it only returns branch
refs; keep the rest of the pipeline (head -n1 | cut -f1) intact to extract the
commit SHA into REF.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1256399-2782-4011-8819-dd2a3fd8f497
📒 Files selected for processing (1)
.github/actions/setup-mistkit/action.yml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #380 +/- ##
==========================================
- Coverage 68.95% 67.66% -1.29%
==========================================
Files 111 111
Lines 2641 2641
==========================================
- Hits 1821 1787 -34
- Misses 820 854 +34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code ReviewOverviewReplaces Root-cause fix: Clear, well-motivated, and the fallback is a sensible defensive choice. Positives
Suggestions / Issues1. No timeout on
# Unix
REF=$(timeout 10 git ls-remote https://github.com/brightdigit/MistKit.git "$BRANCH" 2>/dev/null | head -n1 | cut -f1)
# PowerShell — harder, but a job-level timeout on the step is an alternativeA 10-second timeout keeps CI fast on network hiccups and falls through to the 2. REF is not validated as a SHA (low risk, defensive suggestion)
if [[ "$REF" =~ ^[0-9a-f]{40}$ ]]; then
REQ='revision: "'"$REF"'"'
...3. The
4. Fallback log message could be more informative (cosmetic) Consider adding that this degrades cache-invalidation behavior so the operator knows what they're accepting: 5. Windows: PowerShell's AssessmentApprove with minor suggestions. The fix is correct and the fallback is sound. Items 1–2 above are worth considering before merging; 3–4 are cosmetic. No blocking issues. |
Code Review: PR #380 — setup-mistkit: pin to resolved revisionOverviewThis PR switches the Issues / Concerns1. Potential shell injection —
|
Code Review — PR #380: setup-mistkit: pin to resolved revisionOverviewSmall but impactful CI/CD fix. The root cause (SPM cache keyed on Changes
IssuesNone. The implementation is clean:
SummaryClean, focused fix with correct fallback behavior. ✅ |
Code Review — setup-mistkit: pin to resolved revision (#380)Small, targeted fix with clear motivation. A few notes: Correctness: works correctly
Minor: network latency on every CI setup step
Minor:
|
Code ReviewPR: OverviewThis is a well-motivated fix for a real CI caching problem. Correctness
Issues & Suggestions1. Missing -REF=$(git ls-remote https://github.com/brightdigit/MistKit.git "$BRANCH" | head -n1 | cut -f1)
+REF=$(git ls-remote --heads https://github.com/brightdigit/MistKit.git "$BRANCH" | head -n1 | cut -f1)Without Same note for the PowerShell step: -$ref = (git ls-remote https://github.com/brightdigit/MistKit.git $branch | Select-Object -First 1) -split "`t" | Select-Object -First 1
+$ref = (git ls-remote --heads https://github.com/brightdigit/MistKit.git $branch | Select-Object -First 1) -split "`t" | Select-Object -First 12. BRANCH='${{ inputs.branch }}'GitHub Actions expands 3. Fallback log message could be more visible (nit) The fallback prints to stdout ( echo "::warning::Could not resolve $BRANCH to a commit; falling back to branch pin"Test CoverageNo automated tests exist for this action (normal for CI plumbing). The PR description's manual test plan is appropriate — re-running a consumer workflow with a no-op MistKit commit to verify a cache miss is the right validation. Summary✅ Approve with minor suggestions. The core change is correct and solves a real problem. Adding 🤖 Reviewed with Claude Code |
1 participant
Summary
setup-mistkitwritesrevision: "<sha>"into Package.swift instead ofbranch: "<name>"so swift-build@v1's package-hash invalidates when MistKit advances.git ls-remote; falls back tobranch:if resolution fails.v1.0.0-beta.2(Tag and validate ambiguous FieldValue scalar types (#375) #377); this lands the same change onmainso consumers referencing@main(BushelCloud, CelestraCloud) pick it up.Why
CelestraCloud PR brightdigit/CelestraCloud#36 was failing on every Linux/Windows job with errors like
value of type 'RecordInfo' has no member 'get'andtype '_ErrorCodeProtocol' has no member 'recordOperationFailed'. Root cause: swift-build@v1 hashesswift package dump-packagefor the SPM cache key. Withbranch:pinning, that JSON is constant for a given branch name, so a stale.build/from before #372 (RecordResult) landed kept being restored. Macros macOS passes because Xcode DerivedData cache also keys ongithub.sha.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit