Skip to content

fix: address code review findings across installer, auth, and brew#105

Merged
fullstackjam merged 2 commits into
mainfrom
fix/code-review-followups
May 25, 2026
Merged

fix: address code review findings across installer, auth, and brew#105
fullstackjam merged 2 commits into
mainfrom
fix/code-review-followups

Conversation

@fullstackjam
Copy link
Copy Markdown
Collaborator

Summary

  • installer — dead step* functions removed: `stepMacOS`, `stepPostInstall`, `stepShell`, `stepDotfiles`, `hasDotfiles` had no production callers; `Apply()` uses the `apply*` variants exclusively. The 14 `TestStep*` and 5 `TestHasDotfiles*` tests were exercising dead code, providing false coverage confidence.
  • installer — real coverage added: replaced the removed tests with equivalent tests on `applyShell`, `applyDotfiles`, and `Plan()` entry points.
  • auth — httputil bypass fixed: `pollOnce` was calling `httpClient.Get()` directly, skipping HTTP 429 + Retry-After handling. Now uses `httputil.Do()` consistently with `startAuthSession` in the same file.
  • brew — error propagation: `InstallWithProgress` now returns a non-nil error when packages permanently fail after retries (previously swallowed failures silently).
  • brew — context threading: `ctx context.Context` added as first parameter to `InstallWithProgress` and all internal brew helpers; `exec.CommandContext` used so brew subprocesses are cancellable.
  • brew — retry deduplication: `retryBrew()` helper extracted, eliminating duplicated retry-with-backoff boilerplate between `installFormulaWithError` and `installSmartCaskWithError`.
  • brew — truncation limit: `parseBrewError` default line truncation increased from 60 → 120 chars.
  • brew — disk estimation: `PreInstallChecks` now takes `(formulaeCount, caskCount int)` and uses 0.1 GB/formula vs 0.5 GB/cask instead of a flat 0.2 GB/pkg.
  • archtest baselines regenerated to reflect removed dead code and shifted line numbers.

Test plan

  • `go test ./internal/...` — all 21 packages pass
  • `go vet ./...` — clean
  • Archtest baseline regenerated with `ARCHTEST_UPDATE_BASELINE=1`

- installer: remove four dead step* functions (stepMacOS, stepPostInstall,
  stepShell, stepDotfiles, hasDotfiles) that had no production callers;
  tests in installer_test.go were exercising this dead code instead of the
  live apply* paths, producing false coverage
- installer: replace 14 TestStep* and 5 TestHasDotfiles* tests with
  equivalent tests on the real apply* functions and Plan() entrypoints
- auth: pollOnce now uses httputil.Do() instead of httpClient.Get(),
  consistent with startAuthSession(); HTTP 429 + Retry-After handling
  now applies to poll requests; archtest baseline updated
- brew: InstallWithProgress now returns a non-nil error when packages
  permanently fail after retries (was silently swallowing failures)
- brew: ctx context.Context threaded through InstallWithProgress and all
  internal brew helpers; exec.CommandContext used so installs are
  cancellable
- brew: retryBrew() helper extracted to deduplicate the retry-with-backoff
  loop shared by installFormulaWithError and installSmartCaskWithError
- brew: parseBrewError default line truncation increased from 60 to 120
  chars to preserve enough error context
- brew: PreInstallChecks(formulaeCount, caskCount int) uses type-aware
  estimates (0.1 GB/formula, 0.5 GB/cask) instead of a flat 0.2 GB/pkg
- archtest baselines regenerated to reflect removed dead code and shifted
  line numbers
@github-actions github-actions Bot added installer Package installation logic brew Homebrew related tests Tests only labels May 25, 2026
@fullstackjam fullstackjam merged commit 9950ddd into main May 25, 2026
12 checks passed
@fullstackjam fullstackjam deleted the fix/code-review-followups branch May 25, 2026 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

brew Homebrew related installer Package installation logic tests Tests only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant