Make native package releases immutable and complete - #15
Conversation
|
Warning Review limit reached
Next review available in: 50 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 (6)
📝 WalkthroughWalkthroughChangesNative release evidence and immutable publication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant NativeArtifacts
participant release-evidence.ts
participant GitHubRelease
Workflow->>NativeArtifacts: download package and Homebrew artifacts
Workflow->>release-evidence.ts: assemble and verify release evidence
release-evidence.ts-->>Workflow: return provenance, checksums, and package subjects
Workflow->>GitHubRelease: compare existing release or create immutable release
GitHubRelease-->>Workflow: return release metadata and asset digests
Workflow->>release-evidence.ts: verify published release contents
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/workflow-provenance.test.ts (1)
84-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a substring check instead of an escaped dynamic
RegExp.All snippets are fully escaped literals, so the regex adds nothing but the dynamic-regexp lint warning.
♻️ Proposed simplification
- assert.match(workflow, new RegExp(snippet.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"))); + assert.ok(workflow.includes(snippet), `workflow is missing ${snippet}`);🤖 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 `@tests/workflow-provenance.test.ts` around lines 84 - 86, Replace the escaped dynamic RegExp assertion in the workflow snippet checks with a direct substring assertion using the existing workflow and snippet values. Preserve the current behavior of verifying that each literal snippet occurs within workflow.Source: Linters/SAST tools
.github/workflows/images-release.yml (1)
794-801: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider
persist-credentials: falseon checkout in the publishing job.This job holds
contents: writeandid-token: write; leaving the checkout token in.git/configwidens the blast radius of any subsequent step, and the checkout is only needed for the scripts. Same applies to therelease-assemblycheckout at line 744.🔒 Proposed hardening
- - uses: actions/checkout@v7 + - uses: actions/checkout@v7 + with: + persist-credentials: false🤖 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/images-release.yml around lines 794 - 801, Update the checkout steps in both the publishing job and the release-assembly job to set persist-credentials to false, including the checkout near the artifact download and the one referenced around the release-assembly section. Keep the existing checkout behavior otherwise unchanged.Source: Linters/SAST tools
scripts/release-evidence.ts (1)
141-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRelease completeness counts are duplicated instead of derived. The 11-row matrix requirement and the 47-asset publication check encode the same contract (
4 * rows + 3) in two places, plus a mirrored string intests/workflow-provenance.test.ts. Adding a distro or backend requires synchronized edits, and a mismatch only surfaces at publish time.
scripts/release-evidence.ts#L141-L158: name the expected row count as a shared constant (or accept it as a CLI/matrix-derived input) rather than inlining11..github/workflows/images-release.yml#L873-L877: derive the expected asset count from the assembled evidence/matrix instead of the literal47.As per coding guidelines: "When adding a distro or backend, update matrix validation in
scripts/image-matrix.ts" — keeping the completeness contract in one derived place preserves that single update point.🤖 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 `@scripts/release-evidence.ts` around lines 141 - 158, Update matrixRows in scripts/release-evidence.ts to use a shared or matrix-derived expected row count instead of the literal 11, preserving validation of the complete matrix. Update .github/workflows/images-release.yml lines 873-877 to calculate the expected asset count from the assembled evidence or matrix using the 4 × rows + 3 contract, replacing the literal 47; keep the completeness contract derived from the matrix validation in scripts/image-matrix.ts.Source: Coding guidelines
🤖 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/images-release.yml:
- Around line 309-315: Update the provenance handling around the “Namespace and
require BuildKit package provenance” step so the emitted in-toto subject uses
the exact matrix package filename expected by statementSubject(), rather than a
Docker PURL. Ensure each native-package provenance subjectName matches
package_file before renaming or assembling the evidence, while preserving the
existing provenance existence check.
In `@docs/publishing.md`:
- Around line 21-23: Update the native package SPDX contract sentence in the
publishing documentation to include `.apk` alongside `.deb` and `.pkg.tar.zst`,
preserving the requirement that SPDX scans and subjects reference the exact
package file.
In `@scripts/release-evidence.ts`:
- Around line 358-369: Update the asset-processing loop in the release
comparison flow around requireString(asset.digest, ...) to tolerate missing or
null digests: skip those assets from digest comparison or emit a non-fatal
warning instead of throwing. Preserve duplicate-name and count/mismatch
validation for assets with available digests.
---
Nitpick comments:
In @.github/workflows/images-release.yml:
- Around line 794-801: Update the checkout steps in both the publishing job and
the release-assembly job to set persist-credentials to false, including the
checkout near the artifact download and the one referenced around the
release-assembly section. Keep the existing checkout behavior otherwise
unchanged.
In `@scripts/release-evidence.ts`:
- Around line 141-158: Update matrixRows in scripts/release-evidence.ts to use a
shared or matrix-derived expected row count instead of the literal 11,
preserving validation of the complete matrix. Update
.github/workflows/images-release.yml lines 873-877 to calculate the expected
asset count from the assembled evidence or matrix using the 4 × rows + 3
contract, replacing the literal 47; keep the completeness contract derived from
the matrix validation in scripts/image-matrix.ts.
In `@tests/workflow-provenance.test.ts`:
- Around line 84-86: Replace the escaped dynamic RegExp assertion in the
workflow snippet checks with a direct substring assertion using the existing
workflow and snippet values. Preserve the current behavior of verifying that
each literal snippet occurs within workflow.
🪄 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: 89828ede-7bb9-4b52-96cb-3366faccf086
📒 Files selected for processing (13)
.github/workflows/images-precheck.yml.github/workflows/images-release.ymlTODO.mddocs/package-signing.mddocs/publishing.mddocs/release-checklist.mdscripts/image-matrix.tsscripts/release-evidence.tsscripts/verify-sbom-subject.tstests/image-matrix.test.tstests/release-evidence.test.tstests/sbom-subject.test.tstests/workflow-provenance.test.ts
1 participant
Summary
Validation
Local environment notes
Two untouched
client-readiness-smokecases remain timing-sensitive on macOS, and Docker Desktop cannot resolve the advertisedarchlinux:base-develamd64 manifest locally. Linux PR CI remains the authoritative gate for those existing checks. The TODO QA item stays open until PR CI passes.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests