Skip to content

fix(build): honest quality gates — propagate exit codes, fix hidden lint/race findings#1723

Open
krisarmstrong wants to merge 8 commits into
mainfrom
fix/honest-quality-gates
Open

fix(build): honest quality gates — propagate exit codes, fix hidden lint/race findings#1723
krisarmstrong wants to merge 8 commits into
mainfrom
fix/honest-quality-gates

Conversation

@krisarmstrong

@krisarmstrong krisarmstrong commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Make the quality gates honest and fix what they were hiding. All nine -quiet wrappers in mk/lint.mk / mk/test.mk / mk/security.mk captured output through | grep ... || true, so make lint, make test, and make security exited 0 regardless of real failures. This PR propagates real exit codes (keeping the quiet summaries), then fixes the findings that were hiding behind the green: 3 golangci findings fixed structurally (NewServer decomposed into initXxx helpers; catalog.Store segregated into ProfileRepository + ActiveProfileStore along the real adapter seam, all callers migrated, no compat alias; unparam probe ID varied), and the flaky TestStartContinuousCallback root-caused to a real race — StopContinuous returned while the goroutine could still be mid-callback — fixed with a sync.WaitGroup (waited outside the mutex to avoid deadlock). Also: mk/vars.mk is now actually included (duplicated root-Makefile blocks deleted), CI govulncheck/npm-audit steps delegate to the make targets, golangci v2.12.1→v2.12.2, Node 26.2.0→26.3.0 (setup-node composite + license-check.yml + dead-code.yml), and the missing todo label added to labels.yml (todo-tracker.yml has failed every weekly run since 2026-06-22). CI build steps intentionally stay raw: CI owns multi-arch builds; make is local-only.

Linked Issue

Fixes #1724

Type of Change

  • Defect fix
  • Feature
  • Chore / refactor / dependency update
  • Documentation
  • CI / release / packaging
  • Security hardening

Risk

  • Medium

Testing Evidence

Injected-failure proof (wrapper honesty):
  temp Go file with `declared and not used` -> make lint-backend-quiet:
  make: *** [lint-backend-quiet] Error 1   (was exit 0 before the fix)
  injection removed before commit.

Flaky-test proof:
  go test -race -run TestStartContinuousCallback -count=30 ./internal/diagnostics/gateway/  -> ok
  3x fresh full-package -race runs -> ok

Final gates (honest wrappers, all exit 0):
  make lint       -> golangci-lint 0 issues; Biome clean
  make fmt-check  -> clean
  make test       -> full backend -race, coverage 54.6%; 26 Vitest files pass
  make security   -> govulncheck clean; npm audit 0 vulnerabilities; gitleaks no leaks

Security and Release Checklist

  • No secrets, tokens, credentials, or customer data are included.
  • Mutating routes, auth surfaces, permission checks, and output encoding were reviewed if touched.
  • Dependencies are pinned and justified if changed.
  • Documentation, screenshots, or operator notes were updated if behavior changed.

Kris Armstrong added 8 commits July 5, 2026 21:44
fix(build): propagate exit codes through quiet gate wrappers
1f6005f 
The -quiet wrappers in mk/lint.mk, mk/test.mk, and mk/security.mk piped
tool output through head/grep/tail with a trailing '|| true' (or
'|| printf'), so the recipe's exit status came from the filter, never
from the tool. make lint / make test / make security reported green
regardless of lint errors, test failures, or audit findings.

Each wrapper now captures the tool's output and real exit status
(OUT=$(cmd); STATUS=$?), prints the same quiet/pretty summary, dumps
the tail of the full output on failure for diagnosis, and exits with
the tool's status. Verified by injecting a Go compile/lint error:
make lint-backend-quiet now exits 2 instead of 0.

Also fixed: fix-backend-quiet, fix-frontend-quiet, lint-frontend-quiet,
test-frontend-quiet, security-secrets-quiet (same masking pattern).
refactor(api): extract NewServer init helpers (funlen 51>50)
3e69ad3 
NewServer exceeded the funlen statement limit. Extract three cohesive
helpers following the existing initXxx convention in this file:

- initAuthSecurity: auth/CSRF/setup-token/recovery/WebAuthn managers +
  login/endpoint rate limiters
- initTelemetryAndWiFiServices: DNS/gateway/VLAN/speedtest/iperf/cable/
  public-IP testers + Wi-Fi manager/scanner (no cross-dependencies)
- initSettingsUseCases: ADR-0020 settings/profiles/network-IP/alert-rule
  use-case wiring

NewServer now reads as a composition sequence of named subsystems.
refactor(profiles): segregate catalog Store into two ports (interface…
5be09ee 
…bloat 11>10)

catalog.Store had 11 methods spanning two distinct persistence seams:
profile records (backed by the database Profiles repository) and the
active-profile pointer (backed by the Settings repository). Split along
that real adapter seam into ProfileRepository (9 methods) and
ActiveProfileStore (2 methods).

NewService now takes both ports; internal/app.profilesStore satisfies
both and is passed twice at the composition root. All callers (app
adapter + catalog tests) migrated in this change — no compat alias
(pre-alpha, no legacy burden).
fix(test): vary probe ID in anomaly producer test (unparam)
0aeffc4 
latencyEvent's probeID parameter always received "p1", so unparam
flagged it. TestObservePersistsThroughCoordinator now uses a distinct
non-trivial probe ID, which also proves the record-ID templating is not
accidentally hardcoded to the shared fixture value.
fix(gateway): make StopContinuous wait for the ping goroutine; deflak…
206749a 
…e callback test

TestStartContinuousCallback raced under full-suite -race contention:
StopContinuous only signaled the background goroutine via close(stopCh)
and returned immediately, so the goroutine could still be mid-callback
(writing callCount/lastStats) while the test read them — a genuine data
race in the API's stop semantics, not just the test.

Fix the code: Tester now tracks the StartContinuous goroutine with a
sync.WaitGroup and StopContinuous blocks (outside t.mu, to avoid
deadlocking the goroutine's own Test() locking) until it has fully
returned. Post-Stop reads are then ordered by the WaitGroup.

Fix the test: guard callback state with a mutex, poll for the first
callback with a generous 5s deadline instead of a fixed 200ms sleep,
and assert the callback actually fired instead of logging and passing.

Verified: go test -race -run TestStartContinuousCallback -count=30 and
3x full-package -race runs all pass.
refactor(build): include mk/vars.mk as the single variable source
c9838f1 
mk/vars.mk describes itself as the single source of truth for version,
platform, and color variables, but nothing included it — the root
Makefile duplicated the VERSION/COMMIT/BUILD_TIME, platform/arch
detection, ANSI color, and VERSION_PKG blocks inline.

Include mk/vars.mk first and delete the duplicated blocks. Move
PLATFORM_PRETTY (used by 'make version') into vars.mk so platform
detection lives in one place. Also fix the stale header comment:
Go 1.25.5+ -> 1.26.4+ (Node line was already 26.3.0).

Verified: make version and make build still work.
ci: call make security targets; bump golangci-lint v2.12.2 + Node 26.…
4f17376 
…3.0 pins

CI-calls-make where behavior-equivalent:
- govulncheck step -> make security-backend-quiet (same 'govulncheck
  ./...' invocation, now with honest exit propagation)
- npm audit step -> make security-frontend-quiet after npm ci (same
  'npm audit --audit-level=high')

Not converted (documented): the backend test step keeps its explicit
package list + SKIP_* envs and feeds coverage.out to the threshold
check; the deterministic/CGO-free build checks intentionally compile
'./...' without ldflags; the release-shaped builds already inject the
Makefile-equivalent ldflags inline; golangci-lint stays on its action
for caching/annotations.

Version pins brought to the fleet standard: golangci-lint v2.12.1 ->
v2.12.2 (ci.yml), Node 26.2.0 -> 26.3.0 (setup-node composite default,
dead-code.yml, license-check.yml, ci.yml header comment).
ci: fix todo-tracker labels to match the label taxonomy
6657919 
The weekly TODO summary issue was created with labels 'todo' and
'needs-triage'; neither existed in .github/labels.yml or the repo
('status: needs-triage' is the taxonomy name), so gh issue create
would fail. Use 'status: needs-triage' and add a 'todo' label
definition to labels.yml.
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

📋 License Compliance Report

✅ All dependencies pass license compliance checks

Go Dependencies

  • Unknown: 118 package(s)
  • MIT: 30 package(s)
  • BSD-3-Clause: 17 package(s)
  • Apache-2.0: 12 package(s)
  • BSD-2-Clause: 2 package(s)

npm Dependencies

See full report in workflow artifacts

Allowed Licenses: MIT, Apache-2.0, BSD-*, ISC, CC0-1.0, MPL-2.0
Forbidden: GPL, AGPL, SSPL (strong copyleft)

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

area: build Build area. area: ci Ci area. area: tests Tests area. component: api

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Quality gates swallow exit codes: make lint/test/security always report success

1 participant

@krisarmstrong