Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces golangci-lint to the project (config, Makefile target, and CI integration) and updates Go code to satisfy/quiet newly enabled linter checks (formatting, blank lines before returns, and targeted //nolint annotations).
Changes:
- Add a
.golangci.ymlconfiguration and amake linttarget, and run golangci-lint in CI. - Apply linter-driven formatting changes across Go packages (notably extra newlines before returns).
- Add
//nolintannotations forerrcheck/gosecfindings (e.g.,Close()and some file/HTTP operations).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds a lint target to run golangci-lint. |
| .golangci.yml | Introduces golangci-lint configuration and enabled linters. |
| .github/workflows/_ci.yml | Replaces go vet step with golangci-lint action invocation. |
| internal/ui/spinner.go | Formatting-only change (newline). |
| internal/keyring/keyring.go | Formatting-only changes (newlines). |
| internal/crypto/age.go | Formatting-only changes (newlines). |
| internal/config/paths_dev.go | Formatting-only change (newline). |
| internal/config/config.go | Adds gosec/errcheck suppressions and formatting updates around token persistence. |
| internal/auth/oidc.go | Adds Close() errcheck suppressions, removes an empty status-code block, and formats function signature. |
| internal/api/user.go | Formatting-only change (newline). |
| internal/api/transfer.go | Formatting changes and line-wrapping for function signatures; minor style updates. |
| internal/api/login.go | Adds Close() errcheck suppressions and minor formatting. |
| internal/api/client.go | Adds gosec/errcheck suppressions and formatting changes. |
| cmd/transfer.go | Adds errcheck/gosec suppressions and formatting changes in CLI transfer flows. |
| cmd/insecure_dev.go | Wraps a long flag description line. |
| cmd/auth.go | Adds errcheck/gosec suppressions and wraps a long flag description line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| - name: golangci-lint | ||
| uses: golangci/golangci-lint-action@v9 | ||
| with: | ||
| version: v2.11.3 |
There was a problem hiding this comment.
The CI workflow pins golangci-lint to latest, which can cause non-reproducible builds and sudden CI failures when a new golangci-lint release changes rules or defaults. Prefer pinning to a specific version (and updating it deliberately) to keep lint results stable.
| version: v2.11.3 | |
| version: v1.61.0 |
| - gosec # security issues | ||
| - lll # report long lines | ||
| - misspell # detect commonly misspelled words | ||
| - nlreturn # check for missing return statements in functions |
There was a problem hiding this comment.
The inline comment for the nlreturn linter is inaccurate: nlreturn enforces newline-before-return style, it does not check for “missing return statements”. Updating this comment would avoid confusion about why the linter is enabled.
| - nlreturn # check for missing return statements in functions | |
| - nlreturn # enforce newline-before-return style |
| - whitespace # check for whitespace issues | ||
|
|
||
| issues: | ||
| fix: true |
There was a problem hiding this comment.
Having issues.fix: true in the golangci-lint config can make lint runs apply auto-fixes implicitly (depending on golangci-lint version/config semantics), which is risky for CI and can lead to unexpected workspace mutations. Consider disabling auto-fix in CI (and running golangci-lint run --fix explicitly in a separate, developer-invoked target if desired).
| fix: true | |
| fix: false |
| f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer f.Close() | ||
| defer f.Close() //nolint:errcheck | ||
|
|
||
| return json.NewEncoder(f).Encode(tok) | ||
| return json.NewEncoder(f).Encode(tok) //nolint:gosec // G117: token storage is intentional |
There was a problem hiding this comment.
SaveToken writes to a file but suppresses Close() errors via //nolint:errcheck. For files opened for writing, Close() can surface final flush/fsync failures; ignoring it can silently lose token writes. Consider capturing and returning the close error (e.g., via a deferred close that updates the named return) instead of suppressing it.
| out, err := os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer out.Close() | ||
| defer out.Close() //nolint:errcheck | ||
|
|
There was a problem hiding this comment.
This function writes the decrypted file to disk but suppresses out.Close() errors via //nolint:errcheck. For output files, Close() can fail (flush/sync), and ignoring it can result in a corrupted/partial file without reporting an error. Prefer checking the close error and returning it if non-nil (often easiest with a named return + deferred close).
No description provided.