lint: stand up golangci-lint and clear the high-noise findings#347
Open
cpcloud wants to merge 16 commits into
Open
lint: stand up golangci-lint and clear the high-noise findings#347cpcloud wants to merge 16 commits into
cpcloud wants to merge 16 commits into
Conversation
roborev: Combined Review (
|
Squashes the mechanical golangci-lint cleanup into a single commit: - Initial .golangci.yml: aggressive default-all setup with curated disables (complexity metrics, nlreturn/wsl/varnamelen/lll, forbidigo, gochecknoinits), and tunes for errorlint, exhaustive, goconst, gocritic, revive, wrapcheck. - Pure auto-fix output: modernize, perfsprint, usestdlibvars, godot, whitespace, intrange, canonicalheader, mirror. - Config disables added over time: dupword, fatcontext, noctx (test scope), gocheckcompilerdirectives, govet inline analyzer, unqueryvet. - Threshold tweaks: goconst min-occurrences raised to 10, wrapcheck ignore list expanded for stdlib + ctx.Err(). - Mechanical cleanups: clear-style commits for sloglint, godoclint, durationcheck, iface, recvcheck, godox/interfacebloat, nosprintfhostport, wastedassign, inamedparam, makezero, musttag, embeddedstructfieldcheck, rowserrcheck. - Additional modernize pass (min/max, slices.Contains, CutPrefix, any, omitzero). Reasoning-heavy linter changes are split into per-linter commits that follow this one.
Squashes 21 commits that incrementally replaced `*Query`/`*Exec`/ `net.Listen`/`exec.Command`/`httptest.NewRequest` with their context-aware variants across the codebase. Coverage: - DB layer: dedup, applemail, scheduler, textimport, vector/hybrid, imessage, query, whatsapp, fbmessenger, importer, store, sync - Service layer: api, microsoft, oauth, update - Test helpers: testutil, testutil/storetest, testutil/dbtest - Scripts/cmd: mimeshootout, cmd/msgvault (build_cache, repair, export, deduplicate) Tests use t.Context() or t.Helper-aware contexts so query lifetimes follow the test. The noctx test-suite exclusion added earlier was removed once tests were ported.
Two waves combined: - First pass: replace `err == io.EOF` / `err != io.EOF` (plus sql.ErrNoRows and context.Canceled) with `errors.Is(...)` so the comparison survives error wrapping. Adds the errors import to ~20 files. verify.go had a local []string named errors that shadowed the package; renamed to sampleErrs. - Second pass: convert remaining patterns to multi-%w `fmt.Errorf`, `errors.As` for typed-error extraction, and `errors.Is` for the rest of the wrapped-sentinel comparisons.
… params Resolves all 43 thelper findings: - Add t.Helper() to anonymous func helpers accepting *testing.T in table tests (executor_test, manifest_test, attachments_test, sqlite_crud_test, sync_test, actions_test, build_cache_test). - Rename `t testing.TB` to `tb testing.TB` for helpers accepting the TB interface (testutil.AssertStringSet, dbtest.NewTestDB, tbmock.NewMockTB, testutil.newErrRecorder, and 6 fixture helpers in query/testfixtures_test.go). Helpers that take *testing.T keep the conventional `t` name. Also reformats noctx-touched files (gofmt fix for missing space after ctx parameter from earlier sed work).
Resolves all 116 forcetypeassert findings: - TUI tests: add a mustModel(t, tm) helper next to sendKey/sendMsg in setup_test.go and route every `.(Model)` assertion through it (model_test, nav_detail_test, nav_drill_test, nav_view_test, search_test, selection_test, view_render_test). - store/db_logger_test: add recString/recFloat helpers that wrap map[string]any access with a typed lookup + t.Fatalf on mismatch. - api/handlers_test, fbmessenger/importer_test, mcp/server_test, cmd/msgvault/cmd/export_attachment_test: add explicit `, ok` checks with t.Fatalf for JSON-decoded map assertions and MCP content variants. Each test now reports a clear failure message instead of panicking on a type mismatch.
Resolves 16/17 usetesting findings: - cmd/msgvault/cmd: refactor setupTestSQLite/setupTestSQLiteEmpty/ setupQueryTestParquet/setupTestAttachment to return only the path (t.TempDir auto-cleans), drop the trailing func() cleanup return and all defer cleanup() call sites. - internal/api/handlers_test: switch token/config tests to t.TempDir. - internal/query/benchmark_test: switch buildBenchData to b.TempDir. - internal/sync/testenv_test: switch newTestEnv to t.TempDir. The remaining finding in internal/config/config_test.go is a deliberate probe of a chmod-restricted parent directory (not the default tmp); it gets an inline `//nolint:usetesting` with the rationale.
Resolves all 12 nilerr findings. Every site was an intentional swallow-and-continue: - emlx/discover.go (2): permissive WalkDir during mailbox discovery - fbmessenger/importer_test.go: defense-in-depth walk skips errors - gvoice/client.go: silently skip non-normalizable phone numbers - importer/emlx_import.go: ctx cancellation flows via summary, not error - mcp/handlers.go: MCP convention sends tool errors via ToolResultError - pst/reader.go: 32-bit PST sub-folder reads fail; keep walking siblings - pst/reader_test.go (5): ErrMessagesNotFound / empty folder, keep walking Each gets an inline `//nolint:nilerr` with a brief rationale so the next reader sees the intent.
… rest Resolves all 26 nilnil findings: - cmd/msgvault/cmd/addaccount: findGmailSource now returns store.ErrSourceNotFound when no Gmail source is found. The 3 cobra callers (addaccount.go x2, serve.go x1) skip the error with errors.Is(...). Test updated to match. - Every other site is part of an established (nil, nil) = "not found / empty / disabled / no-op" convention spanning store.GetMessage, store.GetSourceByIdentifier, sync run lookups, query engine GetAttachment/GetMessage, remote engine/store mirrors, mcp arg parsers, vector stats (disabled backend), and update check (up-to-date). Each gets an inline `//nolint:nilnil` with a brief reason instead of a refactor that would ripple to every caller.
Removes four unused test helpers (strPtr, intPtr in applemail/accounts_test.go; boolPtr, intPtr in vector/config_test.go). Originally added as //go:fix-inline migration aids but never adopted; staticcheck flagged them as dead code.
Resolves all 7 dogsled findings, all in tests that only need a subset of the returns from multi-value helpers: - MigrateLegacyIdentityConfig returns 5 values; 5 test sites only need applied + err (or just err). - newIdentityCLITest returns (store, root, stdout, stderr); 2 sites only need root. Each gets an inline `//nolint:dogsled` with the reason. Refactoring to return a struct is more churn than the stylistic finding warrants.
Resolves all 12 errchkjson findings: - cmd/msgvault/cmd/addaccount_test.go: introduce mustMarshalJSON helper, route 8 token-fixture Marshal calls through it. - cmd/msgvault/cmd/build_cache_test.go: handle the error from marshaling syncState (contains a time.Time field). - internal/importer/emlx_import_test.go: handle the error from marshaling emlxCheckpoint. - internal/microsoft/oauth_test.go: thread t through makeIDToken and Fatalf on Marshal failure; 13 callers updated. - internal/api/handlers.go: writeJSON encodes `any`; mid-response failure is unrecoverable once headers are sent. Documented inline with //nolint:errchkjson.
Resolves all 17 sqlclosecheck findings. Each site replaces the
"hand-rolled close on every exit path" with a deferred close right
after the QueryContext error check.
For sites inside a for-loop iterating chunks/tables, the body is wrapped
in an inline func() so the defer is bounded to the iteration:
- internal/whatsapp/queries.go: fetchMedia / fetchReactions /
fetchQuotedMessages chunked loops.
- cmd/msgvault/cmd/repair_encoding.go: repairDisplayNames and
repairOtherStrings table loops.
For non-loop sites, just hoist the defer:
- internal/store/subset.go: PRAGMA foreign_key_check.
- internal/store/subset_test.go: 7 test sites (rename rows for clarity
where the variable was reused across queries).
- cmd/msgvault/cmd/build_cache_test.go: 2 query helpers.
- internal/vector/embed/{enqueue,queue}.go: extract the rows lifecycle
into a named inner func that returns the collected IDs, so the defer
closes before the tx.Commit fires.
- .golangci.yml: flip exhaustive.default-signifies-exhaustive to true. A `default:` clause is an acceptable way to express "intentionally lumped together" for the remaining enum members. - Add explicit case branches (with brief explanatory comments) for the enum values that don't need real handling: query.ViewSenderNames / ViewRecipientNames / ViewTypeCount in tui keys/model, tui modalNone in handleModalKeys and overlayModal, query.TextView* leftovers in textDrillDown, textLevelTimeline in textDrillDown, and gvoice fileTypeText/Group + query.SortByCount in non-drill-able callsites.
…lse-if, etc
Resolves all 30 gocritic findings:
- stringXbytes (14): auto-fix replaced `string(b1) != string(b2)` with
`!bytes.Equal(b1, b2)` across nine test files; goimports added the
missing `bytes` import.
- appendCombine (8): merge consecutive `conditions = append(...)` calls
into a single append with multiple args (internal/query/duckdb.go,
internal/query/sqlite.go, internal/tui/view.go).
- elseif (4): collapse `else { if X { ... } }` to `else if X { ... }`
in test files; convert one if/else-if/else chain to a switch in
sqlite_crud_test.go.
- appendAssign (2): swap `remaining := append(failedIDs, retryIDs[ri:]...)`
for slices.Concat in deletion/executor.go - the lint correctly flags
that we want a NEW slice, not in-place semantics.
- dupBranchBody (1): merge two identical else-if/else branches in
textimport/phone.go into the switch default with a brief comment.
- exitAfterDefer (1): scripts/mimeshootout/main.go closes the DB
explicitly before os.Exit(1) so the deferred close isn't skipped,
with a nolint annotation explaining the manual close.
Two waves combined: - Exclude false-positive categories: G304 (file inclusion — msgvault is a CLI on user-supplied paths), G301 (directory permissions on user-owned dirs), G201/G202 (SQL string building from internal fragments, never caller input). G306/G302 excluded from tests (fixture permissions don't model a real threat). Production state file (build_cache.go) and docker-compose.yml (setup.go) tightened from 0644 -> 0600. internal/testutil/fs_helpers.go keeps 0644 with a nolint annotation because it's the shared fixture writer. - Fix the remaining real findings: tighten http.Server timeouts in microsoft/oauth.go and oauth/oauth.go, harden extractors in fbmessenger/slug.go, imessage/parser.go, importer/mboxzip/mbox_zip.go, vcard/vcard.go, store/messages.go; add input-size caps and explicit error returns where gosec flagged unchecked stdlib calls. Drops gosec findings from 363 to a handful of remaining real issues.
…shadowing, ctx-arg, etc Configuration: - Disable revive.unused-parameter. Most "unused" params are imposed by the cobra RunE / http.HandlerFunc / tea.Cmd signatures; renaming each to _ adds churn without value (211 of 255 findings). - Disable revive.var-naming for testutil.EncodedSamplesT: underscore- separated encoding+content field names are intentional and readable. Fixes: - blank-imports (6): annotate each SQL/DuckDB driver blank import. - redefines-builtin-id (7): rename `max` parameter to maxLen/maxN/maxChars/ limit and `close` to closeIdx across cmd/, internal/store, internal/sync, internal/importer/mboxzip, internal/tui test helper. - context-as-argument (3): move ctx to the first parameter slot in isSignalCanceled (cmd/msgvault/main.go), printImportSummary (cmd/msgvault/cmd/import_emlx.go), and acquireAsync (internal/gmail/ratelimit_test.go); update call sites accordingly. - indent-error-flow (1): drop the unnecessary else after a return in cmd/msgvault/cmd/import.go. - var-declaration (1): drop `= nil` from the zero-value typed-nil pointer in internal/store/sqlite_error_test.go. - empty-block (1): flatten the empty `if phone == ""` skip branch in internal/whatsapp/importer.go into a non-empty `if phone != ""` block. Drops revive to 0 findings.
cpcloud
force-pushed
the
worktree-elegant-churning-ripple
branch
from
5f028d2 to
903f7bb
Compare
roborev: Combined Review (
|
Member
|
Let me get the testify branch merged and then I'll rebase this |
Contributor
Author
|
no worries, i can also rebase once things are settled on the testify side |
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
2 participants
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.
Sets up golangci-lint and clears the bulk of the noise from a fresh
default-allconfig. Reduces findings from ~1670 to ~150 across 16 commits.Commit shape
One mechanical commit at the base, then one reasoning commit per linter on top. The subject line tells you which is which:
lint: mechanical changes (...)— the single mechanical commit. Bundles every auto-fix output (modernize, perfsprint, usestdlibvars, godot, whitespace, intrange, canonicalheader, mirror), every.golangci.ymlconfig decision (disables, threshold tweaks, exclusion lists), and theclear-*cleanups for one-shot linters (sloglint, godoclint, durationcheck, iface, recvcheck, embeddedstructfieldcheck, rowserrcheck, inamedparam, makezero, musttag, nosprintfhostport, wastedassign). Nothing in this commit required judgment beyond accepting the linter's auto-fix.lint(<linter>): ...— one commit per linter where the fix needed real reasoning. Each commit's body explains what changed and why.The per-linter commits, top to bottom of the stack:
lint(noctx)— thread ctx through ~21 packages' DB calls and test helperslint(errorlint)—errors.Is/errors.As/ multi-%wfor wrapped sentinel comparisonslint(thelper)—t.Helper()on anonymous test helpers; renameTBparams for consistencylint(forcetypeassert)— check type assertions in testslint(usetesting)—t.TempDir()/b.TempDir()overos.MkdirTemplint(nilerr)— annotate intentional nil-on-error returns with//nolintlint(nilnil)—findGmailSourcereturnsErrSourceNotFoundinstead of(nil, nil); remaining sites annotatedlint(unused)— drop dead pointer helpers from applemail and vector testslint(dogsled)— annotate test sites that ignore 3+ return valueslint(errchkjson)— checkjson.Marshalerrors in tests; annotate the handler call siteslint(sqlclosecheck)—defer rows.Close()instead of manual closelint(exhaustive)—default-signifies-exhaustiveplus explicit case handlinglint(gocritic)— string-byte compares, combine appends, fold else-if, etclint(gosec)— exclude noise categories (G304 file inclusion, G301 dir perms, G201/G202 SQL building, test-only G306/G302); fix the real findings (http server timeouts, extractor hardening, 0600 perms on production state files)lint(revive)— disableunused-parameter; clear blank-imports, builtin shadowing (max→maxLen),ctx-arg, etcNotes
.golangci.ymlis added by the mechanical commit and carries every linter decision in this stack.contextcheck,goconst,unparam,wrapcheck— those need follow-up passes that involve real refactors, not config.