fix(channels): hiddenChannels privacy test and #257 follow-ups (#276)

[adminopenclaw8-sketch]

Relates to #276

What

Follow-ups from the round-2 review of #257. Four of the five points were test
gaps — three mutants the merged suite did not catch — plus a semantics
decision and a docs sentence. No production behaviour changes: the diff is
tests, docs and one openapi.go description string.

Plan, point by point

1. Privacy: hiddenChannels must never name a proposal without traffic (#276 point 3)

hiddenChannels is served on the public GET /api/channels, while
proposals themselves are only visible through the authenticated admin route.
The AND EXISTS (… transmissions …) clause in
ListNotApprovedChannelsWithTraffic is what keeps a pending suggestion — a
name anyone can submit — out of that field. Nothing in channels changes
whether the clause is there or not, so only an assertion on hiddenChannels
catches it.

TestHiddenChannelsNeverNamesAProposalWithoutTraffic puts one real hidden
channel (revoked, with stored traffic) next to three proposals with no traffic
— pending, rejected and revoked — and asserts hiddenChannels is exactly the
one with traffic, on the default request and on ?includeEncrypted=true.

2. Cache-safety copy on the default path (#276 point 2)

handleChannels copies the slice only on the includeEncrypted path, which
is the one TestRevokedFilterDoesNotMutateCacheAndKeepsEncrypted exercises.
On a plain request hideRevoked receives GetChannels' cached slice itself,
so compacting in place corrupts the shared cache for every later request.

TestPlainChannelListRequestDoesNotMutateTheCachedSlice gives three channels
distinct first_seen values so GetChannels' last_activity DESC order is
deterministic and the hidden channel sits in the middle (asserted as a
precondition: the test fails loudly if the hidden channel ends up last, where
an in-place compaction would be invisible). It then compares the cached names
before and after three list requests.

3. The client cache TTL hole (#276 point 4)

The node harness in test-channels-client-state-152.js stubbed api() with no
cache and invalidateApiCache() with a no-op, so a page that forgets to
invalidate looked correct. The harness now has an opt-in (apiTtlCache)
per-path TTL cache that mirrors public/app.js: ttl, bust, and a
prefix-wide invalidateApiCache.

Two tests use it, one per entry point into refreshChannelList
(onRevoked/#251 and onApproved/#232). Both cache two /channels variants
(all regions and ?region=CPH), apply the admin decision, then switch the
region back without a bust and inside the 15 s TTL. That is the real
browser path: RegionFilter.onChange calls loadChannels(true) with no bust,
so only invalidateApiCache('/channels') keeps the pre-decision entry from
answering. The bust: true that #243 added covers just the one request
refreshChannelList makes itself.

4. Semantics: an anonymous suggestion can hide a channel the admin never acted on (#276 point 1)

Decision: keep the current rule and document the trade-off. Reason: "hide
only names that were ever approved" needs a marker that survives a
re-suggestion, and the obvious candidate does not exist.
submitChannelProposal resurrects a revoked row with reviewed_at = NULL
(cmd/ingestor/channel_proposals.go) — that reset is what makes a
resubmission idempotent across a crash-and-retry — so a reviewed_at rule
cannot tell a fresh suggestion from a re-suggestion after a revoke. It would
re-open exactly the hole #257 round 2 closed, where anyone could unhide a
revoked channel by suggesting the name again. A real history column is an
ingestor-side schema change plus a write path, out of scope for a test-gap
follow-up, and cmd/server stays read-only.

What bounds the trade-off, and what the new test pins:

• it needs stored payload_type = 5 traffic for that exact name, so a
  never-decrypted channel can never be hidden;
• a name the ingestor still decrypts through its config is never hidden,
  whatever its proposal says;
• the effect is list-level only — the history stays readable at
  GET /api/channels/{hash}/messages, and /api/analytics/channels still
  counts it;
• the administrator sees the suggestion under Pending, and one Approve
  lists the channel again.

TestAnonymousPendingSuggestionHidesAHistoricalConfigChannel walks all four:
#oldcfg (traffic, not in the ingestor's names file) and #chat (traffic, in
the names file) are both listed with no proposal; after an unreviewed pending
row for each, #oldcfg is hidden and named in hiddenChannels while #chat
stays listed; the history still answers; approving lists #oldcfg again.

5. Docs (#276 point 5)

docs/api-spec.md, docs/user-guide/channels.md and cmd/server/openapi.go
now say hiddenChannels is global and not filtered by region, and that
another open tab keeps the set it last loaded until its list reloads —
including after a re-approval, where live messages do not re-create the row in
that tab until then. api-spec.md and channels.md also carry the point-4
trade-off.

Tests

#	Requirement	Test	Mutant → red
1	pending suggestion never in hiddenChannels	TestHiddenChannelsNeverNamesAProposalWithoutTraffic	G1 AND EXISTS (… transmissions …) dropped
2	cache copy on the default path	TestPlainChannelListRequestDoesNotMutateTheCachedSlice	G2 out := resp.Channels (compact in place)
3	invalidateApiCache('/channels')	the two #276 tests in test-channels-client-state-152.js	F3 the call removed from refreshChannelList
4	documented semantics	TestAnonymousPendingSuggestionHidesAHistoricalConfigChannel	S1 p.status = 'revoked' only; S2 built-in exception dropped

G1, G2 and F3 are the three mutants the round-2 review reported as surviving;
each now fails, and each is caught only by the new test for its point.

Runs on this head: cmd/server, cmd/ingestor and
internal/channelregistry vet + go test -count=1 all ok; sh test-all.sh
220/220; node test-frontend-helpers.js 707/707; 16 channel E2Es against a
local Go server on a CI-prepared e2e-fixture.db, plus
test-channel-proposals-e2e.js 26/26 and
test-channels-client-state-152-decrypt-e2e.js 18/18.
scripts/check-xss-sinks.sh --diff origin/master exit 0. No new
map[string]interface{} outside tests, no .github/ change (fork guards
unchanged: 9 and 1).

Remaining

• A real "was ever approved" marker on channel_proposals (ingestor-side
  schema + write path) stays unfiled; this PR documents the trade-off instead.
• A hidden channel still returns when retention prunes its non-approved
  proposal row while messages are stored (documented in fix(channels): hide revoked shared channels from the channel list (#251) #257, needs an
  ingestor change).
• hiddenChannels has no cross-tab push; only documented, not changed.

🤖 Generated with Claude Code

[adminopenclaw8-sketch]

Rapport — CS-Macmini PR#283 #276 — head c0e6508

Status: All five points of #276 are addressed; the three mutants the round-2 review reported as surviving (G1, G2, F3) now each fail, and each is caught only by the new test for its point. Draft, not marked ready, not merged. No production behaviour changes.

Evidence tags: [T] = test or command run in this session on this head. [A] = analysis/reading of code. [K] = taken from #276 or the #257 review, not re-checked here.

Requirements

#	Requirement	Test	Mutant → red	Evidence
1	A pending proposal without traffic is never named in hiddenChannels on the public GET /api/channels	TestHiddenChannelsNeverNamesAProposalWithoutTraffic (cmd/server/channel_revoked_hidden_test.go) — one revoked channel with traffic next to pending/rejected/revoked rows without traffic; hiddenChannels must be exactly [#helloworld], on the default request and on ?includeEncrypted=true	G1 AND EXISTS (… transmissions …) dropped from ListNotApprovedChannelsWithTraffic	G1 over the whole cmd/server suite: the only failure is this test [T]
2	hideRevoked's copy on the default path (no includeEncrypted), where it receives GetChannels' cached slice	TestPlainChannelListRequestDoesNotMutateTheCachedSlice — three channels with distinct first_seen so last_activity DESC is deterministic and the hidden one sits mid-list (asserted as a precondition); the cached names are compared before and after three requests	G2 out := resp.Channels (compacting in place)	G2 over the whole cmd/server suite: the only failure is this test. Diff it reports: before [#test #zulu #helloworld #alpha] → after [#test #zulu #alpha #alpha] [T]
3	invalidateApiCache('/channels') in refreshChannelList, for both onRevoked and onApproved	Two #276 tests in test-channels-client-state-152.js, on a new opt-in harness TTL cache (apiTtlCache) that mirrors public/app.js's api() — per-path ttl, bust, prefix-wide invalidateApiCache. Each caches two /channels variants (all regions, ?region=CPH), applies the admin decision, then switches the region back inside the 15 s TTL without a bust	F3 the invalidateApiCache('/channels') call removed	F3 over the whole sh test-all.sh: 219 passed, 1 file failed — only these two assertions [T]
4	The semantics of point 1 in #276, decided and locked	TestAnonymousPendingSuggestionHidesAHistoricalConfigChannel — #oldcfg (traffic, not in the ingestor's names file) and #chat (traffic, in it) both listed with no proposal; after an unreviewed pending row for each, #oldcfg is hidden and named in hiddenChannels while #chat stays listed; the history still answers total: 1; approving lists #oldcfg again	S1 p.status = 'revoked' only → red (with TestPendingAndRejectedProposalsKeepAChannelHidden); S2 the built-in exception dropped in hiddenNames → red (with two existing tests)	Both mutants run on the channel subset [T]
5	One sentence that hiddenChannels is global, not per region, and that another open tab keeps its old set until it reloads, also after a re-approval	docs/api-spec.md, docs/user-guide/channels.md, cmd/server/openapi.go	—	[A]

Points 1–4 are test gaps, so there is no production change to be "red before". Each test is red under its mutant and green on this head; points 1–3 are additionally shown to be red only by the new test, over the full suite.

Point 4: the decision

Kept the current rule, documented the trade-off. "Hide only names that were ever approved" needs a marker that survives a re-suggestion, and the obvious candidate does not exist: submitChannelProposal resurrects a revoked row with reviewed_at = NULL (cmd/ingestor/channel_proposals.go:126), and that reset is what makes a resubmission idempotent across a crash-and-retry. A reviewed_at rule therefore cannot tell a fresh anonymous suggestion from a re-suggestion after a revoke, and would re-open exactly the hole #257 round 2 closed — anyone unhiding a revoked channel by suggesting the name again [A]. A real history column is an ingestor-side schema change plus a write path, out of scope for a test-gap follow-up, and cmd/server stays read-only.

What bounds the trade-off, each pinned by the new test: it needs stored payload_type = 5 traffic for that exact name, so a never-decrypted channel can never be hidden; a name the ingestor still decrypts through its config is never hidden; the effect is list-level only (the history stays readable, /api/analytics/channels still counts it); and the administrator sees the suggestion under Pending, where one Approve lists the channel again [T][A].

Tests on this head

• cmd/server: go vet ./... + go test -count=1 ./... ok (38 s) [T]
• cmd/ingestor: go vet ./... + go test -count=1 -timeout 20m ./... ok (105 s) [T]
• internal/channelregistry: vet + test ok [T]
• gofmt -l on the touched Go files: clean [T]
• sh test-all.sh: 220 passed, 0 failed (220 files) [T]
• node test-frontend-helpers.js: 707 passed, 0 failed [T]
• E2E against a local Go server on a CI-prepared e2e-fixture.db (freshen, the two seed rows from the workflow, corescope-migrate, seeds 2073/199/245). All 16 passed: channel 1087, 1111, client-state-152, 154-155, fluid, decrypt, qr, color-picker, list-render, selection-flow, add-modal, share-color, ws-batch, ws-race-1498, observed-path-hash-size, modal [T]
• Self-started stacks: test-channel-proposals-e2e.js 26/26, test-channels-client-state-152-decrypt-e2e.js 18/18 [T]
• The server was stopped by the pid of its port; the fixture DB and the build artifacts were restored, and the tree is clean [T]

Rules

• cmd/server stays read-only: the only non-test change there is one openapi.go description string; readonly_invariant_test.go and readonly_sql_literal_test.go are in the green suite [T][A]
• No new map[string]interface{} outside tests: git diff -U0 origin/master -- ':(exclude)*_test.go' ':(exclude)test-*.js' gives 0 added lines [T]
• No hardcoded colours: the diff adds no CSS and no colour value [A]
• scripts/check-xss-sinks.sh --diff origin/master: exit 0 (no public/** change to scan) [T]
• Fork guards unchanged: 9 repository == in deploy.yml, 1 in release-fast-path.yml; no .github/ change in this PR [T]
• No closing keywords; the PR body and the commit both say "Relates to Follow-ups to #257: suggestion-hides-unapproved edge, hiddenChannels privacy test, cache-copy and TTL test gaps #276" [T]
• The commit is authored and committed by dborup <kontakt@meshview.dk> [T]

CI

Run 37398802504 on head c0e65083, per job [T]:

Job	Result
✅ Go Build & Test	pass (15m17s)
🎭 Playwright E2E Tests	pass (26m22s)
🏗️ Build & Publish Docker Image	pass (57s)
📦 Release Artifacts	skipped (not a release)
🚀 Deploy Staging	skipped (draft, no deploy)
📝 Publish Badges & Summary	skipped

Green first time: no job was re-run, and the known flaky test (#271) did not fire.

Remaining

• A real "was ever approved" marker on channel_proposals (ingestor-side schema + write path) is not filed as an issue; this PR documents the trade-off instead.
• A hidden channel still returns when retention prunes its non-approved proposal row while messages are stored — documented in fix(channels): hide revoked shared channels from the channel list (#251) #257, needs an ingestor change [K].
• hiddenChannels has no cross-tab push; now documented, not changed.
• The harness TTL cache is opt-in, so the other tests in test-channels-client-state-152.js keep the old cache-free api(). Other pages that call invalidateApiCache are still uncovered.

Not verified

• A browser check of the multi-tab behaviour the new docs sentence describes; this is code reading plus the harness tests only.
• Point 4 end to end through the real submit endpoint and ingestor: the test inserts the row submitChannelProposal writes, as the round-2 review's probe did [A][K].
• BenchmarkHideRevoked was not re-run; this PR does not touch hideRevoked.
• Staging or production behaviour (out of scope).

[dborup-agent]

Review — CS-MacBook PR#283 — head c0e6508

Dom: APPROVE

Independent, read-only review of a merged tree (git merge-tree origin/master <head>, base 4f1de049) extracted to scratch. I re-derived every requirement test, re-ran all five claimed mutants, added my own edge-case test, and ran the full suites. The diff is tests, docs and one openapi.go description string; no production behaviour changes.

Evidence tags: [T] run this session on the merged tree · [A] code/diff reading · [K] from #276 / the #257 round-2 review, not re-derived.

Findings

#	Sev	Finding
—	—	No blocking or correctness issues found.
N1	note	The opt-in apiTtlCache harness is used only by the two new tests; the other invalidateApiCache callers (nodes.js, /stats, /nodes) stay uncovered. Author acknowledges this as remaining work — acceptable for a test-gap follow-up. [A]
N2	note	test-all.sh reports 222/222 on the merged tree, not the 220 in the PR body — master gained 2 test files since the author's run. Not a defect; just a stale count. [T]
N3	note	Point 4's hide-a-historical-config-channel behaviour is a deliberately kept trade-off (documented, locked by a test), not a fix. Rationale in the PR is sound: reviewed_at is reset on re-suggestion, so a "was ever approved" marker needs an ingestor-side schema+write — out of scope, and cmd/server stays read-only. [A][K]

Requirements (each acceptance criterion: test red-before / green-after)

#276	Requirement	Test	Mutant I ran → red	Verdict
1	pending/rejected/revoked proposal without traffic is never in hiddenChannels on public GET /api/channels	TestHiddenChannelsNeverNamesAProposalWithoutTraffic	G1 dropped AND EXISTS (… transmissions …) in ListNotApprovedChannelsWithTraffic → hiddenChannels=[#helloworld #pendingsecret #rejectedsecret #revokednotraffic]; over the whole cmd/server suite only this test fails	✅ [T]
2	cache-safety copy on the default path (hideRevoked receives GetChannels' cached slice)	TestPlainChannelListRequestDoesNotMutateTheCachedSlice	G2 out := resp.Channels (compact in place) → cached slice became [#test #zulu #alpha #alpha]; over the whole suite only this test fails	✅ [T]
3	invalidateApiCache('/channels') in refreshChannelList for both onRevoked (#251) and onApproved (#232)	the two #276 tests in test-channels-client-state-152.js	F3 removed the call → 70 passed, 2 failed, exactly the two new tests	✅ [T]
4	semantics decided & locked	TestAnonymousPendingSuggestionHidesAHistoricalConfigChannel	S1 p.status = 'revoked' → #oldcfg wrongly listed; S2 builtin exception dropped in hiddenNames → #chat wrongly hidden. Each makes the test fail	✅ [T]
5	docs: hiddenChannels is global, not per-region; another tab keeps its set until reload, also after re-approval	docs/api-spec.md, docs/user-guide/channels.md, cmd/server/openapi.go	— (docs)	✅ [A]

Points 1–4 are test gaps, so there is no production line to be "red before"; each test is green on this head and red under its mutant, and I confirmed G1/G2/F3 are each caught only by their new test over the full suite.

Mutants (all run by me on the merged tree)

1. G1 — drop the EXISTS (… payload_type=5 … channel_hash=p.name) clause → point-1 test red, rest of cmd/server green. [T]
2. G2 — out := resp.Channels instead of the append(…[:0:0]…) copy → point-2 test red, rest green; observed duplicate #alpha proving in-place corruption of the shared cache. [T]
3. F3 — remove invalidateApiCache('/channels') from refreshChannelList → the two new JS tests red, other 70 green. [T]
4. S1 — p.status <> 'approved' → p.status = 'revoked' → point-4 test red. [T]
5. S2 — drop the if !builtin[n] exception in hiddenNames → point-4 test red (#chat hidden). [T]

All mutants reverted; the merged tree was diffed back to the head archive for channel_proposals.go, sql.go, routes.go (identical). [T]

My own edge case (not in the PR)

TestEdgeLonePendingSuggestionWithoutTrafficProducesNoHiddenChannels — the point-1 test always keeps one real hidden channel present. I added the fresh-suggestion case where the only not-approved proposal is a traffic-less pending suggestion and no real hidden channel exists (exercises hideRevoked's len(snap.hidden)==0 early return / nil HiddenChannels). Green on this head, and red under G1 (hiddenChannels=[#brandnewsecret]). The privacy guarantee holds in this path too. [T]

I also checked the consistency the privacy fix depends on: channelsSQL lists channels on payload_type = 5 and the hide query gates on the same payload_type = 5, so a channel the list shows is matched by the hide query — no revoked channel can leak past hideRevoked due to a criterion mismatch. [A]

Tests on the merged tree (base 4f1de04)

• cmd/server: go vet . clean + go test -count=1 . ok (40.8s), full suite green incl. readonly_invariant_test / readonly_sql_literal_test. [T]
• cmd/ingestor: vet + go test -count=1 ./... ok (108s). [T]
• internal/channelregistry: vet + test ok. [T]
• sh test-all.sh: 222 passed, 0 failed. [T]
• node test-frontend-helpers.js: 709 passed, 0 failed. [T]
• The three new Go tests and the two new JS tests: green on head, red under their mutants (above). [T]

Scope / invariants

• cmd/server read-only: only non-test change is one openapi.go Description string (confirmed no non-string line added); read-only invariant tests pass. [T][A]
• No new map[string]interface{} outside tests: git diff -U0 origin/master…head excluding *_test.go/test-*.js adds 0. [T]
• No hardcoded colours: diff adds no hex/rgb/hsl. [T]
• scripts/check-xss-sinks.sh --diff origin/master: the --diff gate scans added lines in public/**/*.{js,html} only; the PR changes no public/ file (the edited test-channels-client-state-152.js is at repo root), so the gate has nothing to scan → clean. [A] (author reports exit 0 [K])
• Fork guards unchanged: repository == is 9 in deploy.yml, 1 in release-fast-path.yml; no .github/ change in the diff. [T]
• No closing keywords; PR body and commit both say "Relates to Follow-ups to #257: suggestion-hides-unapproved edge, hiddenChannels privacy test, cache-copy and TTL test gaps #276". [T]
• Author and committer: dborup <kontakt@meshview.dk>. [T]
• git ls-remote head identical before and after the review: c0e65083. [T]

Not verified

• The 16 browser E2Es were not re-run locally; CI Playwright is green on this exact head [A], and the PR makes no production behaviour change (the merge pulls in master's unrelated sender-path-hash work in channels.js, which is separately tested and green across the suites above), so local E2E adds little over CI. The affected client-state harness (test-channels-client-state-152.js) was run directly. [T][A]
• Multi-tab/cross-tab behaviour the new docs sentence describes: code + harness reading only, no live browser. [A]
• Point 4 through the real submit endpoint + ingestor write path: the test inserts the row submitChannelProposal writes, not driven end-to-end. [A][K]
• Staging / production (out of scope).

I ran this review from an isolated scratch copy; I did not touch the PR, other worktrees, or any remote.