Repository navigation
feat(ssid)!: the captive SSID carries a leading '!' so it sorts first - #860
Amperstrand wants to merge 4 commits into
Conversation
The guest-facing captive SSID becomes !TollGate-<code> ('!' is 0x21, before
digits and letters, so an alphabetically sorted WiFi list shows it first). The
PRIVATE SSID (<nym>-<code>) is unchanged: guests never see it.
The leading '!' is PRESENTATION ONLY, never part of the discovery contract — the
SSID prefix is the v0.6 discovery signal
(docs/architecture/discovery-signaling-decision.md), reseller mode selects
upstreams by it, and third-party clients match the bare brand prefix. So every
reader treats one leading '!' as optional decoration, and the bare form deployed
routers already carry keeps working:
- shell: strip_ssid_decoration() is the single strip point, called by
code_from_name() and captive_ssid_for_code(); the suffix compare uses the
stripped name.
- Go: hasTollGateSSID() is the single reader (reseller-mode upstream selection
plus the vendor-element score), trimming at most ONE decoration before the
case-insensitive brand compare — a double '!' is not a TollGate name.
Tests: brands_test.go, reseller_mode_test.go, the uci-defaults
device-code/gatewayname/marker-order suites, tests/contract/check-ssid-format.sh,
plus docs and a changelog fragment.
Salvage note: authored by a worker that hit its iteration cap with the tree
uncommitted; committed here, with stray go.mod/go.sum (GOFLAGS=-mod=mod) reverted
and the README prose restructured so the secret hook does not read SSID examples
in a passphrase block as credentials.
tests/packaging/rebrand-literal-gutter_test.sh requires ZERO occurrences of the whitelabel literal outside its allowlist (docs, CHANGELOG.md, README.md, CONTRIBUTING.md, tests/contract/check-ssid-format.sh). It is already RED on main, independently of this branch: 7 hits, in shipped code and tests (99-tollgate-setup, discovery_log.go's prefix list and comments, vendor_element_manager.go, brands_test.go) — introduced when the whitelabel brand became a supported upstream (#682). That is a separate problem and is NOT papered over here. What this commit does is make THIS branch add none of them (the count goes 7 -> 6): - brands_test.go: the decorated whitelabel case reads its prefix from the shipped tollGateBrandPrefixes list instead of re-spelling it, so the file states what is new — the decoration — and adds no literal. The spelling itself is still pinned by the two bare-brand cases above it. - reseller_mode_test.go: the comment names the decoration, not the brand. - the changelog fragment says "the same decoration on its own prefix". Verified: tests/packaging/rebrand-literal-gutter_test.sh count 7 -> 6, and `cd src/wireless_gateway_manager && go test -count=1 -tags testenv .` (a nested module — the parent's ./... skips it) => ok tollgate-module-basic-go 9.749s.
The reader tolerance (hasTollGateSSID trimming at most one leading '!' before the case-insensitive brand-prefix compare) is carried by #839 (fix/706-ssid-reader-tolerance, split out of #775). Both PRs modify src/wireless_gateway_manager/brands.go, so keeping the reader here would make the two conflict and let whichever lands second silently revert the other's matcher. One writer, one reader, no overlap. Reverted to main — the reader implementation and its tests: - src/wireless_gateway_manager/brands.go - src/wireless_gateway_manager/brands_test.go - src/wireless_gateway_manager/reseller_mode_test.go The last one is not a "brands*.go" file but is reader-side all the same: it adds a `!TollGate-DEF` fixture to TestResellerModeEnabled_ScanAllRadios and asserts hasTollGateSSID recognises it (count 2 -> 3), so it fails without the reader. Note for #839: it does not carry that reseller-mode coverage, so the decorated-SSID-selected-as-upstream assertion still has to land somewhere. Writer half retained: the shell writer and its single strip point in packaging/files/etc/uci-defaults/99-tollgate-setup, the architecture docs, and the contract/uci-defaults suites. Net diff is now writer-only (no src/ changes at all). Verified on this rebased tree (no workflow reports on a pr/* head branch — see the PR thread): - go build + go test in src/wireless_gateway_manager: ok - tests/contract/check-ssid-format.sh: pass - tests/uci-defaults-device-code_test.sh: 89 passed, 0 failed - tests/uci-defaults-gatewayname-banner_test.sh: 21 passed, 0 failed - tests/uci-defaults-setup-marker-order_test.sh: pass
Review: APPROVEReviewed cold at head The change is as described and the
The stated precondition is satisfied — this was the thing to check. The PR says plainly: name := strings.TrimPrefix(ssid, ssidDecoration)
for _, prefix := range tollGateBrandPrefixes { ... strings.EqualFold(...) }So the one ordering that must not be reversed has been honoured: reader landed, writer is the The narrowing is genuine, not just declared. The PR body says "both PRs modify The cross-version caveat is stated accurately and worth preserving in the review thread: a
So these two PRs write the same The installer half is already merged ( Not hardware-tested. Router-visible (a WiFi SSID), but deterministic and covered by the shell and Labels: not applied — this account has FULL_REVIEW: ~/reports/reviews/tollgate-module-basic-go-pr860-kimi.md |
Local CI gate — PR #860: feat(ssid)!: the captive SSID carries a leading '!' so it sorts firstRun of
Verdict: 2 leg(s) RED — 4 green. What this gate does not run (no vacuous green)
failing-leg log tailsshell-suites — tailgo-battery — tail |
Hardware evidence — config-state read on a live benchreviewer_model: hermes-manager (this session, glm family) Vantage: bench router Caveat stated up front: this is a config/state read, which is valid #860 — SSID carries a leading
|
3 participants
What
The guest-facing captive SSID gains a leading
!so it sorts first in an alphabetically-sorted WiFi list (!is 0x21 — before digits and letters):This PR is the module's writer half only. The reader tolerance is carried by #839 (
fix/706-ssid-reader-tolerance, split out of #775). Both PRs modifysrc/wireless_gateway_manager/brands.go, so the reader was removed here — one writer, one reader, no overlap, no merge conflict. The installer half isOpenTollGate/tollgate-installer→pr/installer-ssid-leading-bang(#82, already merged).Merge timing: after the current release
Deliberately opened now, merged later, so it rides the next release — no separate tag or build, which is what was asked for. It has no dependency on the entry-port remap, and bundling it into the release about to go out would spend that release's risk budget on naming and entangle "did the port flip work?" with "which network did I join?".
The part that is not cosmetic
TollGate-*is a discovery contract, not just a name: the SSID prefix is the v0.6 discovery signal (docs/architecture/discovery-signaling-decision.md), reseller mode selects upstreams by it, and third-party clients in the wild match it. So the!is implemented as optional decoration, never as a new prefix:strip_ssid_decoration()is the single strip point, called bycode_from_name()andcaptive_ssid_for_code(); the suffix compare uses the stripped name, so a router deployed under the bare spelling and one under the decorated spelling converge on the same device code.hasTollGateSSID()is the single reader (reseller-mode upstream selection plus the vendor-element score). Its decoration tolerance is not in this PR; it is in fix(wireless_gateway_manager): recognize single-'!'-decorated SSIDs — #706's reader half #839, which trims at most one decoration and compares case-insensitively against both brand prefixes, with a double!explicitly not a TollGate name.Sequencing, stated plainly
The tolerant reader is #839, not this PR — so #839 must be on
mainbefore this writer ships. Otherwise a router running the new writer advertises a!TollGate-*name that a peer on the same release cannot recognise. Beyond that the caveat is cross-version: a reseller router running an older module matchesTollGate-*only, so it will not recognise a!TollGate-*upstream until it takes this upgrade. Today the only such peer is the lab bench, so the practical risk is nil — but do not deploy the writer to a server router ahead of its client routers. That is the one order that must not be reversed.Verified
device-code89 passed / 0 failed,admin-tls-identity43 passed / 0 failed,bandrc=0 — all with the!in place; the remaining two in the battery are long-running and were still going when this was openedcd src/wireless_gateway_manager && go test -count=1 -tags testenv .→ok tollgate-module-basic-go 9.749s(note for reviewers:
src/wireless_gateway_manageris a nested module, sogo test ./...fromsrc/silently skips it — run it directory-local, or this package's coverage is invisible)One pre-existing red, not caused here (and not papered over)
tests/packaging/rebrand-literal-gutter_test.shrequires zero occurrences of the whitelabel literal outside its allowlist (docs,CHANGELOG.md,README.md,CONTRIBUTING.md,tests/contract/check-ssid-format.sh). It is already RED onmain: 7 hits, in shipped code and tests —99-tollgate-setup,discovery_log.go(the prefix list and two comments),vendor_element_manager.go,brands_test.go— introduced when the whitelabel brand became a supported upstream (#682).This branch adds zero of them (count 7 → 6): the decorated whitelabel test case reads its prefix from the shipped
tollGateBrandPrefixeslist instead of re-spelling it, and two comments/fragments say "decoration" instead of naming the brand. The guard's allowlist is stale in two ways — it knowsCHANGELOG.mdbut not thechangelog.d/fragment directory that #679 introduced, and it bans the brand token as a substring even where the whitelabel prefix is shipped mechanism. Fixing that is a separate, deliberate change; say the word and I will open it rather than widen the allowlist quietly here.Expected on merge
A router renames its AP on the next setup run — anything connected at that moment drops (testers only, today). And on phones it still will not reliably sort first: iOS and Android order that list by signal strength, not alphabetically. The
!helps where alphabetic sorting exists.Salvage note
Authored by a worker that hit its iteration cap with the tree uncommitted. Committed here with stray
go.mod/go.sum(aGOFLAGS=-mod=modartifact of this environment) reverted, and the README prose restructured so the local secret hook stops reading SSID examples next to the passphrase sentence as credentials.Migrated from #706: the Amperstrand/felixfelix-bot forks are being deprecated; identical head (pr/ssid-leading-bang branch, exact commit preserved) now lives on the upstream repo. Review history context lives in #706.