feat: add linting to go backend#845
Conversation
dosubot
Bot
added
the
size:M
📝 WalkthroughWalkthroughThis PR introduces golangci-lint linting infrastructure, fixes functional error handling in OAuth flows, removes unused code, standardizes patterns, and systematically annotates intentionally ignored errors across the codebase using linter suppression comments. ChangesLinting and Code Quality
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/tinyauth/healthcheck.go (1)
69-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose response body before early returns to avoid leaks.
Line 73 registers
defer resp.Body.Close()too late. On non-200 responses (Line 69), the function returns without closingresp.Body.Suggested fix
resp, err := client.Do(req) if err != nil { return fmt.Errorf("failed to perform request: %w", err) } +defer resp.Body.Close() //nolint:errcheck if resp.StatusCode != http.StatusOK { return fmt.Errorf("service is not healthy, got: %s", resp.Status) } - -defer resp.Body.Close() //nolint:errcheck🤖 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 `@cmd/tinyauth/healthcheck.go` around lines 69 - 73, The response body is closed too late causing a leak on non-200 responses; ensure resp.Body is closed before any early return by moving the defer resp.Body.Close() (or calling resp.Body.Close() directly) immediately after the successful http request/response is obtained (i.e., right after resp is assigned) in the health check function so that the body is always closed even when the StatusCode != http.StatusOK check (the resp.StatusCode branch) returns early; look for the resp variable and the status check in the health check routine and adjust placement accordingly.
🧹 Nitpick comments (1)
internal/controller/user_controller_test.go (1)
182-182: ⚡ Quick winNarrow
staticchecksuppressions to avoid masking real findings.The current suppressions are broad (especially on large
runclosures). Prefer eliminating the reassignment pattern (e.g., localrec := httptest.NewRecorder()) or scoping//nolintonly to the minimal line that truly needs it.Also applies to: 204-204, 296-296, 319-319, 348-348, 357-357
🤖 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/controller/user_controller_test.go` at line 182, The test helper closure run (signature run func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder)) uses broad //nolint:staticcheck on the whole function; narrow the suppression by either refactoring the reassignment pattern (avoid reassigning recorder, e.g., create a new local rec := httptest.NewRecorder()) or move the //nolint:staticcheck comment to the specific line that triggers the linter warning (only on the single assignment/reassignment), and apply the same narrow change for the other occurrences around the run closures at the noted locations (204, 296, 319, 348, 357) so staticcheck warnings elsewhere remain visible.
🤖 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 @.golangci.yml:
- Around line 10-17: Remove the misconfigured exclusions block in the
golangci-lint config: delete the entire "exclusions" -> "rules" entries that use
the "text" field with literal "//nolint:errcheck" and "//nolint:staticcheck",
because golangci-lint matches source nolint directives directly and the "text"
matcher targets lint messages, not source comments; simply remove the
"exclusions" section (keys: exclusions, rules, linters, text) so the linter uses
native //nolint:<linter> handling.
In `@internal/bootstrap/app_bootstrap.go`:
- Line 295: The response body is being closed immediately with res.Body.Close()
instead of deferring it; change the call to defer res.Body.Close() (keeping the
//nolint:errcheck if desired) so the body is closed at function return and not
before subsequent checks—locate the res variable and the res.Body.Close() call
in the bootstrap logic and replace it with deferred close.
In `@internal/utils/fs_utils_test.go`:
- Line 21: Replace the hardcoded "/tmp/tinyauth_test_file" cleanup with a
test-scoped temp directory from t.TempDir(); create the test file path via
filepath.Join(t.TempDir(), "tinyauth_test_file") and update the deferred cleanup
to defer os.Remove(tempFilePath) (or drop the defer since t.TempDir()
auto-cleans), ensuring you add an import for "path/filepath" if needed and
update any references to the old hardcoded path (the
os.Remove("/tmp/tinyauth_test_file") call) accordingly.
In `@internal/utils/security_utils_test.go`:
- Line 22: Replace the hard-coded "/tmp/tinyauth_test_secret" cleanup with a
test-scoped temp path: call t.TempDir() in the test, construct the secret path
via filepath.Join(tmpDir, "tinyauth_test_secret"), use that path wherever the
test currently references the fixed string, and remove the explicit os.Remove
defer (or change it to defer os.Remove(secretPath) if you still want explicit
cleanup); also add an import for "path/filepath" if missing. This ensures the
test uses an isolated temp directory rather than a global /tmp path.
---
Outside diff comments:
In `@cmd/tinyauth/healthcheck.go`:
- Around line 69-73: The response body is closed too late causing a leak on
non-200 responses; ensure resp.Body is closed before any early return by moving
the defer resp.Body.Close() (or calling resp.Body.Close() directly) immediately
after the successful http request/response is obtained (i.e., right after resp
is assigned) in the health check function so that the body is always closed even
when the StatusCode != http.StatusOK check (the resp.StatusCode branch) returns
early; look for the resp variable and the status check in the health check
routine and adjust placement accordingly.
---
Nitpick comments:
In `@internal/controller/user_controller_test.go`:
- Line 182: The test helper closure run (signature run func(t *testing.T, router
*gin.Engine, recorder *httptest.ResponseRecorder)) uses broad
//nolint:staticcheck on the whole function; narrow the suppression by either
refactoring the reassignment pattern (avoid reassigning recorder, e.g., create a
new local rec := httptest.NewRecorder()) or move the //nolint:staticcheck
comment to the specific line that triggers the linter warning (only on the
single assignment/reassignment), and apply the same narrow change for the other
occurrences around the run closures at the noted locations (204, 296, 319, 348,
357) so staticcheck warnings elsewhere remain visible.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 96557668-e2ef-4515-bd37-1c00bb237e77
📒 Files selected for processing (17)
.github/workflows/ci.yml.golangci.ymlcmd/tinyauth/generate_totp.gocmd/tinyauth/healthcheck.gointernal/bootstrap/app_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/controller/oauth_controller.gointernal/controller/user_controller_test.gointernal/middleware/context_middleware.gointernal/service/auth_service.gointernal/service/ldap_service.gointernal/service/oauth_extractors.gointernal/utils/fs_utils_test.gointernal/utils/security_utils.gointernal/utils/security_utils_test.gointernal/utils/string_utils_test.gointernal/utils/user_utils_test.go
💤 Files with no reviewable changes (1)
- internal/service/auth_service.go
| exclusions: | ||
| rules: | ||
| - linters: | ||
| - errcheck | ||
| text: "//nolint:errcheck" | ||
| - linters: | ||
| - staticcheck | ||
| text: "//nolint:staticcheck" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the correct golangci-lint v2 configuration format for exclusions
echo "Checking golangci-lint documentation for v2 configuration..."
# Check if the exclusions.rules.text field is meant for error messages or source patterns
rg -i "exclusions.*rules.*text" --type yaml -C 5Repository: tinyauthapp/tinyauth
Length of output: 125
🌐 Web query:
golangci-lint v2 configuration format exclusions rules documentation
💡 Result:
golangci-lint v2 uses YAML, TOML, or JSON configuration files (.golangci.yml,.golangci.yaml,.golangci.toml,.golangci.json) with version: "2" at the top.[1][2] Exclusions are configured under the top-level linters.exclusions section.[1][3][4] Key exclusion options: - generated: Controls exclusion of generated files (strict, lax, auto, disable). Default: lax.[1] - warn-unused: Log warning if exclusion rule unused. Default: false.[1] - presets: Predefined exclusion sets like comments, std-error-handling, common-false-positives, legacy.[1][5] - rules: Array of fine-grained exclusion rules matching by path, path-except, text, source, and/or linters (regex patterns).[1][3][4] Example excluding specific linters on test files: linters: exclusions: rules: - path: _test\\.go linters: - gocyclo - errcheck - dupl - gosec [1][4] Example excluding by text and source: - linters: [lll] source: "^//go:generate " [1][4] Example path-except (exclude everywhere except tests): - path-except: _test\\.go linters: [forbidigo] [1] - paths: File/directory paths to exclude issues from (regex, OS separator normalized). Default: [].[1] - paths-except: Paths not to exclude.[1] Patterns use regex (not globs); escape dots like \. in _test\.go. Paths can use placeholders like ${base-path}, ${config-path}.[6][1] Separate exclusions exist for formatters.formatters.exclusions.[5] See official docs for full details: https://golangci-lint.run/docs/configuration/file/ [1][2], false positives: https://golangci-lint.run/docs/linters/false-positives/ [3][4], migration from v1: https://golangci-lint.run/docs/product/migration-guide/ [7]. Reference config: https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml [8].
Citations:
- 1: https://golangci-lint.run/docs/configuration/file/
- 2: https://golangci-lint.run/docs/configuration/file
- 3: https://golangci-lint.run/docs/linters/false-positives/
- 4: https://golangci-lint.run/docs/linters/false-positives
- 5: https://github.com/golangci/golangci-lint/blob/main/.golangci.yml
- 6: Fix golangci-lint v2 configuration structure and exclusion patterns github/gh-aw#8418
- 7: https://golangci-lint.run/docs/product/migration-guide/
- 8: https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml
🌐 Web query:
golangci-lint //nolint directive native support how it works
💡 Result:
golangci-lint has native support for //nolint directives. It works by parsing “directive comments” in Go source files and then filtering/removing reported issues that match the directive’s scope (line, block, or file) and the specified linter list. 1) The supported //nolint syntaxes and scopes - Directives are not arbitrary comments; golangci-lint treats them as machine-readable “directive comments” matching the Go directive/comment syntax [1]. - Use exact machine syntax with no spaces in the critical positions, e.g. //nolint:xxx (not // nolint:xxx) [1]. - Excluding all linters: //nolint:all. If used inline (on the same line as the code), it suppresses issues only for that line [1]. Example: var bad_name int //nolint:all [1] - Excluding specific linters: //nolint:, (comma-separated). Example: //nolint:wsl,unused [1]. - Excluding a whole block: place the directive at the beginning of a line, e.g. at the start of a function or before a var block [1]. Example: //nolint:all func... {... } [1] and //nolint:govet var (...) [1] - Excluding whole files: place //nolint: at the beginning of the file, e.g. before the package declaration [1]. Example: //nolint:unparam package pkg [1] 2) How golangci-lint applies the directive to reported issues (how it “works”) - golangci-lint includes a dedicated processing stage for nolint directives (a “nolint filter”), which removes issues from the final results when an issue’s FromLinter and position match a nolint directive in scope. This is tested in the nolint filter unit tests, which construct a nolint filter and assert that issues are removed from results when a directive applies [2]. 3) Validation / correctness of directives (nolintlint) - There is also a “nolintlint” linter that checks that //nolint directives are well-formed and (optionally) properly explained. - The nolintlint package README describes its purpose: ensuring nolint exceptions have explanations and enforcing machine-readable formatting like //nolint:all and specifying which linter is suppressed (e.g. //nolint:gosec) [3]. Practical takeaway - If your goal is to suppress a specific set of warnings, prefer //nolint: (or //nolint:all) rather than disabling via configuration; and place it inline for single-line suppression, at the beginning of a line for block/file suppression [1]. If you want, paste a small code example (where the directive is placed) and I’ll explain exactly which issues it should suppress according to these rules.
Citations:
- 1: https://golangci-lint.run/docs/linters/false-positives
- 2: https://github.com/golangci/golangci-lint/blob/ab192139/pkg/result/processors/nolint_filter_test.go
- 3: https://github.com/golangci/golangci-lint/tree/HEAD/pkg/golinters/nolintlint/internal
🏁 Script executed:
cat -n .golangci.ymlRepository: tinyauthapp/tinyauth
Length of output: 635
Remove the exclusions section—it's misconfigured and won't work.
The exclusions.rules configuration uses the text field to match against lint error message text, not source code comments. The literal string "//nolint:errcheck" will never appear in actual lint error messages, so these rules won't exclude anything.
More importantly, golangci-lint natively supports //nolint:<linter> directives in your source code, which already suppress warnings without requiring exclusion rules. Delete the entire exclusions section (lines 10–17).
🤖 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 @.golangci.yml around lines 10 - 17, Remove the misconfigured exclusions
block in the golangci-lint config: delete the entire "exclusions" -> "rules"
entries that use the "text" field with literal "//nolint:errcheck" and
"//nolint:staticcheck", because golangci-lint matches source nolint directives
directly and the "text" matcher targets lint messages, not source comments;
simply remove the "exclusions" section (keys: exclusions, rules, linters, text)
so the linter uses native //nolint:<linter> handling.
| } | ||
|
|
||
| res.Body.Close() | ||
| res.Body.Close() //nolint:errcheck |
There was a problem hiding this comment.
Critical: Missing defer keyword on response body close.
The defer keyword is missing from the res.Body.Close() call. Without defer, the body closes immediately on line 295, before the status code is checked on line 297. While reading StatusCode doesn't strictly require an open body, this breaks the standard pattern and could cause issues if the code is modified later. Response bodies should always be deferred closed to ensure proper cleanup across all execution paths.
🐛 Proposed fix
- res.Body.Close() //nolint:errcheck
+ defer res.Body.Close() //nolint:errcheck📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| res.Body.Close() //nolint:errcheck | |
| defer res.Body.Close() //nolint:errcheck |
🤖 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/bootstrap/app_bootstrap.go` at line 295, The response body is being
closed immediately with res.Body.Close() instead of deferring it; change the
call to defer res.Body.Close() (keeping the //nolint:errcheck if desired) so the
body is closed at function return and not before subsequent checks—locate the
res variable and the res.Body.Close() call in the bootstrap logic and replace it
with deferred close.
| err = file.Close() | ||
| require.NoError(t, err) | ||
| defer os.Remove("/tmp/tinyauth_test_file") | ||
| defer os.Remove("/tmp/tinyauth_test_file") //nolint:errcheck |
There was a problem hiding this comment.
Avoid fixed /tmp filename in tests.
Line 21 uses cleanup for a hardcoded path, which can conflict with parallel test execution. Use t.TempDir() for isolation and keep the same deferred cleanup pattern.
🤖 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/utils/fs_utils_test.go` at line 21, Replace the hardcoded
"/tmp/tinyauth_test_file" cleanup with a test-scoped temp directory from
t.TempDir(); create the test file path via filepath.Join(t.TempDir(),
"tinyauth_test_file") and update the deferred cleanup to defer
os.Remove(tempFilePath) (or drop the defer since t.TempDir() auto-cleans),
ensuring you add an import for "path/filepath" if needed and update any
references to the old hardcoded path (the os.Remove("/tmp/tinyauth_test_file")
call) accordingly.
| err = file.Close() | ||
| require.NoError(t, err) | ||
| defer os.Remove("/tmp/tinyauth_test_secret") | ||
| defer os.Remove("/tmp/tinyauth_test_secret") //nolint:errcheck |
There was a problem hiding this comment.
Use test-scoped temp files instead of a fixed /tmp path.
Line 22 still cleans up a globally fixed file path, which can collide across concurrent test runs and cause flakes. Prefer t.TempDir() + a unique path per test.
Suggested change
- file, err := os.Create("/tmp/tinyauth_test_secret")
+ tmpDir := t.TempDir()
+ secretPath := tmpDir + "/tinyauth_test_secret"
+ file, err := os.Create(secretPath)
require.NoError(t, err)
// ...
- defer os.Remove("/tmp/tinyauth_test_secret") //nolint:errcheck
+ defer os.Remove(secretPath) //nolint:errcheck
- assert.Equal(t, "secret", utils.GetSecret("", "/tmp/tinyauth_test_secret"))
+ assert.Equal(t, "secret", utils.GetSecret("", secretPath))🤖 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/utils/security_utils_test.go` at line 22, Replace the hard-coded
"/tmp/tinyauth_test_secret" cleanup with a test-scoped temp path: call
t.TempDir() in the test, construct the secret path via filepath.Join(tmpDir,
"tinyauth_test_secret"), use that path wherever the test currently references
the fixed string, and remove the explicit os.Remove defer (or change it to defer
os.Remove(secretPath) if you still want explicit cleanup); also add an import
for "path/filepath" if missing. This ensures the test uses an isolated temp
directory rather than a global /tmp path.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Will rework this later. |
1 participant
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
Release Notes
Bug Fixes
Chores