[Amperstrand]

What

One-line behavioral fix found while installing 0.6.0-alpha4-pre15 on a fresh OpenWrt 25.12.5 box (#552 provisioning session): the uci-defaults printed

/proc/self/fd/7: /etc/uci-defaults/99-tollgate-setup: line 60: can't open /etc/tollgate/brand: no such file
/proc/self/fd/7: /etc/uci-defaults/99-tollgate-setup: line 162: can't open /etc/tollgate/brand: no such file

BusyBox ash reports the failed < file redirect itself, before the command's own 2>/dev/null (line 60) or fallback assignment (line 162, no suppression at all) can silence it. Both brand reads now probe with [ -r ] and only then read, so the default-brand path — the overwhelmingly common case — is silent. No behavior change for present brand files, either brand.

Verification

• sh -n on the modified uci-defaults; brand resolution logic unchanged (case after read, same defaults).
• The failing context is the postinst itself; repro of the noise is in the tollgate-wrt uninstallable on OpenWrt 25.12: nodogsplash's iptables deps missing from every 25.12 feed #552 comment (this PR's change makes both lines impossible — the file open never happens when absent).

[Amperstrand]

Technical review (evidence pass)

Verified against b304a37958edcf4624ac69be66ff05cd213064aa in an isolated worktree. The diagnosis in the body is correct and the diff does exactly what it claims — proven under BusyBox ash (busybox sh), the shell family that produced the field noise.

Noise on absent file (literal /etc/tollgate/brand, which does not exist on the test host). Both pre-PR lines reproduce the leak verbatim: { read -r BRAND < /etc/tollgate/brand 2>/dev/null || BRAND=""; } prints can't open /etc/tollgate/brand: no such file — ash reports the failed < redirect itself, before the command's own 2>/dev/null applies — and the un-suppressed read -r cfgui_brand < /etc/tollgate/brand || cfgui_brand="" leaks the same line. The PR's [ -r ]-guard pattern (load_brand, setup_uhttpd_configui) is fully silent with BRAND=""/cfgui_brand="". That matches the two alpha4-pre15 postinst lines quoted in the body and in the #552 provisioning report.

No behavior change when the file exists — both load_brand variants sourced via TOLLGATE_SETUP_LIB_ONLY=1 (brand path redirected to a temp file), five cases each under BusyBox ash:

brand file	pre-PR	PR
net4sats\n	Net4sats	identical
somebrand\n	TollGate + coercion log line	identical
empty	default, silent	identical
absent	default + shell noise	default, silent
net4sats (no trailing newline)	default	Net4sats

The last row is the only delta and it is a strict improvement: read returns nonzero at EOF-without-delimiter after storing the value, so the old || BRAND="" wiped a perfectly good net4sats; the new form keeps it. The unrecognized-brand coercion log still fires identically in both versions. Neither function's caller checks its status (called bare at L837/L914) and the script has no set -e, so the guard compound failing when the file is absent has no caller-visible effect; setup_uhttpd_configui with no brand file returns 0 without touching uci.

Regression and lint. bash tests/uci-defaults-band_test.sh on the PR tree: passed=22 failed=0. busybox sh -n and dash -n both clean. shellcheck output is identical to the pre-PR file apart from line-number shifts — no new findings.

Findings (non-blocking):

1. CHANGELOG text/link mismatch: the entry's visible text is [#557] while the URL points at pull/568 (CHANGELOG.md L29-L36). Issue test(cloud-lab): run-scoped lab runner + shared-host runbook #557 is an unrelated open cloud-lab issue, and the file's convention is text = PR number (fix(session): renew after expiry; add an explicit session state (none/active/expired) #541, fix(ln): canonicalise the client-supplied mint URL so the Lightning lane can sell time #553, fix(gate): a failed gate close is not a close — keep the gate tracked and retried until ndsctl confirms it (audit C1-2) #545). One-character fix: [#557] → [#568] — the third commit ("correct the PR link to fix(setup): silence the absent-brand-file shell noise in uci-defaults #568") fixed the URL but left the text.
2. Base is 8 commits behind main and CHANGELOG.md is a hot file, so a trivial rebase conflict is possible; squash-merge handles the three commits.
3. Optional follow-up: the offline band-test harness already sources this file through the lib hook, so a one-case assertion (absent brand file → empty stderr) would pin the silence this PR adds. Fits naturally with the shell-validation lane proposed in CI proposal: container-based validation lane for the packaging shell scripts (uci-defaults) #521. Not required here.
4. Honesty note: all of the above ran under BusyBox ash on Linux, not on OpenWrt hardware — but the redirect-error ordering reproduced identically, the symptom itself was captured live in the tollgate-wrt uninstallable on OpenWrt 25.12: nodogsplash's iptables deps missing from every 25.12 feed #552 provisioning session, and both versions write byte-identical UCI state, so there is no router-visible config delta either way.

Disposition: land — with the one-character [#557] → [#568] CHANGELOG amendment, which can ride the squash or a follow-up push.

[Amperstrand]

Technical review (evidence pass) — PR head 2be5d9446b59737e461210b5a9a1dea89c0907ba (base 21 commits behind main). Compact informal pass — two guarded reads plus a CHANGELOG entry; the full thirteen-point walk is overkill for this shape.

The diagnosis is correct and I reproduced it under real BusyBox ash: read -r BRAND < /etc/tollgate/brand 2>/dev/null || BRAND="" prints can't open …: no such file — the failed < redirect is reported before the command's own null-stderr applies (redirections apply left to right), and the configUI read had no suppression at all. The [ -r ] guards at 99-tollgate-setup L59-L65 and L165-L169 make the absent-file open impossible. I ran the PR-head script through the TOLLGATE_SETUP_LIB_ONLY=1 seam the offline tests use, path-redirected, across five cases — absent, net4sats, tollgate, unknown brand, empty-but-readable file: brand resolution is identical to the old code in every case (unknown coerces to the default with its log line, empty file reads as default), setup_uhttpd_configui still returns 0 on the stock path, and stderr is byte-empty in all five. sh -n clean, and the four offline uci-defaults suites pass on the head (101/101). No dependencies, no schema or protocol surface; the CHANGELOG entry sits in Fixed in house style.

One mechanical blocker: the branch conflicts with main in CHANGELOG.md only — the script itself auto-merges, because main's three commits touching it since the fork (#578/#588/#593) moved these reads to L319 and L698 but left them unguarded, so the patch applies as-is. Rebase onto main and re-slot the entry; while you're in that hunk the stray blank line after the entry can go (every other bullet sits flush) and the link line wants its trailing period like its neighbours.

Follow-up, non-blocking: a three-line case in the offline uci-defaults harness — load_brand with an absent brand file, assert empty stderr and the default hostname — would pin this forever; #530/#521 exist precisely because this script's first-boot noise class is otherwise invisible until a fresh install. The three changelog link-fixup commits are harmless under squash-merge; no rewrite needed.

Disposition: land-with-followups — rebase to clear the CHANGELOG conflict (mechanical); optional silence case in the offline uci-defaults harness.

[felixfelix-bot]

Gate-1 review — APPROVED (code as-is; one mechanical rebase required before merge)

reviewer_model: glm-5.3 · review_tier: tier/review-glm
Reviewed at live head 2be5d9446b59737e461210b5a9a1dea89c0907ba. (The review card pinned b304a37958ed, which the 09-24 branch rewrite orphaned; the PR's delta vs main is exactly the two [ -r ] guards + one CHANGELOG entry.)

The fix is correct and minimal: probe with [ -r ], open only if readable. Reproduced the defect and the fix under BusyBox ash on the review node: the old read -r BRAND < /etc/tollgate/brand 2>/dev/null || BRAND="" prints can't open …: no such file straight through its own 2>/dev/null (redirections apply left-to-right; the failed < reports before stderr is redirected), and the guarded form is byte-silent on an absent file (0-byte stderr). These are the only two brand reads in the tree (99-tollgate-setup :64, :167). The script has no set -e, so the guard statement returning 1 on the absent path cannot abort it, and the driver still ends exit 0 (uci-defaults deletion semantics unchanged).

Evidence run at head: sh -n clean; all 8 offline uci-defaults/packaging suites pass (241 assertions, 0 failures — a superset of the author's 101; 6 suites/177 assertions independently re-confirmed by the cold audit). No GitHub check-runs exist for this head, so offline suites are the only test evidence. Main tip 74cd6f99 still carries both reads unguarded (:319, :952), so the fix is still needed.

• [RISK] branch vs main — CHANGELOG.md conflicts (mergeable_state: dirty). git merge-tree against main 74cd6f99: single content conflict, CHANGELOG.md; the script auto-merges and the guards land on main's relocated reads (merged result carries them at :323/:957 — verified, not assumed). Rebase and re-slot the entry; nothing else moves.
• [RISK] PR-body claim "No behavior change for present brand files, either brand" is false. A brand file without a trailing newline: read returns 1 at EOF after assigning the variable, so the old || BRAND="" clobbered the just-read value and silently fell back to the default brand; the guarded form keeps it. Measured (dash/bash/busybox): old, net4sats-sans-newline → [] → TollGate; new → [net4sats] → Net4sats, in both load_brand and the configUI read. The new behavior is the correct one — but the description must not claim a delta-free change.
• [NIT] CHANGELOG.md :75 — the entry's link line is missing the trailing period every flanking entry carries (:46, :68, :95, :127). (The blank line after the entry is not a deviation — the section legitimately mixes both styles, e.g. :96, :128.) Also :71-72: "before the command's own stderr redirection applies" is true only of the load_brand read; the configUI read had no suppression at all (the PR body states this correctly — the changelog entry blurs it).
• [NIT] no pinning test. Every sibling behavioural fix to this exact script shipped with one (guest-client-isolation, admin-board-requires-credential, nodogsplash-443). Three cases on the existing TOLLGATE_SETUP_LIB_ONLY=1 seam: absent → empty stderr + default hostname; net4sats\n → Net4sats; net4sats without final newline → Net4sats. The third case is the one that matters — it pins the intentional behaviour change above as a test, not a review comment.
• [INFO] residual hole, accepted: [ -r ] is true for a readable directory at /etc/tollgate/brand; read < dir would fail loudly again. Pathological config, not worth a guard.

Gate 2.5 cold audit: PASS — fresh-context audit of this draft confirmed all load-bearing claims (incl. byte-level busybox repro and an independent merge-tree re-run); one sub-claim (a CHANGELOG blank-line style nit) was refuted and dropped. The named worker-reviewer-ds profile is not instantiated on this node; the audit was discharged same-family — full trail in the FULL_REVIEW file.

FULL_REVIEW: /home/c03rad0r/reports/reviews/tollgate-module-basic-go-t_60cf9c11-glm-5.3.md

[c03rad0r]

is this still relevant and why? How does it relate to the SSID one?

[Amperstrand]

Still relevant, and unrelated to the SSID PR functionally. This one is the router side: on a fresh/manual install where /etc/tollgate/brand does not exist yet, the uci-defaults brand read printed can't open /etc/tollgate/brand twice into the setup log (BusyBox ash reports the failed redirect itself); the fix probes with [ -r ] so the default-brand path is silent. The SSID work (#618) is the other end — a TollGate client recognizing both TollGate- and Net4sats- prefixes when scanning for an upstream. Same brand vocabulary, different ends of the wire. Tiny and safe — fine to take any time in 0.6.x; not a release blocker.

[felixfelix-bot]

Conflicts resolved on a rebase of your four commits onto main (ac5811ff), authorship kept.

Replacement PR: #680 — 2 files, +17/−2, bash -n clean.

Two things you should know, since one of them is a deliberate deviation:

1. load_brand() conflicted as a whole region — main rewrote that comment block and the coercion case after this branch. Resolution is main's body plus your guard; none of main's rewrite is reverted.
2. Your second hunk no longer applies to anything: the cfgui_brand read in setup_uhttpd_configui() is gone from main, replaced by brand_token(). That function reads the brand file the same way, so the [ -r ] guard goes there instead. Same two guarded sites, relocated. If you would rather keep the PR's diff byte-identical and file brand_token() separately, say so and I'll split it.

Proof the noise is actually gone (both functions extracted, /etc/tollgate/brand absent — a fresh install):

before   stderr='… cannot open /etc/tollgate/brand: No such file'
after    stderr=''          BRAND_HOSTNAME=TollGate brand_token=tollgate in both

Same values, no noise. Merge #680 and this PR closes as superseded, or force-push pr/568-brand-noise-rebased onto fix/brand-file-absent-noise.

[Amperstrand]

Rebased in place onto current main (ec77401) — one commit (the fix + the three changelog-link fixups squashed).

One deliberate resolution to flag: the rebase conflict offered a ~27-line whitelabel setup_uhttpd_configui block — that was inherited from the branch's old base (whitelabel-era work), not part of this PR's delta (the PR is +15/−2 vs its merge-base), and current main deliberately doesn't carry it. Carrying it produced an unbalanced script; it is dropped. The surviving change is the real fix: the line-327 brand reader now uses a [ -r ] guard instead of 2>/dev/null redirection, because BusyBox ash prints can't open … for the failed redirect itself (seen live in the alpha4-pre15 postinst).

Verification: bash -n clean; configui-8090-single-owner 40/0, device-code 73/0, nodogsplash-convergence 29/0. Pre-existing, unrelated: rebrand-literal-gutter fails 5/1 on this branch AND identically on pristine origin/main (the Net4sats literal ban vs main's recent discovery-signaling/README mentions) — a main-side follow-up, not this PR.

@felixfelix-bot skip — done (see #676).

[Amperstrand]

@c03rad0r still relevant, verified on current main (ac5811ff): both brand reads remain in the unguarded form — 99-tollgate-setup lines 327 and 357 read read -r … < /etc/tollgate/brand 2>/dev/null || …, and BusyBox ash prints can't open … for the failed redirect itself, before the command's 2>/dev/null applies (observed live in the alpha4-pre15 postinst). Unrelated to the SSID PR — that one is about recognizing prefixes, this is about shell noise when the brand file is absent on stock routers. The live vehicle carrying it onto current main is #680 (felix's rebase; applies the guard at both call sites, main's rewrite preserved).

[Amperstrand]

Skipped per PR-REVIEW.md Step 1 (draft). Small setup-noise fix — when it un-drafts it qualifies for the informal one-paragraph pass, not a 13-point one.

[c03rad0r]

🏷️ Next action — on-hold

On hold — draft PR. Not in the active merge queue.

[Amperstrand]

Closing as superseded: main already carries this in a stronger form — packaging/files/etc/uci-defaults/99-tollgate-setup reads the brand via [ -r /etc/tollgate/brand ] && read -r BRAND ... at lines 407 and 440 (with an explicit guard comment at 437). The branch's history has no merge base with main (old working-fork root), which is why it showed as permanently conflicting. No functional change remains that main lacks.