From 4e863b4e43cbe1c91550f441dd0d2ab8fc111d87 Mon Sep 17 00:00:00 2001 From: Tri Lam Date: Wed, 3 Jun 2026 17:35:08 -0700 Subject: [PATCH 1/2] ci(lint): centralize nolint patterns (#499) Extend .golangci.yml exclude-rules from PR #507's _test.go G304 base to cover the remaining structural patterns. Strip 22 inline waivers across 14 files (35 -> 13 remaining; -63%). Untouched: patterndetectorprocessor (lane J overlap) and 11 point-specific load-bearing keepers (unsafe aliasing, weak-rng for chaos repro, pre-deprecation parity asserts, doc/export refs). Closes #499. Signed-off-by: Tri Lam --- .golangci.yml | 89 ++++++++++++++++--- .../pyspy/integration_helper_test.go | 2 +- internal/integration/ocb_scrape_test.go | 4 +- .../patterns/silent_data_corruption_test.go | 5 +- module/pkg/patterns/thermal_throttle_test.go | 5 +- module/pkg/replay/runner.go | 6 +- .../testutil/jsonschematest/jsonschematest.go | 11 ++- tools/coverage-check/main.go | 2 +- tools/failure-inject/main.go | 2 +- tools/failure-inject/main_linux_test.go | 2 +- tools/failure-inject/main_test.go | 2 +- tools/failure-inject/podevict/podevict.go | 2 +- tools/genfixtures/main.go | 4 +- tools/genfixtures/main_test.go | 12 +-- 14 files changed, 103 insertions(+), 45 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index a86d2281..3e5a3ee4 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -147,25 +147,86 @@ linters: exclusions: # Path-scoped exclusions consolidate repeated per-site nolint - # waivers into one place so reviewers grep the config, not 20 - # scattered comments. Issue #499 lane A: gosec G304 (potential file - # inclusion via variable) is the canonical noise in test files — - # every detector test reads "testdata/.schema.json" through - # filepath.Join, which is by definition a variable path. The path - # is checked into the repo, the working dir is set by `go test`, - # and the file is not operator-controlled at runtime. Waiving G304 - # on `_test.go` collapses ~22 inline waivers across - # module/pkg/patterns/** and the integration / SDK test trees with - # no loss of coverage on production code (gosec still runs G304 on - # non-test files; see module/pkg/replay/runner.go and - # module/receiver/ncclfrreceiver/nccl_fr.go which keep their inline - # rationale comments because they read paths derived from operator - # config). + # waivers into one place so reviewers grep the config, not 20+ + # scattered comments. Issue #499: progressive centralization waves. + # + # Lane A (PR #507): G304 in `_test.go` — every detector test reads + # "testdata/.schema.json" through filepath.Join, which is by + # definition a variable path. The path is checked into the repo, + # the working dir is set by `go test`, and the file is not + # operator-controlled at runtime. + # + # Lane K (this PR): extend to remaining structural patterns — + # subprocess/network exec in integration tests, hermetic loaders + # in module/pkg/replay, fixture-generation tools, and pattern- + # detector test vocabularies. After this wave, the only inline + # nolint waivers left carry truly point-specific rationale (unsafe + # aliasing, weak-rng for chaos injection, pre-deprecation parity + # asserts) that don't generalize to a path scope. rules: + # Lane A: G304 (potential file inclusion via variable) on test + # files — every _test.go in the tree reads fixture/testdata + # paths via variables. - linters: - gosec path: _test\.go text: "G304" + # Integration tests legitimately invoke subprocesses (G204) and + # hit localhost endpoints (G107) — they orchestrate built + # binaries against ephemeral servers under t.TempDir control. + - linters: + - gosec + path: internal/integration/.*_test\.go + text: "G204|G107" + # Component integration helpers spawn child processes + # (e.g. py-spy) under test-controlled args. + - linters: + - gosec + path: components/.*/integration_helper_test\.go + text: "G204" + # Replay corpus loader is hermetic by design: the fixture dir + # passed in is test-controlled and the loader never sees + # operator-derived paths. Production callers go through the + # receiver's config-validated path, which carries its own + # inline G304 waiver pinpointing the operator boundary. + - linters: + - gosec + path: module/pkg/replay/runner\.go + text: "G304" + # Schema-test helper only reachable from tests; schemaPath is a + # relative path baked into the test corpus. + - linters: + - gosec + path: module/pkg/testutil/jsonschematest/.*\.go + text: "G304" + # genfixtures is a dev tool that writes 0644 JSON/Pkl fixtures + # for checkin under tools/genfixtures/testdata. The output is + # public test data by design — G306 (file perms) does not apply. + - linters: + - gosec + path: tools/genfixtures/.*\.go + text: "G306" + # coverage-check is a developer-only CLI: it ingests a coverage + # profile path supplied by the operator running the tool. Not a + # server; G304 is not the relevant threat model. + - linters: + - gosec + path: tools/coverage-check/.*\.go + text: "G304" + # failure-inject is a chaos-testing CLI that opens operator- + # supplied target paths with 0o600 perms. G304 on the open and + # G404 on the deterministic-seed RNG (chaos repro needs + # reproducibility, not crypto) are intentional. + - linters: + - gosec + path: tools/failure-inject/.*\.go + text: "G304|G404" + # Pattern-detector test corpora reuse the same handful of + # vocabulary literals ("Xid 79", "throttle", etc.) across many + # cases by design — see the "input vocabulary" docstrings. + - linters: + - goconst + path: module/pkg/patterns/.*_test\.go issues: max-issues-per-linter: 0 diff --git a/components/receivers/pyspy/integration_helper_test.go b/components/receivers/pyspy/integration_helper_test.go index 5f2d4693..a1b03e87 100644 --- a/components/receivers/pyspy/integration_helper_test.go +++ b/components/receivers/pyspy/integration_helper_test.go @@ -59,7 +59,7 @@ except KeyboardInterrupt: // The python binary is resolved via exec.LookPath; the script // is a constant inline literal. No external untrusted input. - cmd := exec.CommandContext(ctx, python, "-u", "-c", script) //nolint:gosec // G204 + cmd := exec.CommandContext(ctx, python, "-u", "-c", script) cmd.Env = append(os.Environ(), "PYTHONPATH_TRACECORE="+pythonPkgDir, "PYSPY_DIR="+dir, diff --git a/internal/integration/ocb_scrape_test.go b/internal/integration/ocb_scrape_test.go index ed8b38f4..7f315373 100644 --- a/internal/integration/ocb_scrape_test.go +++ b/internal/integration/ocb_scrape_test.go @@ -118,7 +118,7 @@ service: t.Fatalf("write config: %v", err) } - cmd := exec.Command(bin, "--config="+cfgPath) //nolint:gosec // both args produced under test control (findBuiltBinary + t.TempDir) + cmd := exec.Command(bin, "--config="+cfgPath) // Capture combined output so a startup failure surfaces in the // test log instead of vanishing into the void. cmd.Stdout = io.Discard @@ -213,7 +213,7 @@ func waitForMetrics(t *testing.T, url string, required []string, budget time.Dur var lastErr error var lastBody string for time.Now().Before(deadline) { - resp, err := http.Get(url) //nolint:gosec // localhost ephemeral + resp, err := http.Get(url) switch { case err != nil: lastErr = err diff --git a/module/pkg/patterns/silent_data_corruption_test.go b/module/pkg/patterns/silent_data_corruption_test.go index 35b3b3bd..ff6b0a0b 100644 --- a/module/pkg/patterns/silent_data_corruption_test.go +++ b/module/pkg/patterns/silent_data_corruption_test.go @@ -604,9 +604,8 @@ func TestSDCVerdict_SchemaConformance(t *testing.T) { // // The repeated `rejected` / `maybe` / `full` literals are the test's // input vocabulary — extracting them to package-level constants would -// obscure which row mutates which schema axis. -// -//nolint:goconst // See "input vocabulary" note above. +// obscure which row mutates which schema axis. goconst is path-scoped +// off for module/pkg/patterns/*_test.go in .golangci.yml. func TestSDCVerdict_SchemaRejectsDrift(t *testing.T) { t.Parallel() diff --git a/module/pkg/patterns/thermal_throttle_test.go b/module/pkg/patterns/thermal_throttle_test.go index 0a35f63d..e99ac188 100644 --- a/module/pkg/patterns/thermal_throttle_test.go +++ b/module/pkg/patterns/thermal_throttle_test.go @@ -380,9 +380,8 @@ func TestThermalThrottleVerdict_SchemaConformance(t *testing.T) { // // The repeated `full` literals are the test's input vocabulary — // extracting them to a package-level constant would obscure which row -// mutates which schema axis. -// -//nolint:goconst // See "input vocabulary" note above. +// mutates which schema axis. goconst is path-scoped off for +// module/pkg/patterns/*_test.go in .golangci.yml. func TestThermalThrottleVerdict_SchemaRejectsDrift(t *testing.T) { t.Parallel() diff --git a/module/pkg/replay/runner.go b/module/pkg/replay/runner.go index 2799812e..705bdaa7 100644 --- a/module/pkg/replay/runner.go +++ b/module/pkg/replay/runner.go @@ -77,7 +77,7 @@ func LoadFixture(dir string) (Fixture, error) { name := filepath.Base(dir) f := Fixture{Name: name} - manBytes, err := os.ReadFile(filepath.Join(dir, manifestFile)) //nolint:gosec // fixture dir is test-controlled; replay corpus loader is hermetic by design + manBytes, err := os.ReadFile(filepath.Join(dir, manifestFile)) if err != nil { return f, fmt.Errorf("replay.LoadFixture(%s): %s: %w", name, manifestFile, err) } @@ -92,7 +92,7 @@ func LoadFixture(dir string) (Fixture, error) { return f, fmt.Errorf("replay.LoadFixture(%s): %w", name, err) } - golden, err := os.ReadFile(filepath.Join(dir, goldenFile)) //nolint:gosec // fixture dir is test-controlled; replay corpus loader is hermetic by design + golden, err := os.ReadFile(filepath.Join(dir, goldenFile)) if err != nil { return f, fmt.Errorf("replay.LoadFixture(%s): %s: %w", name, goldenFile, err) } @@ -105,7 +105,7 @@ func LoadFixture(dir string) (Fixture, error) { // Missing-file returns nil; malformed-content returns the unmarshal // error so silent fixture drift is impossible. func readJSONOptional(path string, target any) error { - bs, err := os.ReadFile(path) //nolint:gosec // path comes from the fixture-dir join above; replay loader is hermetic + bs, err := os.ReadFile(path) if os.IsNotExist(err) { return nil } diff --git a/module/pkg/testutil/jsonschematest/jsonschematest.go b/module/pkg/testutil/jsonschematest/jsonschematest.go index 1f25c58a..47199d33 100644 --- a/module/pkg/testutil/jsonschematest/jsonschematest.go +++ b/module/pkg/testutil/jsonschematest/jsonschematest.go @@ -42,12 +42,11 @@ import ( func Compile(t testing.TB, schemaPath string) *jsonschema.Schema { t.Helper() - // G304: the path is test-local. The package-level gosec G304 - // exclusion for *_test.go in .golangci.yml covers callers; this - // helper is non-test code but is only reachable from tests, so - // we accept one waiver here in exchange for deleting 22 inline - // waivers across the patterns suite. - schemaBytes, err := os.ReadFile(schemaPath) //nolint:gosec // schemaPath is a test-controlled relative path; helper is reached only from tests + // G304: the path is test-local. Covered by the + // module/pkg/testutil/jsonschematest path-scoped gosec G304 + // exclusion in .golangci.yml — this helper is non-test code but + // is only reachable from tests. + schemaBytes, err := os.ReadFile(schemaPath) if err != nil { t.Fatalf("jsonschematest.Compile: read %s: %v", schemaPath, err) return nil diff --git a/tools/coverage-check/main.go b/tools/coverage-check/main.go index c70d9a17..e9762157 100644 --- a/tools/coverage-check/main.go +++ b/tools/coverage-check/main.go @@ -91,7 +91,7 @@ func main() { } func parseProfile(path string) (map[string]*pkgCov, error) { - f, err := os.Open(path) //nolint:gosec // operator-supplied profile path; this is a dev tool, not a server. + f, err := os.Open(path) if err != nil { return nil, err //nolint:wrapcheck // os.Open errors already include the path; wrapping would stutter. } diff --git a/tools/failure-inject/main.go b/tools/failure-inject/main.go index 9c1f6c06..472c3622 100644 --- a/tools/failure-inject/main.go +++ b/tools/failure-inject/main.go @@ -200,7 +200,7 @@ func openOutput(path string, stdout io.Writer) (io.Writer, func(), error) { // OpenFile and keep the operator-trust assumption documented in // the README. flags := os.O_WRONLY | os.O_CREATE | os.O_TRUNC | openNoFollow - f, err := os.OpenFile(path, flags, 0o600) //nolint:gosec + f, err := os.OpenFile(path, flags, 0o600) if err != nil { return nil, nil, fmt.Errorf("open --out: %w", err) } diff --git a/tools/failure-inject/main_linux_test.go b/tools/failure-inject/main_linux_test.go index ffd61b3f..37b6550b 100644 --- a/tools/failure-inject/main_linux_test.go +++ b/tools/failure-inject/main_linux_test.go @@ -39,7 +39,7 @@ func TestRun_OutRefusesSymlink(t *testing.T) { "open should fail with ELOOP, proving O_NOFOLLOW is in the flag set; stderr=%q", stderr.String()) // The symlink target must be untouched (no truncation). - got, err := os.ReadFile(target) //nolint:gosec // path is t.TempDir-scoped + got, err := os.ReadFile(target) require.NoError(t, err) require.Equal(t, "sentinel\n", string(got), "symlink target was clobbered — O_NOFOLLOW guard is broken") diff --git a/tools/failure-inject/main_test.go b/tools/failure-inject/main_test.go index 515f16bd..8c14d333 100644 --- a/tools/failure-inject/main_test.go +++ b/tools/failure-inject/main_test.go @@ -136,7 +136,7 @@ func TestRun_OutFlagWritesFile(t *testing.T) { require.Equal(t, exitOK, rc, "stderr=%q", stderr.String()) require.Zero(t, stdout.Len(), "with --out, stdout must stay empty") - data, err := os.ReadFile(outPath) //nolint:gosec // path is t.TempDir-scoped + data, err := os.ReadFile(outPath) require.NoError(t, err) require.Contains(t, string(data), "reason: Evicted") } diff --git a/tools/failure-inject/podevict/podevict.go b/tools/failure-inject/podevict/podevict.go index 45dcbf07..44324c37 100644 --- a/tools/failure-inject/podevict/podevict.go +++ b/tools/failure-inject/podevict/podevict.go @@ -69,7 +69,7 @@ func Run(ctx context.Context, opts Options, w io.Writer) error { // Same gosec G404 rationale as xidgen: deterministic PCG IS the // contract; this is not a key-generation surface. - rng := rand.New(rand.NewPCG(opts.Seed, opts.Seed^0xBF58476D1CE4E5B9)) //nolint:gosec + rng := rand.New(rand.NewPCG(opts.Seed, opts.Seed^0xBF58476D1CE4E5B9)) uid := newUID(rng) podName := fmt.Sprintf("training-job-%04x", rng.Uint64N(0x10000)) nodeName := fmt.Sprintf("gpu-node-%04x", rng.Uint64N(0x10000)) diff --git a/tools/genfixtures/main.go b/tools/genfixtures/main.go index c0935682..355f2a52 100644 --- a/tools/genfixtures/main.go +++ b/tools/genfixtures/main.go @@ -35,7 +35,7 @@ func run(outDir string) error { return err } pklPath := filepath.Join(outDir, f.Slug+".pkl") - if err := os.WriteFile(pklPath, bs, 0o644); err != nil { //nolint:gosec // fixture, not sensitive + if err := os.WriteFile(pklPath, bs, 0o644); err != nil { return fmt.Errorf("write %s: %w", pklPath, err) } @@ -49,7 +49,7 @@ func run(outDir string) error { } raw = append(raw, '\n') jsonPath := filepath.Join(outDir, f.Slug+".golden.json") - if err := os.WriteFile(jsonPath, raw, 0o644); err != nil { //nolint:gosec // fixture, not sensitive + if err := os.WriteFile(jsonPath, raw, 0o644); err != nil { return fmt.Errorf("write %s: %w", jsonPath, err) } } diff --git a/tools/genfixtures/main_test.go b/tools/genfixtures/main_test.go index a503fbc2..049715c2 100644 --- a/tools/genfixtures/main_test.go +++ b/tools/genfixtures/main_test.go @@ -44,13 +44,13 @@ func TestRun_HappyPath(t *testing.T) { pklPath := filepath.Join(outDir, f.Slug+".pkl") jsonPath := filepath.Join(outDir, f.Slug+".golden.json") - gotPkl, err := os.ReadFile(pklPath) //nolint:gosec // test-controlled path + gotPkl, err := os.ReadFile(pklPath) require.NoError(t, err, "missing pkl output") wantPkl, err := f.Bytes() require.NoError(t, err) require.Equal(t, wantPkl, gotPkl, "pkl bytes mismatch for %s", f.Slug) - gotJSON, err := os.ReadFile(jsonPath) //nolint:gosec // test-controlled path + gotJSON, err := os.ReadFile(jsonPath) require.NoError(t, err, "missing golden json output") // Output must be valid JSON with the expected top-level shape. @@ -92,9 +92,9 @@ func TestRun_Determinism(t *testing.T) { for _, f := range frparser.BuildFixtures() { for _, suffix := range []string{".pkl", ".golden.json"} { name := f.Slug + suffix - aBytes, err := os.ReadFile(filepath.Join(a, name)) //nolint:gosec // test-controlled path + aBytes, err := os.ReadFile(filepath.Join(a, name)) require.NoError(t, err) - bBytes, err := os.ReadFile(filepath.Join(b, name)) //nolint:gosec // test-controlled path + bBytes, err := os.ReadFile(filepath.Join(b, name)) require.NoError(t, err) require.Equal(t, aBytes, bBytes, "non-deterministic output for %s", name) } @@ -117,9 +117,9 @@ func TestRun_MatchesCommittedGoldens(t *testing.T) { for _, f := range frparser.BuildFixtures() { for _, suffix := range []string{".pkl", ".golden.json"} { name := f.Slug + suffix - fresh, err := os.ReadFile(filepath.Join(outDir, name)) //nolint:gosec // test-controlled path + fresh, err := os.ReadFile(filepath.Join(outDir, name)) require.NoError(t, err) - gold, err := os.ReadFile(filepath.Join(committed, name)) //nolint:gosec // test-controlled path + gold, err := os.ReadFile(filepath.Join(committed, name)) require.NoError(t, err, "committed golden missing: %s", name) require.Equal(t, gold, fresh, "committed %s differs from a fresh run(); run `make generate-fixtures`", name) From b6911ccc47bd0b8ee10c11bbcdc698512034496b Mon Sep 17 00:00:00 2001 From: Tri Lam Date: Wed, 3 Jun 2026 17:46:41 -0700 Subject: [PATCH 2/2] ci(lint): strip PR-# ref from .golangci.yml comment Signed-off-by: Tri Lam --- .golangci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.golangci.yml b/.golangci.yml index 3e5a3ee4..f7671d8c 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -150,7 +150,7 @@ linters: # waivers into one place so reviewers grep the config, not 20+ # scattered comments. Issue #499: progressive centralization waves. # - # Lane A (PR #507): G304 in `_test.go` — every detector test reads + # G304 in `_test.go` — every detector test reads # "testdata/.schema.json" through filepath.Join, which is by # definition a variable path. The path is checked into the repo, # the working dir is set by `go test`, and the file is not