diff options
| author | Paul Buetow <paul@buetow.org> | 2026-07-22 18:53:43 +0300 |
|---|---|---|
| committer | Paul Buetow <paul@buetow.org> | 2026-07-22 18:53:43 +0300 |
| commit | 456cb2d3be55d431ba507367764ccfd13b13b4b4 (patch) | |
| tree | 71c241dcf9fd963661023779e8edefc30ad7a4be /internal/release | |
| parent | 23ecaa2c7f731a4f7188aec2404594641faf2f7f (diff) | |
fix(errcheck): handle all unchecked error returns without changing behaviormain
errcheck ./... flagged unchecked HTTP response Close(), file Close(),
os.RemoveAll(), fmt.Scanln(), and fmt.Fprintf() calls across codeberg,
github, release, showcase, and sync. None of these were bugs causing
incorrect behavior today, but leaving them unchecked hid real failure
modes (e.g. a lagging NFS mount failing a file Close() after writes,
which this codebase has hit before per commit 23ecaa2).
- internal/codeberg/codeberg.go, internal/github/github.go,
internal/release/release.go: added a small closeResponseBody(resp)
helper per package and used it for all deferred resp.Body.Close()
calls. The body is always fully read (or abandoned on an earlier
error) by the time Close() runs, so the error is intentionally
discarded - matching the explicit `_ = ...` discard convention
already used elsewhere in this repo (e.g. showcase.go's
os.RemoveAll on the worktree-add failure path).
- internal/release/release.go: the two fmt.Scanln(&response) prompts
now explicitly discard the return values; a Scanln error already
leaves response == "", which the existing y/yes check already
treats as a safe decline, so behavior is unchanged.
- internal/showcase/code_extractor.go, images.go, language_detector.go:
added a shared closeFile(*os.File) helper (package showcase) for the
read-only file Close() calls, matching the same discard rationale.
copyFile's destination Close() is the one write-side case where a
close failure is real data-loss information, so it now uses a named
return to surface it via err without masking any earlier error.
- internal/showcase/showcase.go: the deferred os.RemoveAll(tempRoot)
now explicitly discards its error, matching the sibling `_ =` call
three lines above in the same function.
- internal/sync/branch_analyzer.go: GenerateDeleteScript's per-repository
fmt.Fprintf(file, ...) calls are now checked and wrapped with
fmt.Errorf(...: %w), matching the error-wrapping convention already
used by writeBranchDeletionBlock right below it in the same file
(the repeated repo-header writes were pulled into a new
writeDeleteScriptRepoHeader helper to keep this readable). The
script file's defer file.Close() now uses a named return so a close
failure is reported instead of silently discarded.
Added focused tests for the two behavior-relevant paths: copyFile's
missing-source/success paths after the named-return change, and
PromptConfirmation's empty-input-declines/explicit-yes behavior after
touching the Scanln call. Trivial defer-Close discards elsewhere are
not additionally unit tested per the task's guidance against
overengineering.
Verified: go build ./..., go vet ./..., errcheck ./... (clean), and
go test ./... all pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Diffstat (limited to 'internal/release')
| -rw-r--r-- | internal/release/release.go | 43 | ||||
| -rw-r--r-- | internal/release/release_prompt_test.go | 74 |
2 files changed, 103 insertions, 14 deletions
diff --git a/internal/release/release.go b/internal/release/release.go index fc414c9..5608f33 100644 --- a/internal/release/release.go +++ b/internal/release/release.go @@ -38,6 +38,15 @@ type Manager struct { aiTool string } +// closeResponseBody closes an HTTP response body. By the time this runs the +// body has already been fully read (or the caller is bailing out on an +// earlier error), so a close failure here cannot change the outcome that was +// already determined - the error is intentionally discarded rather than +// treated as actionable. +func closeResponseBody(resp *http.Response) { + _ = resp.Body.Close() +} + // NewManager creates a new release manager func NewManager(workDir string) *Manager { return &Manager{ @@ -79,7 +88,7 @@ func (m *Manager) EnsureCodebergReleasesEnabled(owner, repo string) error { if err != nil { return err } - defer resp.Body.Close() + defer closeResponseBody(resp) if resp.StatusCode != 200 { body, _ := io.ReadAll(resp.Body) return fmt.Errorf("failed to get repo info: %s - %s", resp.Status, string(body)) @@ -112,7 +121,7 @@ func (m *Manager) EnsureCodebergReleasesEnabled(owner, repo string) error { if err != nil { return err } - defer patchResp.Body.Close() + defer closeResponseBody(patchResp) if patchResp.StatusCode != 200 { pbody, _ := io.ReadAll(patchResp.Body) return fmt.Errorf("failed to enable releases: %s - %s", patchResp.Status, string(pbody)) @@ -544,7 +553,7 @@ func (m *Manager) GetGitHubReleases(owner, repo string) ([]string, error) { if err != nil { return nil, err } - defer resp.Body.Close() + defer closeResponseBody(resp) if resp.StatusCode == 404 { // Repository might not exist on GitHub @@ -588,7 +597,7 @@ func (m *Manager) GetCodebergReleases(owner, repo string) ([]string, error) { if err != nil { return nil, err } - defer resp.Body.Close() + defer closeResponseBody(resp) if resp.StatusCode == 404 { // Repository might not exist on Codeberg @@ -669,7 +678,7 @@ func (m *Manager) CreateGitHubRelease(owner, repo, tag, releaseNotes string) err if err != nil { return err } - defer resp.Body.Close() + defer closeResponseBody(resp) if resp.StatusCode != 201 { body, _ := io.ReadAll(resp.Body) @@ -722,7 +731,7 @@ func (m *Manager) CreateCodebergRelease(owner, repo, tag, releaseNotes string) e if err != nil { return err } - defer resp.Body.Close() + defer closeResponseBody(resp) if resp.StatusCode != 201 { body, _ := io.ReadAll(resp.Body) @@ -739,7 +748,7 @@ func (m *Manager) CreateCodebergRelease(owner, repo, tag, releaseNotes string) e probeReq.Header.Set("Authorization", "token "+m.codebergToken) } if probeResp, perr2 := httpclient.Do(probeReq); perr2 == nil { - defer probeResp.Body.Close() + defer closeResponseBody(probeResp) if probeResp.StatusCode == 200 { // Try to detect if releases are disabled var repoInfo struct { @@ -768,7 +777,7 @@ func (m *Manager) CreateCodebergRelease(owner, repo, tag, releaseNotes string) e if rerr != nil { return rerr } - defer retryResp.Body.Close() + defer closeResponseBody(retryResp) if retryResp.StatusCode != 201 { rbody, _ := io.ReadAll(retryResp.Body) return fmt.Errorf("failed to create Codeberg release after enabling releases: %s - %s", retryResp.Status, string(rbody)) @@ -812,7 +821,11 @@ func PromptConfirmation(message string) bool { fmt.Printf("%s [y/N]: ", message) var response string - fmt.Scanln(&response) + // Scanln can return an error for empty input (e.g. a bare newline) or if + // stdin is closed; either way response stays "" and the check below + // already treats that as a decline, so there is nothing extra to do + // with the error here. + _, _ = fmt.Scanln(&response) response = strings.ToLower(strings.TrimSpace(response)) return response == "y" || response == "yes" @@ -828,7 +841,9 @@ func PromptConfirmationWithNotes(message, releaseNotes string) bool { fmt.Printf("%s [y/N]: ", message) var response string - fmt.Scanln(&response) + // See PromptConfirmation: a Scanln error still leaves response == "", + // which the check below already treats as a decline. + _, _ = fmt.Scanln(&response) response = strings.ToLower(strings.TrimSpace(response)) return response == "y" || response == "yes" @@ -856,7 +871,7 @@ func (m *Manager) UpdateGitHubRelease(owner, repo, tag, releaseNotes string) err if err != nil { return err } - defer resp.Body.Close() + defer closeResponseBody(resp) if resp.StatusCode != 200 { body, _ := io.ReadAll(resp.Body) @@ -898,7 +913,7 @@ func (m *Manager) UpdateGitHubRelease(owner, repo, tag, releaseNotes string) err if err != nil { return err } - defer updateResp.Body.Close() + defer closeResponseBody(updateResp) if updateResp.StatusCode != 200 { body, _ := io.ReadAll(updateResp.Body) @@ -929,7 +944,7 @@ func (m *Manager) UpdateCodebergRelease(owner, repo, tag, releaseNotes string) e if err != nil { return err } - defer resp.Body.Close() + defer closeResponseBody(resp) if resp.StatusCode != 200 { body, _ := io.ReadAll(resp.Body) @@ -970,7 +985,7 @@ func (m *Manager) UpdateCodebergRelease(owner, repo, tag, releaseNotes string) e if err != nil { return err } - defer updateResp.Body.Close() + defer closeResponseBody(updateResp) if updateResp.StatusCode != 200 { body, _ := io.ReadAll(updateResp.Body) diff --git a/internal/release/release_prompt_test.go b/internal/release/release_prompt_test.go new file mode 100644 index 0000000..fdb1d27 --- /dev/null +++ b/internal/release/release_prompt_test.go @@ -0,0 +1,74 @@ +package release + +import ( + "os" + "testing" +) + +// withStdin temporarily replaces os.Stdin with r for the duration of fn, then +// restores the original value. Not run in parallel with other tests since +// os.Stdin is a shared global. +func withStdin(t *testing.T, r *os.File, fn func()) { + t.Helper() + + original := os.Stdin + os.Stdin = r + defer func() { os.Stdin = original }() + + fn() +} + +// TestPromptConfirmation_EmptyInputDeclines verifies that when Scanln +// returns an error (e.g. a bare newline, which fmt.Scanln reports as +// "unexpected newline"), PromptConfirmation still safely defaults to +// declining rather than panicking or hanging - this is the discarded-error +// behavior that PromptConfirmation's fmt.Scanln call relies on. +func TestPromptConfirmation_EmptyInputDeclines(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("failed to create pipe: %v", err) + } + defer func() { _ = r.Close() }() + + if _, err := w.WriteString("\n"); err != nil { + t.Fatalf("failed to write to pipe: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("failed to close pipe writer: %v", err) + } + + var got bool + withStdin(t, r, func() { + got = PromptConfirmation("Proceed?") + }) + + if got { + t.Fatal("PromptConfirmation() = true for empty input, want false") + } +} + +// TestPromptConfirmation_YesInputConfirms is a regression check that the +// happy path (explicit "y") still works after touching the Scanln call. +func TestPromptConfirmation_YesInputConfirms(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("failed to create pipe: %v", err) + } + defer func() { _ = r.Close() }() + + if _, err := w.WriteString("y\n"); err != nil { + t.Fatalf("failed to write to pipe: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("failed to close pipe writer: %v", err) + } + + var got bool + withStdin(t, r, func() { + got = PromptConfirmation("Proceed?") + }) + + if !got { + t.Fatal("PromptConfirmation() = false for \"y\" input, want true") + } +} |
