Skip to content

Commit fd12dd3

Browse files
committed
Fix gosec and nilaway findings, enforce both in CI
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.
1 parent 097c8d1 commit fd12dd3

35 files changed

Lines changed: 383 additions & 347 deletions

‎.github/workflows/ci.yml‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,17 @@ jobs:
3939
sudo apt-get update
4040
sudo apt-get install -y --no-install-recommends libsqlite3-dev
4141
42+
# golangci-lint-action@v6's "latest" resolves within the v1.x line
43+
# (v1.64.8 at time of writing), which refuses to run against this
44+
# module's go 1.26.5 directive ("the Go language version used to
45+
# build golangci-lint is lower than the targeted Go version"). v9
46+
# supports golangci-lint v2, and pinning a v2 release keeps the
47+
# Go-version check satisfied without depending on how "latest"
48+
# happens to resolve.
4249
- name: golangci-lint
43-
uses: golangci/golangci-lint-action@v6
50+
uses: golangci/golangci-lint-action@v9
4451
with:
45-
version: latest
52+
version: v2.12.2
4653
args: --build-tags=sqlite_fts5
4754

4855
test:
@@ -225,14 +232,15 @@ jobs:
225232
# set up — gives gosec's type-checking source loader what it needs to
226233
# actually analyze cgo-dependent packages instead of skipping them.
227234
#
228-
# continue-on-error: a first run against this codebase turned up 61
229-
# pre-existing findings (mostly G104 unhandled-error-on-Close, plus a
230-
# handful of G301/G304/G306/G115/G202/G401/G505). None of that is this
231-
# job's fault to gate on — report-only until it's triaged, then remove
232-
# continue-on-error so new findings start blocking merges.
235+
# The 61 pre-existing findings from the first run against this codebase
236+
# (mostly G104 unhandled-error-on-Close, plus a handful of
237+
# G301/G304/G306/G115/G202/G401/G505) have all been triaged: fixed where
238+
# real, suppressed with an inline `// #nosec Gxxx -- reason` comment
239+
# (gosec's own directive — it does not understand golangci-lint's
240+
# `//nolint` comments when run standalone like this) where the flagged
241+
# pattern is safe by inspection. New findings now block merges.
233242
gosec:
234243
runs-on: ubuntu-latest
235-
continue-on-error: true
236244
steps:
237245
- name: Checkout
238246
uses: actions/checkout@v4
@@ -260,12 +268,15 @@ jobs:
260268
# see, which is the flag NilAway's own docs call out as the standard way to
261269
# keep a first run's signal-to-noise ratio sane on an existing codebase.
262270
#
263-
# continue-on-error: a first run against this codebase found 3 potential
264-
# nil panics in indexer/indexer.go, pre-existing and unrelated to this
265-
# workflow. Report-only until triaged, then remove continue-on-error.
271+
# The 3 potential nil panics found by the first run against this codebase
272+
# (all in indexer/indexer.go, one structural root cause) have been fixed:
273+
# a nilable `var x []T` accumulator became a non-nil `make([]T, 0, n)`,
274+
# and the two consuming loops were merged into the same block that
275+
# assigns and validates the embedder's result slices, giving NilAway's
276+
# flow analysis a straight-line path to see them as non-nil. New findings
277+
# now block merges.
266278
nilaway:
267279
runs-on: ubuntu-latest
268-
continue-on-error: true
269280
steps:
270281
- name: Checkout
271282
uses: actions/checkout@v4

‎cmd/codamigo/doctor_cmd.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ func doctorCmd() *cli.Command {
119119
if err != nil {
120120
fmt.Printf("[FAIL] Store open error: %v\n", err)
121121
} else {
122-
defer s.Close()
122+
defer func() { _ = s.Close() }() // best-effort cleanup; the process is exiting either way
123123
stats, err := s.Stats(ctx)
124124
if err != nil {
125125
fmt.Printf("[FAIL] Stats error: %v\n", err)
@@ -157,7 +157,7 @@ func doctorCmd() *cli.Command {
157157
if err != nil {
158158
fmt.Printf("[FAIL] Walker error: %v\n", err)
159159
} else {
160-
defer w.Close()
160+
defer func() { _ = w.Close() }() // best-effort cleanup; the process is exiting either way
161161
count := 0
162162
errCount := 0
163163
for _, err := range w.Walk(ctx) {

‎cmd/codamigo/graph_cmd.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ func runGraphQuery(
106106
if err != nil {
107107
return err
108108
}
109-
defer s.Close()
109+
defer func() { _ = s.Close() }() // best-effort cleanup; the process is exiting either way
110110

111111
refs, err := run(query.New(emb, s), symbol)
112112
if err != nil {

‎cmd/codamigo/init_cmd.go‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ func initCmd() *cli.Command {
7777
}
7878

7979
// Write global config.
80-
if err := os.MkdirAll(filepath.Dir(globalPath), 0o755); err != nil {
80+
if err := os.MkdirAll(filepath.Dir(globalPath), 0o750); err != nil {
8181
return fmt.Errorf("creating config directory: %w", err)
8282
}
8383
// 0o600 below matters: this file may hold an API key or a
@@ -111,11 +111,11 @@ func initCmd() *cli.Command {
111111
// Always create project config if missing — runs regardless of global config state.
112112
projectPath := config.ProjectConfigPath()
113113
if _, err := os.Stat(projectPath); errors.Is(err, fs.ErrNotExist) {
114-
if err := os.MkdirAll(filepath.Dir(projectPath), 0o755); err != nil {
114+
if err := os.MkdirAll(filepath.Dir(projectPath), 0o750); err != nil {
115115
return fmt.Errorf("creating project config directory: %w", err)
116116
}
117117
projectContent := "# codamigo project settings\n# include_patterns: []\n# exclude_patterns: []\n"
118-
if err := os.WriteFile(projectPath, []byte(projectContent), 0o644); err != nil {
118+
if err := os.WriteFile(projectPath, []byte(projectContent), 0o600); err != nil {
119119
return fmt.Errorf("writing project config: %w", err)
120120
}
121121
fmt.Printf("Created project config: %s\n", projectPath)
@@ -134,11 +134,11 @@ func initCmd() *cli.Command {
134134
dataDir, err := config.ProjectDataDir(wd)
135135
if err != nil {
136136
fmt.Fprintf(os.Stderr, "Warning: could not resolve data directory: %v\n", err)
137-
} else if err := os.MkdirAll(dataDir, 0o755); err != nil {
137+
} else if err := os.MkdirAll(dataDir, 0o750); err != nil {
138138
fmt.Fprintf(os.Stderr, "Warning: could not create data directory: %v\n", err)
139139
} else {
140140
pathFile := filepath.Join(dataDir, "project_path")
141-
if err := os.WriteFile(pathFile, []byte(wd), 0o644); err != nil {
141+
if err := os.WriteFile(pathFile, []byte(wd), 0o600); err != nil {
142142
fmt.Fprintf(os.Stderr, "Warning: could not write project_path: %v\n", err)
143143
}
144144
}
@@ -201,9 +201,9 @@ func initCmd() *cli.Command {
201201
// buffered input from piped stdin is not lost between calls.
202202
func readPrompt(scanner *bufio.Scanner, w io.Writer, prompt, defaultVal string) string {
203203
if defaultVal != "" {
204-
fmt.Fprintf(w, "%s [%s]: ", prompt, defaultVal)
204+
_, _ = fmt.Fprintf(w, "%s [%s]: ", prompt, defaultVal)
205205
} else {
206-
fmt.Fprintf(w, "%s: ", prompt)
206+
_, _ = fmt.Fprintf(w, "%s: ", prompt)
207207
}
208208
if scanner.Scan() {
209209
line := strings.TrimSpace(scanner.Text())
@@ -227,6 +227,7 @@ func appendToGitignore(projectRoot string) error {
227227
const entry = ".codamigo/"
228228
gitignorePath := filepath.Join(projectRoot, ".gitignore")
229229

230+
// #nosec G304 -- gitignorePath is derived from the CLI's own project root, not external input
230231
content, err := os.ReadFile(gitignorePath)
231232
if err != nil && !errors.Is(err, fs.ErrNotExist) {
232233
return fmt.Errorf("reading .gitignore: %w", err)
@@ -238,6 +239,9 @@ func appendToGitignore(projectRoot string) error {
238239
}
239240
}
240241

242+
// #nosec G304,G302 -- 0o644 matches .gitignore's conventional world-readable
243+
// permissions; gitignorePath is derived from the CLI's own project root,
244+
// not external input.
241245
f, err := os.OpenFile(gitignorePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644)
242246
if err != nil {
243247
return fmt.Errorf("opening .gitignore: %w", err)

‎cmd/codamigo/main.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,8 @@ func indexCmd() *cli.Command {
130130
if err != nil {
131131
return err
132132
}
133-
defer s.Close()
134-
defer w.Close() //nolint:errcheck
133+
defer func() { _ = s.Close() }() // best-effort cleanup; the process is exiting either way
134+
defer func() { _ = w.Close() }() // best-effort cleanup; the process is exiting either way
135135

136136
// Wrap ctx so the TUI can cancel the indexer directly via ctrl+c.
137137
// In TTY mode the terminal is in raw mode (ISIG cleared), so ctrl+c
@@ -233,8 +233,8 @@ func serveCmd() *cli.Command {
233233
if err != nil {
234234
return err
235235
}
236-
defer s.Close()
237-
defer w.Close()
236+
defer func() { _ = s.Close() }() // best-effort cleanup; the process is exiting either way
237+
defer func() { _ = w.Close() }() // best-effort cleanup; the process is exiting either way
238238
queryEmb, err := queryEmbedderFor(cfg, indexEmb)
239239
if err != nil {
240240
return fmt.Errorf("creating embedder: %w", err)
@@ -251,7 +251,7 @@ func serveCmd() *cli.Command {
251251
if err != nil {
252252
return fmt.Errorf("creating watcher: %w", err)
253253
}
254-
defer wch.Close()
254+
defer func() { _ = wch.Close() }() // best-effort cleanup; the process is exiting either way
255255
srv := mcp.NewServer(q, idx, wch,
256256
mcp.WithNonCodeLanguages(cfg.NonCodeLanguages),
257257
mcp.WithGraph(cfg.GraphEnabled()),
@@ -422,7 +422,7 @@ func buildComponents(cfg *config.Config, storePath string, dim int) (*chunker.Ch
422422
filter := buildExtensionFilter(allLangs)
423423
w, err := walker.New(cfg.ProjectRoot, cfg, walker.WithFileFilter(filter))
424424
if err != nil {
425-
s.Close()
425+
_ = s.Close() // best-effort cleanup; the walker error below is the one worth reporting
426426
return nil, nil, nil, fmt.Errorf("creating walker: %w", err)
427427
}
428428
return c, s, w, nil

‎cmd/codamigo/map_cmd.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func mapCmd() *cli.Command {
5252
if err != nil {
5353
return err
5454
}
55-
defer s.Close()
55+
defer func() { _ = s.Close() }() // best-effort cleanup; the process is exiting either way
5656

5757
q := query.New(emb, s)
5858

‎cmd/codamigo/reset_cmd.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,19 +44,19 @@ func resetCmd() *cli.Command {
4444
// is testable without touching os.Stdin/os.Stdout.
4545
func runReset(storePath string, force bool, in io.Reader, out io.Writer) error {
4646
if _, err := os.Stat(storePath); errors.Is(err, fs.ErrNotExist) {
47-
fmt.Fprintln(out, "Nothing to reset.")
47+
_, _ = fmt.Fprintln(out, "Nothing to reset.")
4848
return nil
4949
}
5050

5151
if !force {
52-
fmt.Fprintf(out, "The following file will be deleted:\n %s\nDelete store file? [y/N]: ", storePath)
52+
_, _ = fmt.Fprintf(out, "The following file will be deleted:\n %s\nDelete store file? [y/N]: ", storePath)
5353
scanner := bufio.NewScanner(in)
5454
answer := ""
5555
if scanner.Scan() {
5656
answer = strings.TrimSpace(scanner.Text())
5757
}
5858
if !strings.EqualFold(answer, "y") {
59-
fmt.Fprintln(out, "Aborted.")
59+
_, _ = fmt.Fprintln(out, "Aborted.")
6060
return nil
6161
}
6262
}
@@ -66,6 +66,6 @@ func runReset(storePath string, force bool, in io.Reader, out io.Writer) error {
6666
return fmt.Errorf("deleting store%s: %w", suffix, err)
6767
}
6868
}
69-
fmt.Fprintf(out, "Store deleted: %s\n", storePath)
69+
_, _ = fmt.Fprintf(out, "Store deleted: %s\n", storePath)
7070
return nil
7171
}

‎cmd/codamigo/search_cmd.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ func searchCmd() *cli.Command {
109109
if err != nil {
110110
return err
111111
}
112-
defer s.Close()
112+
defer func() { _ = s.Close() }() // best-effort cleanup; the process is exiting either way
113113

114114
maxTokens := cmd.Int("max-tokens")
115115
if maxTokens < 0 {

‎config/config.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
package config
1212

1313
import (
14-
"crypto/sha1"
14+
"crypto/sha1" // #nosec G505 -- non-cryptographic use, see ProjectHash
1515
"encoding/hex"
1616
"errors"
1717
"fmt"
@@ -307,6 +307,9 @@ func ProjectConfigPath() string {
307307
// that "/home/user/project/" and "/home/user/project" produce the same hash.
308308
func ProjectHash(projectRoot string) string {
309309
trimmed := strings.Trim(projectRoot, "/")
310+
// #nosec G401 -- non-cryptographic use: stable directory-name hash, not a
311+
// security boundary; switching algorithms would relocate existing users'
312+
// data dirs.
310313
sum := sha1.Sum([]byte(trimmed))
311314
return hex.EncodeToString(sum[:])
312315
}
@@ -365,11 +368,12 @@ func HomeProjectConfigPath(projectRoot string) (string, error) {
365368
// Returns an error if the file does not exist, contains unknown keys, or has
366369
// malformed duration strings.
367370
func Load(path string) (*Config, error) {
371+
// #nosec G304 -- path is the CLI's own config path (flag/default), not external input
368372
f, err := os.Open(path)
369373
if err != nil {
370374
return nil, fmt.Errorf("opening config %q: %w", path, err)
371375
}
372-
defer f.Close()
376+
defer func() { _ = f.Close() }() // best-effort cleanup; the file is only being read
373377

374378
dec := yaml.NewDecoder(f)
375379
dec.KnownFields(true)

‎config/config_test.go‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,9 @@ func writeTempConfig(t *testing.T, content string) string {
288288
if _, err := f.WriteString(content); err != nil {
289289
t.Fatalf("writing temp config: %v", err)
290290
}
291-
f.Close()
291+
if err := f.Close(); err != nil {
292+
t.Fatalf("closing temp config: %v", err)
293+
}
292294
return f.Name()
293295
}
294296

@@ -326,9 +328,13 @@ func ExampleLoad() {
326328
if err != nil {
327329
panic(err)
328330
}
329-
defer os.Remove(f.Name())
330-
f.WriteString("embedding_model: my-model\npoll_interval: 10s\n")
331-
f.Close()
331+
defer func() { _ = os.Remove(f.Name()) }()
332+
if _, err := f.WriteString("embedding_model: my-model\npoll_interval: 10s\n"); err != nil {
333+
panic(err)
334+
}
335+
if err := f.Close(); err != nil {
336+
panic(err)
337+
}
332338

333339
cfg, err := config.Load(f.Name())
334340
if err != nil {
@@ -591,7 +597,7 @@ func TestProjectHash(t *testing.T) {
591597
t.Errorf("hash length = %d, want 40", len(h))
592598
}
593599
for _, c := range h {
594-
if !(c >= '0' && c <= '9' || c >= 'a' && c <= 'f') {
600+
if (c < '0' || c > '9') && (c < 'a' || c > 'f') {
595601
t.Errorf("hash contains non-hex char %q in %q", c, h)
596602
}
597603
}

0 commit comments

Comments
 (0)