fix(channels): hide revoked shared channels from the channel list (#251)

[dborup]

Relates to #251

What

An administrator who revokes a shared hashtag channel expects it to leave the channel list. Until now it stayed, because already-decoded messages keep the channel in the list query.

• GET /api/channels now leaves out channels with stored messages whose proposal is not approved (revoked, re-suggested and pending, or rejected), and names them in hiddenChannels so live WebSocket packets do not re-create their rows. Nothing is deleted: GET /api/channels/{hash}/messages still returns the history, and the channel comes back (with its history) when the proposal is approved again. cmd/server stays read-only; it only filters.
• A name the ingestor decrypts through its built-in/config list (built-in keys, rainbow table, hashChannels, channelKeys) is never hidden. If the ingestor's builtin-channels.json is missing or unreadable, nothing is hidden (fail-open).
• Case of proposal names is decided from source: the hashtag key is sha256("#name")[:16] of the exact bytes (MeshCore docs/companion_protocol.md "Hashtag Channels"; meshcore-open derivePskFromHashtag does not change case), so #HelloWorld and #helloworld are different channels with different keys. They stay separate proposals; the admin list now marks case near-duplicates (nearDuplicateOf, also vs. built-in names) and the review dialog shows "Same name in different case".
• Frontend: an admin revoke now refreshes the channel list (same path as approval). An open conversation on a revoked channel is closed by the existing reconcile step.

Performance

No per-request DB scan. The revoked set is read with one extra bounded query (status = 'revoked', capped at 1024) in the existing 10 s approved-names snapshot refresh, and the hidden set (revoked − approved − built-in) is built once per refresh. Per request the filter is O(channels) map lookups on a copy; with nothing revoked it returns the cached slice untouched. BenchmarkVisibleChannels (5000 channels, 3 revoked): ~80 µs/op, 1 alloc (one copy of the list, only when something is revoked). TestRevokedSetIsServedFromTheSnapshot drops the table and checks the list is still answered from the snapshot.

Tests

• Go (server): hidden after revoke (history still readable), hidden within the TTL once a revoke result is read, re-approval lists it again with its history, built-in never hidden, fail-open without/with a corrupt names file, only the exact name hidden, cached slice not mutated and includeEncrypted unaffected, no-op without revoked rows, missing table, ListRevokedNames, near-duplicate marking incl. status filter, benchmark.
• Go (ingestor): key derivation is case-sensitive (firmware vector for #test); the two case variants are two proposals with two keys.
• Node: near-duplicate rendering (escaped), onRevoked wiring, a revoked row is not carried over by the client merge and closes an open conversation.
• E2E (test-channel-proposals-e2e.js): a stored message on the shared channel; after the revoke the channel is gone from /api/channels, history readable, still gone after a restart.
• Mutants run and caught: filter removed, built-in channel hidden, re-approval ignored, case folded.

Docs

cmd/server/openapi.go, docs/api-spec.md, docs/user-guide/channels.md, docs/user-guide/configuration.md.

Known limits

• Revoked, re-suggested (pending) and rejected proposals all keep a channel with stored messages hidden; only an approval lists it again (changed after review, see the follow-up comment).
• Non-approved rows are pruned by retentionDays; a hidden channel with surviving messages then reappears.
• /api/analytics/channels is unchanged and still counts these channels (documented).
• Another open tab keeps a revoked channel until its list reloads (no periodic reload or cross-tab push).

🤖 Generated with Claude Code

[dborup]

Rapport — CS-MacBook PR#257 #251 — head 02b99c4

Status: Implemented and locally verified; draft PR open, CI pending (not polled); not merged, not marked ready, issue not closed.

Evidence tags: [T] = test or command run in this session, [A] = analysis/reading of code or source, [K] = taken from existing knowledge or the repo's own docs, not re-checked here.

Case handling — decided from source

• Firmware docs/companion_protocol.md:439-444 (MeshCore at a366955): "It is the first 16 bytes of sha256("#test")", with no case folding [A]. The firmware's own hashtag key code (src/helpers/TransportKeyStore.cpp:44-47) hashes the exact name bytes [A].
• meshcore-open lib/models/channel.dart:78-85, derivePskFromHashtag: sha256(utf8('#name'))[:16], no lowercasing; its caller (lib/screens/channels_screen.dart:1229-1252) only trims and adds # [A].
• meshcore-cli help (meshcore_cli.py:4794): it "does not lowercase", and warns that the Android app and Ripple do. I could not read those apps' source [K].
• Decision: different case means different bytes, a different key, a different channel. Names are not normalised; the admin list marks near-duplicates instead (nearDuplicateOf). This matches the existing NormalizeName contract, so the direction did not change.

Acceptance criteria

Criterion	Test	Mutant
Revoked channel leaves GET /api/channels although history exists; history still readable	TestRevokedChannelIsHiddenFromList, TestRevokeHidesChannelWithoutWaitingForTTL; E2E "#251: the revoked channel leaves GET /api/channels…" and the restart step [T]	filter removed: 6 Go tests fail [T]
Re-approval shows it again with its history	TestReapprovedChannelIsListedAgain [T]	re-approval ignored (approved rows counted as revoked, defensive approved-subtraction removed): 5 tests fail incl. TestReapprovedChannelIsListedAgain [T]. With only the SQL change, the server-level tests stay green because hiddenNames also subtracts approved names; only TestListRevokedNames fails [T]
Built-in/config-decrypted name is never hidden	TestBuiltinChannelIsNeverHiddenByRevokedProposal; fail-open: TestRevokedChannelIsNotHiddenWithoutBuiltinNames (missing and corrupt file) [T]	built-in hidden: TestBuiltinChannelIsNeverHiddenByRevokedProposal fails [T]
Case handling decided from source and pinned	TestHashtagKeyDerivationIsCaseSensitive (ingestor, firmware #test vector), TestAdminListFlagsCaseNearDuplicates, TestRevokedChannelHidesOnlyTheExactName, existing NormalizeName case tests [T]	case folded in NormalizeName: 2 channelregistry tests and 2 ingestor tests fail [T]
API docs describe the behaviour	openapi.go (3 routes), docs/api-spec.md, user guide pages; TestOpenAPICompleteness is in the green server suite [T]	n/a
cmd/server read-only, no new map[string]interface{}	Only a SELECT was added; no new map[string]interface{} type or literal introduced (the list type is the existing ChannelListResponse.Channels) [A]	n/a

All four mutants were applied one at a time and reverted; the working tree was clean afterwards [T].

Performance

No per-request DB scan. The revoked names are read with one extra bounded query (capped at 1024) inside the existing 10 s approved-names snapshot refresh, and the hidden set is built once per refresh. Per request the filter is O(channels) map lookups on a copy; with nothing revoked it returns the cached slice untouched. BenchmarkVisibleChannels, 5000 channels with 3 revoked: ~80 µs/op, 1 alloc/op [T] (one copy of the list per request, only when something is revoked). TestRevokedSetIsServedFromTheSnapshot drops the proposals table and still gets the list from the snapshot [T]. Frontend: no per-item API calls, no hardcoded colours (new class reuses var(--warning)), near-duplicate note is escaped text [A].

Follow-up commit 02b99c4

The first commit (0e0ff5e) added two mentions of map[string]interface{} in a new function signature. 02b99c4 reshapes the filter to work on ChannelListResponse so the non-test diff adds none (git diff -U0 grep: 0) [T]. After it: full cmd/server suite green, the filter-removed mutant re-run and caught by 6 tests, test-channel-proposals-e2e.js 26/26 on a rebuilt server [T]. The other checks below ran on 0e0ff5e; the refactor touches only channel_proposals.go, routes.go and the new Go test file, and I did not re-run them (ingestor, test-all.sh, frontend helpers, XSS gate, the other 17 channel E2Es) [A].

Verification run (base 9989b97)

• cd cmd/server && go vet ./... && go test ./... passed; cd cmd/ingestor && go vet ./... && go test ./... passed; internal/channelregistry vet and tests passed [T].
• gofmt -l on the touched Go files is clean [T].
• sh test-all.sh: 218 files, 0 failed [T]. node test-frontend-helpers.js: 707 passed, 0 failed [T].
• scripts/check-xss-sinks.sh --diff exit 0, and --file on public/channel-proposals.js and public/channels.js exit 0 [T].
• E2E against a local Go server on e2e-fixture.db (migrated with corescope-migrate): test-channel-proposals-e2e.js 26/26 [T]; test-channels-client-state-152-decrypt-e2e.js 18/18 [T]; the other 16 channel E2Es that CI runs (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) all passed [T]. The local server was stopped by pid lookup, not $!.
• Merge against origin/master (310c501, which contains d05b0db): git merge-tree is clean [T]. The tests above ran on the 9989b97 base, not on the merged tree [A].
• Fork guards: no file under .github/ changed in this PR [A]. I did not re-count the 9 and 1 separately.

CI

Pending, not polled. Known flaky: #244 and #250.

Remaining / known limits

• A re-suggested name is pending, not revoked, so the channel reappears in the list at that point, before it is approved again.
• When retention prunes the revoked row, a hidden channel with surviving messages reappears.
• /api/analytics/channels still counts revoked channels (out of scope of channels: hide revoked shared channels from the channel list (historical messages keep them visible), and decide case handling of proposal names #251).
• The E2E inserts the history row with the sqlite3 CLI, as the CI workflow already does for its fixture seeds [A].

[dborup]

Reviewed commit 02b99c45e9be6a63fc5d8292b8353dad006c24e0. I found three reproducible P2 issues that should be addressed before merging.

1. [P2] Apply the admin status filter before limiting the result

cmd/server/channel_proposals.go:495–508

The new all-status query applies the default limit of 484 before filtering for the requested status. With one older approved proposal and 484 newer rejected proposals, GET /api/admin/channel-proposals?status=approved returns an empty list although the approved channel is still active. The administrator therefore loses access to its Remove action. The same problem can hide older pending proposals.

I reproduced this with the real handler and existing test fixture. The previous status-filtered SQL query returns the approved row; the new endpoint returns zero rows. Preserve the filtered row query and gather cross-status case-duplicate metadata separately, so that the hint cannot remove actionable proposals from the selected view.

2. [P2] Keep revoked channels hidden when old decoded transmissions arrive over WebSocket

public/channels.js:2017–2019, with the row creation at 1785–1797.

After a revoke, the new REST refresh correctly removes the channel and closes its conversation. However, if a previously decoded transmission is subsequently received through a new observer or path, the ingestor retains its stored decoded_json, and the server broadcasts that old CHAN payload for the new observation. processWSBatch then creates the channel row again even though /api/channels continues to exclude it. No locally stored channel key is needed.

Reproduced using the production frontend from this exact commit and the server's actual type: "packet" envelope:

local revoke: channelStillListed=false, selectedHash=null
after old CHAN WebSocket observation: channelStillListed=true

Apply the revoked-channel visibility policy to live channel-list updates as well. This can be done without deleting historical messages or suppressing the underlying packet observation globally.

3. [P2] Do not let the revoked-name snapshot cap restore still-revoked channels

internal/channelregistry/sql.go:59–62

ListRevokedNames returns only the 1,024 most recently revoked names. Rejected/revoked rows have time-based retention, but there is no corresponding 1,024-row cap on the table. After 1,024 later revocations, an older revoked name falls out of hidden, and its stored messages make it appear in /api/channels again while its proposal remains revoked and is still within retention.

A deterministic handler test confirms that the old channel is initially hidden and then reappears after adding 1,024 newer revoked rows and expiring the snapshot. This differs from the explicitly documented pending-resubmission and retention behavior. Use a bounded approach that preserves the decision for every channel being listed, rather than silently treating an omitted revoked row as visible.

Documentation note

The user guide promises that other open conversations close within about 15 seconds. The Channels page currently has no periodic channel-list reload or cross-tab revoke notification: its interval only updates relative timestamps. Running 60 interval callbacks produced zero new /channels requests, and another tab's selected channel remained open. Either implement the propagation or describe the actual next-refresh behavior.

Verification

• Existing targeted revoked-channel Go tests passed.
• test-channel-proposals.js: 39 passed, 0 failed.
• test-channels-client-state-152.js: 66 passed, 0 failed.
• The three additional review probes reproduced the issues above.
• Pending re-suggestion and retention restoring visibility are explicitly documented policy choices and are not included as findings here.

[dborup-agent]

Review — CS-pve-agent3 PR#257 hide-revoked — head 02b99c4

Dom: REQUEST CHANGES

Independent, read-only review. Evidence tags: [T] = test, probe or command run in this review; [A] = analysis/reading of code or source; [K] = taken from the PR, the author's report or existing knowledge, not re-checked here.

The core filter is well built. It hides the right names, never hides built-in or config names, fails open, rides on the snapshot, and copies instead of mutating the cache. Every mutant I ran was caught. Two problems block approval: a regression in the admin list that this PR introduces, and a re-suggest/reject path that brings a revoked channel back permanently.

Findings

#	Severity	Finding	Evidence
1	Major (regression)	handleAdminChannelProposals now reads all statuses with the old per-status limit (MaxPending+MaxApproved+MaxQueuedRequests = 484 by default) and filters by status in Go. Proposals are ordered created_at DESC. Once 484 newer rows exist in any status, older proposals of the requested status disappear from ?status=…, for example rejected spam inside the 30-day retention window, which takes about 24 h at the default 20 submissions/h. The UI always sends ?status= (public/channel-proposals.js:405), so the Approved tab can lose old approved channels, and the admin can no longer revoke them from the UI.	Probe TestReviewProbeApprovedTabLosesOldApproved: 1 approved row + 484 newer rejected rows. ?status=approved returns 0 on the PR and 1 on origin/master [T]
2	Major	A revoked channel can be brought back by anyone, and the admin cannot hide it again. An anonymous visitor re-suggests the revoked name (when suggestions are open), which makes the row revoked → pending. The channel then returns to the list with its history; this part is documented. If the admin rejects that re-suggestion, the status becomes rejected, which is never hidden. The channel stays listed until packet retention removes its messages. The only way to hide it again is approve + revoke, which turns decryption back on in between. This path is not documented.	Probe TestReviewProbeResuggestThenRejectUnhides: listed after pending, still listed after reject [T]. Resurrect path cmd/ingestor/channel_proposals.go submitChannelProposal (UPDATE … SET status='pending'), reject = pending → rejected [A]
3	Minor (docs)	docs/user-guide/channels.md says an open conversation "closes at the next list refresh (within about 15 seconds…)". channels.js has no periodic list reload. loadChannels() runs on page init, region change, the encrypted toggle and onApproved/onRevoked; 15 s is only the client cache TTL. In a non-admin tab the revoked channel and its open conversation stayed for 35 s+. They closed only after a list reload.	Live check on a local server: after a revoke, still listed and open at +35 s. After a reload, hash #/channels and "Select a channel" [T]
4	Minor (UX)	A deep link to a hidden channel (#/channels/%23bot) opens the conversation with its full history, but the header reads "Channel #bot — 0 messages" because the list entry is missing. This fits "history stays readable", but the header contradicts the 14 messages shown. Probably the existing behaviour for any unlisted hash; I did not check that on master.	Chromium against a local server: 14 messages rendered, header "0 messages" [T]
5	Nit	Firmware citation: src/helpers/TransportKeyStore.cpp:44-47 is getAutoKeyFor, the region/flood-scope transport key (called from RegionMap.cpp), not a channel PSK. The firmware has no code that derives a channel key from a name; apps do that. docs/companion_protocol.md:439-441 is the correct source, and the code comments cite only that. The conclusion stands.	Read MeshCore at a366955 [A]; sha256("#test")[:16] = 9cd8fcf22a47333b591d96a2b848b73f recomputed [T]
6	Nit / follow-up	/api/analytics/channels still lists revoked names with message and sender counts, but no text. Decoded text stays reachable through packet views and the messages endpoint. That is acceptable, because hiding is a list-level measure and not a confidentiality control, but consider saying so in the docs.	Response shape checked: {name, hash, messages, senders, lastActivity, encrypted} [T]
7	Nit	WriteBuiltinNames truncates to 4096 names, sorted. An operator with more config names than that could see a config-decrypted name hidden. Unrealistic today (rainbow table is about 320 names).	[A]
8	Nit	BenchmarkVisibleChannels and TestVisibleChannelsIsNoOp… still use the pre-refactor name. The near-duplicate index shares the 484 cap from finding 1, so very old rows can miss the hint.	[A]

Suggested direction for 1: keep the status-filtered ListProposals(ctx, db, status, limit) for the rows returned. Build the near-duplicate index from a separate bounded name-only read, e.g. all names capped at MaxListed. Add the probe above as a regression test.

Suggested direction for 2 (author's call): hide every non-approved proposal name that is not built-in, i.e. hidden = (revoked ∪ rejected ∪ pending) − approved − builtin. Only a channel that was decrypted at some point can have list entries. A pending or rejected name with history therefore means "was approved before" or "was config-decrypted before". This also removes the documented "reappears while pending" limit. If you keep the current semantics, at least document the reject path and how an admin can hide the channel again.

1. Hide on revoke

• GET /api/channels leaves out revoked names that have stored history, and /api/channels/{hash}/messages still returns that history. Covered by unit tests, by the E2E including after a restart, and by my live check, where the hidden #bot still answered with total: 14 [T].
• cmd/server stays read-only. The only addition is ListRevokedNames, a bounded SELECT. No write paths, and readonly_invariant_test.go is in the green suite [T][A].
• The revoked set rides on the existing approved snapshot. readProposalNames runs both queries once per TTL, hiddenNames is built once per refresh, and noteDecision drops the snapshot early. TestRevokedSetIsServedFromTheSnapshot drops the table and still answers. Per request: O(channels) map lookups on a copy, and none at all with nothing revoked [T][A].

2. Built-in channels are never hidden

• hidden = revoked − approved − builtin. The built-in set is the ingestor's BaseNames(): built-in keys, rainbow table (including CHANNEL_KEYS_PATH), hashChannels and channelKeys. The proposal runner always starts and publishes it, even with suggestions disabled [A].
• Fail-open: a missing file gives os.Stat → ok=false, and a corrupt file gives a parse error → ok=false. Either way nothing is hidden. Tested by the unit tests and live: removing or corrupting the file brought #bot back within one TTL, and restoring it hid #bot again [T].
• Other "known" sources: names without # (Public, PSK channels) cannot be proposed, because NormalizeName prefixes #. Browser-side PSK rows are not in the server list and are kept by the client merge [A][K]. The server holds no channel keys of its own [A]. I found no source the author missed. The only gap is the 4096 cap (finding 7).

3. Re-approval and the author's leftovers

• Re-approval lists the channel again with its history (TestReapprovedChannelIsListedAgain; mutant caught) [T].
• Re-suggest → pending → visible: I do not think this is acceptable as is. It lets any anonymous visitor undo an admin action. Together with reject the effect becomes permanent (finding 2). It is not a confidentiality issue, because history is readable by name anyway, but it defeats the purpose of channels: hide revoked shared channels from the channel list (historical messages keep them visible), and decide case handling of proposal names #251.
• Retention prunes the revoked row → visible again: acceptable as a documented limit. Fixing it needs an ingestor-side change, such as not pruning revoked rows while messages exist; that belongs in a follow-up.
• /api/analytics/channels: counts only, no text. Not a leak that matters; a follow-up at most (finding 6).

4. Letter case

• The firmware doc quote is verified (docs/companion_protocol.md:439-441, sha256("#test"), vector recomputed). The TransportKeyStore citation is about region keys (finding 5). No normalisation is the right call [A][T].
• The decision is locked in by tests: TestHashtagKeyDerivationIsCaseSensitive (ingestor), TestAdminListFlagsCaseNearDuplicates, TestRevokedChannelHidesOnlyTheExactName and the existing NormalizeName tests. Mutants M4a–c were all caught [T].
• nearDuplicateOf exists only on AdminChannelProposal, served by the authenticated admin route. ChannelProposalRequestResponse (public) does not carry it, and the frontend escapes each name [A]. Removing the escape fails test-channel-proposals.js [T].

5. Frontend

• An admin revoke calls onRevoked → refreshChannelList(), the same path as approval. Removing the call fails test-channel-proposals.js, and unwiring it in channels.js fails test-channels-client-state-152.js [T].
• An open conversation closes through reconcileSelectionAfterChannelRefresh(), but only when the list reloads (findings 3 and 4) [T].
• No hardcoded colours: .ch-proposals-neardup uses var(--warning). No per-item API calls. scripts/check-xss-sinks.sh --diff origin/master on head: exit 0. --file on both JS files: exit 0 [T].

6. Documentation

openapi.go, docs/api-spec.md, docs/user-guide/channels.md and configuration.md all say that revoke hides the channel without deleting messages, list the exception for built-in names and describe the retention behaviour [A]. Corrections needed: the "15 seconds" sentence (finding 3) and the reject path (finding 2).

7. Rules

• No new map[string]interface{} in non-test code: git diff -U0 … ':(exclude)*_test.go' gives 0 added lines. The new test file uses it only for fixtures [T].
• Fork guards: 9 in deploy.yml and 1 in release-fast-path.yml, on head and on both merged trees. No .github/ change in this PR [T].
• No closing keywords. The PR body and both commits say "Relates to channels: hide revoked shared channels from the channel list (historical messages keep them visible), and decide case handling of proposal names #251" [T].
• Both commits are authored and committed by dborup <kontakt@meshview.dk> [T].

Tests (merged tree = origin/master 310c501 + head, tree d2adc360)

• cmd/server: go vet + go test -count=1 ./... ok (510 s) [T].
• cmd/ingestor: go test -count=1 -timeout 20m ./... ok (783 s). A first run with Go's default 10 m timeout timed out while E2Es were running in parallel. CI uses 20 m [T].
• internal/channelregistry: vet + test ok [T].
• sh test-all.sh: 218 passed, 0 failed. node test-frontend-helpers.js: 707 passed, 0 failed [T].
• test-channel-proposals-e2e.js, using its own server and ingestor with the binaries built into the repo root: 26/26 [T].
• The 17 other channel E2Es CI runs, against a local Go server on e2e-fixture.db prepared as in CI (freshen, seed SQL, corescope-migrate, seeds 2073/199): all passed. Covered: color-picker, decrypt, fluid, 1087, 1111, modal, qr, 154-155, add-modal, 152-decrypt, 152, list-render, observed-path-hash-size, selection-flow, share-color, ws-batch, ws-race-1498. Servers were stopped by pid looked up from the port [T].
• Master moved to 0572e7f9 during the review. git merge-tree is still clean, and its new commits touch none of this PR's files [T]. I did not re-run on that tree.

Mutants (each on a fresh copy of the merged tree, restored afterwards) [T]:

Mutant	Result
M1 filter removed (hideRevoked returns early)	caught: 6 server tests fail
M2 built-in channel hidden	caught: TestBuiltinChannelIsNeverHiddenByRevokedProposal
M3 re-approval ignored (approved names added to the hidden set)	caught: 4 tests incl. TestReapprovedChannelIsListedAgain
M3b defensive keep subtraction removed on its own	survives. Expected: the UNIQUE name makes it unreachable, as the author notes
M4a hiding made case-insensitive	caught: TestRevokedChannelHidesOnlyTheExactName
M4b near-duplicate fold made case-sensitive	caught: TestAdminListFlagsCaseNearDuplicates
M4c NormalizeName folds case	caught: 1 channelregistry, 7 ingestor, 1 server test
F1 onRevoked call removed / F2 unwired in channels.js / F3 near-dup escape removed	caught by test-channel-proposals.js / test-channels-client-state-152.js / test-channel-proposals.js

CI (run 37316732571 on head): Go Build & Test ✅. Playwright E2E ❌ in test-issue-180-packets-url-modal-e2e.js: "Clear Filters on #/packets/?… reload shows 3 packets, Clear showed 5". This is neither #250 nor #256. It does not touch channel code, and it passed 2/2 locally against the merged build. It looks like a time-window flake, but it is not on the known list. Because the step is fail-fast, the E2Es after it did not run in CI, including test-channel-proposals-e2e.js. They are covered by my local run above [T].

Not verified

• Behaviour on the new master 0572e7f9 (only the merge was checked).
• Whether finding 4 (the "0 messages" header) also happens on master for other unlisted hashes.
• The "Known channels (catalogue)" section of the channel list with a revoked name.
• Android/Ripple lowercasing claims from the meshcore-cli help text [K]; meshcore-open citations [K].
• The author's benchmark numbers; I did not run BenchmarkVisibleChannels.
• Browser validation on a staging or production instance (out of scope).

[dborup]

Review feedback addressed (commit 9c80787e)

Status: all findings of both reviews on head 02b99c4 are addressed; branch merged with origin/master 0572e7f (18dfb6b1, no conflicts) and pushed to the same branch, no rebase or force-push. Draft, not merged, not marked ready. CI pending.

Evidence tags: [T] = test or command run in this session, [A] = analysis/reading of code or source, [K] = taken from a review or the repo's docs, not re-checked here.

Findings

1. Admin list regression (both reviews). GET /api/admin/channel-proposals is back on the status-filtered ListProposals(ctx, db, status, limit), so the filter is applied in SQL before the 484-row limit. The case near-duplicate hint now reads a separate name-only list (ListAllNames, capped at MaxListed). Regression test TestAdminStatusFilterIsAppliedBeforeTheLimit: 1 old approved + 484 newer rejected → ?status=approved returns 1, ?status=rejected returns 484. TestNearDuplicateHintSeesProposalsBeyondTheRowLimit pins that the hint still sees an old case variant [T].
2. Re-suggest → pending → reject made a revoked channel visible. I took the reviewer's direction: a channel with stored messages is hidden when its proposal is not approved (revoked, re-suggested and pending, or rejected), unless the ingestor decrypts the name through its built-in/config list. Reason: only a channel that was decrypted at some point has stored messages, so a pending or rejected proposal for such a name means "was approved or config-decrypted before"; a pending/rejected proposal can never hide a channel that was never decrypted. This also removes the "reappears while pending" limit, and an anonymous visitor can no longer undo an admin revoke by re-suggesting. Only an approval lists the channel again. TestPendingAndRejectedProposalsKeepAChannelHidden walks revoked → pending → rejected → approved → revoked and checks a built-in name stays listed with a rejected proposal; the E2E now also asserts the channel stays out of the list after the re-suggestion step [T].
3. Live WebSocket re-created a hidden channel's row. GET /api/channels now also returns hiddenChannels (names, omitted when empty). channels.js keeps them from each load and processWSBatch does not create a list row for one of them. The packet is not suppressed and no history is deleted; other new channels still appear live; once the channel is approved (no longer named hidden) a live message lists it as before. Test a live CHAN packet does not re-create the row of a hidden channel (#251) uses the server's type: "packet" envelope; it failed (66 passed, 1 failed) before the change and passes after (67/67) [T]. TestChannelListNamesTheHiddenChannels covers the API side [T].
4. LIMIT 1024 restored still-revoked channels. The hidden set is no longer read from a capped list of revoked rows. It is read per channel that has stored messages: SELECT p.name FROM channel_proposals p WHERE p.status <> 'approved' AND EXISTS (SELECT 1 FROM transmissions t WHERE t.payload_type = 5 AND t.channel_hash = p.name). The result is a subset of the distinct channel_hash values GetChannels already returns in full, so it needs no limit, and an omitted row can no longer mean "visible". Still one extra query per 10 s snapshot refresh, no per-request query [A]. TestRevokedChannelStaysHiddenBehindManyNewerRevokedRows: 1 old revoked + 1025 newer revoked rows, each with stored traffic, stays hidden [T]. ListRevokedNames is removed.
5. Documentation. The "closes within about 15 seconds" promise is gone: docs/user-guide/channels.md now says the page reloads its list on open, region change, the encrypted toggle and in the tab where the administrator removed the channel; another open tab keeps it until its list reloads. Reject path, hiddenChannels, and the no-row-cap behaviour are in docs/api-spec.md, openapi.go and docs/user-guide/configuration.md. All three now say /api/analytics/channels is not filtered and still counts these channels (counts, no text; hiding is not a confidentiality control) [A].
6. Nits.
   • The firmware citation src/helpers/TransportKeyStore.cpp:44-47 was wrong: it is getAutoKeyFor, the region/flood-scope transport key, not a channel key. The firmware has no code that derives a channel key from a name; the source is docs/companion_protocol.md:439-441 (sha256("#test")), and the code comments cite only that. The earlier report line is retracted; the case decision is unchanged [A].
   • Stale names fixed: TestVisibleChannelsIsNoOp… is now TestHideRevokedIsNoOpWithoutHiddenNames, BenchmarkVisibleChannels is BenchmarkHideRevoked [T].
   • Header "0 messages" on a deep link to an unlisted channel: not changed here. selectChannel takes the count from the list row (ch?.messageCount || 0, public/channels.js:2500); this PR does not touch that line, so master shows the same header for any hash that is not in the list [A]. Not verified in a browser on master [K].
   • Reviewer's finding 7 (4096-name cap in builtin-channels.json) is not changed; see remaining.

Tests, on the merged tree (origin/master 0572e7f + head 9c80787)

• cmd/server: go vet ./..., go test -count=1 ./... ok; cmd/ingestor: go vet ./..., go test -count=1 -timeout 20m ./... ok; internal/channelregistry vet + test ok; gofmt -l on the touched Go files is clean [T].
• sh test-all.sh: 219 passed, 0 failed. node test-frontend-helpers.js: 707 passed, 0 failed [T].
• scripts/check-xss-sinks.sh --diff origin/master: exit 0 [T].
• Fork guards: 9 repository == in deploy.yml, 1 in release-fast-path.yml; no .github/ change in this PR [T].
• E2E against a local Go server on port 13950 (fixture freshened, migrated, seeds 2073 and 199 as in CI), plus the two that start their own stack: all 18 channel E2Es passed — 1087, 1111, client-state-152, 154-155, fluid, 152-decrypt, decrypt, qr, color-picker, list-render, selection-flow, add-modal, share-color, ws-batch, ws-race-1498, observed-path-hash-size, modal, and test-channel-proposals-e2e.js (with the new "pending re-suggestion stays hidden" assertion). Server stopped by pid lookup [T].
• No new map[string]interface{} in non-test code (the new field is []string) [A].

Mutants (one per finding, applied one at a time and reverted; tree clean afterwards)

Finding	Mutant	Result
1	status filter moved out of SQL (ListProposals(…, "", limit))	caught: TestAdminStatusFilterIsAppliedBeforeTheLimit, TestAdminChannelProposalListAndDecisions, TestAdminListFlagsCaseNearDuplicates [T]
2	only revoked hidden (p.status = 'revoked')	caught: TestPendingAndRejectedProposalsKeepAChannelHidden [T]
3	processWSBatch guard removed	caught: the red run of the new test before the change (66 passed, 1 failed) [T]
4	ORDER BY … LIMIT 1024 reintroduced	first survived, because my test's newer rows had no stored traffic and so never reached the cap; I added traffic to each newer row, after which it is caught by TestRevokedChannelStaysHiddenBehindManyNewerRevokedRows [T]
round 1, re-run on the new code	built-in channel hidden	caught: TestBuiltinChannelIsNeverHiddenByRevokedProposal, TestPendingAndRejectedProposalsKeepAChannelHidden [T]
round 1, re-run on the new code	re-approval ignored (approved rows also hidden)	caught: 6 tests incl. TestReapprovedChannelIsListedAgain [T]

The round-1 "filter removed" and "case folded" mutants were not re-run on this code; the filter and NormalizeName are unchanged apart from hideRevoked also setting hiddenChannels [A].

CI

Run 37324492678 on head 9c80787: Go Build & Test pending at the time of writing; not polled. The previous head's Playwright step failed in test-issue-180-packets-url-modal-e2e.js (reported in the review as a time-window flake, not on the known list); I did not investigate it and it does not touch channel code [K].

Remaining

• A hidden channel returns when retention prunes its non-approved proposal row (retentionDays after the review) while messages are still stored. Fixing that needs an ingestor-side change (do not prune while messages exist); a follow-up.
• builtin-channels.json is capped at 4096 names, so an operator with more config names than that could see one of them hidden if it also has a non-approved proposal. Unrealistic today (the rainbow table has about 320 names); not changed.
• /api/analytics/channels still counts revoked channels (documented).
• A second open tab keeps a revoked channel and its conversation until its list reloads (documented; no cross-tab push).

[dborup-agent]

Review — CS-pve-agent3 PR#257 runde 2 — head 9c80787

Dom: APPROVE med nits

This is an independent, read-only re-review of round 2. Evidence tags: [T] means a test, probe or command I ran in this review. [A] means analysis or reading of code. [K] means taken from the PR, the author's follow-up or an earlier review, and not re-checked here.

Every finding from my round-1 review and from the formal review (three P2s and the docs note) is fixed, and a test locks each fix. My two round-1 probes now pass on head. The new semantics, hidden = not-approved − builtin restricted to channels with stored traffic, behaves as described. The EXISTS query uses the partial index and costs about 2 ms per 10 s refresh at 500k CHAN rows. Nothing blocks the merge. Of the nits below, finding 1 is a behaviour change that should at least be documented, and finding 2 is a test gap worth closing.

Findings

#	Severity	Finding	Evidence
1	Minor (semantics/docs)	An anonymous suggestion can now hide a channel the admin never acted on. Take a channel with stored history whose name the ingestor no longer decrypts through its config, for example a name removed from hashChannels/channelKeys by a hot reload (hotKeys.setBase). The same applies to a name past the 4096-name cap of builtin-channels.json. That channel is listed on master and was listed in round 1. On head, anyone can hide it by suggesting the name: submitChannelProposal inserts pending for an unknown name when auto-approve is off. Only Approve lists it again, and approving also turns decryption back on. Reject keeps it hidden. The effect is list-level only, the history stays readable, and the admin sees the suggestion in the queue, so I do not consider it blocking. However, the docs say a pending proposal "never hides a channel that was never decrypted". That is true, but it does not mention this case. Suggestion: name it under "Known limits" and in docs/api-spec.md. Alternatively, only let pending/rejected rows hide when the row was revoked before. That needs a marker column on the ingestor side, so it is your call.	Probe TestReviewProbeAnonymousPendingHidesHistoricalConfigChannel: no proposal, #oldcfg listed; after inserting a pending row, listed=false hidden=[#oldcfg] on head. On origin/master it is still listed [T]. Insert path: cmd/ingestor/channel_proposals.go submitChannelProposal [A]
2	Minor (test gap)	The cache-safety copy in hideRevoked is not covered on the path that needs it. TestRevokedFilterDoesNotMutateCacheAndKeepsEncrypted only sends ?includeEncrypted=true. On that path handleChannels has already copied the slice (channels[:len:len] + append), so the test passes even without the copy. On the default request (no includeEncrypted), hideRevoked receives GetChannels' cached slice itself. Mutant G2 (out := resp.Channels, compacting in place) passes the whole PR suite but corrupts the shared cache for every later request. Please add a plain-request assertion. The code is correct today.	G2 survived the PR's channel tests. My probe TestReviewProbePlainRequestDoesNotMutateCache kills it: cache before [#test #bot #zed] → after [#test #zed #zed] [T]
3	Nit (test gap)	Mutant G1 drops the AND EXISTS (… transmissions …) clause, so every non-approved proposal is hidden whether or not it has traffic. It survives the PR suite. The list is unaffected, but hiddenChannels then names proposals without traffic, including pending suggestions. Those are otherwise visible only through the admin route, so this would publish them on the public /api/channels. The clause matters for that reason, so one assertion ("a proposal without stored traffic is not named") is worth adding.	G1 survived. My probes TestReviewProbeNeverDecryptedIsNeverHidden and …HiddenChannelsIsSubsetOfAnalytics kill it (hiddenChannels = [#never #never2 #never3]) [T]
4	Nit (test gap)	Mutant F3 removes invalidateApiCache('/channels') from refreshChannelList. It survives test-channels-client-state-152.js, because the harness api() has no TTL cache. In the browser, loadChannels(true) would then hit the 15 s client cache, and the revoke would not show until the TTL expired. The same gap already existed for onApproved.	F3: 67/67 still pass [T]; api('/channels'…, { ttl: CLIENT_TTL.channels }) in loadChannelsFor [A]
5	Nit	snapshot() holds approvedMu while it runs both queries and the names-file stat/parse. Concurrent /api/channels requests wait for that once per TTL. The pattern already existed for ListApprovedNames, and the cost is small (see section 4), so no change is needed.	[A] + timings below [T]
6	Nit	hiddenChannels is global: ?region=ZZZ returned channels=0 hidden=[#bot #zed]. Another open tab keeps its old hidden set until it reloads. After a re-approval elsewhere, live messages therefore do not recreate the row in that tab until the next load. This mirrors the documented revoke limit; you could add one sentence about it.	Probe log [T]; hiddenChannelNames replaced only in loadChannelsFor [A]

1. Earlier findings: fixed and locked

Finding	Fix	Locked by	Mutant
Round 1 #1 / formal P2-1: status filter after the limit	ListProposals(ctx, db, status, limit) again. Hint from a separate ListAllNames (cap 1024)	TestAdminStatusFilterIsAppliedBeforeTheLimit, TestNearDuplicateHintSeesProposalsBeyondTheRowLimit. My round-1 probe TestReviewProbeApprovedTabLosesOldApproved passes on head [T]	G5 (hint built from the returned rows only): caught by TestNearDuplicateHintSeesProposalsBeyondTheRowLimit [T]
Round 1 #2: re-suggest → reject unhides	Hide every non-approved name with traffic	TestPendingAndRejectedProposalsKeepAChannelHidden, E2E "pending re-suggestion stays hidden". My round-1 probe TestReviewProbeResuggestThenRejectUnhides passes on head (revoked → pending → rejected, never listed) [T]	G6 (stale hidden set kept across refreshes): 4 tests fail [T]
Formal P2-2: live WS recreates the row	hiddenChannels + guard in processWSBatch	a live CHAN packet does not re-create the row of a hidden channel (#251) [T]	F1 (set accumulates, never cleared): caught. F2 (set read from approvedChannels): caught [T]
Formal P2-3: LIMIT 1024 restores old revoked rows	Per-channel EXISTS query, no row cap	TestRevokedChannelStaysHiddenBehindManyNewerRevokedRows [T][K]	Author's reintroduced cap is caught [K]
Docs "15 s" note / round 1 #3	Text rewritten	n/a	see 5
Round 1 nits (citation, stale test names)	Fixed	n/a	n/a

Other mutants: G3 (hiddenChannels not filled) is caught by TestChannelListNamesTheHiddenChannels. G4 (fail-closed on an unreadable names file) is caught by TestRevokedChannelIsNotHiddenWithoutBuiltinNames [T]. G1, G2 and F3 survive; see findings 2–4.

2. New semantics hidden = not-approved − builtin

• Can a never-decrypted channel be hidden? No. The query only returns proposal names that have a stored payload_type = 5 row with channel_hash = name. Undecrypted GRP_TXT is stored as enc_XX and listed as "Encrypted (0x..)". Proposal names always start with #, so they cannot match it. Companion-bridge channels are chN, so they cannot be proposed [A]. The probe with pending, rejected and revoked proposals and no traffic gives no hidden names, and #test plus the encrypted rows stay listed [T].
• Can an anonymous suggestion hide a channel that is visible today? A built-in/config name cannot be hidden in any status; probe TestReviewProbeBuiltinNeverHiddenAnyStatus [T]. Browser PSK channels cannot either: they are user:<name> rows and the guard keys on channelKey, and the server never lists them [A]. A historically config-decrypted name can be hidden; see finding 1.
• Fail-open: with no names file, nothing is hidden and hiddenChannels is omitted. When the file comes back, the hidden set returns within one refresh. Probe TestReviewProbeFailOpenWithoutBuiltinFile [T]. A corrupt file gives ok=false as well [A], and mutant G4 shows it is tested [T].

3. hiddenChannels and the processWSBatch guard

• Leak: none beyond what is already public. The names are a subset of stored channel_hash values; probe …HiddenChannelsIsSubsetOfAnalytics [T]. /api/analytics/channels lists them publicly, and its difference from /api/channels already reveals the same set [A]. The one thing that keeps unreviewed pending names out of the field is the EXISTS clause, which is untested (finding 3).
• Client on re-approval: onApproved → refreshChannelList() → the new load replaces hiddenChannelNames → live messages create the row again. The new node test covers this [T]. Other tabs: finding 6.

4. EXISTS and the partial index

EXPLAIN QUERY PLAN on the CI-prepared e2e-fixture.db [T]:

|--SCAN p
`--CORRELATED SCALAR SUBQUERY 1
   `--SEARCH t USING COVERING INDEX idx_tx_channel_hash (channel_hash=?)

The partial index (WHERE payload_type = 5) is used. Timings with sqlite3 .timer [T]:

• fixture plus 1001 non-approved proposals: < 1 ms;
• the same plus 500k extra CHAN rows over 1500 channels (after ANALYZE): ~2 ms, same plan.

This runs once per 10 s snapshot refresh, never per request.

5. Documentation

• The "15 s" promise is gone (no match for "15 s/seconds" in channels.md) [T].
• The reject path is described in api-spec.md, channels.md, configuration.md and openapi.go.
• All four say that /api/analytics/channels is not filtered [A].
• Missing: finding 1.

6. Rules

• cmd/server stays read-only. The new SQL is SELECT only, and readonly_invariant_test.go is in the green suite [T][A].
• No new map[string]interface{} outside tests: git diff -U0 … ':(exclude)*_test.go' gives 0 [T].
• Fork guards: 9 in deploy.yml and 1 in release-fast-path.yml, with no .github/ change [T].
• All four commits are authored and committed by dborup <kontakt@meshview.dk> [T].
• scripts/check-xss-sinks.sh --diff origin/master: exit 0 [T].

Tests

The head already contains origin/master 0572e7f9 (merge 18dfb6b1), so the merged tree is the head tree c7c6a47f. All runs are on git archive copies [T]:

• cmd/server: go test -race -count=1 ./... ok (2191 s).
• cmd/ingestor: go test -count=1 ./... ok (1750 s).
• internal/channelregistry: ok.
• sh test-all.sh: 219 passed, 0 failed. node test-frontend-helpers.js: 707 passed, 0 failed.
• E2E against a local Go server on e2e-fixture.db, prepared as in CI (freshen, the two seed rows from deploy.yml, corescope-migrate, seeds 2073 and 199). All 19 server-based channel E2Es passed: 1087, 1111, client-state-152, 154-155, fluid, 1657 analytics sprites, 1224 mobile, 1367 chat-app, decrypt, qr, color-picker, list-render, selection-flow, add-modal, share-color, ws-batch, ws-race-1498, observed-path-hash-size, modal.
• Self-started stacks: test-channel-proposals-e2e.js 26/26 and test-channels-client-state-152-decrypt-e2e.js 18/18.
• The server was stopped by the pid of its port.
• CI on head (run 37324492678): Go Build & Test ✅, Playwright E2E ✅ [T].

Mutants (mine, each applied to a fresh copy and reverted) [T]:

Mutant	Result
G1 EXISTS clause dropped	survives PR suite (finding 3); killed by my probes
G2 hideRevoked compacts the cached slice in place	survives PR suite (finding 2); killed by my probe
G3 hiddenChannels never filled	caught
G4 unreadable names file treated as known-empty (fail-closed)	caught
G5 near-duplicate index from returned rows only	caught
G6 hidden set not rebuilt on refresh	caught (4 tests)
F1 client hidden set accumulates	caught
F2 client hidden set from the wrong field	caught
F3 refreshChannelList without cache invalidation	survives unit tests (finding 4)

Probes (scratch file, not part of the PR), results on head / on origin/master:

• Round-1 probes: pass / approved-tab passes, resuggest fails (master has no hiding).
• Never-decrypted, built-in in any status, fail-open, hiddenChannels subset, plain-request cache: pass / n/a.
• Anonymous pending: hidden / listed (finding 1).

Not verified

• A browser check of the multi-tab behaviour after re-approval (finding 6). This is code reading only.
• Finding 1 end to end through the real submit endpoint and ingestor. I simulated it with the row the ingestor inserts.
• The author's BenchmarkHideRevoked numbers; I did not run it.
• Staging or production behaviour (out of scope).