test: address 17 Copilot-flagged test-rigor concerns from #548 (#552)

Closes #549

Follow-up to #548 (coverage 24.4% → 82.5%). Copilot flagged 17
test-quality
concerns that merged alongside the coverage uplift; this PR addresses
all of
them as a focused test-rigor pass. No production code behaviour changes.

## Per-concern status

### Goroutine lifecycle (7 threads) — fixed
- `newAppForCoverage` now calls `registerAppShutdown(t, app)` which
installs
  `t.Cleanup(app.Shutdown)`, shutting down `periodicCacheLogging`,
template-cache cleanup, and rate-limiter cleanup goroutines. Every test
  built on `newAppForCoverage` (`TestNewApp_*`, `TestApp_*`,
`TestUserModifyHandler_AuthenticatedPaths`, `TestGroupModifyHandler_*`,
  `TestModifyHandlers_DirectNoCSRF`, `TestCSRFProtectedModifyHandlers`,
  `TestAuthenticatedGETHandlers`) now tears down cleanly. The cleanup is
defensive: if a test calls Shutdown explicitly first, the cleanup
recovers
  from the `close(stopCacheLog)` panic on second call.
- `TestNewApp_WithPersistSessions` keeps its explicit Shutdown because
it
  asserts on the return value; it does not also register the cleanup.

### port 65535 flaky assumption — fixed
`TestRunHealthCheck/returns 1 when no server is listening` now uses
`net.Listen("tcp", "127.0.0.1:0")` to acquire a known-free ephemeral
port,
closes it, and reuses the port number.

### `testTemplateRenderError` sad-path assertion — fixed
`TestRender_WithCancelledContext` (the cancelled-context render test)
now
asserts `err != nil` and `errors.Is(err, context.Canceled)` instead of
only
`t.Logf`-ing on mismatch. A silent nil return from Render with a
cancelled
context is now a fail.

### Comment/behaviour mismatch (no-op middleware) — fixed
Removed the `bare.Use(func(c *fiber.Ctx) error { /* attach cookies */
return c.Next() })`
block in `TestModifyHandlers_DirectNoCSRF`. Cookies are attached
per-request
via `postTo`; no middleware is needed.

### No-assertion test variants — fixed
`Users(nil, ...)`, `Groups(nil)`, and `Computers(nil)` render blocks in
`render_test.go` now use `require.NoError` +
`require.NotZero(buf.Len())` so
the empty-state branch is actually verified.

### `-coverpkg` scope too wide — fixed
The reusable `netresearch/.github/.github/workflows/go-check.yml` has no
hook
between `go test` and threshold enforcement, so we cannot post-filter
`coverage.out` via inputs. Fix:
- Call the reusable workflow with `enable-build-test: false` and
  `enable-codecov: false`, keeping smoke/fuzz/license/security jobs.
- Add an inline `build-test-coverage` job that expands `-coverpkg` to
include
  `internal/web/templates/...`, filters `*_templ.go` lines out of
`coverage.raw.out` → `coverage.out`, then enforces the 80% threshold and
  uploads to Codecov.

Authored helpers in `internal/web/templates` (`flash.go` Flashes /
SuccessFlash / ErrorFlash / InfoFlash / IsSuccess / IsError / IsInfo /
BorderColor, `specializeUsers`/`specializeGroups`/`specializeComputers`,
`formatLastLogon`, `getNavbarClasses`) now count toward the 80% floor.
The
generated `*_templ.go` files do not. `.github/template.yaml` drift note
updated.

### `StatusCode == 0` always-true assertions — fixed
Replaced the nine `StatusCode == 0` / `NotEqual(0, StatusCode)` checks
across
`server_coverage_test.go`, `handlers_authenticated_test.go`,
`modify_handlers_test.go`, `server_test.go`, and
`ldap_integration_test.go`
with explicit expected-code lists (200/302/403/404/500/503 as
appropriate).

`TestApp_RoutesRegistered` now uses a `wantCodes []int` slice per route
so
each is validated against the code(s) the registered handler actually
emits
(302 for protected routes without session, 200 for `/login`, 200-or-503
for
`/health*`).

### Misleading `cancel()` in ListenCancelled — fixed
Renamed `TestApp_ListenCancelled` → `TestApp_ListenGracefulShutdown`.
The
test no longer passes a cancellable context (Listen forwards to
`fiber.Listen` and doesn't observe ctx), and now asserts Listen returns
within 2s of `Shutdown()`.

### Stale file-header comment — fixed
`login_handler_test.go` header previously advertised "successful login
with
regenerated session"; no such case exists. Replaced with a header that
matches actual contents and notes the success case is in the LDAP
integration suite.

## Verification

- `go test -race -timeout 120s ./...` passes all packages.
- `golangci-lint run --timeout 3m ./...` is clean (0 issues).
- Filtered coverage: **82.6%** (up from 82.5% baseline — now includes
  `flash.go` authored helpers at 100%).

## Test plan

- [x] All existing tests pass under `-race`
- [x] `golangci-lint` reports 0 issues
- [x] Filtered coverage >= 80% threshold
- [x] `flash.go` helpers visible in coverage output
- [x] `TestApp_RoutesRegistered` asserts specific status codes per route
- [x] `TestRender_WithCancelledContext` fails if Render returns nil
error
- [x] `TestRunHealthCheck` no-server case uses ephemeral port

Author: CybotTM

.github/template.yaml

@@ -5,7 +5,12 @@ template: go-app
 intentional-drift:
 - path: .github/workflows/ci.yml
   reason: 'TypeScript/templ frontend assets must be built via bun before go test;
-    requires setup-bun + pre-build-cmd at caller level. Custom test-flags pass
-    -coverpkg to exclude the auto-generated internal/web/templates package from
-    the coverage calculation (its *_templ.go files contain framework-internal
-    defer/error branches unreachable from unit tests).'
+    requires setup-bun + pre-build-cmd at caller level. The reusable go-check
+    workflow has no hook between "go test" and coverage threshold enforcement,
+    so we disable its build-test job and run an inline build-test-coverage job
+    that expands -coverpkg to include internal/web/templates/... and
+    post-filters generated *_templ.go lines from coverage.out before enforcing
+    the 80% threshold. This keeps authored helpers in the templates package
+    (flash.go, specializeUsers/Groups/Computers, formatLastLogon) in the
+    numerator while excluding the framework-internal defer/error branches of
+    generated templ code.'

.github/workflows/ci.yml

@@ -15,23 +15,100 @@ permissions:
   contents: read
 
 jobs:
+  # Primary quality pipeline. We disable the reusable workflow's
+  # build-test job (including coverage threshold enforcement) because
+  # the reusable workflow has no hook between "go test" and threshold
+  # enforcement, and we need to post-filter *_templ.go lines from
+  # coverage.out. The build-test-coverage job below replaces it.
   go-check:
     uses: netresearch/.github/.github/workflows/go-check.yml@main
     with:
+      enable-build-test: false
       enable-smoke-fast-feedback: true
       enable-fuzz: true
       enable-license-check: true
-      enable-codecov: true
-      # Exclude the auto-generated internal/web/templates package from the
-      # coverage calculation: the *_templ.go files are produced by the templ
-      # CLI and contain many framework-internal defer/error branches that
-      # cannot be reached from unit tests. Authored-code coverage is the
-      # metric that matters for the fleet-wide 80% floor.
-      test-flags: "-race -covermode=atomic -coverprofile=coverage.out -coverpkg=github.com/netresearch/ldap-manager/cmd/...,github.com/netresearch/ldap-manager/internal/ldap_cache/...,github.com/netresearch/ldap-manager/internal/options/...,github.com/netresearch/ldap-manager/internal/retry/...,github.com/netresearch/ldap-manager/internal/version/...,github.com/netresearch/ldap-manager/internal/web"
+      enable-codecov: false
       setup-bun: true
       pre-build-cmd: "go install github.com/a-h/templ/cmd/templ@latest && bun install --frozen-lockfile && bun run build:assets"
     permissions:
       contents: read
       security-events: write
     secrets:
       CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
+
+  # Replacement for the reusable workflow's build-test + coverage
+  # enforcement, with post-filtering of generated *_templ.go lines so
+  # authored helpers in internal/web/templates (flash.go,
+  # specializeUsers/Groups/Computers, formatLastLogon, etc.) count
+  # toward the fleet-wide 80% coverage floor, while framework-internal
+  # defer/error branches in the generated templ code do not.
+  build-test-coverage:
+    name: Build & Test (with templ-filtered coverage)
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+    permissions:
+      contents: read
+    env:
+      COVERAGE_THRESHOLD: "80"
+    steps:
+      - name: Checkout
+        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
+
+      - name: Setup Go
+        uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0
+        with:
+          go-version-file: go.mod
+          cache: true
+
+      - name: Setup Bun
+        uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0
+        with:
+          bun-version: latest
+
+      - name: Pre-build (templ + frontend assets)
+        run: |
+          go install github.com/a-h/templ/cmd/templ@latest
+          bun install --frozen-lockfile
+          bun run build:assets
+
+      - name: go vet
+        run: go vet ./...
+
+      - name: go build
+        run: go build -v ./...
+
+      - name: go test (with coverage)
+        run: |
+          go test -v -race -covermode=atomic -coverprofile=coverage.raw.out \
+            -coverpkg=github.com/netresearch/ldap-manager/cmd/...,github.com/netresearch/ldap-manager/internal/ldap_cache/...,github.com/netresearch/ldap-manager/internal/options/...,github.com/netresearch/ldap-manager/internal/retry/...,github.com/netresearch/ldap-manager/internal/version/...,github.com/netresearch/ldap-manager/internal/web/... \
+            ./...
+
+      - name: Filter generated *_templ.go lines from coverage
+        run: |
+          set -euo pipefail
+          # Generated *_templ.go files contain many framework-internal
+          # defer/error branches that unit tests cannot reasonably reach.
+          # Filter them out here so the coverage total reflects only
+          # authored code, including hand-written helpers inside the
+          # internal/web/templates package.
+          grep -v '_templ\.go:' coverage.raw.out > coverage.out
+          echo "Before filter: $(wc -l <coverage.raw.out) lines"
+          echo "After filter:  $(wc -l <coverage.out) lines"
+
+      - name: Enforce coverage threshold
+        env:
+          THRESHOLD: ${{ env.COVERAGE_THRESHOLD }}
+        run: |
+          set -euo pipefail
+          TOTAL=$(go tool cover -func=coverage.out | awk '/^total:/ {gsub("%","",$3); print $3}')
+          echo "Total coverage (authored code only): ${TOTAL}% (threshold ${THRESHOLD}%)"
+          awk -v t="$THRESHOLD" -v a="$TOTAL" \
+            'BEGIN { if (a+0 < t+0) { printf "::error::Coverage %s%% is below threshold %s%%\n", a, t; exit 1 } }'
+
+      - name: Upload coverage to Codecov
+        uses: codecov/codecov-action@57e3a136b779b570ffcdbf80b3bdc90e7fab3de2 # v6.0.0
+        with:
+          token: ${{ secrets.CODECOV_TOKEN }}
+          files: coverage.out
+          flags: unittests
+          fail_ci_if_error: false

cmd/ldap-manager/main_test.go

@@ -3,9 +3,11 @@ package main
 // Tests for main.go's CLI helpers.
 
 import (
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strconv"
 	"strings"
 	"testing"
 )
@@ -51,8 +53,18 @@ func TestRunHealthCheck(t *testing.T) {
 	})
 
 	t.Run("returns 1 when no server is listening", func(t *testing.T) {
-		// Pick an unlikely high port that almost certainly has no server bound.
-		got := runHealthCheck("65535")
+		// Acquire a known-free ephemeral port from the OS, close it, and reuse
+		// the number. This avoids the flaky assumption that a hard-coded port
+		// (e.g. 65535) is available on shared CI runners.
+		l, err := net.Listen("tcp", "127.0.0.1:0")
+		if err != nil {
+			t.Fatalf("listen: %v", err)
+		}
+
+		port := strconv.Itoa(l.Addr().(*net.TCPAddr).Port)
+		_ = l.Close()
+
+		got := runHealthCheck(port)
 		if got != 1 {
 			t.Errorf("expected 1 when no server listening, got %d", got)
 		}

internal/web/handlers_authenticated_test.go

@@ -115,8 +115,8 @@ func TestUserModifyHandler_AuthenticatedPaths(t *testing.T) {
 
 		// Without CSRF token → 403. With invalid session/LDAP → 302 (redirect
 		// to /login). Either code path is exercised.
-		if resp.StatusCode == 0 {
-			t.Error("got zero status code")
+		if resp.StatusCode != http.StatusForbidden && resp.StatusCode != http.StatusFound {
+			t.Errorf("expected 302 or 403, got %d", resp.StatusCode)
 		}
 	})
 }
@@ -145,8 +145,9 @@ func TestGroupModifyHandler_AuthenticatedPaths(t *testing.T) {
 		}
 		defer func() { _ = resp.Body.Close() }()
 
-		if resp.StatusCode == 0 {
-			t.Error("got zero status code")
+		// CSRF rejection (403) or redirect to detail page / login (302).
+		if resp.StatusCode != http.StatusForbidden && resp.StatusCode != http.StatusFound {
+			t.Errorf("expected 302 or 403, got %d", resp.StatusCode)
 		}
 	})
 
@@ -166,8 +167,8 @@ func TestGroupModifyHandler_AuthenticatedPaths(t *testing.T) {
 		}
 		defer func() { _ = resp.Body.Close() }()
 
-		if resp.StatusCode == 0 {
-			t.Error("got zero status code")
+		if resp.StatusCode != http.StatusForbidden && resp.StatusCode != http.StatusFound {
+			t.Errorf("expected 302 or 403, got %d", resp.StatusCode)
 		}
 	})
 
@@ -187,8 +188,8 @@ func TestGroupModifyHandler_AuthenticatedPaths(t *testing.T) {
 		}
 		defer func() { _ = resp.Body.Close() }()
 
-		if resp.StatusCode == 0 {
-			t.Error("got zero status code")
+		if resp.StatusCode != http.StatusForbidden && resp.StatusCode != http.StatusFound {
+			t.Errorf("expected 302 or 403, got %d", resp.StatusCode)
 		}
 	})
 }
@@ -255,12 +256,9 @@ func TestModifyHandlers_DirectNoCSRF(t *testing.T) {
 	cookies := simulatedSession(t, app)
 
 	// Mount modify handlers behind only RequireAuth (no CSRF) on a bare app
-	// so session-based auth works but CSRF does not block.
+	// so session-based auth works but CSRF does not block. Cookies are attached
+	// per-request below (see postTo); no middleware is needed for that.
 	bare := fiber.New()
-	bare.Use(func(c *fiber.Ctx) error {
-		// Attach the simulated session cookies.
-		return c.Next()
-	})
 	bare.Post("/users/*", app.RequireAuth(), app.userModifyHandler)
 	bare.Post("/groups/*", app.RequireAuth(), app.groupModifyHandler)
 
@@ -423,8 +421,8 @@ func TestAuthenticatedGETHandlers(t *testing.T) {
 
 			// Post-auth, the LDAP call fails → fiber.StatusUnauthorized
 			// → handle500 → /login redirect. 302 is the expected outcome.
-			if resp.StatusCode == 0 {
-				t.Error("zero status")
+			if resp.StatusCode != http.StatusFound {
+				t.Errorf("expected 302 redirect after LDAP failure, got %d", resp.StatusCode)
 			}
 		})
 	}

internal/web/ldap_integration_test.go

@@ -368,8 +368,9 @@ func TestLDAPIntegration_UsersHandler(t *testing.T) {
 		resp := makeLDAPAuthRequest(t, app, "/users", cookies)
 		defer func() { _ = resp.Body.Close() }()
 
-		// Handler connects to real LDAP — success or error page
-		assert.NotEqual(t, 0, resp.StatusCode)
+		// Handler connects to real LDAP — 200 on success, 302 on redirect
+		// to /login if session LDAP bind fails.
+		assert.Contains(t, []int{http.StatusOK, http.StatusFound}, resp.StatusCode)
 		if resp.StatusCode == http.StatusOK {
 			body, _ := io.ReadAll(resp.Body)
 			assert.Contains(t, resp.Header.Get("Content-Type"), "text/html")
@@ -381,7 +382,7 @@ func TestLDAPIntegration_UsersHandler(t *testing.T) {
 		resp := makeLDAPAuthRequest(t, app, "/users?show-disabled=1", cookies)
 		defer func() { _ = resp.Body.Close() }()
 
-		assert.NotEqual(t, 0, resp.StatusCode)
+		assert.Contains(t, []int{http.StatusOK, http.StatusFound}, resp.StatusCode)
 	})
 }
 

internal/web/login_handler_test.go

@@ -1,7 +1,9 @@
 package web
 
-// Tests for loginHandler POST paths: invalid credentials, rate-limit block,
-// successful login with regenerated session.
+// Tests for loginHandler POST paths: invalid credentials, empty fields,
+// rate-limit block, and logoutHandler session destruction. A live-bind
+// "success" case is not present here because it requires a real LDAP server;
+// that path is covered by the LDAP integration test suite.
 
 import (
 	"context"

internal/web/modify_handlers_test.go

@@ -188,8 +188,11 @@ func TestGroupModifyHandler_DeeperPaths(t *testing.T) {
 		}
 		defer func() { _ = resp.Body.Close() }()
 
-		if resp.StatusCode == 0 {
-			t.Error("zero status")
+		// On example-server modify failure, handle500 renders the 500 page
+		// (still exercising the target branch). On success it would render
+		// the group detail page (200) or redirect (302).
+		if !intInSlice(resp.StatusCode, []int{http.StatusOK, http.StatusFound, http.StatusInternalServerError}) {
+			t.Errorf("expected 200, 302, or 500, got %d", resp.StatusCode)
 		}
 	})
 
@@ -209,8 +212,8 @@ func TestGroupModifyHandler_DeeperPaths(t *testing.T) {
 		}
 		defer func() { _ = resp.Body.Close() }()
 
-		if resp.StatusCode == 0 {
-			t.Error("zero status")
+		if !intInSlice(resp.StatusCode, []int{http.StatusOK, http.StatusFound, http.StatusInternalServerError}) {
+			t.Errorf("expected 200, 302, or 500, got %d", resp.StatusCode)
 		}
 	})
 }
@@ -251,8 +254,11 @@ func TestListHandlers_ExampleServer(t *testing.T) {
 			}
 			defer func() { _ = resp.Body.Close() }()
 
-			if resp.StatusCode == 0 {
-				t.Error("zero status")
+			// Example-server mocked responses render a page (200), redirect to
+			// /login on auth failure (302), or render the 500 page on a
+			// downstream LDAP modify failure. Never 0.
+			if !intInSlice(resp.StatusCode, []int{http.StatusOK, http.StatusFound, http.StatusInternalServerError}) {
+				t.Errorf("expected 200, 302, or 500, got %d", resp.StatusCode)
 			}
 		})
 	}