Verify update release assets#84
Merged
Merged
Conversation
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. WalkthroughThe PR implements platform-specific release asset validation for the update checker. It adds ChangesRelease Asset Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
docs/UPDATE.md (1)
20-20: ⚡ Quick winInconsistent capitalization of
Options.Endpoint.Line 16 uses
Options.Endpoint(capitalized, suggesting the Go struct field), but this line usesoptions.endpoint(lowercase). For consistency and clarity, use the same capitalization as line 16.📝 Proposed consistency fix
-`options.endpoint` and `ZERO_UPDATE_RELEASE_URL` may be a full URL or an `owner/repo` slug. +`Options.Endpoint` and `ZERO_UPDATE_RELEASE_URL` may be a full URL or an `owner/repo` slug.🤖 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 `@docs/UPDATE.md` at line 20, The text is inconsistent: change the lowercase `options.endpoint` to match the earlier capitalization `Options.Endpoint` for consistency with the Go struct reference; update the sentence so it reads using `Options.Endpoint` (and leave `ZERO_UPDATE_RELEASE_URL` as-is) to match line 16's usage of `Options.Endpoint`.internal/update/update_test.go (3)
73-75: ⚡ Quick winSplit compound assertion for better test failure diagnostics.
The single assertion checking multiple ReleaseAsset fields makes it difficult to identify which specific field caused a failure. Consider splitting into separate assertions:
- if !result.ReleaseAsset.Verified || result.ReleaseAsset.ArchiveName != "zero-v0.2.0-linux-x64.tar.gz" || result.ReleaseAsset.ChecksumName != "zero-v0.2.0-linux-x64.tar.gz.sha256" { - t.Fatalf("unexpected release asset check: %#v", result.ReleaseAsset) - } + if !result.ReleaseAsset.Verified { + t.Fatalf("ReleaseAsset.Verified = false, want true") + } + if result.ReleaseAsset.ArchiveName != "zero-v0.2.0-linux-x64.tar.gz" { + t.Fatalf("ReleaseAsset.ArchiveName = %q, want zero-v0.2.0-linux-x64.tar.gz", result.ReleaseAsset.ArchiveName) + } + if result.ReleaseAsset.ChecksumName != "zero-v0.2.0-linux-x64.tar.gz.sha256" { + t.Fatalf("ReleaseAsset.ChecksumName = %q, want zero-v0.2.0-linux-x64.tar.gz.sha256", result.ReleaseAsset.ChecksumName) + }🤖 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 `@internal/update/update_test.go` around lines 73 - 75, Split the compound assertion that checks result.ReleaseAsset.Verified, result.ReleaseAsset.ArchiveName and result.ReleaseAsset.ChecksumName into three separate assertions so failures show which field failed; specifically replace the single if that Fatalfs with three checks that call t.Fatalf (or t.Errorf) with clear messages referencing each symbol (result.ReleaseAsset.Verified, result.ReleaseAsset.ArchiveName, result.ReleaseAsset.ChecksumName) and the expected values ("true", "zero-v0.2.0-linux-x64.tar.gz", "zero-v0.2.0-linux-x64.tar.gz.sha256").
174-174: ⚡ Quick winConsider improving JSON payload readability.
The URL-encoded JSON payload is difficult to read and maintain. Consider using a helper function or multi-line raw string:
Optional refactor for readability
+ releaseJSON := `{ + "tag_name": "v0.2.0", + "html_url": "https://example.test/release", + "assets": [ + {"name": "zero-v0.2.0-linux-x64.tar.gz", "browser_download_url": "https://example.test/zero-v0.2.0-linux-x64.tar.gz"}, + {"name": "zero-v0.2.0-linux-x64.tar.gz.sha256", "browser_download_url": "https://example.test/zero-v0.2.0-linux-x64.tar.gz.sha256"} + ] + }` + payload := url.QueryEscape(releaseJSON) - payload := url.QueryEscape(`{"tag_name":"v0.2.0","html_url":"https://example.test/release","assets":[{"name":"zero-v0.2.0-linux-x64.tar.gz","browser_download_url":"https://example.test/zero-v0.2.0-linux-x64.tar.gz"},{"name":"zero-v0.2.0-linux-x64.tar.gz.sha256","browser_download_url":"https://example.test/zero-v0.2.0-linux-x64.tar.gz.sha256"}]}`)🤖 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 `@internal/update/update_test.go` at line 174, Replace the single-line, URL-encoded JSON literal used to build payload in the test with a readable multi-line raw string or a small helper function; e.g., extract the JSON into a variable or helper (e.g., releaseJSON or buildReleasePayload) and then call url.QueryEscape(releaseJSON) where payload is assigned, keeping the final value assigned to payload via url.QueryEscape(...) so tests behave the same but the JSON is easy to read and maintain.
258-258: ⚡ Quick winSplit compound assertion for clearer test failure diagnostics.
Similar to TestCheckReportsAvailableUpdate, this compound assertion makes it difficult to identify which field failed:
- if check.ArchiveName != tt.archiveName || check.ChecksumName != tt.archiveName+".sha256" || check.Platform != tt.platform || check.Arch != tt.arch { - t.Fatalf("expectedAssetCheck = %#v, want archive %s", check, tt.archiveName) - } + if check.ArchiveName != tt.archiveName { + t.Errorf("ArchiveName = %q, want %q", check.ArchiveName, tt.archiveName) + } + if check.ChecksumName != tt.archiveName+".sha256" { + t.Errorf("ChecksumName = %q, want %q", check.ChecksumName, tt.archiveName+".sha256") + } + if check.Platform != tt.platform { + t.Errorf("Platform = %q, want %q", check.Platform, tt.platform) + } + if check.Arch != tt.arch { + t.Errorf("Arch = %q, want %q", check.Arch, tt.arch) + }🤖 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 `@internal/update/update_test.go` at line 258, The test currently uses a single compound condition comparing check.ArchiveName, check.ChecksumName, check.Platform, and check.Arch to tt.archiveName/tt.platform/tt.arch which hides which field failed; update the assertion in Test (file internal/update/update_test.go) to split into individual checks—one assertion each for check.ArchiveName == tt.archiveName, check.ChecksumName == tt.archiveName+".sha256", check.Platform == tt.platform, and check.Arch == tt.arch—using t.Errorf or t.Fatalf with concise messages naming the expected and actual values so failures indicate exactly which field mismatched.
🤖 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 `@docs/UPDATE.md`:
- Line 20: The text is inconsistent: change the lowercase `options.endpoint` to
match the earlier capitalization `Options.Endpoint` for consistency with the Go
struct reference; update the sentence so it reads using `Options.Endpoint` (and
leave `ZERO_UPDATE_RELEASE_URL` as-is) to match line 16's usage of
`Options.Endpoint`.
In `@internal/update/update_test.go`:
- Around line 73-75: Split the compound assertion that checks
result.ReleaseAsset.Verified, result.ReleaseAsset.ArchiveName and
result.ReleaseAsset.ChecksumName into three separate assertions so failures show
which field failed; specifically replace the single if that Fatalfs with three
checks that call t.Fatalf (or t.Errorf) with clear messages referencing each
symbol (result.ReleaseAsset.Verified, result.ReleaseAsset.ArchiveName,
result.ReleaseAsset.ChecksumName) and the expected values ("true",
"zero-v0.2.0-linux-x64.tar.gz", "zero-v0.2.0-linux-x64.tar.gz.sha256").
- Line 174: Replace the single-line, URL-encoded JSON literal used to build
payload in the test with a readable multi-line raw string or a small helper
function; e.g., extract the JSON into a variable or helper (e.g., releaseJSON or
buildReleasePayload) and then call url.QueryEscape(releaseJSON) where payload is
assigned, keeping the final value assigned to payload via url.QueryEscape(...)
so tests behave the same but the JSON is easy to read and maintain.
- Line 258: The test currently uses a single compound condition comparing
check.ArchiveName, check.ChecksumName, check.Platform, and check.Arch to
tt.archiveName/tt.platform/tt.arch which hides which field failed; update the
assertion in Test (file internal/update/update_test.go) to split into individual
checks—one assertion each for check.ArchiveName == tt.archiveName,
check.ChecksumName == tt.archiveName+".sha256", check.Platform == tt.platform,
and check.Arch == tt.arch—using t.Errorf or t.Fatalf with concise messages
naming the expected and actual values so failures indicate exactly which field
mismatched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e59e1d4-611d-4ab9-af12-035ee3a6ebd2
📒 Files selected for processing (4)
docs/UPDATE.mdinternal/cli/app_test.gointernal/update/update.gointernal/update/update_test.go
gnanam1990
approved these changes
Jun 6, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the release-asset verification changes. The expected archive/checksum naming matches the current installer and package-release contract for linux, macOS, and Windows, and the CLI output/JSON coverage exercises the new releaseAsset payload.
Validation run locally:
GOCACHE=/tmp/zero-go-cache go test ./internal/update -run 'Normalize|CheckReportsAvailableUpdate|CheckReturnsFetchError|CheckRespectsContextCancellation|CheckRejectsNegativeTimeout|CheckRejectsMissingTagName|CheckRejectsInvalidLatestVersion|CheckFallsBackReleaseURL|CheckFetchesDataEndpoint|CheckRejectsMissingReleaseAssets|ExpectedAssetCheck|ResolveEndpoint|FormatResult'GOCACHE=/tmp/zero-go-cache go test ./internal/cli -run Updateenv GOCACHE=/tmp/zero-go-cache ZERO_UPDATE_RELEASE_URL=<macos-arm64 data release> go run -ldflags '-X github.com/Gitlawb/zero/internal/cli.version=0.1.0' ./cmd/zero update --check --jsongit diff --check origin/main...HEADGOCACHE=/tmp/zero-go-cache go test ./...
One broad-suite run initially timed out in existing provider-command config tests, but the exact failing tests passed in isolation and the full suite passed on rerun. CI is also green across Ubuntu, macOS, Windows, Performance Smoke, Zero Review, and CodeRabbit.
Collaborator
BlockersNone found. Non-Blocking
Looks Good
Verdict: Approve — Clean release-asset verification for update checks, aligned with the current archive/checksum naming contract. |
Vasanthdev2004
approved these changes
Jun 6, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Approved: clean release-asset verification aligned with the archive/checksum naming contract.
Collaborator
Review: PR #84 — Verify update release assets (anandh8x / Anandan)URL: #84 (MERGED) Ownership & Split ComplianceDirect hit on Anandan's explicit lane:
Also fulfills the "Platform/release contract" DRI and the rule that his PRs must include "user-runnable command behavior" or "install/update/release artifact verification". What Landed
No change to actual download/replace logic — this is metadata verification only (correct scope for "trust" without full updater yet). Quality Notes (on current main)
Minor Observations
Verdict (Post-Merge)Well executed, exactly the platform product behavior required by the split. This (together with #81 sandbox platform report, #86/#85 build/release Go moves, #75 PR automation) shows Anandan delivering on the "not only docs/CI" rule. If any follow-up (e.g. wiring the checksum verification into actual No blocking issues observed on main. Non-gnanam1990 PR review. See sibling reviews for open work and other platform PRs. |
This was referenced Jun 6, 2026
This was referenced Jun 6, 2026
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
3 participants
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.
Summary
Scope
This is metadata verification only. It does not download release assets or replace the local binary.
Tests
Summary by CodeRabbit
New Features
update --checknow validates platform-specific release assets (archive and checksum) before reporting available updates.Documentation
update --checkreturn codes and release validation requirements.