feat: add the upgrade command#24
Merged
Merged
Conversation
extractZip had no test coverage even though it is the only extraction path a real Windows manual install exercises, and a stale skip comment claimed otherwise. Also add coverage for checksums.txt downloading successfully but omitting an entry for our asset, which must fail closed rather than pass silently.
Previously a dev/unparseable build under --json fell into the dev-build branch, printed human text to stderr, and returned nil with empty stdout and exit 0 — leaving a parsing script unable to distinguish 'no update available' from 'the command did nothing'. --json now always emits a report object, even when the current version can't be parsed.
After a manual replacement, nothing re-ran the new binary to confirm the swap took effect. If another commitbrief comes earlier on the user's PATH, the file swap succeeds but the command they type is still the old binary, and 'Upgraded to vX.Y.Z' would report success regardless. verifyReplacement re-runs <exe> --version and warns (never fails) on a mismatch; manual path only, since a package manager may relocate its own binary. reportsVersion trims the tag's leading v before comparing, since goreleaser injects the version without one.
Final review fix wave on the upgrade command before merge: - Download used the 30s whole-request API client timeout, which counts time spent reading the body; a slow connection on a ~12 MiB archive failed permanently. Downloads now use a separate client with no whole-request timeout, bounded by a 30s ResponseHeaderTimeout instead. The API client's own timeout is restored to the originally specified 10s. - verifyReplacement's mismatch warning blamed PATH shadowing, but it execs the resolved binary directly and cannot observe PATH. Split into two independent checks: a version-mismatch warning that says only what it knows, and a new PATH-shadow check (LookPath + EvalSymlinks) that can actually detect a shadowing commitbrief and names both paths. - CleanupStale only swept the Windows .old file; interrupted manual installs left .commitbrief-dl-*/.commitbrief-bin-* scratch files behind forever. It's now shared across platforms and also sweeps those temps. - --check skipped the write-permission gate on the manual path, so it could report an install that a real run would refuse; it now runs the same preflight and warns without failing. - A malformed API response was reported as a network failure; it now maps to a distinct ErrBadResponse sentinel and message. - Fixed a tautological mode-preservation test and removed a dead no-op string replacement in path normalization. Docs (.ssot, wiki) corrected to match the accurate two-check verify rationale and the --check permission-gate behavior.
…shadow check
Second review round on the timeout/verify fixes from the previous commit:
- Assets' transport was a bare &http.Transport{}, not DefaultTransport with
ResponseHeaderTimeout changed. A zero-valued Transport drops Proxy (so
HTTPS_PROXY/HTTP_PROXY was silently ignored for downloads, while the API
client still honored it — a proxied user would see an update detected and
then fail to download it) and DialContext/TLSHandshakeTimeout (so a
blackholed connect or stalled handshake had no bound at all, since
ResponseHeaderTimeout only starts after connect+TLS finish). Now built via
http.DefaultTransport.(*http.Transport).Clone() with only
ResponseHeaderTimeout overridden.
- The download test asserted nothing about NewClient's actual client — it
overrode Assets with its own and used a delay far short of any timeout it
claimed to exercise, so it passed even against a broken Assets config.
Replaced with a structural test that reads NewClient's fields directly;
kept a corrected functional test alongside it.
- shadowingPath compared paths with ==, which can false-positive on Windows
over letter case or separator style that EvalSymlinks doesn't normalize.
Exported detect.go's existing normalizePath logic as SamePath and used it
instead — normalizePath itself is untouched.
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.
Adds
commitbrief upgrade [--check], which checks GitHub Releases for a newer version and installs it using the right mechanism for how the binary was installed. Design: ADR-0034.Behavior
brew upgrade/scoop update/go install …@latest. Overwriting a manager-owned binary desynchronizes its metadata, so the next upgrade would conflict or silently revert.checksums.txt. Unix uses a single atomic rename; Windows renames the running.exeaside and rolls back on failure, naming the recovery path if the rollback itself also fails.sudoitself. This gate is also what protects distro packages (nix/AUR/apt) that detection can only classify as "manual" — a wrong guess produces a clear error, never a broken install.commitbriefshadows it earlier onPATH. Both are warnings; neither turns a completed upgrade into a failure.--checkreports without installing and always exits 0.--jsonimplies--check.Privacy
The version check runs only when the user invokes the command — no automatic check, no nudge, no config key, no telemetry. ADR-0034 records that this narrows
system-context.md's previously unqualified "no update check" to "no automatic update check", and adds the corresponding egress-table row.Honest limitation, recorded in the ADR:
checksums.txtis unsigned, so the trust anchor is TLS to github.com. Verification catches a corrupt or truncated download, not a compromised release. Release-artifact signing (cosign / goreleasersigns:) is listed as future work.Notes
man/commitbrief-guard.1andman/commitbrief-mcp.1appear as new files: those pages had never been committed, andmake manpageregenerates the whole tree.[Unreleased]), the.ssot/contracts/architecture set, ADR-0034, and a newwiki/Upgrade-command.md(the wiki is a separate repo, pushed by hand).make checkis green: gofmt, vet, golangci-lint,go test ./..., release-check, i18n-check (EN/TR parity), spdx-check (274 files), gosec.