Repository navigation
Fix gosec and nilaway findings, enforce both in CI - #6
Merged
Merged
Conversation
eshanclio
force-pushed
the
fix/gosec-nilaway-findings
branch
2 times, most recently
from
August 5, 2026 19:18
e96c581 to
fd12dd3
Compare
Triages all 61 gosec findings and all 3 nilaway findings, then removes continue-on-error from the existing gosec/nilaway CI jobs so new findings block merges going forward. Also fixes two other pre-existing CI failures (govulncheck, lint) uncovered while getting this branch fully green end to end, and the lint job's own full pre-existing findings backlog once fixing its version-pin bug let it actually run. gosec: - G104 (unhandled Close errors): replaced with plain `_ =` assignments matching the codebase's existing convention, needing no suppression at all. - G301/G306/G302 (permissions): tightened directories to 0o750 and app-owned files to 0o600; .gitignore intentionally kept at the conventional 0o644 (world-readable, shared in the repo). - G304 (dynamic path), G202 (SQL concat), G115 (int narrowing), G401/G505 (SHA-1), G103 (unsafe.Pointer): verified genuine false positives by inspection and suppressed with an inline `// #nosec Gxxx -- reason` comment (gosec's own directive, since golangci-lint's `//nolint` is not understood when gosec runs standalone as it does in CI). This includes six findings in the Linux-only inotify backend that a local macOS run can't see, since it's gated by a linux build tag. - watcher/backend_inotify.go also hoists an int->uint32 conversion out of a for-loop header: gosec's own nosec-comment scanner fails to suppress a finding on the very next line when the loop header carries its own nosec comment, so the conversion moves to its own statement before the loop. nilaway: restructured indexer's batch-embedding path so the `origins` accumulator is a non-nil `make([]T, 0, n)` instead of a nil `var`, and merged two loops into one straight-line block, giving nilaway's flow analysis a provably non-nil path. Behavior-preserving; covered by existing indexer_test.go fakes. govulncheck: bumps golang.org/x/text to v0.39.0, fixing GO-2026-5970 (infinite loop on invalid input), reached transitively through the tokenizer's Unicode normalization. lint: golangci-lint-action@v6's `version: latest` resolves within the v1.x line, which refuses to run against this module's go 1.26.5 directive. Moves to golangci-lint-action@v9 pinned to golangci-lint v2.12.2, which supports it. That version-pin fix let the job actually lint for the first time (it was crashing before reaching any file), surfacing a 223-issue backlog (218 errcheck + 5 staticcheck) across 21 files hidden behind both the crash and golangci-lint's own default max-issues-per-linter/max-same-issues caps. Fixed all of them: unchecked errors become `_ =` (or the package's existing closeQuietly helper for io.Closer values in store/sqlite.go and store/graph.go), and the 5 staticcheck simplifications (3x WriteString(Sprintf(...)) -> Fprintf, one De Morgan's law rewrite, one redundant embedded-field selector) are applied as suggested.
eshanclio
force-pushed
the
fix/gosec-nilaway-findings
branch
from
August 5, 2026 19:23
fd12dd3 to
ab03d07
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.
Summary
Triages all 61 pre-existing
gosecfindings and all 3nilawayfindings against this codebase, then removescontinue-on-errorfrom the existinggosec/nilawayCI jobs so new findings block merges going forward.gosec
Close()errors, the bulk of the findings): replaced with plain_ = x.Close()assignments, matching the codebase's own existing convention (e.g.watcher/backend_kqueue.go's_ = unix.Close(...)). This satisfies gosec and errcheck natively — no suppression needed.0o750and app-owned files (config, model cache, path files) to0o600..gitignoreintentionally stays at0o644— it's conventionally world-readable and shared in the repo — and is suppressed with a documented reason.// #nosec Gxxx -- reasoncomment — gosec's own directive, since golangci-lint's//nolintisn't understood when gosec runs standalone as it does in this CI job.nilaway
All 3 findings were the same structural root cause in
indexer.go's batch-embedding path: a nil-zero-value slice accumulator indexed later in a separated block. Fixed by making the accumulator a non-nilmake([]T, 0, n)and merging two consuming loops into one straight-line block right after the assigning call, giving nilaway's flow analysis a provably non-nil path. Behavior-preserving; already covered by the existingindexer_test.gofakes.CI
Removed
continue-on-error: truefrom both jobs now that their findings are addressed.Verification
gosec -tags=sqlite_fts5 ./...→ 0 issues (11 remaining#nosecsuppressions, each justified above)nilaway -include-pkgs="github.com/ieshan/codamigo" ./...→ cleango build ./...,go vet ./...,gofmt -l .→ cleango test ./...andgo test -race ./indexer/... ./watcher/... ./store/...→ passgolangci-lint run --build-tags=sqlite_fts5→ unchanged from the pre-existing 55-issue baseline