ci(lint): require a godoc on pkg's exported symbols - #6989
Draft
otavio wants to merge 6 commits into
Draft
Conversation
revive has been enabled here since the config was written, but with no rules block it reports nothing: `golangci-lint run` over pkg/api is clean, and the same package with `rules: [exported]` reports 50. staticcheck's equivalents (ST1000, ST1020-22) are disabled a few lines below. So nothing in this repo has ever asked for a doc comment. Turn revive's `exported` rule on. pkg/ is where a doc comment pays first — these packages are the surface server/, agent/ and cloud/ consume, by a caller who never opens the body. The per-module .golangci.yaml files are symlinks to this one, so the rule cannot be scoped in the config; qa.yml scopes it instead, with -D revive on the modules whose exported symbols are still undocumented. A module rejoins by dropping its flag. Counts, measured with --max-issues-per-linter=0: server 605, agent 71, pkg 229, gateway 0. pkg's 229 are the backfill this PR does not do: they are worth writing by hand, one package at a time, because a doc comment restating its symbol name clears the rule while telling a caller nothing.
otavio
force-pushed
the
comment-policy/allowlist
branch
from
August 31, 2026 21:51
2097e17 to
68247d9
Compare
229 doc comments, so that revive's exported rule passes on the module a caller consumes without opening the body. golangci-lint reports 0 issues over ./... with the rule on. The rule also removes 31 in-body comments, and where one carried something a reader needs it moved rather than went: why Read hides io.EOF, why a failed ping tears the adapter down, why the lockout deadline is compared and not the attempt count. Two became names instead — limit is now rejectAtOrAbove, and lockoutSTR is lockoutDeadline — which is the outcome the rule is after. pkg/envs, pkg/wsconnadapter and pkg/api/jwttoken were done first as a sample, and are folded in here.
2,120 comment lines across 234 files, so the rule can be judged on the codebase rather than on one module. Every module builds, golangci-lint is clean, and the tests pass apart from the cases that need a Docker daemon inside the test container. Read this diff as the scale of the change, not as the finished state. The rule says a why that has nowhere left to sit goes in the commit message, and pkg/ (in the previous commits) is what that looks like done properly: 31 in-body comments moved up into the contract above them, two became names. Here the same 2,089 elsewhere were deleted where they stood. That is the work the rule creates, and it is the number worth arguing about — one commit cannot carry 2,000 explanations, and neither can one reviewer. What is left standing is the answer to "how much does this actually remove": 2,120 of 8,161 comment lines, a quarter. The rest are doc comments on declarations and their members, which both variants keep, plus suppressions, licence headers and external references.
A draft skipped the whole validate job, so the branch that changes 200 UI files was the one branch CI never looked at: lint, unit tests and build all ran locally and nowhere else. A draft is where a check is worth the most — it is the state a change sits in while it is still wrong. The other workflows keep their gate. This one is dropped alone because the comment-policy branches are drafts on purpose and their UI half is the part with no other verification.
The four Go modules had revive's exported rule sitting dormant, so the policy had something to ask a linter for. The UI had nothing: no jsdoc plugin was installed and eslint carried no doc rule at all, which would have left the removals enforced by the Stop hook alone — and the hook only ever sees the lines a session touched. eslint-plugin-jsdoc supplies the floor. jsdoc/require-jsdoc with publicOnly is scoped to packages/design-system, the surface all three apps consume without opening the body, and is the counterpart to enabling revive on shellhub/pkg and cloud/pkg rather than on server/ or internal/. require-description is on with it: require-jsdoc alone is satisfied by an empty block, and its own --fix writes exactly that. 54 doc comments written by hand, none of them generated. The design-system is where a doc comment earns its place — Button and IconButton rendered as anything but a button cannot be disabled by the DOM, so loading falls back to aria-disabled and the caller has to guard the click; Callout fixes role and aria-live per variant, so an error interrupts a screen reader and a success waits; Checkbox keeps the native input in the tree, so focus and form participation are the browser's. None of that is legible from a signature. 1,173 comment lines removed across 188 files. The generated SDK under apps/console/src/client is excluded: it is gitignored and regenerated from the OpenAPI spec on every build, and carries 6,609 lines of its own. Design-system lint now runs in CI. It was skipped by matrix.app != 'design-system' in validate-ui.yml, so a rule declared there would have been enforced nowhere. As with the Go passes, this deletes in-body whys rather than relocating them, except in the design-system where the contract above the declaration was written properly. That is the scale of the change, not its finished state. Three classifier bugs surfaced only by running the rule over real TypeScript, each fixed in claude/ with a bats case before this pass: a bare asterisk read as a JSDoc continuation (it is a JSX required marker), a triple-slash reference read as a comment (it is how vite/client types are pulled in), and // @vitest-environment read as a note (it picks the test environment, and losing it broke the design-system token test with a URL-scheme error naming nothing).
The 21 empty catch blocks each carried a sentence saying why an error was
swallowed. Under the allowlist that sentence is allowed to stay — the body of
an empty block is one of the four kinds of comment the rule keeps — but the
comment was never the best form for it. None of these needed a comment. They
needed a name.
utils/failure.ts holds four, each documented with when it is the right one:
ignoreFailure and nullOnFailure for .catch(), attempt for a synchronous call
that may not be there at all, and succeeded for a call whose error is already
recorded somewhere the screen reads.
Most sites did not even need those. The failure had an answer the code was not
stating: parseMessage returns null, because a frame that is not JSON is
ordinary terminal output; loadConfig falls back in the expression, so the
default is visible in the data flow; markWelcomeSeen returns whether it stuck,
mirroring hasSeenWelcome; listRecordings reads each sidecar through a helper
returning RecordingMeta | null, so a corrupt one is skipped by an if; and four
react-query call sites used mutateAsync inside a try that discarded the
rejection, where mutate() reports through the mutation's own state.
ForgotPassword keeps its reason in the code, as a name:
const silenceToPreventAccountEnumeration = ignoreFailure;
await recoverPassword({ ... }).catch(silenceToPreventAccountEnumeration);
Two behaviours are better than before rather than merely equivalent: Chatwoot
records the identity only when setUser actually succeeded, and the settings
toggles clear their busy flag through onSettled instead of a finally that ran
whether or not the mutation was still relevant.
This is the same change as on the total-ban branch, where it removes 21 lint
suppressions. Here it removes 21 comments the rule would have allowed, which
is the more interesting result: the allowance was carrying code that wanted
rewriting.
otavio
force-pushed
the
comment-policy/allowlist
branch
from
August 31, 2026 23:35
02f4b83 to
f125767
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
1 participant
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.
Policy PR: shellhub-io/claude#54. Sibling: #6990, the wider
version of the same rule. Pick one.
What is true today
revivehas been enabled in.golangci.yamlsince the config was written, but with norules:block it reports nothing. Measured, not assumed:
staticcheck's equivalents (
ST1000,ST1020-22) are disabled a few lines below. So nothing in thisrepo has ever asked for a doc comment.
What this does
Turns revive's
exportedrule on.pkg/is where a doc comment pays first — these packages are thesurface
server/,agent/andcloud/consume, by a caller who never opens the body.The per-module
.golangci.yamlfiles are symlinks to the root one, so the rule cannot be scopedin config.
qa.ymlscopes it instead, with-D reviveon the modules whose exported symbols arestill undocumented; a module rejoins by dropping its flag.
disableStutteringCheckkeeps the ruleon the comment — a package-stuttering name is a rename, a different change with different reviewers.
Counts, measured with
--max-issues-per-linter=0(the default caps at 50, so an unflagged run readsas exactly "50" for every module):
pkggatewayserveragentWhat this does not do
The 229. They are worth writing by hand, one package at a time: a doc comment restating its symbol
name clears the rule while telling a caller nothing, which is exactly what the policy exists to
prevent. CI is red on
pkguntil they are written — that cost is the point of the PR, not anoversight.
What it looks like on real code
The last commit applies the rule to three packages —
pkg/envs,pkg/wsconnadapter,pkg/api/jwttoken— so it can be judged on a diff. 22 godocs written, and the whys that wereburied in function bodies move up into the contract they belong to: why
Readhidesio.EOF, whya failed ping write tears the adapter down, why
Readholds a mutex. The floating pair aboveErrUnexpectedMessageTypebecomes the package doc, wheregorilla/websocket#441keeps its placeas case 3.
jwttoken's TODO list leaves the source.go testpasses and golangci-lint is clean onall three with the rule on.
Scaled to everything (non-test sources, mocks and the generated SDK excluded), the rule would
remove ~3,238 comment lines — shellhub Go 1,537, cloud Go 420,
ui/1,281 — and add 503godocs (
shellhub/pkg229,cloud/pkg274). Compare the sibling: B removes ~5,241 and adds1,557.
Backfill done —
pkg/is greengolangci-lint run ./...inpkg/now reports 0 issues with the rule on. All 229 exportedsymbols carry a godoc, written to be worth reading rather than to clear the linter: what a caller
cannot see from the signature (
HasMaxDevicesReachedcompares against -1 when there is no ceiling,so ask
HasMaxDevicesfirst; aFilterwith an unrecognizedTypenarrows nothing instead offailing;
KindInvalidis what makes a forgotten scope a rejection rather than an unbounded query).The other direction is the same work read backwards. 31 comments sitting inside function bodies
moved up into the contract above them — why
AddressFromHosttolerates a trailing dot, why the APIclient sends
Content-Length: 0, whyValidateFilterstreats virtual bool fields differently — andtwo became names instead (
limit→rejectAtOrAbove,lockoutSTR→lockoutDeadline). Four staleNOTE/TODO pairs about structs that moved to the cloud repo are gone; the fact is now in the type's
doc.
Tests pass apart from the two asynq cases that need a Docker daemon inside the test container.