Make the lint job actually lint, and clear what it finds - #4
Merged
Conversation
The lint job has not linted anything for at least the last three runs on
main. It was not failing on findings, it was failing to start:
can't load config: unsupported version of the configuration: ""
CI pins golangci-lint v2.9.0, which requires a version key and a
restructured file, while .golangci.yml was still v1 format. So the repo
has had a lint job, a Makefile target and no lint coverage.
This is the output of `golangci-lint migrate` run by v2.9.0 itself rather
than a hand conversion. It keeps all eight linters: errcheck, govet,
ineffassign, misspell, staticcheck and unused stay linters, and gofmt and
goimports move to the new top-level formatters section. The exclusion
presets it adds reproduce v1's default exclusions, which were on by
default before and would otherwise silently switch off.
The one key it drops is run.timeout: 5m. That is correct rather than
lossy: v1 defaulted to a 1m timeout and 5m was raising it, while v2
disables the timeout by default, so dropping it preserves the intent
instead of reimposing a limit.
This commit only makes the linter run. It reports 54 pre-existing findings
that nothing has ever enforced; the commits that follow clear them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The formatters now run, and goimports had findings in nineteen files where a packetcode import had drifted into the stdlib block or sat against the third-party block with no separator. goimports on its own pulls a stray local import out of stdlib but leaves it in a group of its own, ahead of third-party, which is not what any of these files were reaching for: each already had a packetcode group at the bottom. Setting goimports local-prefixes teaches the formatter that packetcode is local, so it sorts those imports into the trailing group instead, and the six files with a stranded single import have it merged into the group that was already there. The result is the convention the files were already using -- stdlib, then third-party, then packetcode -- and running the formatter again changes nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ipping code Sixty-five errcheck findings, none of which anything had ever reported. The real ones are fixed. cmd/packetcode/main.go registered a cleanup that called jobsMgr.Shutdown and then returned nil unconditionally, so a job manager that failed to stop cleanly reported success on the way out; it now returns what Shutdown says. Deferred Shutdown and CloseHandle calls in tests discard the result explicitly instead of silently. The setup writes in mentions_test.go are asserted, because a test whose fixture failed to write is not testing what it claims to. Two production type assertions in internal/procrun were unchecked. trackedJobs only ever holds a windows.Handle so neither can fail, but a value of the wrong type now falls through to the same outcome as "no job tracked" rather than panicking, which is the honest reading of an impossible state. The remaining forty are type assertions in tests, and those are excluded by a scoped rule rather than rewritten. check-type-assertions stays on so that shipping code is held to it, but in a test a panicking assertion already fails the test and names the offending type, while turning forty inline assertions like assert.Equal(t, root, tool.(*ReadFileTool).Root) into two-statement checks would cost real readability for no safety. The rule matches only errcheck's unnamed message, so unchecked errors stay enforced in tests as well. That last part is a policy choice rather than a fix, and it is one line to reverse if the project would rather rewrite the assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eighteen unused findings, all of them genuinely dead rather than false positives from build tags or reflection. Three test helpers had outlived their tests: approvalProvider, a full provider stub with nine methods; scriptedAlias, which existed to give the scripted provider a second slug; and bytesReader, a hand-rolled io.Reader whose comment explains it avoided importing bytes. Nothing refers to any of them. Two pieces of production code go with them. App.startTurnDisplaying was the entry point for a turn whose transcript line differs from what the model receives, but every caller reaches startTurnWith directly, so the wrapper was never on a live path. The App.err field was never read. Registry.loadDir is a two-argument wrapper around loadDirWith that nothing calls; loadScopeDir is the live one. Deleting rather than annotating is deliberate: this is the same call the audit made when it removed internal/tools/atomic.go. Code that no longer has a caller is easier to re-add from history than to keep explaining. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Twenty-nine findings, and the largest group needed care rather than the obvious fix. Fifteen ST1005 hits are error strings beginning with a capital, but every one of them begins with a product name: Conduit, Sugar, Packet Computers. Lowercasing the word would have been wrong, so the messages lead with the condition instead -- "Conduit run idempotency key is invalid" becomes "invalid Conduit run idempotency key". Only two of these strings are asserted anywhere in the tests, and both keep the asserted substring. Four SA1019 hits are bubbles viewport methods that were renamed: LineDown/LineUp/HalfViewDown/HalfViewUp become ScrollDown/ScrollUp/HalfPageDown/HalfPageUp, which are the same behaviour under the current names. The rest are simplifications that say what the code already meant: two guards around TrimSuffix/TrimPrefix that those functions already perform, a conditional assignment folded into its declaration, a De Morgan negation, a struct literal that is a conversion, and two if/else chains over a single value that are switches. Two were dead scaffolding with comments admitting it: an `if false` block "to silence unused-result lint", and a closure whose critical section locked and unlocked around nothing before being discarded with `_ = track // silence unused`. One ST1018 hit is a test string holding real ESC bytes. It now spells them \x1b, which is the same bytes and readable in a diff; the comment above it explaining why the escapes must be literal still holds. golangci-lint run now exits 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f36af64ec7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| runtime.AddCleanup(func() error { | ||
| jobsMgr.Shutdown(5 * time.Second) | ||
| return nil | ||
| return jobsMgr.Shutdown(5 * time.Second) |
There was a problem hiding this comment.
When a job worker survives the five-second timeout or snapshot flushing fails, this cleanup now returns the error to packetRuntime.Close, but the TUI path invokes that method as defer runtime.Close() at line 290 and discards its result. Consequently, if the Bubble Tea run itself succeeds, run still returns nil and the process reports success—the shutdown failure this change intends to expose remains invisible.
Useful? React with 👍 / 👎.
releaseTree on POSIX asserted trackedGroups' value to int without checking it, the exact counterpart of the two assertions already fixed in process_windows.go. It was missed because linting on Windows never compiles the !windows files, so the finding only appeared once CI ran on Linux. Same treatment as the Windows side: the map only ever holds a pgid, so this cannot fail, and a value of the wrong type now yields the same outcome as no group being tracked rather than panicking. Verified by running golangci-lint under GOOS=linux, darwin and windows, which is what should have been done before the first push. All three report zero issues. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The
lintjob has been failing on every run, including onmain, and not because of lint findings. It exited before linting anything:CI pins golangci-lint v2.9.0, which needs a
versionkey and a restructured file, while.golangci.ymlwas still v1 format. So the repository had a lint job, amake linttarget, and no lint coverage at all.This makes the linter run, then clears everything it found.
golangci-lint runnow exits 0.The config
The migration is the output of
golangci-lint migraterun by v2.9.0 itself, not a hand conversion. All eight linters are preserved: errcheck, govet, ineffassign, misspell, staticcheck and unused stay linters, and gofmt and goimports move to the new top-levelformatterssection. The exclusion presets it adds reproduce v1's default exclusions, which were on before and would otherwise have silently switched off.The one key it drops is
run.timeout: 5m, and that is correct rather than lossy. v1 defaulted to a 1m timeout and 5m was raising it; v2 disables the timeout by default, so dropping it preserves the intent instead of reimposing a limit.What the linter found
112 findings, not the 54 a first run suggests — golangci-lint truncates repeated issues by default, so the honest count needs
--max-same-issues=0.Things worth a second look
Error strings that legitimately start with a capital. All 15 ST1005 hits begin with a product name — Conduit, Sugar, Packet Computers — so lowercasing the first word would have been wrong. The messages lead with the condition instead:
"Conduit run idempotency key is invalid"became"invalid Conduit run idempotency key". Only two of these strings are asserted anywhere in the tests ("integration is disabled"and"must use HTTPS") and both keep the asserted substring.One deliberate policy choice. 40 of the 65 errcheck findings are unchecked type assertions in tests, reported because the original config set
check-type-assertions: true. They look like:Rewriting 40 of those into two-statement checked forms would cost real readability for no safety, because a panicking assertion in a test already fails the test and names the offending type. So
check-type-assertionsstays on for shipping code — and the two production sites ininternal/procrunare fixed — while a scoped rule exempts type assertions in_test.goonly. The rule matches errcheck's unnamed message, so unchecked errors remain enforced in tests too.That is the one part of this PR that is a judgement rather than a fix, and it is one config block to reverse if the project would rather have the rewrites.
A real bug, small.
cmd/packetcode/main.goregistered a cleanup that calledjobsMgr.Shutdownand then returnednilunconditionally, so a job manager that failed to stop cleanly reported success on the way out.Dead code deleted rather than annotated. 67 lines: three test helpers that outlived their tests (
approvalProviderwith nine methods,scriptedAlias,bytesReader), two unused production members (App.startTurnDisplaying,App.err), one unused wrapper (Registry.loadDir), plus anif falseblock and a closure that locked and unlocked around nothing — both of which carried comments admitting they existed to silence a linter that was never running. This matches the call the audit made when it removedinternal/tools/atomic.go.Deprecated viewport API. Four
SA1019hits were bubbles methods renamed upstream:LineDown/LineUp/HalfViewDown/HalfViewUpare nowScrollDown/ScrollUp/HalfPageDown/HalfPageUp. Same behaviour, current names.Import grouping. goimports on its own pulls a stray local import out of the stdlib block but parks it ahead of third-party, which is not what any of these files were reaching for — each already had a packetcode group at the bottom. Setting
goimports.local-prefixessorts them into that group, and the six files with a stranded single import have it merged in. The result is the convention the files already used: stdlib, third-party, packetcode.Verification
Run on windows/amd64 against the branch tip, with golangci-lint v2.9.0, the version CI pins:
golangci-lint run --max-issues-per-linter=0 --max-same-issues=0 ./...golangci-lint fmt ./...golangci-lint config verifygo build ./...go vet ./...internal/mcp'sTestMcpTool_Execute_ToolTimeoutfailed once during this work while golangci-lint was saturating the machine, and passed 3 for 3 on its own afterwards; nothing references the helper deleted from that package.Linting was run under
GOOS=linux,darwinandwindows, and all three report zero issues. That matters here: the first push of this branch still failed CI'slintjob, because linting on Windows never compiles the!windowsfiles and so never saw an unchecked type assertion ininternal/procrun/process_posix.go-- the exact POSIX counterpart of one already fixed on the Windows side. That is fixed inb5b6351, and cross-GOOS linting is how it was caught rather than by another CI round trip.CI
lintpasses. So do all sixbuildjobs,release dry run, andsmokeon all three runners.The jobs still red are the ones already red on
mainat96865bb, the commit this branch forks from, and none of them are touched by this PR:main?vulncheckTUI golden and protocol safetymake tui-golden-checkexits 1test (ubuntu-latest),test (macos-latest)TestApp_Undo_RestoreAndDepth,TestLoopSelfPaced_StartedWhileStreamingKeepsOwnership,TestKillTree*test (windows-latest)TestRunUserPromptSubmit_CollectsStdouttimes out at 5s on a PowerShell hookinternal/procrundeserves the explicit check, since this PR modifies it. Its two failures are byte-identical onmainand on this branch -- same file, sameprocess_posix_test.go:70, sameexec: command with a non-nil Cancel was not created with CommandContext. That is a pre-existing problem in the test harness, not a consequence of these changes.Not addressed here
The other jobs red on
mainare separate problems and untouched by this PR:vulncheck(x/crypto v0.43.0), the TUI golden check, and the platform test failures including the intermittent Windows PowerShell hook timeout.🤖 Generated with Claude Code