crueber/walhub

Watch

Fork 0

CI coverage gate failing: internal/server at 93.6% vs the 95% floor — blocks GHCR publish on the GitHub mirror #289

New issue

Closed

opened 2026-09-10 13:58:09 +00:00 by crueber · 3 comments

crueber commented

2026-09-10 13:58:09 +00:00

Owner

What's wrong

The GitHub mirror CI (run 34484949364, job test → step "coverage gate") is failing on the 95% per-package coverage floor, which blocks the GHCR docker publish for every push to main.

Root cause (reproduced locally)

make cover fails on exactly one package:

git.packden.us/crueber/walhub/internal/server — 93.6% (floor 95.0%)
covergate: 1 package(s) below 95.0%

Every other package passes (api 95.2, bundle 95.7, checks 96.4, repoimport 95.8, mirror 96.9, …). 64 functions in internal/server sit below 95% individually; the meaningful gaps:

Function	Coverage
`auth.go` `wgtPrincipal`	75.0%
`auth_oidc.go` `verifyStateTicket`	76.9%
`health.go` `sdkReposJS`	26.7%
`health.go` `explorePage`	38.5%
`auth.go` `Authenticate`	80.0%
`bind_ssh.go` `adoptPlaceholder`	81.8%
`auth_oidc.go` `authTokensMint`	88.2%
`bind_wal.go` `PublishSettings`	77.8%
file-level: `install_sh.go` 79.4%, `listener.go` 88.9%, `router.go` 90.5%, `setup_api.go` 92.1%

(Local run note: go test ./internal/server/ needs web/dist to exist for the embed — an empty web/dist/.keep is enough to compile; CI builds the real dist first, so the CI failure is genuine coverage, not the embed.)

The coverage dropped across the recent feature wave that touched internal/server without adding matching tests — git log -- internal/server shows #247 (last-commit tracking, sizecatalog wiring), #240 (mirror endpoints), #215 (external SSH port) landed on the auth/bind/health surfaces.

Fix

Raise internal/server back over the floor by testing the uncovered paths — in rough order of value:

health.go sdkReposJS (26.7%) and explorePage (38.5%) — likely large simple-branch functions, cheapest wins.
OIDC paths: verifyStateTicket (76.9%), authTokensMint (88.2%), exchangeCode (89.3%) — these are security-relevant error branches (bad state tickets, failed exchanges) that deserve tests regardless of the gate.
auth.go wgtPrincipal (75.0%) and Authenticate (80.0%) — same security rationale.
bind_wal.go PublishSettings (77.8%) and bind_ssh.go adoptPlaceholder (81.8%).
File-level stragglers (install_sh.go, listener.go, setup_api.go) — whatever remains to clear 95%.

Test style follows the package's existing table-driven fixtures (fakes_test.go patterns); no production-code changes should be needed. If a block is genuinely unreachable dead code, that's a separate deletion commit with its own justification — don't exclude files from the gate.

Acceptance criteria

make cover passes locally (server ≥ 95.0%, all other packages unchanged).
The GitHub test workflow completes green and the publish job runs again (GHCR image publishes on push to main).
The new tests target real behavior (error branches, ticket validation, mint paths) — no empty asserts, no coverage-by-triviality.
If any uncovered code is judged dead, it is removed in the same change with a stated reason, not excluded from the gate.

## What's wrong The GitHub mirror CI (run [34484949364](https://github.com/crueber/walhub/actions/runs/34484949364/job/102896761489), job `test` → step "coverage gate") is failing on the 95% per-package coverage floor, which blocks the GHCR docker publish for every push to main. ## Root cause (reproduced locally) `make cover` fails on exactly **one** package: ``` git.packden.us/crueber/walhub/internal/server — 93.6% (floor 95.0%) covergate: 1 package(s) below 95.0% ``` Every other package passes (api 95.2, bundle 95.7, checks 96.4, repoimport 95.8, mirror 96.9, …). 64 functions in `internal/server` sit below 95% individually; the meaningful gaps: | Function | Coverage | |---|---| | `auth.go` `wgtPrincipal` | 75.0% | | `auth_oidc.go` `verifyStateTicket` | 76.9% | | `health.go` `sdkReposJS` | 26.7% | | `health.go` `explorePage` | 38.5% | | `auth.go` `Authenticate` | 80.0% | | `bind_ssh.go` `adoptPlaceholder` | 81.8% | | `auth_oidc.go` `authTokensMint` | 88.2% | | `bind_wal.go` `PublishSettings` | 77.8% | | file-level: `install_sh.go` 79.4%, `listener.go` 88.9%, `router.go` 90.5%, `setup_api.go` 92.1% | | (Local run note: `go test ./internal/server/` needs `web/dist` to exist for the embed — an empty `web/dist/.keep` is enough to compile; CI builds the real dist first, so the CI failure is genuine coverage, not the embed.) The coverage dropped across the recent feature wave that touched `internal/server` without adding matching tests — `git log -- internal/server` shows #247 (last-commit tracking, `sizecatalog` wiring), #240 (mirror endpoints), #215 (external SSH port) landed on the auth/bind/health surfaces. ## Fix Raise `internal/server` back over the floor by testing the uncovered paths — in rough order of value: 1. `health.go` `sdkReposJS` (26.7%) and `explorePage` (38.5%) — likely large simple-branch functions, cheapest wins. 2. OIDC paths: `verifyStateTicket` (76.9%), `authTokensMint` (88.2%), `exchangeCode` (89.3%) — these are security-relevant error branches (bad state tickets, failed exchanges) that deserve tests regardless of the gate. 3. `auth.go` `wgtPrincipal` (75.0%) and `Authenticate` (80.0%) — same security rationale. 4. `bind_wal.go` `PublishSettings` (77.8%) and `bind_ssh.go` `adoptPlaceholder` (81.8%). 5. File-level stragglers (`install_sh.go`, `listener.go`, `setup_api.go`) — whatever remains to clear 95%. Test style follows the package's existing table-driven fixtures (`fakes_test.go` patterns); no production-code changes should be needed. If a block is genuinely unreachable dead code, that's a separate deletion commit with its own justification — don't exclude files from the gate. ## Acceptance criteria - [ ] `make cover` passes locally (server ≥ 95.0%, all other packages unchanged). - [ ] The GitHub `test` workflow completes green and the `publish` job runs again (GHCR image publishes on push to main). - [ ] The new tests target real behavior (error branches, ticket validation, mint paths) — no empty asserts, no coverage-by-triviality. - [ ] If any uncovered code is judged dead, it is removed in the same change with a stated reason, not excluded from the gate.

crueber referenced this issue from a commit

2026-09-10 14:45:57 +00:00

Fix #289: internal/server coverage gate (98.5%) + store key layout (95.1%)

crueber referenced this issue from a pull request that will close it,

2026-09-10 14:46:10 +00:00

Fix #289: internal/server coverage gate #291

crueber commented

2026-09-10 14:46:20 +00:00

Author

Owner

Fix ready for review: #291 (branch fix/issue-289).

Note on scope drift vs the issue body: the per-function table in the issue matches a no-dist local run (sdkReposJS 26.7%/explorePage 38.5% happen with an empty web/dist, where the UI tests 500). With the CI-order build (make web first) origin/main measures server 95.6% — but the margin was one bad day from red, and the listed OIDC/auth/bind gaps were real, so I tested them all anyway: server → 98.5%. The actual local gate failure was internal/store at 94.97% (#234's StatsKey/OwnerProfileKey landed uncovered — one statement short of the floor), fixed with layout pins in the existing key table → 95.1%. make cover fully green; -race green.

Two real bugs came out of the tests (fixed + declared in the PR): equalFoldLast panics on short input, and serveStatic mapped store outages to 404 instead of 503. The leftover sub-95% functions are defensive-dead branches (verifyStateTicket re-checks, mint error returns, template errors, etc.) — catalogued in the PR description for a separate deletion pass, not excluded from the gate. Not merging; review requested.

Fix ready for review: #291 (branch `fix/issue-289`). Note on scope drift vs the issue body: the per-function table in the issue matches a no-dist local run (sdkReposJS 26.7%/explorePage 38.5% happen with an empty `web/dist`, where the UI tests 500). With the CI-order build (`make web` first) origin/main measures **server 95.6%** — but the margin was one bad day from red, and the listed OIDC/auth/bind gaps were real, so I tested them all anyway: **server → 98.5%**. The actual local gate failure was `internal/store` at 94.97% (#234's `StatsKey`/`OwnerProfileKey` landed uncovered — one statement short of the floor), fixed with layout pins in the existing key table → **95.1%**. `make cover` fully green; `-race` green. Two real bugs came out of the tests (fixed + declared in the PR): `equalFoldLast` panics on short input, and `serveStatic` mapped store outages to 404 instead of 503. The leftover sub-95% functions are defensive-dead branches (verifyStateTicket re-checks, mint error returns, template errors, etc.) — catalogued in the PR description for a separate deletion pass, not excluded from the gate. Not merging; review requested.

crueber commented

2026-09-10 14:51:18 +00:00

Author

Owner

Review of PR #291 (fix/issue-289) — verified in scratch worktrees, main untouched.

TEST GENUINENESS (law 11): PASS. Sampled TestEqualFoldLastTable (x_gaps10_test.go:111 — includes {"a","abc"} which panics pre-fix), TestServeStaticHeadError (:609 — asserts 404 miss vs 503 outage), TestErrorsAsTable (:589 — direct/wrapped/nil error identity), TestRegistryStoreFaults (:986 — torn k-doc, dead-store add, k-doc rollback verified gone, blind list), TestVerifyTokenWireNegatives + TestConfigCoerceNegatives + TestWGTPrincipalRoundTripAndReject (x_gaps9_test.go — forged/tampered/expired tokens assert ErrInvalid kind + Why strings; coerce asserts per-branch error substrings). All assert error codes/kinds/state changes, not mere execution.

PROD FIX 1 — equalFoldLast length guard (health.go:336): CORRECT + IN-SCOPE. Only caller is hasSuffixFold (:332) which already guards len, so the fix only converts a latent direct-call panic into false. New table test pins it. No behavior change for existing callers.

PROD FIX 2 — serveStatic 404-vs-503 ordering (static.go:21-34): CORRECT + IN-SCOPE. Old code returned 404 when a non-NotFound store error coincided with nil meta (outage masquerading as not-found); new code returns 503. All other paths identical. Pinned by TestServeStaticHeadError. No doc update needed (no doc pins the old ordering; fix aligns with fail-closed law 9).

STORE 94.97->95.1: LEGITIMATE. keys_test.go:33-36 adds exact-equality rows for previously untested StatsKey/StatsKeySuffix/OwnerProfileKey/OwnerProfileKeySuffix, values match keys.go definitions.

RESULTS (scratch /tmp/pr291, origin/fix/issue-289 @ fd76be8): gofmt clean; go vet clean (exit 0; note: scratch worktree needs web/dist present for the embed — copied from main's build output, read-only); go test -race ./internal/store/... PASS (store 95.1%); go test -race ./internal/server/... PASS, 98.5%. One env note: TestUIAssetConcepts fails on a stale dist (missing concepts/*.gif) — reproduced identically on pristine origin/main, so pre-existing/environmental, not a PR regression; after 'make web' in scratch the full suite is green. Did NOT run full 'make cover' (expensive); only server+store ran, which are the only packages the PR touches, both above the 95% gate. No browser (backend-only change). No production behavior change beyond the 2 fixes (diff prod files = health.go + static.go only).

No fixes pushed — nothing found needing correction.

Review of PR #291 (fix/issue-289) — verified in scratch worktrees, main untouched. TEST GENUINENESS (law 11): PASS. Sampled TestEqualFoldLastTable (x_gaps10_test.go:111 — includes {"a","abc"} which panics pre-fix), TestServeStaticHeadError (:609 — asserts 404 miss vs 503 outage), TestErrorsAsTable (:589 — direct/wrapped/nil error identity), TestRegistryStoreFaults (:986 — torn k-doc, dead-store add, k-doc rollback verified gone, blind list), TestVerifyTokenWireNegatives + TestConfigCoerceNegatives + TestWGTPrincipalRoundTripAndReject (x_gaps9_test.go — forged/tampered/expired tokens assert ErrInvalid kind + Why strings; coerce asserts per-branch error substrings). All assert error codes/kinds/state changes, not mere execution. PROD FIX 1 — equalFoldLast length guard (health.go:336): CORRECT + IN-SCOPE. Only caller is hasSuffixFold (:332) which already guards len, so the fix only converts a latent direct-call panic into false. New table test pins it. No behavior change for existing callers. PROD FIX 2 — serveStatic 404-vs-503 ordering (static.go:21-34): CORRECT + IN-SCOPE. Old code returned 404 when a non-NotFound store error coincided with nil meta (outage masquerading as not-found); new code returns 503. All other paths identical. Pinned by TestServeStaticHeadError. No doc update needed (no doc pins the old ordering; fix aligns with fail-closed law 9). STORE 94.97->95.1: LEGITIMATE. keys_test.go:33-36 adds exact-equality rows for previously untested StatsKey/StatsKeySuffix/OwnerProfileKey/OwnerProfileKeySuffix, values match keys.go definitions. RESULTS (scratch /tmp/pr291, origin/fix/issue-289 @ fd76be8): gofmt clean; go vet clean (exit 0; note: scratch worktree needs web/dist present for the embed — copied from main's build output, read-only); go test -race ./internal/store/... PASS (store 95.1%); go test -race ./internal/server/... PASS, 98.5%. One env note: TestUIAssetConcepts fails on a stale dist (missing concepts/*.gif) — reproduced identically on pristine origin/main, so pre-existing/environmental, not a PR regression; after 'make web' in scratch the full suite is green. Did NOT run full 'make cover' (expensive); only server+store ran, which are the only packages the PR touches, both above the 95% gate. No browser (backend-only change). No production behavior change beyond the 2 fixes (diff prod files = health.go + static.go only). No fixes pushed — nothing found needing correction.

crueber closed this issue

2026-09-10 14:52:02 +00:00

crueber referenced this issue from a commit

2026-09-10 14:52:02 +00:00

Merge pull request 'Fix #289: internal/server coverage gate' (#291) from fix/issue-289 into main

crueber commented

2026-09-10 14:52:03 +00:00

Author

Owner

Fixed by PR #291 (review: genuine behavioral pins, 2 real bugs fixed; server 98.5%, store 95.1%), merged. Closing.

crueber added the

git storage