From 33a6b23cde80ccfaca211900a2a96e1a2093dcf5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 12:25:10 +0000 Subject: [PATCH] fix: resolve golangci-lint failures and correct audit-doc inaccuracies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pre-merge review caught things my local verification missed, because two CI gates could not run in the sandbox and I mis-read a third. golangci-lint (BLOCKER — would have turned CI red on both server matrix legs). My local binary was built for Go 1.25 against a repo targeting 1.26, so I could not run it. Installed 2.11.3 with the repo's own toolchain: 5 issues, all in files this PR adds, base clean. Now 0 issues: - bodyclose x3 in api/livekit_proxy_ws_test.go — websocket.Dial's *http.Response was discarded; adopt the repo's existing pattern from ws/ws_integration_test.go - gocritic stringXbytes — string(got) != string(payload) -> !bytes.Equal - staticcheck SA4000 in ws/topic_rate_limiter_test.go — `!Allow() || !Allow()`. This was a real defect, not just a lint: || short-circuits, so a failing first call skipped the second, left a token unspent, and the next assertion would have reported the wrong thing. Split into two statements. Playwright: I reported this suite as passing. It does not. I read the exit code of `tail` through a pipeline instead of playwright's own. Re-run properly: 229 of 255 web tests fail, all cascading from the shared login helper (navigateToMainPage never sees [data-testid='app-layout']). It reproduces on a clean b3caceb worktree, so it is pre-existing on main and unrelated to this diff — but it was never true that I had verified it. Recorded as new finding T-2026-07-25-21 and promoted to backlog #2; the client-e2e job stays continue-on-error and now carries timeout-minutes so a red suite cannot burn unbounded Actions minutes. rust-tests gets a timeout too. Audit-doc corrections (all confirmed by re-measurement): - screenShare.ts was listed as "untouched by this pass" at 61.1% when this PR takes it to 100%; T-12's wording made it the exception when it is the best - IsEitherBlocked was NOT zero-coverage — 83.3% at base via message_test.go - excluded LOC 2,229 -> 1,827 - "40 Playwright spec files" -> 44 (33 web + 11 native), 255 web tests - HandleLiveKitHealthForTest callers: eight -> seven - admin coverage: 71.4% is with only the T-01 fix; 77.9% with this PR's tests Verified after the fixes: golangci-lint 0 issues, go vet, go test -race, go test -tags deadlock, vitest 94.87%, cargo test --lib 74/74, tsc, prettier. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AEETs3Vh6sAHHb1jMBL75g --- .github/workflows/ci.yml | 16 +++++++++++++--- Server/api/livekit_proxy_ws_test.go | 18 ++++++++++++++---- Server/ws/topic_rate_limiter_test.go | 11 +++++++++-- docs/audit-test-coverage-2026-07-25.md | 21 +++++++++++---------- 4 files changed, 47 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 28480224..44dd9219 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -178,6 +178,7 @@ jobs: rust-tests: name: Rust Unit Tests runs-on: ubuntu-22.04 + timeout-minutes: 30 defaults: run: working-directory: Client/tauri-client/src-tauri/ @@ -212,15 +213,24 @@ jobs: - name: Rust unit tests run: cargo test --lib - # Playwright e2e against the mocked-Tauri dev server. Non-blocking for now - # (backlog #10): the suite has never run in CI, so it gets a soak period - # before it becomes a gate. Remove continue-on-error to promote it. + # Playwright e2e against the mocked-Tauri dev server. Non-blocking, and it + # will very likely be RED at first: the suite has never run in CI, and a + # local run of the 255 web tests on `main` itself fails ~229 of them, all + # cascading from the shared login helper in tests/e2e/helpers.ts + # (navigateToMainPage never sees [data-testid='app-layout']). That breakage + # predates this PR — it reproduces on a clean 70caa6c worktree. + # + # The job is wired up anyway so the breakage is visible instead of invisible, + # but it MUST stay continue-on-error until the suite is repaired, and + # timeout-minutes caps the minutes it can burn while it is failing. + # See docs/audit-test-coverage-2026-07-25.md T-2026-07-25-21. # The native config (playwright.config.native.ts) is deliberately not wired # up — it needs a real server and a built desktop binary. client-e2e: name: Client E2E (Playwright, non-blocking) runs-on: ubuntu-latest continue-on-error: true + timeout-minutes: 25 defaults: run: working-directory: Client/tauri-client/ diff --git a/Server/api/livekit_proxy_ws_test.go b/Server/api/livekit_proxy_ws_test.go index eb24d3b0..696e0944 100644 --- a/Server/api/livekit_proxy_ws_test.go +++ b/Server/api/livekit_proxy_ws_test.go @@ -1,6 +1,7 @@ package api_test import ( + "bytes" "context" "encoding/json" "net/http" @@ -60,7 +61,10 @@ func TestLiveKitProxy_WebSocket_RoundTrip(t *testing.T) { ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) defer cancel() - conn, _, err := websocket.Dial(ctx, "ws://"+proxy.Listener.Addr().String()+"/rtc", nil) + conn, dialResp, err := websocket.Dial(ctx, "ws://"+proxy.Listener.Addr().String()+"/rtc", nil) + if dialResp != nil && dialResp.Body != nil { + defer dialResp.Body.Close() //nolint:errcheck // best-effort close in test + } if err != nil { t.Fatalf("dial through proxy: %v", err) } @@ -87,7 +91,10 @@ func TestLiveKitProxy_WebSocket_ForwardsBinary(t *testing.T) { ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) defer cancel() - conn, _, err := websocket.Dial(ctx, "ws://"+proxy.Listener.Addr().String()+"/rtc", nil) + conn, dialResp, err := websocket.Dial(ctx, "ws://"+proxy.Listener.Addr().String()+"/rtc", nil) + if dialResp != nil && dialResp.Body != nil { + defer dialResp.Body.Close() //nolint:errcheck // best-effort close in test + } if err != nil { t.Fatalf("dial through proxy: %v", err) } @@ -102,7 +109,7 @@ func TestLiveKitProxy_WebSocket_ForwardsBinary(t *testing.T) { if err != nil { t.Fatalf("read: %v", err) } - if typ != websocket.MessageBinary || string(got) != string(payload) { + if typ != websocket.MessageBinary || !bytes.Equal(got, payload) { t.Errorf("echo = (%v, %v), want (binary, %v)", typ, got, payload) } } @@ -126,7 +133,10 @@ func TestLiveKitProxy_WebSocket_PreservesQueryString(t *testing.T) { ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) defer cancel() - conn, _, err := websocket.Dial(ctx, "ws://"+proxy.Listener.Addr().String()+"/rtc?access_token=abc", nil) + conn, dialResp, err := websocket.Dial(ctx, "ws://"+proxy.Listener.Addr().String()+"/rtc?access_token=abc", nil) + if dialResp != nil && dialResp.Body != nil { + defer dialResp.Body.Close() //nolint:errcheck // best-effort close in test + } if err != nil { t.Fatalf("dial through proxy: %v", err) } diff --git a/Server/ws/topic_rate_limiter_test.go b/Server/ws/topic_rate_limiter_test.go index ea6b2241..bbe1b730 100644 --- a/Server/ws/topic_rate_limiter_test.go +++ b/Server/ws/topic_rate_limiter_test.go @@ -73,8 +73,15 @@ func TestTopicRateLimiter_Allow_EnforcesQuotaThenRefills(t *testing.T) { trl := NewTopicRateLimiter(2, 50*time.Millisecond) topic := Topic("channel:1") - if !trl.Allow(topic) || !trl.Allow(topic) { - t.Fatal("the first two messages were rejected despite a quota of 2") + // Kept as two statements rather than `a || b`: `||` short-circuits, so a + // failing first call would skip the second and leave a token unspent — + // the third Allow below would then be within quota and the test would + // report the wrong thing. + if !trl.Allow(topic) { + t.Fatal("the first message was rejected despite a quota of 2") + } + if !trl.Allow(topic) { + t.Fatal("the second message was rejected despite a quota of 2") } if trl.Allow(topic) { t.Error("a third message was allowed within the same window") diff --git a/docs/audit-test-coverage-2026-07-25.md b/docs/audit-test-coverage-2026-07-25.md index 2d81c29e..f8e01029 100644 --- a/docs/audit-test-coverage-2026-07-25.md +++ b/docs/audit-test-coverage-2026-07-25.md @@ -28,18 +28,18 @@ The client and Rust numbers come from `vitest run --coverage` and a per-file cen | ID | Sev | Finding | Status | |----|-----|---------|--------| -| T-2026-07-25-01 | HIGH | `Server/admin` reported **0.3% coverage despite 307 passing tests**, and `go test` printed "[no tests to run]" for it. `TestSpawnDetached_*` re-execs the test binary; the child inherited `GOCOVERDIR` and the parent's stdout, so it clobbered the coverage profile and polluted the result stream. CI's uploaded `coverage.out` artifact was wrong for this package | **RESOLVED** — `Server/admin/middleware_and_spawn_test.go` now points the child's counters at `t.TempDir()` and uses `-test.list` (silent exit) instead of `-test.run`. Package reports **71.4%** | -| T-2026-07-25-02 | HIGH | User blocking had **zero coverage at every layer** — `db.BlockUser`/`UnblockUser`/`IsBlocked`/`ListBlockedUsers`, all of `service/block.go`, and the `PUT`/`DELETE`/`GET /api/v1/blocks` routes. A whole user-facing feature, including the DM-authorization predicate `IsEitherBlocked` | **RESOLVED** — `Server/db/block_queries_test.go`, `Server/service/block_test.go`, `Server/api/blocks_handler_test.go` | +| T-2026-07-25-01 | HIGH | `Server/admin` reported **0.3% coverage despite 307 passing tests**, and `go test` printed "[no tests to run]" for it. `TestSpawnDetached_*` re-execs the test binary; the child inherited `GOCOVERDIR` and the parent's stdout, so it clobbered the coverage profile and polluted the result stream. CI's uploaded `coverage.out` artifact was wrong for this package | **RESOLVED** — `Server/admin/middleware_and_spawn_test.go` now points the child's counters at `t.TempDir()` and uses `-test.list` (silent exit) instead of `-test.run`. Package reports **71.4%** with only that fix applied, and **77.9%** with this PR's two new admin test files | +| T-2026-07-25-02 | HIGH | User blocking had **zero coverage at every layer** — `db.BlockUser`/`UnblockUser`/`IsBlocked`/`ListBlockedUsers`, all of `service/block.go`, and the `PUT`/`DELETE`/`GET /api/v1/blocks` routes. A whole user-facing feature. (`IsEitherBlocked` is the exception: it was already at 83.3% via `service/message_test.go`'s `TestCanPost_DMBlockEnforced`) | **RESOLVED** — `Server/db/block_queries_test.go`, `Server/service/block_test.go`, `Server/api/blocks_handler_test.go` | | T-2026-07-25-03 | HIGH | Auth lockout **persistence** untested (`UpsertLockout`, `CleanupExpiredLockouts`, `DeleteLockout`). `auth/ratelimit_test.go` covers the in-memory limiter but not the DB round-trip that makes a brute-force lockout survive a restart | **RESOLVED** — `Server/db/lockout_queries_test.go` | | T-2026-07-25-04 | HIGH | Rust: `ws_proxy.rs` (340 LOC) and `livekit_proxy.rs` (332 LOC) had **no tests at all** — the two proxies carrying every byte of app traffic, including TOFU cert pinning and proxy header rewriting | **RESOLVED** — pure helpers extracted (matching the existing `tofu.rs` pattern) and tested: cert-fingerprint validation, `remote_host` CRLF/charset validation, `Host`/`Origin` rewriting, TLS server-name parsing | | T-2026-07-25-05 | HIGH | Rust unit tests ran **only on PRs to `main`**, inside the expensive `tauri-build` job. Pushes and PRs to `dev` never compiled `#[cfg(test)]` code, so it could rot for a full release cycle | **RESOLVED** — standalone `rust-tests` job in `ci.yml`, runs on every event, with `cargo clippy --all-targets` (the existing lib-only clippy skips test code) | -| T-2026-07-25-06 | HIGH | **40 Playwright spec files had never run in CI.** No e2e job existed in any workflow | **RESOLVED (partial)** — new `client-e2e` job runs the mocked-Tauri config, `continue-on-error: true` for a soak period per backlog #10. The native config still is not wired (needs a real server + built binary) | -| T-2026-07-25-07 | MEDIUM | `vitest.config.ts` excluded **2,229 LOC** from coverage with no stated reason — including `window-state.ts` and `UpdateNotifier.ts`, which *already had passing tests*. Coverage for those never appeared in any report | **RESOLVED** — exclude list cut to three entries, each justified inline. `credentials.ts` and `updater.ts` gained tests and were un-excluded | +| T-2026-07-25-06 | HIGH | **44 Playwright spec files (33 web + 11 native) had never run in CI; the web config collects 255 tests across those 33.** No e2e job existed in any workflow | **RESOLVED (partial)** — new `client-e2e` job runs the mocked-Tauri config, `continue-on-error: true` for a soak period per backlog #10. The native config still is not wired (needs a real server + built binary) | +| T-2026-07-25-07 | MEDIUM | `vitest.config.ts` excluded **1,827 LOC** from coverage with no stated reason — including `window-state.ts` and `UpdateNotifier.ts`, which *already had passing tests*. Coverage for those never appeared in any report | **RESOLVED** — exclude list cut to three entries, each justified inline. `credentials.ts` and `updater.ts` gained tests and were un-excluded | | T-2026-07-25-08 | MEDIUM | Plugin install/enable/disable/uninstall lifecycle and the entire plugin KV store untested. `plugin/registry.go` (558 LOC) was the largest untested source file in the repo; the KV namespace is the isolation boundary between plugins | **RESOLVED** — `Server/plugin/registry_test.go`, `Server/db/plugin_queries_test.go` (including a namespace-isolation test and cascade-on-uninstall) | | T-2026-07-25-09 | MEDIUM | `handleWebhookParticipantJoined` untested — the guard that evicts a LiveKit participant presenting a replayed or unmatched join token. Untrusted-input entry point | **RESOLVED** — `Server/ws/livekit_webhook_joined_test.go` | | T-2026-07-25-10 | MEDIUM | `proxyWebSocket` / `copyWS` untested: the existing `livekit_proxy_test.go` stopped at the path allowlist and Origin check, before the upgrade. Every LiveKit signalling frame flows through the untested half | **RESOLVED** — `Server/api/livekit_proxy_ws_test.go` (real backend WS server, round-trip, 502 on backend failure, blocked-path and cross-origin upgrades) | -| T-2026-07-25-11 | MEDIUM | `api.HandleLiveKitHealthForTest` **re-implemented** `handleLiveKitHealth` instead of calling it. Eight test call sites asserted against a copy, so the production handler had 0% coverage and the two could drift silently | **RESOLVED (partial)** — added `LiveKitHealthHandlerForTest`, which returns the real handler, plus tests through it. The old hook is retained with a comment marking it as a duplicate; migrating its eight callers is follow-up work | -| T-2026-07-25-12 | MEDIUM | Client: six modules well under the 70% threshold with no test file of their own — `livekitDiagnostics` 30.4%, `drag-reorder` 38.8%, `deep-link` 44.1%, `roomEventHandlers` 57.1%, `screenShare` 61.1%, `volume-menu` 77.7% | **RESOLVED** — eight new test files; all now ≥96% except `screenShare`, whose remaining gap is the untested-by-design capture paths | +| T-2026-07-25-11 | MEDIUM | `api.HandleLiveKitHealthForTest` **re-implemented** `handleLiveKitHealth` instead of calling it. Seven test call sites asserted against a copy, so the production handler had 0% coverage and the two could drift silently | **RESOLVED (partial)** — added `LiveKitHealthHandlerForTest`, which returns the real handler, plus tests through it. The old hook is retained with a comment marking it as a duplicate; migrating its seven callers is follow-up work | +| T-2026-07-25-12 | MEDIUM | Client: six modules well under the 70% threshold with no test file of their own — `livekitDiagnostics` 30.4%, `drag-reorder` 38.8%, `deep-link` 44.1%, `roomEventHandlers` 57.1%, `screenShare` 61.1%, `volume-menu` 77.7% | **RESOLVED** — eight new test files; all six now ≥96%, five of them at 100% (`screenShare.ts` 61.1 → 100) | | T-2026-07-25-13 | MEDIUM | Event replay/retention partly untested (`GetMaxEventSeq` seeds the hub's sequence counter at startup; `PruneEventsOlderThan` is the retention job) | **RESOLVED** — `Server/db/event_queries_test.go`, including the channel filter that stops a replay leaking events for channels a client cannot see | | T-2026-07-25-14 | MEDIUM | Admin live-log stream: 11 consecutive uncovered functions in the `multiHandler` slog fan-out, including `Subscribe` — what a connected admin's SSE session hangs off | **RESOLVED** — `Server/admin/multihandler_test.go` | | T-2026-07-25-15 | MEDIUM | `Server/Makefile` had **no test target at all**, so there was no blessed way to reproduce the CI run or read coverage locally | **RESOLVED** — `test`, `test-deadlock`, `cover`, `cover-all` added; `cover-all` prints the zero-coverage function list | @@ -47,6 +47,7 @@ The client and Rust numbers come from `vitest run --coverage` and a per-file cen | T-2026-07-25-17 | LOW | `Server/main.go` (452 LOC, `package main`) and `Server/scripts/seed.go` (371 LOC dev tool) have no tests | **OPEN.** `main.go` is wiring with no seam below the integration level; `seed.go` is a developer tool. Both are low-risk, but `main.go` is the largest untested single file on the server | | T-2026-07-25-18 | LOW | `src/pages/MainPage.ts` (561 LOC orchestrator) and `src/main.ts` (597 LOC bootstrap) remain excluded from client coverage | **OPEN (documented).** Both exclusions now carry a written justification; `MainPage.ts` is explicitly tracked for unit tests, `main.ts` is bootstrap covered by e2e | | T-2026-07-25-19 | LOW | No coverage threshold or ratchet on the Go side; no coverage instrumentation for Rust at all. The client's 70% vitest threshold is the only enforced floor anywhere | **OPEN.** Deliberately not added — a floor set below current coverage (84–92% per package) is theatre, and a ratchet needs a baseline store this repo does not have | +| T-2026-07-25-21 | HIGH | **The Playwright web e2e suite does not pass — on `main`.** A local run of the 255 web tests fails **229**, all cascading from `navigateToMainPage` in `tests/e2e/helpers.ts:818` never seeing `[data-testid='app-layout']` after login. Reproduced on a clean `70caa6c` worktree (5/5 failures in `banners-toasts.spec.ts` alone), so it predates this PR and is unrelated to it. It went unnoticed precisely because e2e has never run in CI (T-…-06) | **OPEN — newly discovered.** Found only because this pass tried to wire e2e into CI. The `client-e2e` job is `continue-on-error` with `timeout-minutes: 25`, so it surfaces the breakage without gating on it or burning unbounded minutes. Repairing the login helper is a prerequisite for backlog #2 (promoting the job to blocking) | | T-2026-07-25-20 | LOW | `Server/ws` failed twice under full-suite `-coverpkg` runs, but passed 5/5 in isolation and under `-race`, and the failing test name was not captured | **OPEN — watch.** Load-sensitive and unreproduced. Not present in the `-race` gate CI actually runs | --- @@ -95,7 +96,7 @@ recorded here so they are not re-flagged: `fenwick.ts` 95.9%, `formatting.ts` 10 Still below 85% and untouched by this pass (pre-existing): `MessageList.ts` 83.5%, `attachments.ts` 82.7%, `MemberList.ts` 81.4%, `livekitSession.ts` 79.4%, -`screenShare.ts` 61.1%, `UserProfilePopup.ts` 55.2% *branches*. +`UserProfilePopup.ts` 55.2% *branches*. ### Rust (`cargo test --lib`) @@ -136,7 +137,7 @@ immediately while no drag is active — so the test pins the real behaviour unde | Client unit tests + 70% threshold | every PR (blocking) | unchanged | | Rust unit tests | **PRs to `main` only** | **every event** (`rust-tests`) | | Rust clippy | lib only | lib (`tauri-build`) **+ `--all-targets`** (`rust-tests`) | -| Playwright e2e | **never** | **every PR, non-blocking** (`client-e2e`) | +| Playwright e2e | **never** | **every PR, non-blocking** (`client-e2e`) — expect RED until T-…-21 is fixed | | `-tags wazero` / `-tags otel` tests | never | **still never** (T-…-16) | | Coverage floor / ratchet (Go, Rust) | none | none (T-…-19) | @@ -147,9 +148,9 @@ immediately while no drag is active — so the test pins the real behaviour unde | # | Item | Finding | Sev | |---|---|---|---| | 1 | Run tag-gated tests in CI (`-tags wazero ./plugin/...`, `-tags otel ./telemetry/...`) — unlocks ~598 lines of existing tests | T-…-16 | MEDIUM | -| 2 | Promote `client-e2e` to blocking once it has soaked | T-…-06 | MEDIUM | +| 2 | Repair the e2e login helper so the web suite passes, then promote `client-e2e` to blocking | T-…-21, T-…-06 | HIGH | | 3 | Fix the `drag-reorder.ts` ref-count asymmetry and update its pinning test | §4 | MEDIUM | -| 4 | Migrate the eight `HandleLiveKitHealthForTest` callers to `LiveKitHealthHandlerForTest` and delete the duplicated hook | T-…-11 | LOW | +| 4 | Migrate the seven `HandleLiveKitHealthForTest` callers to `LiveKitHealthHandlerForTest` and delete the duplicated hook | T-…-11 | LOW | | 5 | Unit tests for `src/pages/MainPage.ts`, then remove its coverage exclusion | T-…-18 | LOW | | 6 | Decide on `logctx.WithGroup` nesting before any logger-level group is introduced | §4 | LOW | | 7 | Watch for the `ws` flake under instrumented full runs; capture the test name if it recurs | T-…-20 | LOW |