Skip to content

fix(go): nilness/staticcheck/gosec audit on cmd/ + pkg/ - #362

Merged
lusoris merged 1 commit into
masterfrom
fix/go-nilness-staticcheck-audit
May 31, 2026
Merged

fix(go): nilness/staticcheck/gosec audit on cmd/ + pkg/#362
lusoris merged 1 commit into
masterfrom
fix/go-nilness-staticcheck-audit

Conversation

@lusoris

@lusoris lusoris commented May 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Static-analysis sweep across all fork-added Go code under cmd/ + pkg/,
running three industry-standard analysers beyond the default go vet:
nilness, staticcheck, and gosec. Every real finding fixed
while preserving behaviour; the two false-positive security warnings
are annotated with justified #nosec markers + explanatory comments.

Tool Pre-fix Post-fix
nilness 0 0
staticcheck 13 0
gosec -high 3 0

Breakdown: SA1019 x4, U1000 x8, S1009 x1, G118 x2 (FP), G122 x1 (FP).

Changes

  • SA1019 (deprecated symbol) x4 — replaced prometheus.NewGoCollector /
    NewProcessCollector with the prometheus/collectors subpackage in
    cmd/vmafx-server/main.go and cmd/vmafx-controller/main.go. The
    collectors subpackage is already transitively pinned; no go.mod
    motion     (PR chore(security): bump x/net + x/sys to clear 7 govulncheck advisories #341 owns dependency upgrades).
  • U1000 (unused) x8 — deleted dead mockController scaffolding from
    cmd/vmafx-node/main_test.go (7 symbols orphaned when the
    controller-registration path was replaced — see the file header), and
    an unused mockScoreFunc from pkg/bisect/bisect_test.go.
  • S1009 (redundant guard) x1 — simplified models != nil && len(models) != 0
    to len(models) != 0 in pkg/ai/infer_test.go.
  • G118 (FP) x2 — annotated graceful-shutdown goroutines: the
    canonical net/http shutdown pattern intentionally uses
    context.Background() because the request-scoped ctx is the one being
    cancelled; propagating it would abort the in-flight-request drain.
  • G122 (FP) x1 — added a filepath.Abs + HasPrefix containment
    guard around the os.ReadFile in cmd/vmafx-mcp/impl.go's
    feature-extractor walk (input root is the repo's own source tree).

Test plan

  • nilness ./... -> clean
  • staticcheck ./... -> clean
  • gosec -severity high ./... -> 0 issues, 3 justified #nosec
  • go build ./... -> clean
  • go test -race ./cmd/vmafx-{server,controller,mcp,node}/... ./pkg/{ai,bisect}/... -> all pass
  • gofmt -l on touched dirs -> clean
  • cmd/vmafx-operator/internal/controller tests need /usr/local/kubebuilder/bin/etcd — pre-existing host-env gap, unrelated to this PR.

Avoiding PR overlap

Deep-dive deliverables (ADR-0108)

  • Research digest — docs/research/go-nilness-staticcheck-audit-2026-05-30.md.
  • Decision matrix — no alternatives: only-one-way fix (delete dead code, switch to non-deprecated API, annotate FPs).
  • AGENTS.md invariant note — no rebase-sensitive invariants (every touched file is fork-original Go under cmd/vmafx-* / pkg/*, not shared with upstream).
  • Reproducer / smoke-test command — 
    nilness ./... && staticcheck ./... && gosec -severity high ./...
go test -race ./cmd/vmafx-{server,controller,mcp,node}/... ./pkg/{ai,bisect}/...
  • CHANGELOG fragment — changelog.d/fixed/go-nilness-staticcheck-audit.md.
  • Rebase note — entry added to docs/rebase-notes.md: no rebase impact, all files are fork-original Go.

Bug-status hygiene (ADR-0165)

  • docs/state.md — no state delta: no bug opens/closes/rules-out from this PR (pure lint-debt cleanup).

Netflix golden-data gate

  • No assertAlmostEqual(...) modified.

Reproducer

git fetch origin fix/go-nilness-staticcheck-audit && git checkout FETCH_HEAD
go install golang.org/x/tools/go/analysis/passes/nilness/cmd/nilness@latest
go install honnef.co/go/tools/cmd/staticcheck@latest
go install github.com/securego/gosec/v2/cmd/gosec@latest
nilness ./...; staticcheck ./...; gosec -severity high ./...
# Expect: silence, silence, "Issues: 0".

Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com

[Generated with Claude Code]

This was referenced May 30, 2026
Merged
Closed
Merged
@lusoris
lusoris force-pushed the fix/go-nilness-staticcheck-audit branch from 18be020 to 59cbf19 Compare May 31, 2026 13:03
@lusoris
lusoris marked this pull request as ready for review May 31, 2026 13:03
@lusoris
lusoris merged commit 2f1f62d into master May 31, 2026
26 of 80 checks passed
@lusoris
lusoris deleted the fix/go-nilness-staticcheck-audit branch May 31, 2026 13:03
lusoris added a commit that referenced this pull request Jun 2, 2026
@lusoris @claude
fix(ci,operator): install kubebuilder envtest binaries for operator s…
65ef930 
…uite

The cmd/vmafx-operator/internal/controller envtest suite was hard-failing
in BeforeSuite with a nil-pointer deref from controlplane.(*APIServer).Stop
because the kubebuilder envtest control-plane binaries (etcd +
kube-apiserver + kubectl) were not on PATH. PRs #330, #341, and #362 all
called this out as a pre-existing failure they could not address inline.

Three-pronged fix:

1. Makefile gains a `setup-envtest` target that installs
   `sigs.k8s.io/controller-runtime/tools/setup-envtest@latest` and
   downloads the v1.31 control-plane bundle. A companion
   `setup-envtest-env` target prints the eval-friendly export line so
   developers can do `eval $(make -s setup-envtest-env)`.

2. .github/workflows/go-ci.yml installs setup-envtest and exports
   `KUBEBUILDER_ASSETS` via `$GITHUB_ENV` before `go test ./...`, so the
   operator suite runs for real in CI instead of skipping.

3. cmd/vmafx-operator/internal/controller/suite_test.go gains a
   top-of-`TestControllers` `t.Skip()` guard when `KUBEBUILDER_ASSETS`
   is unset, plus a nil-`testEnv` bailout in `AfterSuite` so the suite
   never panics on a fresh checkout where the assets are missing
   (defense in depth).

Local verification:
- `unset KUBEBUILDER_ASSETS && go test ./cmd/vmafx-operator/...` -> SKIP
  with an actionable message pointing at `make setup-envtest`.
- `eval $(make -s setup-envtest-env) && go test -v ./cmd/vmafx-operator/...`
  -> 3/3 specs pass in ~5 s.

AGENTS.md gains a new invariant #6 documenting the skip-safe envtest
pattern; rebase-notes.md, state.md, and a changelog fragment under
changelog.d/fixed/ are updated per ADR-0108 + CLAUDE.md r10/r13.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Jun 2, 2026
@lusoris @claude
chore(ci): infra/CI bundle batch-1 (#370 coverage override + #396 env…
aca344d 
…test + #446 SYCL parity round 3) (#528)

* chore(ci): audit per-file coverage overrides — tighten tiny_extractor_template.h 10 → 75

Periodic audit of `scripts/ci/coverage-check.sh`'s `PER_FILE_MIN` map
(ADR-0114). Three findings:

1. `core/src/dnn/tiny_extractor_template.h` — actual 77.4 %, override
   10 % (67.4 pp slack). Original 10 % cap was set when only one
   extractor (`feature_lpips.c`) instantiated the inline helpers; four
   extractors now do (`feature_lpips`, `fastdvdnet_pre`,
   `feature_mobilesal`, `feature_transnet_v2`). Tighten to 75 (2.4 pp
   slack, mirroring the 1.3-1.7 pp slack ADR-0114 used for the at-cap
   entries). Locks 65 pp of de-facto regression-coverage.

2. `core/src/dnn/ort_backend.c` — actual 77.8 %, override 78 % (at cap,
   currently failing on master). Keep at 78 — PR #338 in flight adds
   `vmaf_ort_output_name_at` unit test, lifting actual to 78.5 %
   without raising the bar (correct pattern per ADR-0114).

3. `core/src/dnn/dnn_api.c` — actual 78.0 %, override 78 % (at cap).
   Keep at 78 — structural ceiling rationale per ADR-0114 §Context
   unchanged.

No new override entries required. All other dnn/ files plus opt.c and
read_json_model.c clear the global 85 % critical floor
(dnn_attach_api.c 92 %, model_loader.c 87 %, onnx_scan.c 93 %,
op_allowlist.c 100 %, tensor_io.c 98 %, opt.c 100 %,
read_json_model.c 88 %).

ADR-0881 also codifies the recurring audit rule (tighten when slack
> 5 pp; keep at-cap; remove when actual ≥ global 85 % floor; audit
quarterly + before any PER_FILE_MIN edit). Audit procedure documented
in the companion research digest.

Reproducer:
  PKG_CONFIG_PATH=/path/to/onnxruntime/lib/pkgconfig \
    meson setup core/build-coverage core --buildtype=debug \
      -Db_coverage=true -Denable_cuda=false -Denable_sycl=false \
      -Denable_float=true -Denable_avx512=true -Denable_dnn=enabled \
      -Dc_args=-fprofile-update=atomic -Dcpp_args=-fprofile-update=atomic
  ninja -C core/build-coverage
  LD_LIBRARY_PATH=/path/to/onnxruntime/lib \
    meson test -C core/build-coverage --num-processes 1
  cd core && gcovr --root .. --filter 'src/.*' \
    --exclude '.*/test/.*' --exclude '.*/tests/.*' \
    --json-summary build-coverage/coverage.json build-coverage
  bash scripts/ci/coverage-check.sh core/build-coverage/coverage.json 37 85
  # Pre-audit: tiny_extractor_template.h reports "min 10%"
  # Post-audit: same file reports "min 75%" — gate enforces the new floor

Closes the ADR-0114 implicit follow-up "re-audit slack overrides on a
cadence so they don't rot".

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(ci,operator): install kubebuilder envtest binaries for operator suite

The cmd/vmafx-operator/internal/controller envtest suite was hard-failing
in BeforeSuite with a nil-pointer deref from controlplane.(*APIServer).Stop
because the kubebuilder envtest control-plane binaries (etcd +
kube-apiserver + kubectl) were not on PATH. PRs #330, #341, and #362 all
called this out as a pre-existing failure they could not address inline.

Three-pronged fix:

1. Makefile gains a `setup-envtest` target that installs
   `sigs.k8s.io/controller-runtime/tools/setup-envtest@latest` and
   downloads the v1.31 control-plane bundle. A companion
   `setup-envtest-env` target prints the eval-friendly export line so
   developers can do `eval $(make -s setup-envtest-env)`.

2. .github/workflows/go-ci.yml installs setup-envtest and exports
   `KUBEBUILDER_ASSETS` via `$GITHUB_ENV` before `go test ./...`, so the
   operator suite runs for real in CI instead of skipping.

3. cmd/vmafx-operator/internal/controller/suite_test.go gains a
   top-of-`TestControllers` `t.Skip()` guard when `KUBEBUILDER_ASSETS`
   is unset, plus a nil-`testEnv` bailout in `AfterSuite` so the suite
   never panics on a fresh checkout where the assets are missing
   (defense in depth).

Local verification:
- `unset KUBEBUILDER_ASSETS && go test ./cmd/vmafx-operator/...` -> SKIP
  with an actionable message pointing at `make setup-envtest`.
- `eval $(make -s setup-envtest-env) && go test -v ./cmd/vmafx-operator/...`
  -> 3/3 specs pass in ~5 s.

AGENTS.md gains a new invariant #6 documenting the skip-safe envtest
pattern; rebase-notes.md, state.md, and a changelog fragment under
changelog.d/fixed/ are updated per ADR-0108 + CLAUDE.md r10/r13.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* test(sycl): SYCL kernel parity coverage round 3 (5 new tests, ADR-0946)

Adds five new CPU-vs-SYCL parity tests under core/test/, extending
the SYCL parity coverage beyond rounds 1 (PR #351: integer psnr +
vif) and 2 (PR #376: integer adm / ciede / ssim / ms_ssim /
motion_v2).

New parity tests at ADR-0214 places=4 (1e-4) tolerance:

| Kernel              | New test                              | Headline score                            |
|---------------------|---------------------------------------|-------------------------------------------|
| float_psnr_sycl     | test_sycl_float_psnr_parity.c         | float_psnr                                |
| float_adm_sycl      | test_sycl_float_adm_parity.c          | VMAF_feature_adm2_score                   |
| float_vif_sycl      | test_sycl_float_vif_parity.c          | VMAF_feature_vif_scale0_score             |
| float_motion_sycl   | test_sycl_float_motion_parity.c       | VMAF_feature_motion2_score (idx 1)        |
| psnr_hvs_sycl       | test_sycl_psnr_hvs_parity.c           | psnr_hvs                                  |

Each mirrors the round-1 / round-2 pattern: 256x144 synthetic
YUV420P fixture, CPU + SYCL feature extractor, parity assertion
within ADR-0214 places=4, skip-on-no-device via
"[skip: no SYCL device]" printf.

Coverage trajectory: 50% (rounds 1+2) -> 78% (this PR).
Round-4 backlog: float_moment, speed_chroma, speed_temporal,
ssimulacra2 (need scaffold extensions for per-extractor config
dicts and ref_pic_90 fill before the same gate can be added).

Container compile-check evidence:

    docker exec vmaf-dev-mcp bash -lc '
      source /opt/intel/oneapi/setvars.sh --force >/dev/null 2>&1 && \
      cd /tmp/wt-sycl-r3 && \
      CC=icx CXX=icpx meson setup build-sycl-r3 core \
          -Denable_sycl=true -Denable_avx512=false -Db_lto=false && \
      ninja -C build-sycl-r3 \
          test/test_sycl_float_psnr_parity \
          test/test_sycl_float_adm_parity \
          test/test_sycl_float_vif_parity \
          test/test_sycl_float_motion_parity \
          test/test_sycl_psnr_hvs_parity'

All five executables compile + link clean with -Wall -Wextra under
icx/icpx. Registration sub-tests pass on this host; parity
sub-tests hit a pre-existing level_zero device-passthrough issue
(same as PR #376, not introduced by this PR).

Refs ADR-0946, ADR-0214, ADR-0868, ADR-0884.

* test(sycl): SYCL kernel parity coverage round 4 (4 new tests, ADR-0957) (#465)

Closes the SYCL kernel-coverage backlog enumerated in ADR-0946 by
adding four new CPU vs. SYCL parity tests under core/test/.

| Kernel              | New test                              | Tolerance              |
|---------------------|---------------------------------------|------------------------|
| float_moment_sycl   | test_sycl_float_moment_parity.c       | 1e-4 (ADR-0214 default)|
| speed_chroma_sycl   | test_sycl_speed_chroma_parity.c       | 1e-4 (ADR-0214 default)|
| speed_temporal_sycl | test_sycl_speed_temporal_parity.c     | 1e-4 (ADR-0214 default)|
| ssimulacra2_sycl    | test_sycl_ssimulacra2_parity.c        | 5e-3 (FEATURE_TOLERANCE)|

Each test mirrors the round-3 scaffold: 256x144 synthetic YUV420P
fixture, public vmaf_use_feature API with NULL options dict
(defaults match between CPU and SYCL for all four kernels),
parity assertion via fabs(cpu - sycl) <= TOL, skip-on-no-device
via "[skip: no SYCL device]" printf.

The SSIMULACRA2 fixture fills all three planes (the pipeline
consumes YUV -> linear-RGB -> XYB and chroma matters for the
headline score). The speed_temporal fixture submits two frames
(TEMPORAL flag means frame 0 emits 0.0) and asserts at index 1.

Discovery during round-4 implementation:
speed_chroma_sycl.cpp (752 LOC) and speed_temporal_sycl.cpp
(705 LOC) source files exist on disk but are NOT wired into
sycl_feature_sources in core/src/meson.build and their extractor
symbols are NOT declared/registered in
core/src/feature/feature_extractor.c. Both files appear complete
(no TODO/FIXME/-ENOSYS/stub markers) but ship as dormant
scaffold. Wiring them in is out of scope for a kernel-coverage
PR (it changes the production extractor surface). The two SpEED
parity tests are added in dormant form with a
"[skip: <name> not built into libvmaf]" guard that
auto-activates as a real parity gate the day a follow-up PR
wires the TUs into the build + registry.

Container compile-check evidence (CC=icx CXX=icpx,
enable_sycl=true, Intel Arc A380 visible):
- All four test executables link against libvmaf.a successfully.
- speed_chroma/temporal: pass (skip — dormant scaffold).
- float_moment / ssimulacra2: register-existence sub-test passes;
  parity sub-test hits the same pre-existing level_zero
  device-passthrough SIGSEGV that PR #446's round-3 tests hit on
  this dev container (not introduced by this PR). On a host with
  proper Intel-GPU passthrough or with no SYCL device, the tests
  pass / skip cleanly.

Coverage trajectory:
- Round 0 (pre-rounds): 2/18 (11%)
- Round 1 (#351): +2 = 4/18 (22%)
- Round 2 (#376): +5 = 9/18 (50%)
- Round 3 (#446): +5 = 14/18 (78%)
- Round 4 (this PR): +4 = 18/18 (100% of round-3 backlog;
  16 active + 2 dormant SpEED scaffolds)

Refs ADR-0957, ADR-0214, ADR-0867, ADR-0884, ADR-0946.

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* chore(ci): add changelog fragment for infra/CI bundle batch-1

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
@lusoris lusoris added this to the 1.0.0 — First release milestone Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.0.0 — First release

Development

Successfully merging this pull request may close these issues.

1 participant

@lusoris