feat(ws): harden auth, Redis bridge, and origin handling

[Snider]

Routine dev→main PR opened by Hephaestus PR-cadence task.

This PR exists to:

1. Trigger CodeRabbit auto-review of accumulated dev work
2. Surface any blocking review feedback before merge
3. Keep main current with dev once approved

ahead_by: 152 commit(s) (per gh api compare)

If CodeRabbit clears with no blocking comments, this PR is eligible for gh pr merge --merge (real merge commit, no force-push). Conflicts and review feedback should be addressed on the dev branch before merge.

Co-authored-by: Hephaestus hephaestus@cladius

Summary by CodeRabbit

• New Features

  • Strict same‑origin WebSocket validation by default; configurable origin checks
  • Per‑client subscription limits and stronger subscription validation
  • More configurable reconnecting client (max attempts, error callbacks, flexible message handling)
  • New bearer/query token authenticators and improved Redis bridge lifecycle/restart behaviour

• Bug Fixes

  • Authentication claims are deep‑cloned to prevent post‑auth mutation
  • API key/bearer parsing is case‑insensitive and empty user IDs are rejected

• Chores

  • Module path and Go toolchain updated; large test and coverage expansion

• Documentation

  • Updated architecture/history docs to reflect origin‑check default behaviour

[coderabbitai]

📝 Walkthrough

Walkthrough

Finalises and normalises authentication results with claims deep-cloning, tightens origin checks to deny-by-default, hardens and lifecycle-proofs the Redis bridge, enforces per-client subscription limits and identifier validation in the Hub, refactors reconnecting client behaviour, and replaces testify in tests with stdlib helpers; module path and deps updated.

Changes

Cohort / File(s)	Summary
Authentication & Claims auth.go, auth_test.go	Adds AuthResult.Authenticated, finalisation helpers (normalizeAuthResult, finalizeAuthResult), reflection-based deep-cloning of Claims (depth-limited, rejects unsupported kinds), ensures non-blank UserID, adapts AuthenticatorFunc, reworks APIKeyAuthenticator (internal snapshot keys), adds NewBearerTokenAuth and NewQueryTokenAuth, and expands tests (claim cloning matrix, authenticator adapter and integration tests).
Hub & WebSocket Core ws.go, ws_bench_test.go	Switches subscription target to prefer Message.Channel, enforces origin-check deny-by-default using HubConfig.CheckOrigin/AllowedOrigins, adds MaxSubscriptionsPerClient and identifier validation, routes subscription changes when hub running, stamps/validates outbound messages, expands stats, and updates reconnecting client behaviour (errors, backoff, polymorphic OnMessage). Bench updates mirror API/import changes.
Redis Bridge redis.go, redis_test.go	Bounds and JSON-decodes Redis envelopes, validates channel prefix and ProcessID, uses core.ID() for source, adds mutex-protected lifecycle (ctx/cancel/pubsub/client) and stopListener(), timeout-scoped publish/subscribe contexts, delivery via hub internals with nil-hub guards, and extensive rewritten tests including cross-bridge integration and self-echo suppression.
Errors & Sentinels errors.go, errors_test.go	Swaps logging import to dappco.re/go/log, introduces exported errors ErrMissingUserID, ErrInvalidAuthClaims, and ErrSubscriptionLimitExceeded, and adds tests verifying sentinel strings and wrapping behaviour.
Testing Helpers & Suites test_stdlib_helpers_test.go, ax7_v090_test.go	Adds stdlib-based test helpers (testEqual, testIsNil, testIsEmpty, testContains, testEventually, etc.) and a large new integration test suite including a fake Redis server and many hub/redis/authentication scenarios. Tests replace testify usage across codebase.
Build, Modules & CI Tasks go.mod, tests/cli/ws/Taskfile.yaml, .golangci.yml	Module path changed to dappco.re/go/ws, Go toolchain bumped to 1.26.2, dependency reshuffle (adds dappco.re/go, dappco.re/go/log, removes prior core module), adds replace for log, removes direct testify requirement; Taskfile added for build/test/lint tasks; golangci config adjusted (formatter section and disabled linters).
Documentation docs/architecture.md, docs/history.md, docs/index.md	Updates docs to document deny-by-default origin policy, default origin matching logic, and history note about origin check change; removes testify from docs dependency list.
Miscellaneous ws.go (other changes), various tests	Additional small API and behaviour changes across hub/client code (deterministic snapshots, nil-hub guards, wire-format stamping behaviour), widespread test updates to use new helpers and explicit readiness waits.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 13.80% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the three primary areas of hardening addressed across the changeset: authentication (claims snapshotting, deep cloning, bearer/query token constructors), Redis bridge (process ID validation, lifecycle safety, state management), and origin handling (strict default with CheckOrigin callback support).
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.4)

level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: cannot load module ../go listed in go.work file: open ../go/go.mod: no such file or directory\ngo: cannot load module ../go-log listed in go.work file: open ../go-log/go.mod: no such file or directory\n"

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (9)

docs/architecture.md (1)

418-418: Minor style nit: remove hyphen from "narrowly-scoped".

Per the static analysis hint, compound modifiers where the first word is an adverb ending in "-ly" do not require a hyphen. Consider changing to "narrowly scoped".

✏️ Suggested fix

-**Strict origin check by default.** The WebSocket handler rejects cross-origin upgrades unless `HubConfig.CheckOrigin` explicitly allows them. The built-in default requires the `Origin` scheme and host to match the request target, and callbacks are treated as deny-by-default if they panic. Production deployments should keep the default or supply a narrowly-scoped override.
+**Strict origin check by default.** The WebSocket handler rejects cross-origin upgrades unless `HubConfig.CheckOrigin` explicitly allows them. The built-in default requires the `Origin` scheme and host to match the request target, and callbacks are treated as deny-by-default if they panic. Production deployments should keep the default or supply a narrowly scoped override.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/architecture.md` at line 418, In the "Strict origin check by default."
paragraph that mentions HubConfig.CheckOrigin, remove the hyphen in
"narrowly-scoped" and change it to "narrowly scoped" so the adverb + modifier
follows the recommended style; update the sentence that currently reads "...or
supply a narrowly-scoped override." to "...or supply a narrowly scoped
override."

errors_test.go (2)

25-30: Redundant nil check.

Lines 25-27 check if err is nil and fail. Lines 28-30 then check the same condition again (err == nil || ...). The first check is redundant since the second already handles the nil case.

✏️ Simplified version

 		t.Run(tt.name, func(t *testing.T) {
-			if err := tt.err; err == nil {
-				t.Errorf("expected error")
-			}
-			if err := tt.err; err == nil || err.Error() != tt.want {
+			if tt.err == nil || tt.err.Error() != tt.want {
 				t.Errorf("expected error %q, got %v", tt.want, err)
 			}
-
 		})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@errors_test.go` around lines 25 - 30, The test contains a redundant nil check
for tt.err: remove the first if block that only checks "if err := tt.err; err ==
nil { t.Errorf(...)}" and keep the combined check that verifies nil OR
mismatched error string (the second block). Ensure the remaining check uses a
single err := tt.err local and reports the expected vs actual with
t.Errorf("expected error %q, got %v", tt.want, err) so nil and string-mismatch
are both handled.

---

12-34: Missing test coverage for new error sentinels.

The tests only cover the original three sentinels (ErrMissingAuthHeader, ErrMalformedAuthHeader, ErrInvalidAPIKey), but errors.go now also exports ErrMissingUserID, ErrInvalidAuthClaims, and ErrSubscriptionLimitExceeded. Consider extending the test table to include these.

✏️ Suggested addition to the test table

 	tests := []struct {
 		name string
 		err  error
 		want string
 	}{
 		{name: "missing header", err: ErrMissingAuthHeader, want: "missing Authorization header"},
 		{name: "malformed header", err: ErrMalformedAuthHeader, want: "malformed Authorization header"},
 		{name: "invalid api key", err: ErrInvalidAPIKey, want: "invalid API key"},
+		{name: "missing user id", err: ErrMissingUserID, want: "authenticated user ID must not be empty"},
+		{name: "invalid auth claims", err: ErrInvalidAuthClaims, want: "authentication claims are invalid"},
+		{name: "subscription limit exceeded", err: ErrSubscriptionLimitExceeded, want: "subscription limit exceeded"},
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@errors_test.go` around lines 12 - 34, Extend the
TestErrors_AuthSentinels_Good table in TestErrors_AuthSentinels_Good to include
the three new exported sentinels ErrMissingUserID, ErrInvalidAuthClaims, and
ErrSubscriptionLimitExceeded (alongside the existing ErrMissingAuthHeader,
ErrMalformedAuthHeader, ErrInvalidAPIKey); for each new entry set want equal to
the exact error string defined in errors.go and run the same assertions. Locate
the test table in TestErrors_AuthSentinels_Good and add rows for
ErrMissingUserID, ErrInvalidAuthClaims, and ErrSubscriptionLimitExceeded using
their canonical error messages from errors.go so the t.Run checks compare
err.Error() to the correct expected string.

redis.go (2)

274-282: Error precedence: Redis error shadows local broadcast failure.

When PublishBroadcast delivers locally first (line 274), a local error is captured in localErr. However, if Redis publish also fails, redisErr is returned (line 279), and if Redis succeeds, localErr is returned (line 282). This means a Redis failure masks a local failure.

If both errors matter, consider combining them or logging the shadowed error. Current behaviour may be intentional if Redis state is considered authoritative.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@redis.go` around lines 274 - 282, PublishBroadcast currently returns the
Redis error when rb.publish fails, which hides any prior local error from
rb.hub.broadcastMessage; change the return logic around localErr and redisErr so
both failures are preserved — if both localErr and redisErr are non-nil,
return/construct a combined error (or log one and return the other) that
includes both messages (e.g., "local: ...; redis: ..."), if only one is non-nil
return that one; update the block handling rb.hub.broadcastMessage and
rb.publish to aggregate or log the shadowed error rather than letting redisErr
unconditionally overwrite localErr.

---

134-139: Auto-start behaviour change in constructor.

NewRedisBridge now automatically calls Start(context.Background()) before returning. This is convenient but means callers can no longer defer starting the listener to a later point with a custom context. The docs (line 283 in architecture.md) still show a separate bridge.Start(ctx) call after construction.

If this is intentional, consider updating the architecture documentation to reflect that Start is called automatically with context.Background(), and that callers can call Stop() + Start(customCtx) if they need a different context.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@redis.go` around lines 134 - 139, The constructor NewRedisBridge currently
calls bridge.Start(context.Background()) before returning, which forces
immediate background start and prevents callers from supplying their own
context; revert this by removing the automatic Start call in NewRedisBridge so
it only constructs and returns the bridge (allowing callers to call
bridge.Start(ctx) or bridge.Start(customCtx) later), and update any
tests/examples that assumed auto-start; alternatively, if auto-start is
intended, update the architecture.md docs and examples to state that
NewRedisBridge starts the listener with context.Background() and document using
Stop() + Start(customCtx) to change the running context.

tests/cli/ws/Taskfile.yaml (2)

28-36: Consider adding -race flag to integration tests for consistency.

The test-integration task omits the -race flag, unlike test and test-unit. Race detection during integration tests can catch concurrency issues in the Redis bridge code paths.

-        go test -count=1 ./... -run Integration -tags integration
+        go test -count=1 -race ./... -run Integration -tags integration

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/cli/ws/Taskfile.yaml` around lines 28 - 36, The test-integration
Taskfile task named "test-integration" omits the Go race detector; update the go
test invocation in the test-integration cmds (the multi-line shell block that
runs `go test -count=1 ./... -run Integration -tags integration`) to include the
`-race` flag (e.g., `go test -race ...`) so integration runs use the same race
detection as `test` and `test-unit`.

---

38-43: Default task omits gofmt and golangci-lint from QA sequence.

The default task chains build, test, and vet, but project learnings specify running gofmt -l . and golangci-lint run ./... as part of the full QA sequence before commit.

♻️ Suggested addition

+  fmt:
+    dir: ../../..
+    cmds:
+      - gofmt -l .
+
+  lint:
+    dir: ../../..
+    cmds:
+      - golangci-lint run ./...
+
   default:
     deps:
+      - fmt
       - build
       - test
       - vet
+      - lint

Based on learnings: "Run full QA sequence before commit: gofmt -l . && go vet ./... && golangci-lint run ./... && go test -race ./..."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/cli/ws/Taskfile.yaml` around lines 38 - 43, The default Taskfile target
named "default" currently depends only on build, test, and vet; update it to
include the QA steps so the full pre-commit QA sequence runs: add tasks (or
dependencies) that run "gofmt -l .", "golangci-lint run ./...", and the full
test vet chain (e.g., ensure deps for gofmt, golangci-lint and the existing
"vet" and "test" targets are referenced in the "default" task) so the default
deps list contains build, gofmt, golangci-lint, vet, and test.

auth.go (1)

416-429: Use of unsafe.Pointer for unexported field access is intentional but warrants caution.

The setReflectValue function uses unsafe.Pointer to write unexported struct fields during claims cloning. While this enables deep-cloning of claims containing unexported mutable state (as tested), it bypasses Go's type safety guarantees.

Consider adding a brief doc comment explaining the safety invariants:

+// setReflectValue sets dst to value, using unsafe access for unexported fields.
+// The caller must ensure dst is addressable and value is type-compatible.
 func setReflectValue(dst reflect.Value, value reflect.Value) bool {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@auth.go` around lines 416 - 429, Add a short doc comment above
setReflectValue explaining why unsafe.Pointer and reflect.NewAt(dst.Type(),
unsafe.Pointer(dst.UnsafeAddr())) are used and what safety invariants callers
must uphold (e.g., only used for cloning trusted claim structs, destination
value must be addressable and of the exact expected concrete type, no concurrent
access during mutation, and that this intentionally breaks Go export rules);
reference the function name setReflectValue and the use of
dst.UnsafeAddr/unsafe.Pointer/reflect.NewAt so future maintainers understand the
tradeoffs and where to audit if the assumptions are violated.

redis_test.go (1)

1253-1256: Long sleep for subscription settling may cause flaky tests.

A 1-second sleep to allow subscriptions to settle is relatively long and may still be insufficient under load. Consider using the testEventually pattern for more reliable synchronisation.

-	time.Sleep(1 * time.Second)
+	if !testEventually(func() bool {
+		return bridgeA.isListening() && bridgeB.isListening()
+	}, 3*time.Second, 50*time.Millisecond) {
+		t.Fatal("bridges did not start listening in time")
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@redis_test.go` around lines 1253 - 1256, Replace the fixed 1s sleep with a
deterministic wait using the testEventually pattern: remove time.Sleep(1 *
time.Second) and instead poll until the subscription state is settled (for
example by checking a observable condition on bridgeB such as a
Subscribers()/IsSubscribed()/HasReceiver() method or by asserting a test message
is received) using testEventually with a short interval and a reasonable
timeout; keep the defer bridgeB.Stop() in place and ensure the poll asserts
success before proceeding so the test is not flaky.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@auth_test.go`:
- Around line 1436-1446: The if statement checking
testIsNil(hub.config.Authenticator) and the t.Errorf call are split with an
embedded comment, making it hard to read; reformat so the condition is a single
well-formed if (e.g., if !testIsNil(hub.config.Authenticator) { ... }) move or
remove the large comment block outside the if, and make the t.Errorf call
contiguous (t.Errorf("expected nil, got %T", hub.config.Authenticator)) so the
check and error reporting for hub.config.Authenticator and testIsNil are clear
and syntactically correct.
- Around line 268-275: The section comment "Unit tests — AuthenticatorFunc
adapter" is accidentally inside the if block of
TestAPIKeyAuthenticator_NilMap_Good; move that comment out of the function body
(place it before the next test or after the closing brace of
TestAPIKeyAuthenticator_NilMap_Good) so the comment becomes a top-level section
marker again and the test function contains only test logic (refer to
TestAPIKeyAuthenticator_NilMap_Good and the section comment string to locate and
relocate it).

In `@go.mod`:
- Around line 1-10: Update the import in ws_test.go that currently reads coreerr
"dappco.re/go/core/log" to the new module path coreerr "dappco.re/go/log";
locate the import block in ws_test.go (look for the alias coreerr) and replace
the old path, then run go mod tidy/build to verify the test compiles against the
updated dependency.

In `@redis_test.go`:
- Around line 350-357: The section comment "PublishBroadcast — messages reach
local WebSocket clients" is currently placed inside an if block near the
t.Errorf/err.Error() assertion; move that comment out of the conditional and
place it between the two test functions (i.e., above the next test's function
declaration) so it serves as a section header rather than being embedded inside
the if that contains t.Errorf and err.Error().

---

Nitpick comments:
In `@auth.go`:
- Around line 416-429: Add a short doc comment above setReflectValue explaining
why unsafe.Pointer and reflect.NewAt(dst.Type(),
unsafe.Pointer(dst.UnsafeAddr())) are used and what safety invariants callers
must uphold (e.g., only used for cloning trusted claim structs, destination
value must be addressable and of the exact expected concrete type, no concurrent
access during mutation, and that this intentionally breaks Go export rules);
reference the function name setReflectValue and the use of
dst.UnsafeAddr/unsafe.Pointer/reflect.NewAt so future maintainers understand the
tradeoffs and where to audit if the assumptions are violated.

In `@docs/architecture.md`:
- Line 418: In the "Strict origin check by default." paragraph that mentions
HubConfig.CheckOrigin, remove the hyphen in "narrowly-scoped" and change it to
"narrowly scoped" so the adverb + modifier follows the recommended style; update
the sentence that currently reads "...or supply a narrowly-scoped override." to
"...or supply a narrowly scoped override."

In `@errors_test.go`:
- Around line 25-30: The test contains a redundant nil check for tt.err: remove
the first if block that only checks "if err := tt.err; err == nil {
t.Errorf(...)}" and keep the combined check that verifies nil OR mismatched
error string (the second block). Ensure the remaining check uses a single err :=
tt.err local and reports the expected vs actual with t.Errorf("expected error
%q, got %v", tt.want, err) so nil and string-mismatch are both handled.
- Around line 12-34: Extend the TestErrors_AuthSentinels_Good table in
TestErrors_AuthSentinels_Good to include the three new exported sentinels
ErrMissingUserID, ErrInvalidAuthClaims, and ErrSubscriptionLimitExceeded
(alongside the existing ErrMissingAuthHeader, ErrMalformedAuthHeader,
ErrInvalidAPIKey); for each new entry set want equal to the exact error string
defined in errors.go and run the same assertions. Locate the test table in
TestErrors_AuthSentinels_Good and add rows for ErrMissingUserID,
ErrInvalidAuthClaims, and ErrSubscriptionLimitExceeded using their canonical
error messages from errors.go so the t.Run checks compare err.Error() to the
correct expected string.

In `@redis_test.go`:
- Around line 1253-1256: Replace the fixed 1s sleep with a deterministic wait
using the testEventually pattern: remove time.Sleep(1 * time.Second) and instead
poll until the subscription state is settled (for example by checking a
observable condition on bridgeB such as a
Subscribers()/IsSubscribed()/HasReceiver() method or by asserting a test message
is received) using testEventually with a short interval and a reasonable
timeout; keep the defer bridgeB.Stop() in place and ensure the poll asserts
success before proceeding so the test is not flaky.

In `@redis.go`:
- Around line 274-282: PublishBroadcast currently returns the Redis error when
rb.publish fails, which hides any prior local error from
rb.hub.broadcastMessage; change the return logic around localErr and redisErr so
both failures are preserved — if both localErr and redisErr are non-nil,
return/construct a combined error (or log one and return the other) that
includes both messages (e.g., "local: ...; redis: ..."), if only one is non-nil
return that one; update the block handling rb.hub.broadcastMessage and
rb.publish to aggregate or log the shadowed error rather than letting redisErr
unconditionally overwrite localErr.
- Around line 134-139: The constructor NewRedisBridge currently calls
bridge.Start(context.Background()) before returning, which forces immediate
background start and prevents callers from supplying their own context; revert
this by removing the automatic Start call in NewRedisBridge so it only
constructs and returns the bridge (allowing callers to call bridge.Start(ctx) or
bridge.Start(customCtx) later), and update any tests/examples that assumed
auto-start; alternatively, if auto-start is intended, update the architecture.md
docs and examples to state that NewRedisBridge starts the listener with
context.Background() and document using Stop() + Start(customCtx) to change the
running context.

In `@tests/cli/ws/Taskfile.yaml`:
- Around line 28-36: The test-integration Taskfile task named "test-integration"
omits the Go race detector; update the go test invocation in the
test-integration cmds (the multi-line shell block that runs `go test -count=1
./... -run Integration -tags integration`) to include the `-race` flag (e.g.,
`go test -race ...`) so integration runs use the same race detection as `test`
and `test-unit`.
- Around line 38-43: The default Taskfile target named "default" currently
depends only on build, test, and vet; update it to include the QA steps so the
full pre-commit QA sequence runs: add tasks (or dependencies) that run "gofmt -l
.", "golangci-lint run ./...", and the full test vet chain (e.g., ensure deps
for gofmt, golangci-lint and the existing "vet" and "test" targets are
referenced in the "default" task) so the default deps list contains build,
gofmt, golangci-lint, vet, and test.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f8c75087-1193-4902-8d94-50e596427e06

📥 Commits

Reviewing files that changed from the base of the PR and between 4b9176e and d526e0d.

⛔ Files ignored due to path filters (3)

• go.sum is excluded by !**/*.sum
• go.work is excluded by !**/*.work
• go.work.sum is excluded by !**/*.sum

📒 Files selected for processing (14)

• auth.go
• auth_test.go
• docs/architecture.md
• docs/history.md
• errors.go
• errors_test.go
• go.mod
• redis.go
• redis_test.go
• test_stdlib_helpers_test.go
• tests/cli/ws/Taskfile.yaml
• ws.go
• ws_bench_test.go
• ws_test.go

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@auth.go`:
- Around line 485-510: The APIKeyAuthenticator remains backwards-incompatible
for callers who use struct literals (setting APIKeyAuthenticator.Keys) because
Authenticate reads only the private keys snapshot; fix this by ensuring the
private snapshot is populated from the exported Keys when unset: in Authenticate
(or a small helper like ensureKeysInitialized) check if a.keys == nil and if so
set a.keys = cloneStringMap(a.Keys) (handling nil Keys as empty map), then
proceed to use a.keys for lookups; reference APIKeyAuthenticator, Keys, keys,
NewAPIKeyAuth and Authenticate to locate where to add this lazy initialization.

In `@redis_test.go`:
- Around line 156-168: The test TestRedisBridge_InvalidPrefix_Ugly should be
guarded to auto-skip when the shared Redis is unreachable: call skipIfNoRedis(t)
at the start of TestRedisBridge_InvalidPrefix_Ugly before invoking
NewRedisBridge, so the test is skipped if redisAddr is not available;
alternatively, replace the external Redis dependency by using the local fake
Redis helper instead of passing Addr: redisAddr to NewRedisBridge to ensure the
invalid-prefix assertion always runs.

In `@ws.go`:
- Around line 1330-1335: The defer that closes the batch writer inside writePump
(the closed flag and defer calling logCloseError("Client.writePump.writer",
w.Close)) leaks writers because defer runs only when writePump returns; remove
that per-iteration defer and instead explicitly close the writer on every
error/exit path after writing (or refactor the batch write loop into a small
helper function so the defer is scoped to a single iteration). Concretely,
update writePump to stop using the per-iteration defer and call w.Close() (with
logCloseError on non-nil errors) immediately after write or on any write error,
or move the code that creates and uses w into a helper function (e.g.,
writeBatch) where the defer for w.Close() is local to that function.
- Around line 1028-1036: The allowedOriginsCheck currently returns
allowAllOriginsCheck when the provided allowedOrigins slice is empty; change
this to return sameOriginCheck (or an explicit deny function) so that an empty
AllowedOrigins does not permit all origins. Locate the functions
allowedOriginsCheck and allowAllOriginsCheck and update the empty-slice branch
to return sameOriginCheck (and remove or restrict use of allowAllOriginsCheck as
the default) so hubs with authentication do not fall back to permissive origin
checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3920b04a-b75d-4df5-9645-c6a2886d933a

📥 Commits

Reviewing files that changed from the base of the PR and between d526e0d and 485f272.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (15)

• .golangci.yml
• auth.go
• auth_test.go
• ax7_v090_test.go
• docs/architecture.md
• docs/index.md
• errors_test.go
• go.mod
• redis.go
• redis_test.go
• test_stdlib_helpers_test.go
• tests/cli/ws/Taskfile.yaml
• ws.go
• ws_bench_test.go
• ws_test.go

💤 Files with no reviewable changes (1)

• docs/index.md

✅ Files skipped from review due to trivial changes (3)

• docs/architecture.md
• tests/cli/ws/Taskfile.yaml
• test_stdlib_helpers_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• errors_test.go
• redis.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Keep APIKeyAuthenticator backwards-compatible with struct literals.

Keys is still exported, but Authenticate now reads only the private keys snapshot. Any caller constructing &APIKeyAuthenticator{Keys: ...} will now silently reject every API key after this change. Either hydrate the private snapshot from Keys when it is unset, or remove the exported field so the broken initialisation path is impossible.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@auth.go` around lines 485 - 510, The APIKeyAuthenticator remains
backwards-incompatible for callers who use struct literals (setting
APIKeyAuthenticator.Keys) because Authenticate reads only the private keys
snapshot; fix this by ensuring the private snapshot is populated from the
exported Keys when unset: in Authenticate (or a small helper like
ensureKeysInitialized) check if a.keys == nil and if so set a.keys =
cloneStringMap(a.Keys) (handling nil Keys as empty map), then proceed to use
a.keys for lookups; reference APIKeyAuthenticator, Keys, keys, NewAPIKeyAuth and
Authenticate to locate where to add this lazy initialization.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Guard this Redis-dependent test with skipIfNoRedis(t).

This case reaches NewRedisBridge with Addr: redisAddr, so it can fail before the invalid-prefix assertion even runs when the shared Redis is unavailable. Add the skip guard here or switch this case to the local fake Redis helper. As per coding guidelines "Redis tests must auto-skip via skipIfNoRedis(t) when Redis at 10.69.69.87:6379 is unreachable".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@redis_test.go` around lines 156 - 168, The test
TestRedisBridge_InvalidPrefix_Ugly should be guarded to auto-skip when the
shared Redis is unreachable: call skipIfNoRedis(t) at the start of
TestRedisBridge_InvalidPrefix_Ugly before invoking NewRedisBridge, so the test
is skipped if redisAddr is not available; alternatively, replace the external
Redis dependency by using the local fake Redis helper instead of passing Addr:
redisAddr to NewRedisBridge to ensure the invalid-prefix assertion always runs.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Do not default origin checks to “allow all”.

When AllowedOrigins is empty, this falls back to allowAllOriginsCheck, so a hub with authentication but no explicit origin policy will still accept cross-site browser upgrades from any origin. That re-opens WebSocket CSRF for cookie-backed sessions and undermines the hardening in this PR. Default this path to sameOriginCheck or explicit deny instead.

Suggested fix

 func allowedOriginsCheck(allowedOrigins []string) func(*http.Request) bool {
 	allowedOrigins = slices.Clone(allowedOrigins)
 	if len(allowedOrigins) == 0 {
-		return allowAllOriginsCheck
+		return sameOriginCheck
 	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	func allowAllOriginsCheck(*http.Request) bool {
	return true
	}
	// Populate auth fields when authentication succeeded.
	if h.config.Authenticator != nil {
	client.UserID = authResult.UserID
	client.Claims = authResult.Claims
	func allowedOriginsCheck(allowedOrigins []string) func(*http.Request) bool {
	allowedOrigins = slices.Clone(allowedOrigins)
	if len(allowedOrigins) == 0 {
	return allowAllOriginsCheck
	}
	func allowAllOriginsCheck(*http.Request) bool {
	return true
	}
	func allowedOriginsCheck(allowedOrigins []string) func(*http.Request) bool {
	allowedOrigins = slices.Clone(allowedOrigins)
	if len(allowedOrigins) == 0 {
	return sameOriginCheck
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ws.go` around lines 1028 - 1036, The allowedOriginsCheck currently returns
allowAllOriginsCheck when the provided allowedOrigins slice is empty; change
this to return sameOriginCheck (or an explicit deny function) so that an empty
AllowedOrigins does not permit all origins. Locate the functions
allowedOriginsCheck and allowAllOriginsCheck and update the empty-slice branch
to return sameOriginCheck (and remove or restrict use of allowAllOriginsCheck as
the default) so hubs with authentication do not fall back to permissive origin
checks.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Remove the per-message defer from writePump.

That defer does not run at the end of the iteration; it runs only when writePump exits. Under sustained traffic this stacks one closure per batch and retains every writer until disconnect. Close the writer explicitly on each error path, or move the batch write into a small helper so any defer is scoped to a single iteration.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ws.go` around lines 1330 - 1335, The defer that closes the batch writer
inside writePump (the closed flag and defer calling
logCloseError("Client.writePump.writer", w.Close)) leaks writers because defer
runs only when writePump returns; remove that per-iteration defer and instead
explicitly close the writer on every error/exit path after writing (or refactor
the batch write loop into a small helper function so the defer is scoped to a
single iteration). Concretely, update writePump to stop using the per-iteration
defer and call w.Close() (with logCloseError on non-nil errors) immediately
after write or on any write error, or move the code that creates and uses w into
a helper function (e.g., writeBatch) where the defer for w.Close() is local to
that function.