chore(lint): enable the correctness linters and fix what they find - #6986
Merged
Conversation
Code Review CompleteThe automated review ran but did not post an updated summary — this usually means no new issues were found since the previous review. If you've pushed changes and want a fresh pass, comment |
otavio
force-pushed
the
chore/lint-correctness
branch
from
August 28, 2026 22:35
6fc27c7 to
df3d990
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| config := &gossh.ClientConfig{ | ||
| User: s.Target.Username, | ||
| HostKeyCallback: gossh.InsecureIgnoreHostKey(), // nolint: gosec | ||
| HostKeyCallback: gossh.InsecureIgnoreHostKey(), //nolint:gosec |
| config := &gossh.ClientConfig{ | ||
| User: s.Target.Username, | ||
| HostKeyCallback: gossh.InsecureIgnoreHostKey(), // nolint: gosec | ||
| HostKeyCallback: gossh.InsecureIgnoreHostKey(), //nolint:gosec |
|
|
||
| if geteuidFn() == 0 { | ||
| opts = append(opts, gliderssh.WithOwner(int(uid))) //nolint:gosec // uid_t is 32-bit; os.Chown takes an int | ||
| opts = append(opts, gliderssh.WithOwner(int(uid))) // uid_t is 32-bit; os.Chown takes an int |
This was referenced Aug 29, 2026
errcheck was the one linter cloud gated on and shellhub did not, so an unchecked error was a CI failure in one repo and invisible in the other. 169 sites. Almost all are Close on a path where the error has nowhere to go: a deferred close after a read, or cleanup on a branch that is already returning an earlier error. Those follow cloud's existing convention, `//nolint:errcheck` on a defer and an explicit `_ =` elsewhere, so the discard is a decision a reader can see rather than an omission. Two were not that. applyEnvFileFallback dropped the os.Setenv result, so an env-file entry the process rejected went unreported; it warns now. The admin CLI discarded every fmt.Fprint result including the tabwriter Flush, which is the call that reports a failed write to stdout.
An unchecked assertion on an interface panics rather than failing, which in a request handler takes the connection down. 51 sites in shellhub, 43 in cloud. Three were reachable from input rather than from a wiring mistake. The web terminal asserted the decoded websocket message payload straight to string and to Dimensions, so a client sending a resize with a string body panicked the session goroutine; both branches now log and close. The client logger asserted every other variadic element to string, which panicked on a non-string key. The rest are internal invariants. Where the value provably cannot be another type they take the comma-ok form and return an error or bail out; the sync.Map of per-tenant billing mutexes keeps the assertion behind a nolint, because handing back a fresh mutex instead would silently break the serialization the function exists for. Test mocks follow the same rule rather than an exclusion: testify callbacks narrow their arguments with require.True, or panic with the expected type when no *testing.T is in scope.
Dial, listen, exec and HTTP request calls that take no context cannot be cancelled, so the work outlives the request that asked for it. 137 sites, plus 24 in the older shapes. The 137 are httptest.NewRequest in handler tests, mechanically rewritten to NewRequestWithContext with the test's own context. The rest carry weight. The agent's proxy handlers dialled with net.Dial while holding a tunnel context and an echo request context respectively, so a client that hung up left the dial running to its own timeout. The Postgres store pinged without the context it had just used to open the pool. The web-endpoint proxy did the TLS handshake to a device with no deadline from the request. Five exec.Command sites keep a nolint with the reason: NewCmd has a docker and a native build-tag variant sharing one signature, and the session context reaches its callers rather than the constructor, so threading it is a change to the command API rather than a lint fix.
Twelve sites discarded the marshal error with `_`. A marshal that fails returns a nil slice, so the caller carries on with an empty payload and nothing says why. Two were on the revdial control channel, where a failed encode wrote a bare newline to the device instead of a message. The MCP tool output returned an empty string, which reads as an empty result rather than a failure; it returns the encoding error as text now. The rest are tests building fixtures, which use require.NoError so a broken fixture fails where it is built rather than in the assertion that follows.
…body Nine sites, none of them a leak, which is the point of writing the reason down: every one is a protocol handshake where the usual "defer resp.Body.Close()" is either unnecessary or wrong, and nothing in the code said so. Seven are gorilla/websocket dials, whose documented contract is that the handshake response body need not be closed. The eighth is the CONNECT handshake in the SSH dialer, where the body shares the buffered reader handed back to the caller: closing it would eat bytes belonging to the proxied stream, and the caller closes the connection on the error paths anyway. The linter earns its place on the plain HTTP calls added later, where a missed Close does hold a connection out of the pool.
Four findings, one of which was a real defect: UpdatePasswordUser's deprecation notice read "Deprecated, use" rather than "Deprecated:", and godoc only recognises the colon form, so no tool ever reported the function as deprecated. The rest are shape: two if-else chains over the same subject become switches, and a one-case switch becomes an if.
A switch over an enum with no default silently ignores any value added later. Five here were that shape, and each now says out loud what falling through means: a still-pending SSH approval polls again, a Docker event the connector does not care about is dropped, a message kind the client only receives is ignored. default-signifies-exhaustive is on, so a switch that already defaults is accepted. Without it the linter reports 28 sites and demands every one of Stripe's 200-odd event types and Docker's 47 actions be enumerated, which is noise rather than safety. Also corrects a comment that this branch made false: gosec's G104 exclusion described errcheck as disabled, which it no longer is.
Two problems, and the first hid the second. golangci-lint only recognises a directive written as //nolint. Twenty-one were written `// nolint` with a leading space, which the parser ignores, so those suppressions had never done anything. Nobody noticed because the linters they named were either not enabled or no longer had anything to report. Normalising them exposed the rest: 47 directives that suppress nothing at all. A nolint is a claim that the code knowingly breaks a rule, so one that suppresses nothing tells the next reader to expect a problem that is not there. Where the directive carried an explanation, the explanation stays as a plain comment. Two needed care rather than deletion. web.go named errcheck and errchkjson together and only the second was spent. openapi_test.go's suppression sat on the dial inside a helper, but bodyclose follows the response to the helper's callers, so it belongs at the two call sites.
APIKey.IsValid read the wall clock directly, so a test could not place a key either side of its expiry without sleeping. Every other expiry check in the codebase goes through pkg/clock for exactly that reason. Left as-is: time.Now for measuring elapsed time (request and query durations) and for socket write deadlines, where a mocked clock would break the measurement rather than make it testable. That distinction is why forbidigo is not enabled for time.Now: 51 non-test call sites, of which four are the value being compared and the rest are duration or deadline arithmetic, and no pattern can tell them apart.
Reformatting the nolint on this line made semgrep report a pre-existing finding as new, since it only reports findings on lines a change touches. The finding is real and deliberate: the agent's host key is not known to the server, and the hop runs inside the tunnel the agent already authenticated over, so there is nothing to pin. Say so, and suppress the rule at the site.
…mbered "Use clock.Now() unless you are measuring elapsed time" was a convention nobody could check. Four production call sites had already drifted off it, and finding them meant reading 51 call sites by hand. The rule is now in the config, and every exception is written down at the site it applies to: a socket write deadline, the request-duration timer in the log middleware, the query-duration timer in the bun hook, and pkg/clock itself. Test code is excluded from this one rule, with the reason in the config. A test builds its own fixtures, and routing those through clock.Now sends them to whatever clock mock the test installed, which is backwards. Confirmed the hard way: converting them made the admin and cloud service suites panic on unexpected mock calls. Naming any forbid pattern replaces forbidigo's defaults, so the fmt.Print patterns are restated. That surfaced ten fmt.Println(err) calls in the connector sessioner, where SSH session errors went to the container's stdout unstructured instead of the logger; they use log.WithError now.
forbidigo skipped test files, which meant the rule needed explaining in a prompt: "use clock.Now, except in tests". An exception a linter cannot state is an exception every author has to be told about. The exclusion is gone. What it was hiding: Fixtures derived their unique names, emails and SSH fingerprints from time.Now().UnixNano(). Two fixtures built inside the same nanosecond already collided; under a fixed clock they always would. They use random hex now, which is what a uniqueness source should have been, and that drops eight gosec suppressions covering the byte shifts the old fingerprint needed. Four values stay on time.Now with the reason at the site: they are the wall value the test hands to the clock mock, so reading them from the mock would be circular. The deadline and elapsed-time sites keep theirs for the same reason they always did.
otavio
force-pushed
the
chore/lint-correctness
branch
from
August 29, 2026 13:32
d094fca to
a8ab93a
Compare
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.
What
Tasks 6-9 of the linting ledger, landing as one PR with one commit per linter. Still in progress —
errcheckandforcetypeassertare in;noctx,nilnil,exhaustive,nolintlint,gochecknoinits,errchkjson,containedctx,bodyclose,gocritic,unparam, revive extras andforbidigoforclock.Now()are still to come. Review is welcome as commits land.Pairs with shellhub-io/cloud#2525.
Why
The point of the series is to move Go conventions out of prompts and agent harness config and into the linter, so the rules are enforced rather than remembered. This batch is the correctness half.
No test/non-test split anywhere. A linter that exempts test files teaches that test code is held to a lower standard, so test sites get the real fix rather than an exclusion rule.
Changes so far
errcheck(169 sites). shellhub was the only repo not gating on it, so an unchecked error was a CI failure in cloud and invisible here. Almost all areCloseon a path where the error has nowhere to go, following cloud's existing convention (//nolint:errcheckon a defer, explicit_ =elsewhere) so the discard is visible as a decision.Two were not that:
applyEnvFileFallbackdropped theos.Setenvresult, so an env-file entry the process rejected went unreported. It warns now.fmt.Fprintresult including the tabwriterFlush— the call that reports a failed write to stdout.forcetypeassert(51 sites here, 43 in cloud). An unchecked assertion panics rather than failing, which in a handler takes the connection down.Three were reachable from input rather than from a wiring mistake:
server/ssh/web/session.goasserted the decoded websocket message payload directly tostringand toDimensions. A client sending a resize whose data is a string panicked the session goroutine. Both branches log and close now.pkg/api/client/logger.goasserted every other variadic element tostring, panicking on a non-string key.The rest are internal invariants and take the comma-ok form. One keeps a suppression on purpose: the
sync.Mapof per-tenant billing mutexes in cloud, where handing back a fresh mutex on a "failed" assertion would silently break the serialization the function exists for. A panic on an impossible state is the better failure there, and the reason is in the code.Test mocks follow the same rule as production: testify callbacks narrow their arguments with
require.True, or panic naming the expected type where no*testing.Tis in scope.Testing
Lint clean across all six modules and the agent under both
dockerandnative; cloud clean under no tags,enterprise, andenterprise,mocks. Thenativetag caught two assertion sites adocker-tagged sweep missed, which is worth knowing for future passes.Everything that fails locally fails with
rootless Docker not foundat 0.00s — testcontainers, unrelated to the change, and identical on master.