feat(update): add zero upgrade command to apply self-updates#461
feat(update): add zero upgrade command to apply self-updates#461PierrunoYT wants to merge 4 commits into
Conversation
Extends the existing `zero update --check` (which only reported an available release) with an actual install step: - `zero update --apply` / `zero upgrade` download, sha256-verify, and install the latest release. - npm installs delegate to `npm install -g @gitlawb/zero@latest`. - Standalone installs download the release archive, verify its checksum, extract it (zip-slip guarded), and atomically replace the running binary plus any pre-existing optional sandbox helpers. On Windows, the running exe is renamed aside (NTFS allows this even while it's executing) since it can't be overwritten directly; the leftover ".old" file is cleaned up on a later invocation. Closes Gitlawb#459.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds ChangesUpdate Apply Feature
OAuth secret file locking
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
The optional-helper assertion in TestApplyStandaloneUpdateReplacesBinary assumed every non-Windows platform has a linux-style "zero-seccomp" helper to refresh, but applyStandaloneUpdate correctly ships no optional helpers on macOS (matching scripts/postinstall.mjs). This made the test fail on macOS CI. Only assert the helper refresh on linux/windows.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/update.go (1)
61-68: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject
--targeton the apply path
internal/cli/update.go:61-68passes--targetthrough to the release check, butinternal/update/apply.gostill chooses the replacement binary name fromruntime.GOOSonly. That meanszero update --apply --target linux-arm64on a linux-amd64 host can replace the running binary with an incompatible arm64 build. Block--targetwhen applying, or thread the selected target through install as well.🤖 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/cli/update.go` around lines 61 - 68, Reject --target on the apply path by updating the update CLI flow in internal/cli/update.go so the --apply branch does not accept or forward a target into release selection; instead return a usage/error when options.target is set with apply, or pass the resolved target through the apply/install path consistently. Check the update command handler and the apply logic in internal/update/apply.go so the binary replacement name is not derived from runtime.GOOS alone when a target was requested.
🧹 Nitpick comments (3)
internal/update/apply.go (2)
162-172: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winOptional helper refresh failures are silently dropped.
_ = installBinary(source, destPath)discards any error when refreshing an existing sandbox helper (e.g. seccomp binary). A failed refresh leaves a stale helper in place with no indication to the user that the upgrade was incomplete — degrading the sandbox's security guarantees without any signal.♻️ Proposed fix
destPath := filepath.Join(targetDir, name) if _, err := os.Stat(destPath); err != nil { continue // only refresh helpers this install already has } - _ = installBinary(source, destPath) + if err := installBinary(source, destPath); err != nil { + fmt.Fprintf(os.Stderr, "[zero] warning: failed to update helper %s: %v\n", name, err) + } }🤖 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/apply.go` around lines 162 - 172, The optional helper refresh loop in apply.go is ignoring installBinary errors, so an existing sandbox helper can remain stale without any signal. Update the installBinary(source, destPath) call in the optionalBinaries refresh path to handle and propagate/log the error instead of discarding it, using the surrounding apply/update flow to surface a failed helper refresh to the user while keeping the optional-binary fallback behavior intact.
45-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove
CleanupStaleBinarybefore the "already up to date" early return.
CleanupStaleBinaryonly runs when an update is actually applied (Line 55, after the Line 41-43 early return). If the binary is already current, a leftover Windows.oldfile from a prior upgrade never gets cleaned until the next real upgrade — which could be a long time or never for a user who stops upgrading.♻️ Proposed fix
checkResult, err := Check(ctx, options) if err != nil { return ApplyResult{}, err } + if executablePath, err := os.Executable(); err == nil { + if resolved, err := filepath.EvalSymlinks(executablePath); err == nil { + executablePath = resolved + } + CleanupStaleBinary(executablePath) + } if !checkResult.UpdateAvailable { return ApplyResult{Result: checkResult, Message: "already up to date"}, nil } executablePath, err := os.Executable() - if err != nil { - return ApplyResult{}, fmt.Errorf("resolve current executable: %w", err) - } - if resolved, err := filepath.EvalSymlinks(executablePath); err == nil { - executablePath = resolved - } - // Best-effort: remove a "<binary>.old" left behind by a previous Windows - // replaceBinary call now that enough time (a whole separate invocation) - // has passed for the old process to have released the file. - CleanupStaleBinary(executablePath) + if err != nil { + return ApplyResult{}, fmt.Errorf("resolve current executable: %w", err) + } + if resolved, err := filepath.EvalSymlinks(executablePath); err == nil { + executablePath = resolved + }🤖 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/apply.go` around lines 45 - 56, Move the CleanupStaleBinary call in Apply so it runs before the “already up to date” early return path, not only after a successful update. Use the executablePath resolution logic in Apply to clean up the stale "<binary>.old" file as soon as the current binary path is known, while keeping the existing best-effort behavior and error handling unchanged.internal/cli/app_test.go (1)
1270-1389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM!
New tests reasonably cover apply text/JSON output,
upgradedefaulting to apply,--check+--applyrejection, and apply error propagation.Given the
--target+--applyconcern raised ininternal/cli/update.go, consider adding a regression test for that combination once addressed.🤖 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/cli/app_test.go` around lines 1270 - 1389, Add a regression test in TestRunUpdateApplyTextAndJSON or a new test around runWithDeps that covers the --target and --apply combination mentioned in update.go. The issue is that the current tests verify --apply, --check/--apply, and error handling, but do not assert the behavior when a target version is provided with --apply. Add a case using update.Options through appDeps.applyUpdate to ensure the CLI either rejects or correctly handles --target with --apply, and verify the expected exit code and stderr/output.
🤖 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 `@internal/update/apply.go`:
- Around line 214-243: The download path in downloadFile currently uses the
caller’s context directly, so zero upgrade can hang indefinitely if the archive
or checksum endpoint stalls. Add a bounded timeout for the apply download flow
by creating a timeout-aware context in update.Apply or by wrapping the request
context inside downloadFile, and make sure the timeout is applied consistently
to both fetches. Use the existing downloadFile helper and update.Apply
entrypoint to locate the change, and preserve normal error propagation when the
timeout is hit.
In `@internal/update/replace_windows.go`:
- Around line 14-25: The replaceBinary rollback path in replace_windows.go can
silently fail and leave targetPath missing after a failed os.Rename(newPath,
targetPath). Update replaceBinary to capture and check the error from restoring
oldPath back to targetPath, and if that rollback fails, return an error that
includes both the install failure and rollback failure so callers know manual
recovery may be needed. Keep the fix scoped to replaceBinary and its rename
error handling.
---
Outside diff comments:
In `@internal/cli/update.go`:
- Around line 61-68: Reject --target on the apply path by updating the update
CLI flow in internal/cli/update.go so the --apply branch does not accept or
forward a target into release selection; instead return a usage/error when
options.target is set with apply, or pass the resolved target through the
apply/install path consistently. Check the update command handler and the apply
logic in internal/update/apply.go so the binary replacement name is not derived
from runtime.GOOS alone when a target was requested.
---
Nitpick comments:
In `@internal/cli/app_test.go`:
- Around line 1270-1389: Add a regression test in TestRunUpdateApplyTextAndJSON
or a new test around runWithDeps that covers the --target and --apply
combination mentioned in update.go. The issue is that the current tests verify
--apply, --check/--apply, and error handling, but do not assert the behavior
when a target version is provided with --apply. Add a case using update.Options
through appDeps.applyUpdate to ensure the CLI either rejects or correctly
handles --target with --apply, and verify the expected exit code and
stderr/output.
In `@internal/update/apply.go`:
- Around line 162-172: The optional helper refresh loop in apply.go is ignoring
installBinary errors, so an existing sandbox helper can remain stale without any
signal. Update the installBinary(source, destPath) call in the optionalBinaries
refresh path to handle and propagate/log the error instead of discarding it,
using the surrounding apply/update flow to surface a failed helper refresh to
the user while keeping the optional-binary fallback behavior intact.
- Around line 45-56: Move the CleanupStaleBinary call in Apply so it runs before
the “already up to date” early return path, not only after a successful update.
Use the executablePath resolution logic in Apply to clean up the stale
"<binary>.old" file as soon as the current binary path is known, while keeping
the existing best-effort behavior and error handling unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 463c2172-e16d-4439-8833-8c0a14b786fb
📒 Files selected for processing (11)
internal/cli/app.gointernal/cli/app_test.gointernal/cli/update.gointernal/update/apply.gointernal/update/apply_test.gointernal/update/extract.gointernal/update/extract_test.gointernal/update/installmethod.gointernal/update/installmethod_test.gointernal/update/replace_other.gointernal/update/replace_windows.go
- Reject --target when combined with --apply/upgrade: applying installs onto the current machine, so a mismatched --target could replace the running binary with an incompatible-arch build. - Bound archive/checksum downloads with a dedicated timeout separate from the release-metadata check timeout, so a stalled connection can't hang zero upgrade forever. - Surface a failed rollback in the Windows replaceBinary path instead of swallowing it, so a botched install is reported rather than silently leaving the binary missing. - Run CleanupStaleBinary regardless of whether an update is available, so a leftover Windows ".old" file isn't stuck waiting for a future upgrade that may never come. - Report optional sandbox-helper refresh failures as warnings on ApplyResult (surfaced in text and JSON output) instead of discarding them silently.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approve — useful, and the self-update path is genuinely secure
Worth it: yes. A zero upgrade self-update is an oft-requested capability for a CLI shipped as both a standalone binary and an npm package (closes #459), and it doesn't reinvent anything — it extends the existing internal/update Check() pipeline with an apply step. Thanks @PierrunoYT.
I reviewed this primarily as a security change, since it replaces the running executable. It holds up — and I confirmed the load-bearing part by reading the source myself: in applyStandaloneUpdate, the flow is download archive → download checksum → release.VerifySHA256Checksum (which re-hashes the downloaded archive bytes and rejects on mismatch) → only then extract → install. Verification gates the swap; a mismatch returns before anything is extracted or replaced.
- Atomic + safe replacement: stage a
.newsibling on the same filesystem then rename; POSIX renames over the running inode, Windows renames the locked exe aside to.oldand rolls back on failure. - Downgrade-safe:
Applyonly proceeds when latest > current, and--targetis rejected with--apply, so it can't be steered to an older/arbitrary build. - Extraction hardened:
safeExtractPathrejects absolute paths and../escapes and refuses non-regular entries (symlinks/devices). - npm installs correctly delegate to
npm install -g @gitlawb/zero@latest.
Verified on Windows (worktree): build/gofmt/vet clean; internal/update + Update|Upgrade cli tests pass, including the checksum-mismatch rejection and zip-slip rejection tests.
Non-blocking residuals (future hardening, don't hold merge):
- No cryptographic signature — the checksum shares the archive's origin, so authenticity rests on GitHub + TLS. A detached signature (minisign/cosign) would be the one real authenticity upgrade. Optionally assert
https://on asset URLs so a user-overridden--endpointcan't pull over plaintext. - On Windows the optional helper binaries are also renamed aside to
.old, butCleanupStaleBinaryonly sweeps the main exe's.old, sozero-windows-command-runner.exe.oldetc. accumulate. Cosmetic disk leak.
CI: the lone red — Smoke (windows-latest) — failed only on TestLoadOrCreateSecretConcurrentConverges (the known oauth concurrency flake, unrelated; the fix for it is in #445). Touched packages built and tested green in that same run. Approving.
|
just check and fix the ci test @PierrunoYT |
I thought the fix is already in another PR? |
…ck CI TestLoadOrCreateSecretConcurrentConverges was failing this PR's Windows smoke job with "create token secret lock: ...Access is denied." — a pre-existing flake in internal/oauth, not in the upgrade code. On Windows a concurrent holder's os.Remove leaves the .lock in a delete-pending state, so an O_EXCL create races it with ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist; createSecretFile only treated ErrExist as contention and hard-failed otherwise. Treat os.ErrPermission as retryable too, mirroring acquireFileLock in lock.go. This is the same root-cause fix as Gitlawb#445 (diagnosis by @gnanam1990); folded in here to make this PR's CI green. Verified on Windows: the concurrent-converge test passes 30/30.
|
Pushed a small commit ( |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
VERDICT: approve
REGRESSION RISK: low
The PR extends the existing internal/update Check() pipeline with an apply step (zero update --apply / zero upgrade). The check-only path is unchanged. The apply path is new code that does not alter any existing code path, config schema, or user flow. The internal/oauth/encrypt.go change (treating os.ErrPermission as contention on Windows) is a separate fix bundled into this PR — it is a 5-line addition to an existing retry branch and does not alter the success path, but it is scope drift relative to the PR's stated purpose (closes #459).
Security review of the self-update path:
- Checksum verification:
release.VerifySHA256Checksumruns before extraction; a mismatch returns an error before any file is written. Test coverage confirms this (TestApplyStandaloneUpdateRejectsChecksumMismatch). - Archive extraction:
safeExtractPathinextract.gorejects absolute paths,..traversal, and non-regular/non-dir entry types (symlinks, devices). Both tar.gz and zip paths are covered with path-traversal rejection tests. - Binary replacement: POSIX uses atomic
os.Rename(same-filesystem). Windows uses rename-aside (target →.old, new → target) with rollback on failure — if the second rename fails, the original is restored; if restoration also fails, the error message includes the.oldpath so the user can recover manually. - Download: uses
http.DefaultClientwith a 5-minute timeout. No TLS pinning, but this matches the existingCheck()pipeline's transport.
BUILD / TEST
go build ./...— passgo vet ./internal/update/... ./internal/cli/...— passgofmt -lon all changed files — cleango test ./internal/update/... -count=1— all 22 tests pass, including:TestExtractTarGzRejectsPathTraversal,TestExtractZipRejectsPathTraversal,TestApplyStandaloneUpdateReplacesBinary,TestApplyStandaloneUpdateRejectsChecksumMismatch,TestApplyReturnsNoopWhenUpToDatego test ./internal/cli/... -run TestRunUpdate— all 10 tests pass, including:--applytext/JSON output,--check+--applyconflict rejection,--target+--applyrejection, apply error reportingTestApplyStandaloneUpdateWarnsWhenHelperRefreshFailsskips on macOS (no optional helpers) — runs on Linux/Windows- CI all green including Windows Smoke (4m5s)
CONTRIBUTING
Author is a community contributor (PierrunoYT). The PR links Fixes #459. Issue #459 has a bug label but no issue-approved label. Per CONTRIBUTING.md, the issue-approved gate applies to community PRs. Not met. Additionally, the internal/oauth/encrypt.go change is unrelated to #459 (scope drift). Both are process violations, but the code is reviewed regardless as instructed. Maintainer's call on whether the scope expansion is acceptable.
FINDINGS
-
Nit —
internal/update/replace_windows.go:27-31— rollback failure error message could recommend manual recovery. If both the new-binary rename and the original-restore rename fail, the error message includes the.oldpath but does not suggestrename zero.exe.old zero.exeas a recovery step. A user who reads the error on a non-interactive CI runner may not know how to recover. Not a blocker — the path is reported. -
Nit —
internal/oauth/encrypt.go:114-121— scope drift. The WindowsERROR_ACCESS_DENIEDretry fix for concurrent secret creation is unrelated to thezero upgradefeature. It should ideally be a separate PR. The fix itself is correct (mirrorsacquireFileLockinlock.go) and safe, but bundling it here muddies the PR scope.
No security or correctness blockers found.
EXISTING REVIEWS
- Vasanthdev2004 (APPROVED): Reviewed primarily as a security review — confirmed the self-update path is genuinely secure, with checksum verification, zip-slip protection, and atomic replacement. Valid on the current head. Did not explicitly flag the oauth/encrypt.go scope drift; otherwise thorough.
- coderabbitai (APPROVED + 2 comments): Flagged (1)
--targetrejection with--applyas a stability concern — this is already tested (TestRunUpdateRejectsTargetWithApply) and the rejection is intentional; (2) the Windows rollback failure scenario inreplace_windows.go— valid concern, addressed as nit #1 above. Both findings are valid but non-blocking.
All reviews on the current head (e12fbd1). No stale reviews.
BOTTOM LINE
A well-tested, security-conscious self-update feature with proper checksum verification and archive extraction guards; safe to merge, with a note on the oauth/encrypt.go scope drift for maintainer awareness.
3 participants
Summary
Closes #459. Extends the existing
zero update --check(check-only) with an actual apply/install step:zero update --apply/zero upgradedownload, sha256-verify, and install the latest release.npm install -g @gitlawb/zero@latest..oldfile is cleaned up on a later invocation.Test plan
go build ./...,go vet ./...,gofmt -lcleango test ./internal/update/... ./internal/cli/...passes, including new tests for archive extraction (incl. zip-slip rejection), install-method detection, checksum-mismatch rejection, and CLI wiring (--apply,--check+--applyconflict,upgradealias)zero update --checkthenzero upgrade, confirmed the binary was downloaded, checksum-verified, and swapped in place🤖 Generated with Claude Code
Summary by CodeRabbit
upgradecommand that installs available updates by default.update --applyto install updates, with improved messaging and richer--jsonoutput.