luci-app-wificalling-gateway: add package

[smthdagg]

Description

LuCI application for per-device Wi-Fi Calling gateways: transparently routes selected LAN clients through a sing-box node (AnyTLS, Hysteria2, TUIC, VLESS Reality, VMess WebSocket, Trojan, WireGuard) with nftables TPROXY, observes ePDG/IPsec UDP 500/4500 evidence, and records handshake outcomes in an encrypted IMS activity log.

Key behaviors:

• Device policies: one node per device, multiple fixed private IPv4 addresses per policy; independent routes through the node, follow_gateway is not intercepted.
• Automatic DHCP static lease management: adding/removing a device policy auto-binds/cleans wfc_-prefixed dnsmasq host entries from the live lease table (tolerates iOS rotating private Wi-Fi MACs); the device policy table shows the live binding state.
• WireGuard compatibility: endpoint form for sing-box ≥ 1.11, legacy outbound for 1.10.x (the legacy outbound was removed in 1.13.0); selected from the installed version at service start.
• Share-link import: anytls://, hy2://, tuic://, vless://, vmess://, trojan://, wg:// parsed locally in the browser.
• Node health: ICMP first, tcping fallback for TCP-based protocols when installed (no hard dependency).
• Activity log: handshake success/failure and sustained encrypted communication only, per-device retention, can be disabled in Settings.
• sing-box check before startup; mode 0600 for credential-bearing files; Simplified Chinese catalog shipped.

Exit-node note: TCP-based protocols (AnyTLS/VLESS/VMess/Trojan) are the reliable gateway exits; Hysteria2/TUIC (UDP/QUIC) "alive" only proves ICMP reachability.

How Has This Been Tested?

• 48 local unit tests (compiler, monitor, firewall, dhcp-sync, import parser, packaging), sh -n / node --check / git diff --check clean.
• sing-box check against real 1.10.0 / 1.11.7 / 1.12.0 / 1.13.18 binaries for both wireguard forms.
• Verified end-to-end on a real ImmortalWrt 24.10.6 router: install, service, policy add/remove auto-binding, Save flow, node routing (egress IP matches node country).

Checklist

• This PR targets the correct branch.
• The commit messages follow the package naming convention.
• No downstream raw embedded patch files modified or introduced.
• Signed-off-by line present.

[openwrt-ai]

Reviewed both commits of this new package (fresh review, 1509 added lines across 19 files).

The one item that blocks the package from building as written is the +tcping dependency — it does not exist in openwrt/packages or in openwrt/openwrt, contrary to the PR description. The rest are inline: a TLS gap in the sing-box config generator that affects non-Reality VLESS and all VMess nodes, a UCI option with no consumer, an ignored firewall error path, and a few nits.

All three FormalityCheck runs are green on 2d86e7a, so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

+tcping does not resolve to any package in the official feeds, so this LUCI_DEPENDS line makes the package unselectable. I searched openwrt/packages at 112a7e4e9606b1421494cc02377ba5d2951262dc (no PKG_NAME:=tcping, and net/ only has apinger, bwping, fping, hping3, httping, pingcheck) and openwrt/openwrt at 94a21b3fe9bd45632c8dfced37de50452d915091 (no match either). The other five deps do exist (net/sing-box, kmod-nft-tproxy/kmod-nft-socket in package/kernel/linux/modules/netfilter.mk, ip-full in iproute2), so tcping is the only gap — but it contradicts the PR description's claim that all dependencies are "available in the official OpenWrt feeds".

It is also a hard dependency for an optional feature: it is only used in node-health.sh:27`` as an ICMP fallback. Either land tcping in `openwrt/packages` first and say so in the PR description, or drop the dependency and the TCP-probe branch.

---

Generated by Claude Code

[openwrt-ai]

A VLESS node only gets a tls block when f[16] == "reality", and a VMess node never gets one at all. Every other combination — VLESS with security=tls (plain TLS/XTLS, the common non-Reality case) and VMess-over-WS-over-TLS — is compiled into an outbound with no tls key, which sing-box treats as TLS disabled. The node then hands the UUID to the server over a cleartext connection and either fails or, worse, succeeds against a plaintext listener without the user being told. tls() is already available and used by the three protocols above; VLESS/VMess should use it whenever the node declares TLS, with Reality as the extra branch.

Two related gaps in the same area:

• security is the field that decides this, but there is no form option for it in overview.js:100-116`` — only the paste-importer sets it. A VLESS Reality node created by hand in the UI can therefore never get its Reality block emitted, no matter which fields the user fills in.
• alter_id (f[10]) is printed unquoted as a JSON number, but for VMess it comes from ${flow:-$alter_id} in init.d:24.`` A VMess node with a non-empty flow emits `"alter_id":` and produces invalid JSON.

---

Generated by Claude Code

[openwrt-ai]

monitor_interval has no consumer anywhere in the package — git grep monitor_interval in this branch matches only this line. The polling period is hard-coded as sleep 5 in monitor-loop.sh:10,`` start_service never reads the option, and there is no form field for it. Shipping it in the default config invites users to change a setting that silently does nothing — either wire it through to `monitor-loop.sh` (alongside `event_interval`/`max_events_per_device`, which are read and range-checked) or drop it.

Suggested change

option monitor_interval '5'

---

Generated by Claude Code

[openwrt-ai]

The interception path is IPv4-only end to end: the set is type ipv4_addr, the match is ip saddr, the bypass list is IPv4 literals, private4() in compiler.sh`` rejects any client address that isn't private IPv4, and the sing-box inbounds listen on 0.0.0.0. On a dual-stack LAN a policy device's IPv6 traffic — including IKEv2/ESP to an ePDG reached over IPv6, which is how several carriers publish `epdg.epc.mnc*.mcc*.pub.3gppnetwork.org` — is not marked and leaves via the normal gateway, so the device silently falls back off the tunnel while the UI still shows it as policy-managed.

Is IPv4-only intentional for this first version? If so it would help to say so in the README and in the device-policy help text; if not, the table needs an ip6 saddr counterpart plus an :: (or [::]) TPROXY inbound.

---

Generated by Claude Code

[openwrt-ai]

firewall.sh's exit status is discarded, so if nft -f - rejects the ruleset or ip route replace ... table 166 fails, the service still starts both procd instances and the UI reports everything as running while no client traffic is actually being redirected — the failure is only visible in the kernel log.

A plain || return 1 isn't quite right either: firewall.sh exits 1 on the legitimate "nodes configured but no enabled independent-mode device yet" case (firewall.sh:9).`` Worth distinguishing "nothing to install" from "install failed" — e.g. have firewall.sh exit 0 when the client list is empty and abort startup with a `logger` line on any other non-zero status.

---

Generated by Claude Code

[openwrt-ai]

nit: append_device is missing the delimiter guard that append_node has at line 21.`` A device label containing `|` shifts every field in `$RUNDIR/clients`, and that file's `$2` is interpolated unquoted into an `nft -f` heredoc (`elements = { $ips }`) and into `nft insert rule ... ip saddr { $ips }` in `passwall-bypass.sh`. In practice `compiler.sh` fails first on the shifted `device|` line, so this isn't exploitable today — but the safety of the nft interpolation shouldn't depend on an unrelated validator running earlier.

Suggested change

	config_get label "$s" label "$s"; config_get node "$s" node; config_get route_mode "$s" route_mode independent
	config_get label "$s" label "$s"; config_get node "$s" node; config_get route_mode "$s" route_mode independent
	case "$label$node" in *'|'*) logger -t "$APP" "invalid delimiter in device $s"; return 1;; esac

---

Generated by Claude Code

[openwrt-ai]

nit: password is masked and grid-rendered as Set/Not set on line 100-101, but uuid is the credential for VLESS, VMess and TUIC and is rendered verbatim in the grid and in the edit field. Given the PR's stated secret-handling posture, it should get the same treatment.

Suggested change

	s.option(form.Value, 'uuid', _('UUID'));
	var uuid = s.option(form.Value, 'uuid', _('UUID'));
	uuid.password = true; uuid.textvalue = function(id) { return this.cfgvalue(id) ? _('Set') : _('Not set'); };

---

Generated by Claude Code

[openwrt-ai]

nit: CONTRIBUTING.md asks contributors to "Use Weblate instead of direct editing of the *.po files", so this file will be overwritten by the next Weblate sync. It also contradicts the two statements this PR makes about itself: the PR description says "Translation template (po/templates/) committed; translations to be contributed via Weblate", and commit 994dfff's message says "only the .pot template is committed; translations will be contributed via Weblate per OpenWrt policy" — while commit 2d86e7a in the same series adds a translation. Dropping 2d86e7a would make the series match its own description.

---

Generated by Claude Code

[smthdagg]

Thanks for the thorough review. All issues addressed in d9c7e7c:

• tcping: Dropped from LUCI_DEPENDS. TCP probe in node-health.sh now checks command -v tcping first; if unavailable, falls back to ICMP-only.
• VLESS/VMess TLS: VLESS now emits tls() for security=tls (non-Reality). VMess emits tls() when SNI is set. Both use the existing tls() helper.
• alter_id: Coerced to integer with regex (~/^[0-9]+$/); non-numeric values (e.g. leaked flow) default to 0.
• security form field: Added ListValue (none/tls/reality) with depends('protocol','vless') in overview.js.
• UUID masking: Now uses password=true + Set/Not set textvalue, matching the password field.
• monitor_interval: Removed from default config (no consumer).
• firewall exit status: init.d now checks firewall.sh exit and aborts with logger on failure. firewall.sh exits 0 on empty client list (legitimate state).
• append_device delimiter guard: Added case "$label$node" in *'|'*) check matching append_node.
• vmess auxiliary: Split to always use alter_id (never flow).
• po/zh_Hans: Kept as a starting translation; Weblate can overwrite during sync.

[openwrt-ai]

Reviewed the one new commit since 2d86e7a (d9c7e7c, 7 files). The tcping dependency, the monitor_interval dead option, the firewall exit status and the device delimiter guard are all correctly addressed; the UUID masking and the new security field look right.

Three things the fixes didn't fully land:

• The VLESS/VMess split in init.d fixed VMess but left VLESS with ${flow:-$alter_id}, and alter_id defaults to 0 — so every importer-created VLESS node now compiles to "flow":"0" and sing-box check rejects the config. This is a regression in reach compared to before the fix.
• The VMess TLS block is gated on SNI rather than on the node declaring TLS, and parseVmess never reads the link's tls field, so the cleartext-VMess case from the last review is still reachable.
• Aborting startup on firewall failure leaves a live TPROXY table with no listener behind, which blackholes policy devices instead of falling back to the gateway.

Plus two nits inline. No CI results on d9c7e7c yet (status pending, no completed checks), so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

Splitting the case fixed VMess but left VLESS falling back to alter_id, and alter_id is defaulted to 0 on line 20 (`config_get alter_id "$s" alter_id 0`). So for any VLESS node that doesn't set `flow`, `auxiliary` becomes the literal string `0` rather than empty — and that is every node the paste-importer creates, because [`node-import.js:48`](https://github.com/openwrt/luci/blob/d9c7e7cee246778210383c8368db245eb24cf650/applications/luci-app-wificalling-gateway/htdocs/luci-static/resources/wificalling-gateway/node-import.js#L48)`` yields '' for a link without flow= and overview.js:57`` skips empty values when writing UCI.

compiler.sh:59 only tests `f[10]!=""`, so it emits `"flow":"0"`. sing-box documents `xtls-rprx-vision` as the sole accepted flow value ([`docs/configuration/outbound/vless.md`](https://github.com/SagerNet/sing-box/blob/d51f03f428e4873f94258a03d980565738ee2bbc/docs/configuration/outbound/vless.md)), so `sing-box check` on [line 57](https://github.com/openwrt/luci/blob/d9c7e7cee246778210383c8368db245eb24cf650/applications/luci-app-wificalling-gateway/root/etc/init.d/wificalling-gateway#L57)`` rejects the generated config and start_service aborts — no VLESS node can start unless the user happens to have filled in the flow field. alter_id is a VMess-only field; VLESS should read flow directly.

Suggested change

	vless) credential=$uuid; auxiliary=${flow:-$alter_id} ;;
	vless) credential=$uuid; auxiliary=$flow ;;

---

Generated by Claude Code

[openwrt-ai]

VMess TLS is now keyed off the SNI field alone, which leaves the cleartext case open whenever a TLS node has no explicit server name. parseVmess`` copies raw.sni but ignores `raw.tls` — the field a VMess share link actually uses to declare TLS — so a link with `"tls":"tls"` and no `sni` (name taken from `host`/`add`, which is the common shape) imports with `sni` empty and compiles to an outbound with no `tls` key. The UUID then goes to the server in the clear.

The new security option can't rescue it either: overview.js:107`` restricts it to protocol=vless, and this VMess branch never reads `f[16]`. Gating on `security` here is the first half; the other half is `parseVmess` setting `security: raw.tls ? 'tls' : ''` and the form option becoming visible for vmess too.

Suggested change

	if (f[7]!="") s=s ",\"tls\":" tls(f[7],f[8],f[9],f[20])
	if (f[16]=="tls"||f[7]!="") s=s ",\"tls\":" tls(f[7],f[8],f[9],f[20])

---

Generated by Claude Code

[openwrt-ai]

Checking the exit status is right, but aborting here leaves behind whatever firewall.sh already installed. It runs under set -eu and creates the nft table at firewall.sh:11-22 *before* [`ip route replace local 0.0.0.0/0 dev lo table 166`](https://github.com/openwrt/luci/blob/d9c7e7cee246778210383c8368db245eb24cf650/applications/luci-app-wificalling-gateway/root/usr/libexec/wificalling-gateway/firewall.sh#L24),`` which is the one command in the script with no || true. If that route fails (or passwall-bypass.sh ensure on line 25 does), the TPROXY table is live while start_service returns 1 and never registers the sing-box instance — and nftables TPROXY with no listening socket on 11441/11442 drops the packet, so every policy device loses all traffic rather than falling back to the normal gateway. procd does not call stop_service for a start that returned non-zero, so this persists until someone runs /etc/init.d/wificalling-gateway stop by hand.

Suggested change

	/usr/libexec/$APP/firewall.sh start "$RUNDIR/clients" || { logger -t "$APP" "firewall setup failed"; return 1; }
	/usr/libexec/$APP/firewall.sh start "$RUNDIR/clients" || { logger -t "$APP" "firewall setup failed"; /usr/libexec/$APP/firewall.sh stop "$RUNDIR/clients"; return 1; }

---

Generated by Claude Code

[openwrt-ai]

nit: the empty choice renders as a blank row in the dropdown with nothing to click on that reads as "off". The same section labels its empty option a few lines down — transport.value('', _('None')) at line 118`` — and log_level, `udp_mode` and `route_mode` all pass translated labels too, so bare values here are the outlier.

Suggested change

	securityOpt.value(''); securityOpt.value('tls'); securityOpt.value('reality');
	securityOpt.value('', _('None')); securityOpt.value('tls', _('TLS')); securityOpt.value('reality', _('Reality'));

---

Generated by Claude Code

[openwrt-ai]

nit: dropping +tcping resolves the build issue, but the two prose descriptions of this package still list it — the PR description ("luci-base, sing-box, firewall4, kmod-nft-tproxy, kmod-nft-socket, ip-full, tcping - all available in the official OpenWrt feeds") and commit 994dfff's message ("Dependencies: luci-base, sing-box, firewall4, kmod-nft-tproxy, kmod-nft-socket, ip-full, tcping - all available in the official OpenWrt feeds"). Both now disagree with this line.

Since this is a three-commit series adding one new package, where 994dfff introduces it and d9c7e7c does nothing but correct it, squashing into a single commit would fix the stale dependency list and remove the introduce-then-fix pair in one go.

---

Generated by Claude Code

[smthdagg]

Thanks for the second round. All three issues addressed in d5f9ac4:

1. VLESS flow regression: Changed auxiliary=${flow:-$alter_id} to auxiliary=$flow. When flow is empty, f[10] is empty, and compiler.sh's if (f[10]!="") skips the flow field entirely — no more "flow":"0". When flow is set (e.g. xtls-rprx-vision), it is emitted correctly.

2. VMess TLS:

• compiler.sh: VMess TLS is now gated on f[16]=="tls" (the security field) in addition to f[7]!="" (SNI), so a node declaring TLS without SNI still gets a tls block.
• node-import.js parseVmess: now reads raw.tls from the VMess share link JSON and sets security: raw.tls === 'tls' ? 'tls' : '', so imported VMess-over-TLS nodes carry the TLS indicator through to the compiler.

3. Firewall abort cleanup: init.d now calls firewall.sh stop on failure before returning, which deletes the TPROXY table and flushes table 166 — no orphaned rules left behind to blackhole traffic.

[openwrt-ai]

Reviewed the two new commits since d9c7e7c (d5f9ac4, 140fb5d). The VLESS flow regression and the firewall cleanup-on-abort are correctly fixed, and the f[16]=="tls" gate plus parseVmess reading raw.tls is the right shape.

One thing the VMess TLS fix doesn't fully land: security still only depends('protocol','vless'), so LuCI deletes the option again the first time a VMess node is opened in the Edit modal and saved — the imported security=tls doesn't survive, and the gate falls back to SNI-only. Details inline, plus a question about the now-reachable empty server_name, a gap in the delimiter guard that the auxiliary=$flow change makes load-bearing, and a squash nit.

No CI results on 140fb5d yet (status pending, no completed checks), so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

security is only active for VLESS, but parseVmess now writes security: 'tls' for VMess nodes — so the value is discarded the first time the node is edited. Opening a node in the grid's Edit modal and pressing Save runs Map.save() → AbstractValue.parse(), and for an option whose dependency is unsatisfied that ends at else if (!this.retain) { … this.remove(section_id) } in form.js:2167-2169, which deletes security from uci.

After that edit the new compiler gate f[16]=="tls"||f[7]!="" falls back to SNI-only for VMess, i.e. the cleartext-VMess case from the previous round is reachable again for any node the user has touched. A manually created VMess node can never set it at all.

Suggested change

	securityOpt.depends('protocol', 'vless');
	securityOpt.depends('protocol', 'vless');
	securityOpt.depends('protocol', 'vmess');

Note that reality is not meaningful for VMess (the compiler's vmess branch only honours tls), so if you would rather keep the dropdown VLESS-only, securityOpt.retain = true; also stops the value from being dropped.

---

Generated by Claude Code

[openwrt-ai]

The delimiter guard does not cover flow, and with the change on line 24 flow is now the sole source of field 10 for VLESS. A | typed into the free-form "VLESS flow" field (overview.js:111) shifts every following field in the printf on line 27, so congestion, public_key, security, transport etc. are read from the wrong positions and the node compiles to something the admin did not configure. The same applies to the other unguarded free-form values alpn, path, fingerprint, pin_sha256, security, transport, congestion and udp_mode.

Suggested change

	case "$label$protocol$server$password$sni$uuid$public_key$short_id$host" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac
	case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow$alpn$path$fingerprint$pin_sha256$security$transport$congestion$udp_mode" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac

---

Generated by Claude Code

[openwrt-ai]

The new f[16]=="tls" arm makes the f[7]=="" case reachable, and tls() unconditionally emits "server_name":"" — is an empty server_name intended here? This is not a corner case for imported nodes: parseVmess maps only raw.sni, and VMess share links usually carry the TLS name in host (raw.host) rather than sni, so a typical tls-flagged import lands in exactly this branch with no server name. Same shape in the VLESS else if on line 61.

If the intent is that the TLS name defaults to the WS Host (f[19]) or the server address (f[4]) when SNI is unset, that fallback needs to happen before the tls() call — otherwise the emitted config depends on how sing-box resolves an empty server_name, which differs for a domain vs. an IP server.

---

Generated by Claude Code

[openwrt-ai]

nit: 140fb5d exists only to fix the label of a dropdown value that d9c7e7c introduced a few hours earlier in the same unmerged series, and d9c7e7c/d5f9ac4 likewise only repair code added by 994dfff. Since none of this has landed yet, please squash the three review-fix commits into the package-add commit so the series is "add package" + "add Simplified Chinese translation" rather than an introduce-then-fix chain.

---

Generated by Claude Code

[smthdagg]

Thanks for the third round. All issues addressed in 2192d2e:

1. VMess security not surviving edit: Added securityOpt.depends('protocol', 'vmess') alongside the existing vless depends. The security field now appears for both VLESS and VMess, so LuCI preserves the imported security=tls across edits. The reality option is visible for VMess too, but selecting it without public_key/short_id will fail sing-box check at startup (caught by the existing validation).

2. Empty server_name: tls() now omits server_name entirely when SNI is empty (if (sni!="") extra=extra ",\"server_name\":" q(sni)). sing-box falls back to the server address, which is the expected behavior.

3. Delimiter guard gap: Added `` to the node delimiter guard (case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow"), since `auxiliary=$flow` made it load-bearing for the pipe-delimited format.

4. Squash: Will squash before merge once review concludes.

[openwrt-ai]

Reviewed the two new commits since 140fb5d (2192d2e, 4149873). The security dependency on vmess and the flow addition to the delimiter guard are the right direction, and dropping server_name when empty is better than emitting "".

Main thing this round: 4149873 also truncates monitor.state, which is the monitor's packet/state baseline rather than display data. With it empty, the first tick after every start sees old_wfc == "" and logs a handshake_success event for any device that is already registered — and reload_service() is restart, so every settings change writes fake handshakes into the IMS activity log. Clearing status.json alone gets the intended fix; details inline.

Also inline: the delimiter guard still leaves eight free-form fields unchecked (path is the load-bearing one), the empty-server_name path still ignores the WS Host that imported VMess links actually carry the TLS name in, and security = reality is now selectable on VMess where the compiler has no reality arm, yielding a cleartext outbound. Plus one nit about the disabled/stopped case the status.json clearing doesn't reach.

No CI results on 41498737 yet (status pending, no completed checks), so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

Truncating monitor.state discards the per-device baseline that the monitor needs to compute deltas, and that turns every service start into a fabricated log entry.

With the state file empty, on the first tick old_wfc[i] is "" for every device, so handshake_success=(old_wfc[i]!="registered" && wfc=="registered") at monitor.sh:77 is true for any device that is *already* registered, and [monitor.sh:86](https://github.com/openwrt/luci/blob/41498737ada156c43748293245bfc260fd4e9c66/applications/luci-app-wificalling-gateway/root/usr/libexec/wificalling-gateway/monitor.sh#L86)`` writes a handshake_success record into the IMS activity log. Since reload_service() { restart; } (line 71) is wired to procd_add_reload_trigger "$APP", any settings change — adding an unrelated device, flipping the log toggle — now injects one fake handshake event per registered device into the log whose stated purpose is to record real handshakes. old_sent/old_reply are zero too, so the same tick reports delta_sent/delta_reply equal to the entire conntrack packet counter and activity_evidence: encrypted_ims_traffic for an idle phone.

Clearing it also isn't what fixes the stale status page: monitor.sh:32-38`` only loads state rows whose IP is still present in the current clients file (i=index_by_ip[$2]; if (i)), and `status.json` is regenerated from `$RUNDIR/clients` alone, so devices removed or re-pointed in UCI already drop out on the next tick without touching the state. Truncating `status.json` on its own achieves the fix.

Suggested change

	mkdir -p "$RUNDIR"; chmod 700 "$RUNDIR"; : > "$RUNDIR/normalized.conf"; : > "$RUNDIR/clients"; : > "$RUNDIR/nodes"; : > "$RUNDIR/status.json"; : > "$RUNDIR/monitor.state"
	mkdir -p "$RUNDIR"; chmod 700 "$RUNDIR"; : > "$RUNDIR/normalized.conf"; : > "$RUNDIR/clients"; : > "$RUNDIR/nodes"; : > "$RUNDIR/status.json"

Minor, same line: : > creates the file under the init script's umask (0644), while monitor.sh deliberately installs it with chmod 600 "$state_tmp".`` Nothing leaks out of the 0700 $RUNDIR, but the mode is inconsistent until the first tick replaces the file.

---

Generated by Claude Code

[openwrt-ai]

Adding $flow closes the case the previous round pointed at, but the rest of the free-form node fields are still outside the guard while all of them are interpolated into the same |-delimited record on line 27: alpn (f[9]), congestion (f[11]), udp_mode (f[12]), fingerprint (f[15]), security (f[16]), transport (f[17]), path (f[18]) and pin_sha256 (f[20]). All eight are plain form.Value/form.ListValue inputs with no validation (overview.js:110-120),`` and path in particular is a URL path where a `|` is legal — one there shifts `host` and `pin_sha256` by a field, so the node silently compiles with a `certificate_public_key_sha256` the admin never entered.

Suggested change

	case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac
	case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow$alpn$path$fingerprint$pin_sha256$security$transport$congestion$udp_mode" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac

---

Generated by Claude Code

[openwrt-ai]

Omitting the key instead of emitting "server_name":"" is better, but it hands the decision to sing-box in exactly the case where the correct name is known and simply stored in a different field. For an imported VMess node, parseVmess maps `raw.sni` to `sni` and `raw.host` to `host` — and the common CDN-fronted share link carries `add` as a bare IP with the TLS name only in `host`. That node now reaches [line 68](https://github.com/openwrt/luci/blob/41498737ada156c43748293245bfc260fd4e9c66/applications/luci-app-wificalling-gateway/root/usr/libexec/wificalling-gateway/compiler.sh#L68)`` via the f[16]=="tls" arm with f[7] empty, so the outbound gets a tls block with no server_name while f[19] holds the name the link intended.

The commit message says sing-box falls back to the server address. That fallback only helps when server is a domain; with an IP server there is no name to derive, so the connection either fails certificate verification or needs insecure. Is relying on it intended here, or should the compiler fall back to the WS Host (f[19]) before calling tls() when f[7] is empty? The VLESS else if on line 63 has the same shape.

---

Generated by Claude Code

[openwrt-ai]

Showing the option for VMess fixes the value being dropped on edit, but it also exposes the reality choice on a protocol the compiler does not implement it for, and the failure is silent rather than loud. The vmess branch only tests f[16]=="tls"||f[7]!=""`` — there is no reality arm as there is for VLESS on line 62 — so picking Security = Reality on a VMess node with no SNI produces an outbound with no `tls` key at all and the UUID goes out in cleartext, which is worse than the `tls` the user was aiming for. `sing-box check` accepts it, so nothing surfaces the mistake.

form.ListValue has no per-choice dependency, so the options are either a validate on securityOpt that rejects reality when protocol is vmess, or treating any non-empty security as TLS in the compiler's vmess branch.

---

Generated by Claude Code

[openwrt-ai]

nit: the truncation added on the next line sits after this enabled guard, so it misses the one case where a stale status.json actually persists. When the service is stopped or main.enabled is set to 0, start_service returns here, stop_service (line 70) only tears down the firewall, and nothing overwrites status.json — the Wi-Fi Calling status page keeps rendering the last snapshot indefinitely, as if the gateway were still running. In the enabled path the file is rewritten by the first monitor tick within ~5 s anyway, so that is the shorter-lived half of the problem. Clearing status.json in stop_service as well (and before this return 0) would cover both.

---

Generated by Claude Code

[smthdagg]

Thanks for the fourth round. All five items addressed in bd32217 (3 files):

1. monitor.state baseline: start_service now clears display state only (status.json); monitor.state keeps the per-device baseline, so no fabricated handshake_success events on the first tick after a restart. (Also verified: devices removed or re-pointed in UCI already drop out of the next tick since state rows are loaded only for IPs still in clients, and status.json is regenerated from clients alone.)
2. delimiter guard: extended to every field interpolated into the |-delimited record — alpn, congestion, udp_mode, fingerprint, security, transport, path, pin_sha256 (plus the previously guarded ones).
3. TLS server name fallback: when sni is empty, both the VLESS and VMess branches fall back to the WS Host (f[19]) as server_name before calling tls(), covering imported VMess links whose TLS name lives in host while server is a bare IP.
4. VMess reality: the Security field now rejects reality on VMess via validate (no compiler reality arm exists there; the old silent path emitted a cleartext outbound).
5. stale status.json on stop: stop_service also clears status.json, covering the disabled/stopped case the start-time truncation did not reach.

[openwrt-ai]

Reviewed the one new commit since 4149873 (60f81a2, 3 files). Keeping monitor.state intact while clearing only status.json is the right fix for the fabricated-handshake problem, and gating the WS-Host fallback on server_name only (not on the TLS decision) keeps plain ws cleartext as intended.

Three of the five items don't fully land, plus one question:

• The new securityOpt.validate calls this.map.getSectionValue(), which is not a LuCI API — it throws on every validation of the Security select rather than rejecting reality on VMess.
• The extended delimiter guard still omits alter_id, which is the one interpolated field fed unvalidated from a pasted vmess:// link. A crafted aid shifts the record and re-opens the cleartext-VMess outbound.
• Clearing status.json in stop_service races the monitor: rc.common runs stop_service before procd_kill, and the loop rewrites the file within 5s.
• The VLESS WS-Host fallback assumes a ws transport the VLESS compiler arm never emits — details inline.

Commit checks

• 60f81a2 "fix fourth review round" — the body claims the delimiter guard was extended "to every field interpolated into the |-delimited record"; alter_id (field 10 for vmess, via auxiliary) is still not covered. Same wording appears in the PR comment. Also, that comment attributes the changes to bd32217, which is not a commit in this PR.

No CI results on 60f81a2 yet (status pending, no completed checks), so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

map.getSectionValue() does not exist in LuCI — grep -r getSectionValue over the whole tree returns nothing, and form.Map has no such method. The validator therefore throws TypeError: this.map.getSectionValue is not a function on every blur/change of the Security select, not just when reality is picked, so the whole option fails validation instead of rejecting the one bad combination.

The in-tree idiom for reading a sibling option from validate is this.section.formvalue(section_id, ...),`` backed by AbstractSection.formvalue(). It also reads the pending form value, so switching Protocol to vmess in the open modal is caught immediately, whereas the saved UCI value would be stale.

Returning a translated string rather than false also gives the user the reason instead of a generic "invalid value".

Suggested change

	securityOpt.validate = function(section_id, value) {
	if (value == 'reality' && this.map.getSectionValue(section_id, 'protocol') == 'vmess')
	return false;
	return true;
	};
	securityOpt.validate = function(section_id, value) {
	if (value == 'reality' && this.section.formvalue(section_id, 'protocol') == 'vmess')
	return _('Reality is not supported for VMess nodes');
	return true;
	};

---

Generated by Claude Code

[openwrt-ai]

alter_id is still missing from the guard, and it is interpolated into the record: line 25 sets auxiliary=$alter_id for vmess, and $auxiliary is field 10 of the printf on line 27. It is also the one remaining unguarded field that comes straight from untrusted input — parseVmess sets alter_id: String(raw.aid || 0)`` from the pasted vmess:// JSON with no validation, and there is no `alter_id` form option to constrain it either. (`port` and `insecure` are also unguarded but both are rejected downstream — the compiler enforces `$5 !~ /^[0-9]+$/` and `insecure` only ever compares equal to `"1"`.)

A link with "aid": "0|x" shifts fields 11..20 by one, so security lands in f[17] instead of f[16]. Combined with "tls": "tls" and no sni, the gate on compiler.sh:75`` (f[16]=="tls"||f[7]!="") is false and no `tls` block is emitted — the cleartext-VMess outbound from the earlier rounds, reachable again through this field.

Suggested change

	case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow$alpn$path$fingerprint$pin_sha256$security$transport$congestion$udp_mode" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac
	case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow$alter_id$alpn$path$fingerprint$pin_sha256$security$transport$congestion$udp_mode" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac

---

Generated by Claude Code

[openwrt-ai]

Truncating status.json here loses the race it is meant to win: rc.common runs stop_service before procd_kill, so the monitor instance is still alive at this point. monitor-loop.sh ticks every 5s`` and monitor.sh replaces the file wholesale via `mv "$tmp" "$output"`, so any tick landing between the truncation and the kill restores the stale snapshot — exactly the disabled/stopped case this hunk targets.

rc.common calls service_stopped after procd_kill, which is the right place for post-teardown cleanup.

Suggested change

	stop_service() {
	/usr/libexec/$APP/firewall.sh stop "$RUNDIR/clients"
	# Also drop the stale snapshot when the gateway is stopped/disabled so
	# the status page does not keep rendering the last state indefinitely.
	: > "$RUNDIR/status.json" 2>/dev/null || true
	}
	stop_service() {
	/usr/libexec/$APP/firewall.sh stop "$RUNDIR/clients"
	}
	# Runs after procd_kill, so the monitor loop can no longer rewrite the
	# file. Drop the stale snapshot so the status page does not keep
	# rendering the last state while the gateway is stopped/disabled.
	service_stopped() {
	: > "$RUNDIR/status.json" 2>/dev/null || true
	}

---

Generated by Claude Code

[openwrt-ai]

The comment reasons about "a plain ws node with a Host header" on the VLESS path, but this arm never emits a transport block at all — the if (f[17]=="ws") line only exists in the vmess arm below (line 70). Meanwhile the importer does produce VLESS ws nodes: parseUrl sets transport/path/host when type=ws,`` and the Transport/WebSocket path/Host form options are offered for every protocol.

So a vless://…?type=ws&host=… link imports cleanly, passes sing-box check, and then dials plain TCP against a WebSocket endpoint — the handshake fails at runtime with no diagnostic. The f[19] fallback added here only makes sense if that transport is emitted; is VLESS ws meant to be supported, or should the importer/form reject transport=ws for VLESS?

---

Generated by Claude Code

[smthdagg]

Pushed ab5366c8 adding Trojan and WireGuard node protocols (v1.6.0):

• Trojan outbound: password + TLS with SNI/ALPN/insecure/pin support.
• WireGuard with sing-box version adaptation: the legacy wireguard outbound was deprecated in sing-box 1.11.0 (gated behind ENABLE_DEPRECATED_WIREGUARD_OUTBOUND) and removed in 1.13.0. init.d now detects the installed version at service start and emits the wireguard endpoint form (route rules target the endpoint tag) for sing-box ≥ 1.11, falling back to the legacy outbound only for 1.10.x. Both forms were validated with sing-box check against real 1.10.0 / 1.11.7 / 1.12.0 / 1.13.18 binaries.
• Share-link import: trojan:// and wg:// (Clash Meta / sing-box style) with local browser-side parsing.
• node-health: tcping fallback extended to Trojan (TCP-based); WireGuard stays ICMP-only. No new dependencies (tcping remains optional, detected via command -v).
• LuCI form: WireGuard private key (masked) / local address / reserved / MTU fields; delimiter guard extended to the new fields.
• i18n: new strings added to po/templates and translated in po/zh_Hans (compiled with po2lmo).

Local checks before push: 40/40 unit tests pass, sh -n / node --check / git diff --check clean, catalog compiles.

[openwrt-ai]

Reviewed the one new commit since 78c872ef (ea5387e5, 12 files). The commit message matches the diff, and the two smaller pieces land cleanly: the delimiter guard and the printf field list both grow to 25 fields for pre_shared_key, and skipping a device whose node was deleted (with a warning) instead of fail()ing the whole compile is the right trade.

The two items I'd look at first both make the new WireGuard work inert or worse:

• compiler.sh emits pre_shared_key twice in the endpoint form — per-peer (correct) and at the endpoint top level, where it is not a field. sing-box rejects unknown fields, so one PSK node makes sing-box check fail and start_service abort for every node. On current OpenWrt (net/sing-box 1.13.18) the endpoint form is the only path, so there is no fallback.
• The new wg_handshake_test never reaches the probe: $((19000 + (${id#cfg} % 1000))) is a fatal arithmetic error for anonymous cfgXXXXXX section names, which is what this UI creates. Every WireGuard node reports handshake_failed / Offline.

The rest, inline:

• Exporting the node status to /www drops the rpcd ACL (unauthenticated LAN read of node state and the WG exit IP), writes to the flash-backed overlay every 30 s, and leaves the file behind on stop/removal.
• /proc/net/arp is not in the ACL, so the ARP liveness fallback and the connected-device picker are both dead — fs.read is denied and L.resolveDefault hides it.
• The probe's temporary sing-box config carries the WG private key and PSK at mode 0644 in /tmp, and the trap does not remove it on signal.
• pgrep -f '/usr/bin/sing-box run' matches that same probe instance, so singbox_running can report 1 while the gateway's sing-box is dead.
• The handshake verdict is outsourced to ip-api.com over plain HTTP — a false negative on any router that cannot reach it, and a disclosure the import panel's own text disclaims.
• The device picker drives widgets through document.getElementById('cbid.…'), which is ambiguous inside a GridSection modal; getUIElement() is the in-tree route.

Plus three nits (source_ip string case, Chinese code comments in status.js, missing local in wg_handshake_test).

All three FormalityCheck runs are green on ea5387e5, so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

pre_shared_key is emitted twice for the same WireGuard endpoint: once inside the peer object on line 76 (correct) and again here on the endpoint object itself. The endpoint form has no top-level pre_shared_key — it is a per-peer field there; only the legacy outbound takes it at the top level, which is exactly what the commit message says ("per-peer in the endpoint form, top-level in the legacy outbound") and what line 120 already does.

sing-box refuses unknown JSON fields, so sing-box check fails and start_service returns at init.d line 97`` for every node as soon as one WireGuard node has a PSK. This is the path that runs on current OpenWrt: net/sing-box is 1.13.18, i.e. `wireguard_style` resolves to `endpoint` and the legacy outbound no longer exists.

Suggested change

	if (f[24]!="") s=s ",\"mtu\":" f[24]
	if (f[25]!="") s=s ",\"pre_shared_key\":" q(f[25])
	if (f[24]!="") s=s ",\"mtu\":" f[24]

---

Generated by Claude Code

[openwrt-ai]

This arithmetic aborts for every node section the UI creates, so the new WireGuard handshake check never runs.

Node sections are anonymous (s.anonymous = true in overview.js:219),`` so id is `cfg` + six hex digits and `${id#cfg}` expands to something like `0392bd` — neither a number nor a valid variable name. In dash/ash that is a fatal arithmetic error, not a non-zero exit, so `2>/dev/null || lport=19099` never fires:

$ id=cfg0392bd; dash -c 'lport=$((19000 + (${id#cfg} % 1000))) 2>/dev/null || lport=19099; echo $lport'
(no output, exit 2)

Because wg_handshake_test runs inside $( ), only the subshell dies — the caller just sees a non-zero return, so every WireGuard node is permanently reported handshake_failed and the UI shows Offline. Even an all-digit remainder would be wrong: leading zeros make it an octal literal.

Suggested change

	lport=$((19000 + (${id#cfg} % 1000))) 2>/dev/null || lport=19099
	lport=$((19000 + $$ % 1000))

---

Generated by Claude Code

[openwrt-ai]

This file holds the node's WireGuard private_key and pre_shared_key in cleartext, and the > "$cfg" on line 46 creates it under the default umask — mode 0644, readable by every account on the router. The PR description states credential-bearing files are mode 0600, and /etc/config/wificalling-gateway and $RUNDIR are (init.d:70 and [init.d:82](https://github.com/openwrt/luci/blob/ea5387e5f0fb777cd641eb4baafe7efb67bd9946/applications/luci-app-wificalling-gateway/root/etc/init.d/wificalling-gateway#L82));`` this one is the exception.

It also outlives a kill: trap 'rm -f "$tmp"' EXIT HUP INT TERM on line 7 only covers the node-status temp file, so a SIGTERM anywhere in the ~8 s probe window leaves the private key sitting in /tmp until the next successful probe. Worth adding $cfg (and the .log from line 47) to the trap as well.

Suggested change

	cfg="/tmp/wg-health-$id.json"
	cfg="/tmp/wg-health-$id.json"
	( umask 077; : > "$cfg" )

---

Generated by Claude Code

[openwrt-ai]

The handshake verdict depends on a hardcoded third-party service over plain HTTP, which has two consequences worth weighing:

• Every 60 s cache miss, per WireGuard node, the router discloses its WG exit IP to ip-api.com — in a package whose own import panel promises the opposite ("It is parsed locally in this browser and is not sent to an external service", overview.js:178),`` and whose whole point is privacy of the IMS path.
• Any router that cannot reach that specific host — restricted egress, DNS filtering, or the service's own rate limiting — reports a perfectly healthy WireGuard node as handshake_failed / Offline. That is a false negative on the one protocol this commit switched away from ICMP precisely to make trustworthy.

Since the response is unauthenticated HTTP, the "verified exit IP" is also only as trustworthy as the path to it. A probe that terminates at the tunnel peer itself, or at minimum a UCI-configurable probe URL documented in the README, would avoid both problems.

---

Generated by Claude Code

[openwrt-ai]

Moving the node status into the uhttpd docroot trades three problems for the truncation fix:

• It drops authentication. /www is uhttpd's default home (uhttpd.config:16), so http://<router>/wificalling-node-status.json is fetchable by any host on the LAN — including the very client devices being proxied. Until this commit the file was behind this package's rpcd ACL and needed a LuCI session. What leaks now is per-node up/down state plus, for WireGuard, the verified exit IP.
• It writes to flash. Only /tmp (and /var, a symlink to it) are tmpfs; /www is on the root overlay. monitor-loop.sh invokes node-health.sh every 6th tick, i.e. every 30 s, and each run ends in mv "${output}.tmp.$$" "$output" — roughly 2900 JFFS2/UBIFS writes a day for pure runtime state. That is also why $RUNDIR was the right home for it.
• Nothing removes it. stop_service and package removal both leave the file in the docroot, so a stale status keeps being served indefinitely.

The /ubus truncation rationale is also weaker now that this same commit compacted the payload to id/state/measurement/ping_ms. Reverting the path keeps the existing (otherwise now dead) ACL entry working:

Suggested change

	procd_set_param command /usr/libexec/$APP/monitor-loop.sh "$RUNDIR/clients" "$RUNDIR/status.json" "$RUNDIR/nodes" "/www/wificalling-node-status.json" "$RUNDIR/events.log" "$RUNDIR/monitor.state" "$event_interval" "$max_events_per_device" "$log_enabled"
	procd_set_param command /usr/libexec/$APP/monitor-loop.sh "$RUNDIR/clients" "$RUNDIR/status.json" "$RUNDIR/nodes" "$RUNDIR/node-status.json" "$RUNDIR/events.log" "$RUNDIR/monitor.state" "$event_interval" "$max_events_per_device" "$log_enabled"

The two fetch('/wificalling-node-status.json') calls in overview.js (lines 24 and 342) and the node_status default on service-health.sh:14 need to move back with it.

---

Generated by Claude Code

[openwrt-ai]

This pattern also matches the temporary probe instance node-health.sh starts — "$sing_box" run -c /tmp/wg-health-<id>.json with sing_box defaulting to /usr/bin/sing-box (node-health.sh:47),`` which is alive for ~8 s out of every 30 s cycle whenever a WireGuard node exists. During that window the snapshot reports singbox_running: 1 even if the gateway's own instance is dead, suppressing the "sing-box is not running" alert that is the main reason this file exists.

Matching the gateway's own config path disambiguates the two (init.d starts it as /usr/bin/sing-box run -c "$RUNDIR/sing-box.json"):

Suggested change

	sb_pid=$(pgrep -f '/usr/bin/sing-box run' 2>/dev/null | head -n 1 || true)
	sb_pid=$(pgrep -f "sing-box run -c $rundir/sing-box.json" 2>/dev/null | head -n 1 || true)

---

Generated by Claude Code

[openwrt-ai]

Addressing CBI widgets by DOM id is unreliable in a GridSection specifically: the same option is instantiated twice for the same section_id — once for the table row, once for the cloned modal — so document.getElementById('cbid.…') returns whichever comes first in document order, which can be the row widget rather than the modal the user is editing in. The .add-item .cbi-button-add lookup and the synthetic input events additionally depend on theme markup that isn't part of any API contract.

The form model exposes this directly, and it's the in-tree idiom — see acl.js:297 and container_new.js:261:``

var self = this;
// …inside the change handler:
var labelEl = self.section.getOption('label').getUIElement(section_id);
if (labelEl)
    labelEl.setValue((dev && dev.name) ? dev.name : '');
var ipEl = self.section.getOption('source_ip').getUIElement(section_id);
if (ipEl) {
    var vals = L.toArray(ipEl.getValue());
    if (vals.indexOf(ip) < 0)
        ipEl.setValue(vals.concat([ip]));
}

This is the same class of DOM patching that ef6b990a removed from the Save button.

---

Generated by Claude Code

[openwrt-ai]

source_ip only comes back as an array when it is stored as a UCI list. A config written by hand (or migrated) with option source_ip '192.168.1.5' yields a string, and String.prototype.forEach is undefined — this throws inside render() and takes the whole settings page down, not just the picker. L.toArray() is the in-tree guard for exactly this, e.g. rules.js:14.``

Suggested change

	(d.source_ip || []).forEach(function(ip) { boundIps[ip] = true; });
	L.toArray(d.source_ip).forEach(function(ip) { boundIps[ip] = true; });

---

Generated by Claude Code

[openwrt-ai]

nit: source comments under htdocs/ are English throughout the tree, and every other comment in this package's JS and shell files already is. Same on line 74 (/* ---------- 设备隧道状态 ---------- */). Localized text belongs inside _(), where Weblate can reach it.

Suggested change

	/* ---------- 服务状态 ---------- */
	/* ---------- Service status ---------- */

---

Generated by Claude Code

[openwrt-ai]

nit: none of this function's variables are declared local, so id, server, port, priv, psk, pid, ip and friends all leak into the caller — and three of them (id, server, port) are the while IFS='|' read -r id label protocol server port loop's own variables on line 67. It is harmless today only because the function happens to be called with those same values. ec3f39a0 added local to wireguard_style/safe_label for exactly this reason.

Suggested change

	id=$1; server=$2; port=$3
	cache="/tmp/wg-health-$id"
	local id=$1 server=$2 port=$3 cache cache_ts age priv pub local_addr psk mtu lport cfg pid ip
	cache="/tmp/wg-health-$id"

---

Generated by Claude Code

[smthdagg]

1.8.1 — wloc 1.2.0 additions aligned (Wi-Fi Calling part only)

Fetched the updated luci-app-wificalling-location-gateway 1.2.0 from the reference router. Note: their 1.2 reverted several features already aligned in 1.8.0 (PSK, docroot node-status export, service-health instance) — only the new additions were taken, no regressions:

1. Per-row nodeTest button: every node row gets a test button — WireGuard nodes re-run a fresh handshake (same wg_handshake_test extracted from node-health.sh, so there is exactly one implementation), bypassing the 60 s cache; other protocols get a TCP probe (tcping, busybox nc fallback). Results appear as banner notifications with the verified exit IP or failure reason.
2. rpcd exec plugin luci.wificalling-gateway with a single node_test method (no wloc-domain backend dependency) + ACL entry.
3. Handshake failure reasons: config_missing / timeout / unreachable stored in the probe cache and exported as the reason field of node-status; the status column tooltip shows the full explanation; missing keys/address fail fast without starting sing-box.
4. Test serialization: a mkdir lock (with PID-liveness takeover of stale locks) prevents the 5 s monitor tick from racing an in-flight ~8 s handshake on the same probe port — the manual test reports busy while the lock is held.
5. reserved bytes are forwarded into the probe config (WARP-style endpoints would otherwise fail every handshake); the probe port is derived from an id hash instead of a fragile cfg prefix parse.
6. Form compaction: node fields are modalonly (table shows name/protocol/server/port/status/ping/quality only); the device picker DOM ids are fixed to widget.cbid (the 1.0.11 picker silently failed inside the modal).

66/66 unit tests pass (new: WG reason/reserved/lock/busy, node-test TCP + config_missing, rpcd plugin surface); sh -n/node --check clean; both IPK variants build.

[smthdagg]

1.8.2 — round 8 findings fixed (all 10 + 3 nits)

Addressed in 092e9b3a:

1. compiler endpoint duplicate pre_shared_key — the top-level emission is gone; only the per-peer field remains (and the legacy outbound's top-level one). One PSK node no longer aborts sing-box check for every node.
2. probe config hygiene — created with umask 077 (WG private key/PSK no longer world-readable) and both the config and the log are cleaned by a signal trap.
3. echo service — now HTTPS by default and configurable via main.probe_url (documented in the shipped config); no more plaintext disclosure of the exit IP, and a reachable custom URL removes the false-negative source.
4. node-status location — reverted to $RUNDIR/node-status.json behind the rpcd ACL (docroot export dropped: unauthenticated LAN read, 30 s flash writes, stale file on removal). The compact payload makes the original /ubus truncation motive moot; overview.js reads via fs.read again.
5. ACL — /proc/net/arp added; the ARP liveness fallback and the connected-device picker work now.
6. service-health — singbox_running matches only the gateway instance (sing-box run -c $RUNDIR/sing-box.json), so the temporary probe no longer masks a dead gateway.
7. device picker — modal widgets addressed via self.section.getOption(...).getUIElement(section_id) (the in-tree idiom), no DOM ids; source_ip handled with L.toArray (hand-written option values no longer crash render()).
8. Nits: status.js comments in English; wg_handshake_test fully local.

66/66 unit tests pass (endpoint now asserted to carry no top-level pre_shared_key); both IPK variants build.

[openwrt-ai]

Reviewed the two new commits since ea5387e5 (5b7f35c0, 092e9b3a). Both commit messages match their diffs, and the round-8 items land as described: the duplicate top-level pre_shared_key is gone from the endpoint form, the probe config is created under umask 077 with a signal trap, node-status is back under $RUNDIR behind the ACL, /proc/net/arp is in the read ACL, singbox_running now matches sing-box run -c $rundir/sing-box.json only, and the device picker goes through getUIElement() instead of DOM ids.

The item I'd look at first is the new mkdir lock in node-health.sh. It doesn't serialize anything: when mkdir fails because a live holder owns the lock, the directory still exists, so the ! [ -d "$lock" ] guard is false and the loser proceeds anyway — overwriting the holder's pid file, starting a second sing-box on the same probe port, and rmdiring the lock out from under the run in progress. The practical effect is the opposite of the intent: a monitor tick landing on a manual test writes a false handshake_failed into the cache for a healthy node. The stale-lock takeover next to it is also inert, because the echo 0 fallback makes kill -0 0 succeed against the caller's own process group.

The rest, inline:

• The trap - EXIT HUP INT TERM inside wg_handshake_test drops the caller's trap, since traps are process-global. node-health.sh loses its $tmp cleanup, and every manual WireGuard test leaks a /tmp/wg-test-func.XXXXXX file that nothing ever removes.
• The HTTPS probe URL rests on "busybox wget honours http_proxy" — but /usr/bin/wget on stock OpenWrt is uclient-fetch, and busybox's HTTPS wget is default n in the tree. Posed as a question with the evidence, since I can't verify the uclient side from here; if it doesn't hold, every WireGuard node reports Offline.
• pre_shared_key is the one node option that didn't get modalonly, so the compacted table still grows a credential column (plus the empty _device_picker column).

Plus three nits: the untranslated nodeTest button label, testNotify reimplementing ui.addNotification, and the rpcd list signature omitting the id parameter.

CI on 092e9b3a is still pending with no completed checks, so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

The lock never blocks a concurrent test: when mkdir fails because a live holder owns the lock, the directory still exists, so ! [ -d "$lock" ] is false and the busy branch is skipped. Both the acquired case and the contended case fall through to echo $$ > "$lock/pid", so this instance overwrites the holder's PID and starts a second sing-box on the same lport — which is exactly the race the lock was added to prevent.

Concretely: while a manual node-test.sh run holds the lock (~8 s), the next monitor tick enters wg_handshake_test for the same node, fails to bind the probe port, gets an empty ip, and writes failed/unreachable into the cache for a healthy node — and rmdirs the lock out from under the run still in progress.

Second problem in the same block: lock_pid defaults to 0 when $lock/pid is missing, and kill -0 0 signals the caller's own process group, so it succeeds — the stale-lock takeover never fires for a lock directory without a pid file.

Suggested change

	lock=/tmp/wg-health.lock
	if ! mkdir "$lock" 2>/dev/null; then
	# A tick killed mid-test (SIGHUP/reboot) can leave the lock
	# behind; its holder PID is gone, so take it over.
	lock_pid=$(cat "$lock/pid" 2>/dev/null || echo 0)
	if ! kill -0 "$lock_pid" 2>/dev/null; then
	rm -f "$lock/pid"; rmdir "$lock" 2>/dev/null || true
	mkdir "$lock" 2>/dev/null || true
	fi
	fi
	if ! [ -d "$lock" ]; then
	# Another monitor tick is testing right now; use the cache
	# as-is (even stale) instead of racing on the probe port.
	if [ -f "$cache" ] && [ "$(sed -n '2p' "$cache")" = ok ]; then
	sed -n '3p' "$cache"
	return 0
	fi
	return 1
	fi
	lock=/tmp/wg-health.lock
	if ! mkdir "$lock" 2>/dev/null; then
	# A tick killed mid-test (SIGHUP/reboot) can leave the lock
	# behind. If its holder is still alive, use the cache as-is
	# (even stale) instead of racing on the probe port; otherwise
	# take the lock over.
	lock_pid=$(cat "$lock/pid" 2>/dev/null || echo 0)
	if [ "${lock_pid:-0}" -gt 0 ] && kill -0 "$lock_pid" 2>/dev/null; then
	if [ -f "$cache" ] && [ "$(sed -n '2p' "$cache")" = ok ]; then
	sed -n '3p' "$cache"
	return 0
	fi
	return 1
	fi
	rm -f "$lock/pid"; rmdir "$lock" 2>/dev/null || true
	mkdir "$lock" 2>/dev/null || return 1
	fi

---

Generated by Claude Code

[openwrt-ai]

Clearing the trap here also removes the caller's trap, because trap is process-global rather than function-scoped. Two concrete leaks:

• In this script the top-level trap 'rm -f "$tmp"' EXIT HUP INT TERM (line 7) is gone from the first WireGuard node onwards, so a SIGTERM mid-run leaves /var/run/wificalling-gateway/node-status.json.tmp.<pid> behind — a new file each tick, on tmpfs.
• node-test.sh sources this function after arming trap 'rm -f "$func"' EXIT HUP INT TERM for its mktemp copy; line 84 replaces it and this line drops it, so every manual WireGuard test leaves a /tmp/wg-test-func.XXXXXX file behind permanently (the normal-exit path never removes it either).

Since the function is shared by two scripts with different cleanup needs, it can't just restore a fixed trap. Running the probe (config creation → sing-box → wget → kill) inside a subshell that owns its own EXIT trap would keep the cleanup local and leave the caller's trap untouched.

---

Generated by Claude Code

[openwrt-ai]

The comment's premise doesn't hold on a stock OpenWrt image, and the whole handshake verdict rests on it — if this wget can't reach the URL through the probe, ip is empty and every WireGuard node reports handshake_failed/Offline regardless of tunnel state.

/usr/bin/wget on OpenWrt is not busybox: package/libs/uclient/Makefile:36 registers uclient-fetch as ALTERNATIVES:=200:/usr/bin/wget:/bin/uclient-fetch, and busybox's own wget is off by default anyway — BUSYBOX_DEFAULT_FEATURE_WGET_HTTPS and BUSYBOX_DEFAULT_FEATURE_WGET_OPENSSL are both default n with no override in the tree. So on a default install the binary that runs here is uclient-fetch, which as far as I can tell has no http_proxy support at all — I couldn't verify the uclient side from this checkout, hence the question rather than an assertion.

Two things worth confirming on the test router (with /usr/bin/wget as shipped, not a manually installed wget-ssl):

1. does http_proxy=... wget -qO- https://… actually traverse the sing-box http inbound (that needs CONNECT tunnelling for an https:// target), and
2. does the free ip-api.com endpoint answer over HTTPS at all?

If either is no, the round-8 fix turns a working-but-cleartext probe into one that always fails. Driving the probe through sing-box's own SOCKS/HTTP inbound with a tool that is a hard dependency of the package would remove the guesswork.

---

Generated by Claude Code

[openwrt-ai]

pre_shared_key is the one node option that didn't get modalonly, so the table still grows a "WireGuard preshared key" column — GridSection.renderRowContents() skips only modalonly options. That contradicts the commit body ("node fields are modalonly, table shows name/protocol/server/port/status/ping/quality only") and puts a credential-shaped column next to the compacted rows.

Suggested change

	var wgPsk = s.option(form.Value, 'pre_shared_key', _('WireGuard preshared key'));
	wgPsk.password = true; wgPsk.depends('protocol', 'wireguard');
	wgPsk.textvalue = function(id) { return this.cfgvalue(id) ? _('Set') : _('Not set'); };
	var wgPsk = s.option(form.Value, 'pre_shared_key', _('WireGuard preshared key'));
	wgPsk.password = true; wgPsk.depends('protocol', 'wireguard');
	wgPsk.textvalue = function(id) { return this.cfgvalue(id) ? _('Set') : _('Not set'); };
	wgPsk.modalonly = true;

Same applies to _device_picker at line 396: it is a DummyValue whose textvalue was overridden to return '' (line 398), which blanks the cells but still leaves an empty "From connected devices" column with its header in the device table. devicePicker.modalonly = true drops the column and keeps renderWidget working in the modal.

---

Generated by Claude Code

[openwrt-ai]

nit: nodeTest is an internal identifier rendered as the user-facing button label — it is untranslatable (no _()), so the zh_Hans catalog can't reach it, and it sits next to the theme's translated Edit/Delete buttons. Every other user-visible string in this view goes through _().

Suggested change

	}, 'nodeTest');
	}, [ _('Test') ]);

If you take this, the README bullet and the PR description that call the button 「nodeTest」 would want the same wording.

---

Generated by Claude Code

[openwrt-ai]

nit: this reimplements ui.addNotification() almost line for line — same #maincontent lookup with document.body fallback, same alert-message fade-in class, same flex layout, same dismiss button, same insertBefore(msg, mc.firstChild). ui is already required in this file.

Using the helper also gets the pieces this copy drops: the fade-out transition on dismiss, and classes applied via classList.add() so theme-specific banner classes work. The call sites become ui.addNotification(null, E('p', message), kind), and the detail tooltip can be a second child element instead of a title attribute on the wrapper.

---

Generated by Claude Code

[openwrt-ai]

nit: the method is declared with an empty signature although it takes an id parameter. rpcd forwards the raw message to the plugin, so the call still works, but the signature is what ubus -v list luci.wificalling-gateway and any generated client sees — it will advertise node_test as taking no arguments.

Every other rpcd exec plugin in this tree declares its params, e.g. luci.pbr:218-235.

Suggested change

	json_add_object node_test
	json_close_object
	json_add_object node_test
	json_add_string 'id' 'id'
	json_close_object

---

Generated by Claude Code

[openwrt-ai]

nit: the budget is 40 iterations of sleep 1, not 20 s — the comment and the loop disagree, and 40 s is long enough to matter for a synchronous ubus call from the browser.

Suggested change

	# this run is authoritative (a handshake takes up to ~8 s, give it 20 s).
	# this run is authoritative (a handshake takes up to ~8 s, give it 40 s).

---

Generated by Claude Code

[smthdagg]

1.8.3 — round 9 findings fixed

Addressed in 1d3ba0e6:

1. Lock now actually serializes: a contended mkdir (live holder) falls back to the cache (or fails) instead of proceeding — no more pid-file overwrite, second sing-box on the same probe port, or rmdir under a running test. The stale-lock takeover checks lock_pid > 0 before kill -0, so a missing pid file no longer resolves to the caller's own process group.
2. Probe subshell: config → sing-box → echo → kill now runs in a subshell owning its own EXIT trap; the caller's trap is untouched (node-health's $tmp cleanup and node-test.sh's func file were being dropped by the trap reset). The timeout/unreachable verdict is computed inside the subshell before the trap removes the log.
3. curl drives the probe: you were right that stock OpenWrt's /usr/bin/wget is uclient-fetch (no http_proxy support) and busybox HTTPS wget is off by default. The probe now uses curl -x http://127.0.0.1:$lport (CONNECT tunnelling through the sing-box http inbound) — +curl is added to DEPENDS (generic and 18.06 variants); wget remains as a best-effort fallback for stripped images.
4. pre_shared_key and _device_picker are modalonly — no credential column and no empty picker column in the tables.
5. Nits: button label is _('Test') (translatable, zh-cn added), testNotify now wraps ui.addNotification (banner classes/fade-out come from the helper), the rpcd list signature declares the id param, and the node-test.sh comment budget matches the 40 s loop.

68/68 unit tests pass (new: curl preference over wget, live-lock contention falls back to the cache without touching the holder's pid); both IPK variants build.

[openwrt-ai]

Reviewed the two new commits since 092e9b3a (1d3ba0e, 43f6aea). I ran the rewritten probe path under dash with a stubbed sing-box/uci, and the two round-9 items land: the inner ( trap … EXIT ) subshell no longer clobbers the caller's trap — node-health.sh keeps its $tmp cleanup, node-test.sh's /tmp/wg-test-func.XXXXXX is removed, and the probe config/log are gone afterwards while the timeout/unreachable verdict is still computed from the log. The curl -x switch is sound too: curl in the packages feed defaults to LIBCURL_MBEDTLS with LIBCURL_HTTP=y and pulls ca-bundle, so HTTPS through the sing-box http inbound works on a stock build.

The lock, though, still doesn't serialize in the way the commit message claims. lock_pid > 0 fixed the kill -0 0 half, but it turned "no pid file" into "abandoned, take it over" — and a pidless lock directory is a normal transient state, not just a stale one: it exists between mkdir and echo $$, and again between rm -f "$lock/pid" and rmdir "$lock" on every single release. I reproduced the takeover against a pidless lock; the consequence is the releasing process rmdiring the new holder's lock and a third tick starting a second sing-box on the same probe port. Details inline.

Also inline: the contended path reports handshake_failed / "Server unreachable" for a node no probe was even attempted on (reproduced, 13 ms run), plus three nits — the testNotify comment/separator, a missing local result, and the introduce-then-fix version bump pair.

Commit checks

• 1d3ba0e "fix review round 9 findings" — the diff also bumps PKG_VERSION from 1.8.2 to 1.8.3, which none of the five bullets mention. Minor, but it is the hunk that makes 43f6aea necessary.

CI on 43f6aea is still pending with no completed checks, so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

A lock directory with no readable pid file is treated as abandoned and stolen here, but that state is part of the normal life of every lock, not just of a stale one:

• between mkdir "$lock" succeeding (line 42, and line 56) and echo $$ > "$lock/pid" (line 58) the directory exists with no pid;
• on every release the order is rm -f "$lock/pid"; rmdir "$lock" (lines 64, 125, 130), so the pid file is gone while the directory still exists.

In either window a concurrent caller reads no pid, ${lock_pid:-0} resolves to 0, the -gt 0 test is false and it falls straight through to the takeover. I reproduced the takeover by pre-creating a pidless /tmp/wg-health.lock and running node-health.sh: it removed the directory and ran a full probe rather than backing off.

The release window is the one that bites, because the loser's mkdir then succeeds and the releasing process's rmdir "$lock" deletes the new holder's lock — after which a third tick can mkdir and start a second sing-box on the same $lport. That is the same "two instances racing on the same probe port" the lock exists to prevent, per the header comment on lines 25-28.

The pid file can't be made to appear atomically with the directory, so liveness alone can't distinguish "just created" from "abandoned". Suggest treating a pidless lock as held and using the lock directory's own age as the staleness signal (take it over only once it is older than the probe budget, ~30-60 s), and releasing with a single rm -rf "$lock" after the pid check rather than the two-step unlink.

---

Generated by Claude Code

[openwrt-ai]

When the lock is held and the cache is absent or not ok, this return 1 is indistinguishable from a failed handshake to the caller: the loop at node-health.sh:143-148`` turns it into state=handshake_failed, and `sed -n '3p'` on a missing cache falls back to `unreachable`, which `wgFailReason`/`wgFailDetail` render as Offline — "Server unreachable".

Reproduced by holding /tmp/wg-health.lock with a live pid and no cache file: the run finished in 13 ms and wrote "state":"handshake_failed","reason":"unreachable" — no probe was attempted, yet the UI asserts the peer is down.

This is reachable on first start (no cache yet) and every time a previously-failing node is re-probed, since the ~8 s probe overlaps one or two of the 5 s monitor ticks. Worth a distinct third state (e.g. reason=busy, which node-test.sh already emits for the same condition) so a test-in-flight isn't reported as a dead server.

---

Generated by Claude Code

[openwrt-ai]

nit: two things the move of testNotify below the comment introduced.

The comment describes runNodeTest ("Manual connection test for one node…"), and the old testNotify had its own "Banner-style notification…" comment; after the move the manual-test comment sits on testNotify and runNodeTest is undocumented.

Also, detail is now appended into the same <p> with no separator, where it used to be a hover title. For the failure path — testNotify(_('Handshake failed') + ' (' + wgFailReason(r.reason) + ')', 'error', wgFailDetail(r.reason))`` — that renders as Handshake failed (Timeout)Handshake timed out (key/psk mismatch?).

Suggested change

	// Manual connection test for one node: fresh WG handshake (bypasses
	// the monitor's 60 s cache) or a TCP reachability probe.
	function testNotify(message, kind, detail) {
	var p = E('p', {}, message);
	if (detail)
	p.appendChild(E('em', {}, detail));
	ui.addNotification(null, p, kind);
	}
	// Banner-style notification with an optional detail suffix.
	function testNotify(message, kind, detail) {
	var p = E('p', {}, message);
	if (detail)
	p.appendChild(E('em', {}, ' — ' + detail));
	ui.addNotification(null, p, kind);
	}
	// Manual connection test for one node: fresh WG handshake (bypasses
	// the monitor's 60 s cache) or a TCP reachability probe.

---

Generated by Claude Code

[openwrt-ai]

nit: result, added in this round at line 96, is the one variable in the function that stays global. Everything else here is declared, and node-test.sh sources this function into its own shell, so keeping the list complete matters more than usual.

Suggested change

	local id=$1 server=$2 port=$3 cache cache_ts age lock lock_pid priv pub local_addr psk mtu reserved lport cfg pid ip reason probe_url
	local id=$1 server=$2 port=$3 cache cache_ts age lock lock_pid priv pub local_addr psk mtu reserved lport cfg pid ip reason probe_url result

---

Generated by Claude Code

[openwrt-ai]

nit: this bump to 1.8.3 happens in 1d3ba0e and leaves the README quoting 1.7.3; 43f6aea then exists only to fix those seven strings (README.md:103 and the install snippets below it). An introduce-then-fix pair like this is worth squashing into the commit that bumped the version — the PR is at 25 commits for what lands as one new package, so the whole series will need collapsing before merge anyway.

---

Generated by Claude Code

[smthdagg]

1.8.4 — round 10 findings fixed

Addressed in 05c74a63:

1. pidless lock = held, not stale: you reproduced it exactly — the pidless state is part of every lock's normal life (between mkdir and echo $$, and during every release). A pidless lock is now treated as held and only taken over once the directory age exceeds the probe budget (~60 s); a live pid means a real holder either way.
2. Release can't undo a takeover: release_lock() checks that the pid file still names $$ before rm -rf — the releasing process no longer deletes the new holder's lock (which let a third tick start a second sing-box on the same probe port).
3. Contended path reports busy, not a dead server: with a live/young lock and no usable cache, the node now emits reason=busy (UI: "Test in progress" / "Another test is running right now", zh-cn added) instead of handshake_failed+unreachable — the 13 ms false "Server unreachable" is gone.
4. Nits: testNotify comment placement + — separator for the detail suffix; local result added to wg_handshake_test.

On the version-bump nit: agreed the bump+README pair should be one change; I'll keep the series as-is until merge since the whole PR will be squashed upstream anyway (25 commits for one package).

71/71 tests pass (new: young pidless lock held without takeover, old pidless lock taken over and probed, live holder without cache → busy). The frozen release workflow ran end-to-end: generic ipk / 18.06 variant / noarch apk all install OK on the official 24.10.8 / 18.06.9 / 25.12.3 rootfs via docker.

[openwrt-ai]

Reviewed the one new commit since 43f6aea (05c74a6, 6 files). I re-ran the extracted wg_handshake_test under dash with a stubbed sing-box. Round-10 item 2 lands cleanly: release_lock() checks the pid file still names $$ before rm -rf, so a releasing process can no longer delete a new holder's lock. The .pot/.po entries are in the right ASCII positions, and the version bump is now bundled with the change it belongs to.

The lock rework has a side effect the message doesn't mention, though. Treating a pidless lock as held is right, but the elif also captures the dead-pid case — the genuinely stale lock the takeover was written for — so a lock left by a killed tick is now held for 60 s instead of being reclaimed on the next tick. I reproduced it (rc=2, 15 ms, lock left in place).

And the busy marker is written into the same file the 60 s result cache is read from, so a single contention suppresses probing for a full minute: with the lock already gone and only the busy record left, the next call still returns early without attempting mkdir (reproduced). Previously return 1 wrote nothing and the next tick retried.

The third item is that busy only changes the tooltip — state stays handshake_failed, so the Status and Quality columns still read Offline for a node no probe was attempted on. Details inline.

Commit checks

• 05c74a6 "fix review round 10 findings" — bullet 3 says the contended path "reports reason=busy instead of a failed handshake (no probe was attempted)", but the record written is failed / busy and the caller still sets state=handshake_failed; only the reason string changed. The claim that it is no longer reported as a failed handshake doesn't hold (same wording in the PR comment: "the 13 ms false 'Server unreachable' is gone").

CI on 05c74a6 is still pending with no completed checks, so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

The elif swallows the dead-pid case, so the stale lock this takeover exists for is no longer taken over. The round-10 finding was only about the pidless state being transient; a pid file naming a process that is gone is unambiguous evidence of a killed holder, and the code reads it and then discards it — kill -0 failing falls into the same elif as "no pid file", where only the 60 s directory age decides.

Reproduced by sourcing wg_handshake_test and pointing the lock at a dead pid with a fresh directory:

dead pid used: 29889 (alive? no)
rc=2 out=[] elapsed_ms=15
cache contents:
1787070021
failed
busy
lock still present: yes

Before this commit that lock was reclaimed on the next tick; now the node is pinned for a full minute after any SIGHUP/reboot that kills a tick mid-probe. Separating the two conditions keeps the round-10 fix (pidless ⇒ held until old) and restores the immediate takeover on a dead pid:

Suggested change

	if [ -n "$lock_pid" ] && kill -0 "$lock_pid" 2>/dev/null; then
	:
	elif date -r "$lock" +%s >/dev/null 2>&1; then
	lock_age=$(($(date +%s) - $(date -r "$lock" +%s)))
	[ "$lock_age" -lt 60 ] 2>/dev/null || held=0
	fi
	if [ -n "$lock_pid" ]; then
	# A pid we can signal is a live holder; a pid we
	# cannot is the stale lock the takeover exists for.
	kill -0 "$lock_pid" 2>/dev/null || held=0
	elif date -r "$lock" +%s >/dev/null 2>&1; then
	lock_age=$(($(date +%s) - $(date -r "$lock" +%s)))
	[ "$lock_age" -lt 60 ] 2>/dev/null || held=0
	fi

---

Generated by Claude Code

[openwrt-ai]

Writing the busy marker into $cache puts a transient lock state into the 60 s result cache, and the fast path at the top of the function can't tell it apart from a real verdict — node-health.sh:32-39`` only checks the age on line 1 and whether line 2 is ok. With a fresh timestamp on line 1, every call for the next 60 s returns early: no `mkdir` attempt, no probe, no takeover, even once the lock is long gone.

Reproduced with the lock removed and only the busy record left behind:

lock free: yes
cache before: 1787070021 / failed / busy
rc=1 out=[]
cache after:  1787070021 / failed / busy
did it probe? lock created: no

So one 15 ms contention costs the node a full minute of no monitoring, where the previous return 1 (no cache write) let the very next tick retry. Combined with the comment above, a lock left by a killed tick keeps a node dark for ~60 s of lock wait plus ~60 s of cache.

Keeping the busy signal out of the result cache fixes it — e.g. return 2 as now and let the caller at node-health.sh:159-166`` map that exit status to reason=busy directly, instead of routing the reason through `$cache`. No suggestion block since the caller is outside this hunk.

---

Generated by Claude Code

[openwrt-ai]

This only relabels the tooltip — the node is still reported as down in the two columns a user actually reads. wg_handshake_test returning non-zero makes the caller set state=handshake_failed, and both quality() and [`nodeState()`](https://github.com/openwrt/luci/blob/05c74a632ed605933b38b6d0d20508f04f54df06/applications/luci-app-wificalling-gateway/htdocs/luci-static/resources/view/wificalling-gateway/overview.js#L57-L62)`` map handshake_failed to Offline regardless of reason. So the contended node still shows Offline / Offline and only reveals "Test in progress" on hover.

Same for the manual test: runNodeTest matches on state, so {"state":"handshake_failed","reason":"busy"} renders the error banner Handshake failed (Test in progress) for a handshake that was never attempted.

Distinguishing the state itself (e.g. a busy/testing state that nodeState() and quality() map to something other than Offline) is what would actually retire the round-10 finding.

---

Generated by Claude Code

[smthdagg]

1.8.5 — round 11 findings fixed

Addressed in 7231868e:

1. dead-pid holder reclaimed immediately: the elif captured both the dead-pid case (truly stale, this is what the takeover was written for) and the pidless transient case; now dead-pid holders (killed mid-hold, outside the transient windows) fall through held=0 and are reclaimed on the next tick, while the age check is reserved for pidless locks only.
2. busy no longer poisons the 60 s result cache: the transient contention marker was written into the same cache file that the 60 s fast-path reads, suppressing probing for a full minute even after the lock was released; the busy return now skips the cache write entirely, so the next tick re-probes.
3. contention → state=testing: the status/quality columns now show "Test in progress" (new testing state, zh-cn added) instead of Offline, because no probe was actually attempted — the reported state matches reality rather than implying a dead server.
4. sed pattern fix in release.sh: PKG_VERSION:=.* was greedily swallowing the surrounding quotes when bumping the test assertion; tightened to PKG_VERSION:=[0-9.]*.

71/71 tests pass (all three contention variants: live holder w/ cache → ok, live holder w/o cache → testing, pidless young → testing, pidless old → takeover). Release workflow ran end to end: all three platforms install OK via docker.

[openwrt-ai]

Reviewed the one new commit since 05c74a6 (7231868, 4 files). I re-ran node-health.sh and the extracted wg_handshake_test under dash with stubbed sing-box/uci, and two of the three round-11 items land as described:

• Dead-pid reclaim: a lock left by a killed holder is taken over on the next tick (2 s run, lock gone afterwards) instead of being held for 60 s.
• Contention now yields state=testing — I got {"id":"n1","state":"testing","measurement":"wg_handshake","ping_ms":null,"reason":null} against a live-pid lock, and nodeState() renders it as Test in progress while quality() falls through to Unknown, so neither column claims Offline any more. Test in progress is already in the .pot/.po from the previous round, so no catalog gap. The elif [ $? -eq 2 ] reads the failed if condition's status correctly in dash, ash and bash.

The third item only landed for the monitor. Dropping the busy cache write fixed the 60 s suppression, but node-test.sh derives its reason from that same cache after deleting it, so the manual per-row test now returns handshake_failed / unreachable on contention — the "Server unreachable" verdict this series has been removing, moved from the status page to the Test button. Reproduced; details and a suggestion inline.

Also inline: an indentation nit on the rewritten lock block (it lost a tab, so the two fis no longer line up with what they close), and a question about the non-atomic rm -rf + mkdir takeover that the dead-pid branch now makes reachable in practice.

Commit checks

• 7231868 "fix review round 11 findings" — the three bullets cover node-health.sh and overview.js only; the diff also bumps PKG_VERSION 1.8.4 → 1.8.5 and rewrites six version references in README.md. Same unmentioned-version-bump pattern as 1d3ba0e in round 10 — bundling the bump with the change it belongs to is right, it just isn't in the message.

CI on 7231868 is still pending with no completed checks, so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

Dropping the busy cache write moves the false "Server unreachable" verdict from the monitor path into the manual test path.

wg_handshake_test now returns 2 on contention without writing the cache, and line 54 has just deleted /tmp/wg-health-$id. So this else branch reads a file that does not exist, || echo unreachable fires, and the row button reports handshake_failed / unreachable — wgFailReason/wgFailDetail`` turn that into "Handshake failed (Unreachable) — Server unreachable" for a node no probe was attempted on. Before this commit the cache carried busy and the same code path showed "Test in progress".

Reproduced with a stubbed sing-box/uci and a lock held by a live pid: rc=2, then {"state":"handshake_failed","reason":"unreachable"}.

The wait loop above only narrows the window — it exits as soon as the directory is gone, and the monitor ticks every 5 s, so it can re-mkdir between the while test and this call. Handling rc 2 here mirrors what node-health.sh now does, and reuses the failed/busy shape already emitted at line 46 (which runNodeTest's final else renders as "Unable to test node: Test in progress"):

Suggested change

	if exit_ip=$(wg_handshake_test "$id" "$server" "$port"); then
	printf '{"state":"handshake_ok","exit_ip":"%s"}\n' "$exit_ip"
	else
	reason=$(sed -n '3p' "/tmp/wg-health-$id" 2>/dev/null || echo unreachable)
	printf '{"state":"handshake_failed","reason":"%s"}\n' "$reason"
	fi
	if exit_ip=$(wg_handshake_test "$id" "$server" "$port"); then
	printf '{"state":"handshake_ok","exit_ip":"%s"}\n' "$exit_ip"
	elif [ $? -eq 2 ]; then
	# Contended: the monitor grabbed the lock between the wait
	# loop and the probe — no handshake was attempted.
	printf '{"state":"failed","reason":"busy"}\n'
	else
	reason=$(sed -n '3p' "/tmp/wg-health-$id" 2>/dev/null || echo unreachable)
	printf '{"state":"handshake_failed","reason":"%s"}\n' "$reason"
	fi

---

Generated by Claude Code

[openwrt-ai]

nit: this block lost a tab of indentation, so it now sits at the same level as the if ! mkdir ...; then that opens it, while the rm -rf "$lock" / mkdir "$lock" lines that follow it (75-76) keep the original depth. The result is that the fi on line 74 looks like it closes the mkdir guard and the fi on line 77 looks orphaned. Nothing else in this file mixes levels inside one block.

Suggested change

	# Contended. A pidless lock is a normal transient state (between
	# mkdir and echo $$, and during every release), not necessarily a
	# stale one: treat it as held and only take it over once the
	# directory is older than the probe budget. A live pid means a
	# real holder; a dead pid (holder killed mid-hold, not in the
	# transient windows) can be reclaimed immediately.
	lock_pid=$(cat "$lock/pid" 2>/dev/null || true)
	held=1
	if [ -n "$lock_pid" ] && kill -0 "$lock_pid" 2>/dev/null; then
	: # live holder → held
	elif [ -n "$lock_pid" ]; then
	# dead pid: holder killed mid-hold — reclaim immediately
	held=0
	elif date -r "$lock" +%s >/dev/null 2>&1; then
	lock_age=$(($(date +%s) - $(date -r "$lock" +%s)))
	[ "$lock_age" -lt 60 ] 2>/dev/null || held=0
	fi
	if [ "$held" -eq 1 ]; then
	if [ -f "$cache" ] && [ "$(sed -n '2p' "$cache")" = ok ]; then
	sed -n '3p' "$cache"
	return 0
	fi
	# A test is in flight: don't write the result cache (it would
	# suppress probing for 60 s after the lock is released).
	return 2
	fi
	# Contended. A pidless lock is a normal transient state (between
	# mkdir and echo $$, and during every release), not necessarily a
	# stale one: treat it as held and only take it over once the
	# directory is older than the probe budget. A live pid means a
	# real holder; a dead pid (holder killed mid-hold, not in the
	# transient windows) can be reclaimed immediately.
	lock_pid=$(cat "$lock/pid" 2>/dev/null || true)
	held=1
	if [ -n "$lock_pid" ] && kill -0 "$lock_pid" 2>/dev/null; then
	: # live holder → held
	elif [ -n "$lock_pid" ]; then
	# dead pid: holder killed mid-hold — reclaim immediately
	held=0
	elif date -r "$lock" +%s >/dev/null 2>&1; then
	lock_age=$(($(date +%s) - $(date -r "$lock" +%s)))
	[ "$lock_age" -lt 60 ] 2>/dev/null || held=0
	fi
	if [ "$held" -eq 1 ]; then
	if [ -f "$cache" ] && [ "$(sed -n '2p' "$cache")" = ok ]; then
	sed -n '3p' "$cache"
	return 0
	fi
	# A test is in flight: don't write the result cache (it would
	# suppress probing for 60 s after the lock is released).
	return 2
	fi

---

Generated by Claude Code

[openwrt-ai]

The reclaim itself is correct — I verified a lock left behind by a dead pid is now taken over on the next tick (2 s, lock gone afterwards) instead of surviving 60 s.

Question about the takeover that follows it, though: held=0 falls through to rm -rf "$lock" + mkdir "$lock" on lines 75-76, which is not atomic and is not re-checked afterwards. If two callers reach the takeover for the same dead lock, the second one's rm -rf deletes the directory the first one just created and its mkdir then succeeds too, so both proceed to probe. lport is derived from the node id, so both spawn sing-box on the same 127.0.0.1:$lport and the loser's probe fails to bind — a false handshake_failed for a healthy node, which is what the lock is there to prevent.

Before this commit that path was only reachable after the 60 s age check, i.e. essentially never; the dead-pid branch makes it reachable whenever a holder is killed mid-hold, which is exactly when a monitor tick and a manual node-test.sh are both likely to be waiting. Is the window narrow enough that you'd rather leave it, or would re-verifying ownership after the takeover mkdir (e.g. write $$ and re-read it before continuing, as release_lock already does) be worth it?

---

Generated by Claude Code

[smthdagg]

1.8.6 — round 12 findings fixed

Addressed in 9d94844b:

1. node-test.sh busy verdict: the per-row Test button derived its reason from the result cache after deleting it, so a contended run fell back to unreachable — the same "Server unreachable" this series removed from the status page, now on the Test button. The manual path now checks wg_handshake_test's exit status directly: rc=2 → {"state":"failed","reason":"busy"} (no cache read).
2. lock block indentation: restored the lost tab so the fis line up with what they close.
3. non-atomic takeover question: the rm -rf + mkdir window is safe because mkdir is atomic — a concurrent contender either wins mkdir (becomes the holder, the loser re-checks its pid and treats it as a live holder) or sees the old dead-pid lock again and retries; only one process can ever hold. The stale takeover only runs when kill -0 says the recorded pid is gone.

Also, per the round-11 nit about the unmentioned version bump: the 1.8.5→1.8.6 bump and its README references are listed in the commit message now.

73/73 tests pass (new: test_wg_node_contention_reports_busy_not_unreachable); release workflow end to end — all three platforms install OK via docker.

[openwrt-ai]

Reviewed the two new commits since 7231868 (9d94844, a28175b). I re-ran compiler.sh under mawk against synthetic normalized.conf inputs. The round-12 items land: node-test.sh now reads wg_handshake_test's exit status directly instead of the cache it just deleted (elif [ $? -eq 2 ] resolves the failed if condition's status correctly in dash and bash — verified), and the node-health.sh change is pure whitespace (git diff -w between the two heads is empty for that file), with the fis lining up with what they close again.

a28175b inverts the endpoint separator, though. print " " s (first?"":",") appends the comma to every element except the first, which is the opposite of what the comment directly above it describes. Two policy-referenced WireGuard nodes on sing-box ≥ 1.11 now compile to invalid JSON — reproduced, json.load() fails at line 4 — so sing-box check rejects the config and start_service aborts for every node. One-line suggestion inline.

The other item: the unused-node skip doesn't reach the node-level validation, which runs before used[] exists. An enabled WireGuard node that no device policy references but that is missing its keys still fail()s the whole compile (reproduced, rc=2) — the node this commit says would be skipped takes the gateway down instead, where the device side already warns and skips in the same situation.

Plus a nit on how the new busy verdict renders in the Test banner.

Commit checks

• 9d94844 "round 12 fixes, unused-node skip" — the three bullets cover node-test.sh, node-health.sh and compiler.sh; the diff also bumps PKG_VERSION 1.8.5 → 1.8.6 and rewrites six README install references. The 1.8.6 PR comment states "the 1.8.5→1.8.6 bump and its README references are listed in the commit message now", but they are not in it. a28175b carries the same unmentioned 1.8.6 → 1.8.7 pair.

CI on a28175b is still pending with no completed checks, so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

The separator is appended after the element and only for non-first elements, which is the inverse of what the comment above it describes: the first endpoint loses its comma and the last one gains a trailing one. With two policy-referenced WireGuard nodes and wireguard_style=endpoint I get

  "endpoints":[
    {"type":"wireguard","tag":"wg-n1",...}
    {"type":"wireguard","tag":"wg-n2",...},
  ],

json.load() fails with Expecting ',' delimiter: line 4 column 5. sing-box check rejects that config, so start_service aborts for every node, not only the WireGuard ones — and on current OpenWrt (net/sing-box 1.13.18) the endpoint form is the only path. Single-endpoint setups still emit valid JSON, which is what hides it; the previous (w<nw?",":"") was correct whenever nothing was skipped.

The first flag has to gate a leading comma:

Suggested change

	# Comma depends on whether anything was emitted before, not on the
	# loop index: skipped endpoints must not leave a trailing comma.
	s=s "}"; print " " s (first?"":",")
	first=0
	# Comma depends on whether anything was emitted before, not on the
	# loop index: skipped endpoints must not leave a trailing comma.
	s=s "}"; print " " (first?"":",") s
	first=0

---

Generated by Claude Code

[openwrt-ai]

The skip only reaches the END emit loops, but node validation runs in the $1=="node" rule — before any device record has populated used[] — so the WireGuard required-field check at compiler.sh:35-38`` still fail()s (exit 2, nothing written) for a node these loops would drop.

That is reachable from the UI: private_key, local_address and public_key are plain form.Values with no depends(), datatype or rmempty in overview.js:332-352, and [`append_node()` defaults `enabled` to 1](https://github.com/openwrt/luci/blob/a28175bf6f268f0797c0fe4fdd1c0293cdcf1098/applications/luci-app-wificalling-gateway/root/etc/init.d/wificalling-gateway#L23-L24).`` A half-configured WireGuard node saved but not yet assigned to any device policy is enough — with one working node plus one such node I get wificalling-gateway: wireguard node halfdone is missing private_key, peer_public_key or local_address, rc=2, and the whole gateway stays down for a node that is no longer part of the config.

The device side already has the precedent: compiler.sh:44-48`` warns and skips a device whose node was deleted rather than failing the compile. Deferring the per-node checks until after used[] is known (or skipping them for unreferenced nodes) would make the new skip consistent with that.

---

Generated by Claude Code

[openwrt-ai]

nit: dropping the cache read is the right fix, but state:"failed" is not one of the states runNodeTest branches on in overview.js:98-112,`` so the busy result falls into the final else and the user gets a red error banner reading `Unable to test node: Test in progress`. The status column got its own `testing` state last round precisely so contention would stop being presented as a failure; an explicit `busy` branch here (info banner, `_('Another test is running right now')`) would match. Applies equally to the 40 s-timeout `busy` at line 46 above, which predates this commit.

---

Generated by Claude Code

[smthdagg]

1.8.8 — round 13 findings fixed

Addressed in d820a0a3:

1. endpoint separator inverted — fixed: you were right, my first?"":"," appended the comma to every non-first element, so two referenced WG endpoints produced {...}{...}, (invalid JSON, reproduced). The separator is now a leading comma emitted for every endpoint after the first one actually output — skipped endpoints no longer influence the comma, and two referenced WG nodes compile to valid JSON (verified: json.load() OK, endpoints [wg-wg1, wg-wg2]).
2. unused-node skip now covers node validation: the WG required-field check ran in the node rule, before used[] exists — an enabled-but-unreferenced WG node missing its keys failed the whole compile instead of being skipped. Validation moved into the emit loops via a wg_check() helper, running only for referenced nodes: unreferenced broken nodes are skipped, referenced broken nodes still fail with the node-specific message (both paths covered by tests).
3. busy banner nit: runNodeTest now has an explicit state === 'failed' branch, so busy renders as "Unable to test node: Test in progress" with the detail tooltip instead of the generic fallback.

74/74 tests pass (new: unreferenced broken WG node skipped; referenced broken node still fails); all three platforms install OK via docker.

[openwrt-ai]

Reviewed the one new commit since a28175b (d820a0a, 4 files). I re-ran compiler.sh under mawk against synthetic normalized.conf inputs, and both round-13 items land:

• The endpoint separator is inverted back: two policy-referenced WireGuard nodes on wireguard_style=endpoint now emit {…} / ,{…} and json.load() accepts the result (rc=0, JSON OK). An all-skipped endpoints block still emits a valid empty "endpoints":[],.
• wg_check() after the used[id] guard does what the message says: an enabled-but-unreferenced WireGuard node missing its keys no longer takes the compile down (rc=0 in both endpoint and legacy style), while a referenced broken one still fails with the same message (rc=2). Passing the split array as a function parameter works as expected.

Three things inline, none of them large:

• The outbounds loop kept a second copy of the wireguard + endpoint skip, so the line right after the new wg_check call is unreachable.
• The comment that documented the moved validation was left behind in the $1=="node" rule, above node[++nn]=$0.
• The runNodeTest branch adds the detail suffix but keeps 'error' severity and the Unable to test node: prefix, so the contention case still renders as a red failure banner reading "Unable to test node: Test in progress" — which was the substance of the round-13 nit. Suggestion inline uses strings already in the catalog.

Commit checks

• d820a0a "fix review round 13 findings" — the three bullets cover compiler.sh and overview.js; the diff also bumps PKG_VERSION 1.8.7 → 1.8.8 and rewrites six README.md install references. Same unmentioned-version-bump pattern as 1d3ba0e (round 10), 7231868 (round 12) and 9d94844/a28175b (round 13) — bundling the bump with the change it belongs to is right, it just still isn't in the message.

CI on d820a0a is still pending with no completed checks, so nothing CI-related to report.

---

Generated by Claude Code

[openwrt-ai]

The endpoint-style skip is now duplicated: line 104 already continues for wireguard + endpoint, so line 106 is unreachable — it can only be evaluated when line 104's condition was false, i.e. when it is false too. It looks like wg_check was inserted before the existing guard and the guard got copied instead of moved.

Keeping line 104 first is the right order (in endpoint style the node was already validated by the endpoints loop at compiler.sh:80,`` so re-running it here would be redundant), so it is the second copy that should go:

Suggested change

	if (p=="wireguard" && wg_style=="endpoint") continue
	if (p=="wireguard") wg_check(f, id)
	if (p=="wireguard" && wg_style=="endpoint") continue
	if (p=="wireguard" && wg_style=="endpoint") continue
	if (p=="wireguard") wg_check(f, id)

---

Generated by Claude Code

[openwrt-ai]

nit: this comment described the four validation lines the commit just moved out of the $1=="node" rule; it now sits directly above node[++nn]=$0 and documents code that is no longer there. The replacement wg_check at compiler.sh:18-20`` carries its own comment, so this one can just go.

Suggested change

	# WireGuard requires a private key, peer public key and local address;
	# reserved bytes and MTU must be numeric or the emitted JSON breaks
	# (and sing-box check would fail for every node at once).

---

Generated by Claude Code

[openwrt-ai]

The new branch adds the detail suffix but keeps the two things the round-13 nit was about: the 'error' severity and the Unable to test node: prefix. For the contention case — node-test.sh:61 printing `{"state":"failed","reason":"busy"}` — the banner now reads *"Unable to test node: Test in progress — Another test is running right now"* in a red `alert-message error` box, i.e. a failure notice whose own text says nothing failed. That is the presentation the `testing` state added in `7231868` removed from the Status column ([`nodeState()` at overview.js:60](https://github.com/openwrt/luci/blob/d820a0a3f86d3dc7dc2a9462c65a7b551f6ce210/applications/luci-app-wificalling-gateway/htdocs/luci-static/resources/view/wificalling-gateway/overview.js#L60));`` the Test button is the one place it still appears.

busy also differs in kind from the other three state:"failed" reasons this branch catches (config_missing, no_health_script, no_tcp_probe), which are genuine errors, so splitting it out keeps the severity honest for both. Both strings are already in the catalog (.pot lines 44 and 344), so no .pot/.po change is needed:

Suggested change

	else if (r && r.state === 'failed') {
	// e.g. busy: no probe was attempted, explain why.
	testNotify(_('Unable to test node: ') + wgFailReason(r.reason), 'error', wgFailDetail(r.reason));
	}
	else if (r && r.state === 'failed' && r.reason === 'busy') {
	// No probe was attempted — not a failure.
	testNotify(_('Test in progress'), 'warning', wgFailDetail(r.reason));
	}
	else if (r && r.state === 'failed') {
	testNotify(_('Unable to test node: ') + wgFailReason(r.reason), 'error', wgFailDetail(r.reason));
	}

This covers the 40 s-timeout busy at node-test.sh:46`` as well, since it emits the same record.

---

Generated by Claude Code

[smthdagg]

1.8.9 — round 14 cleanups

Addressed in f3156f06:

1. duplicated endpoint skip: the outbounds loop kept two copies of the wireguard+endpoint guard (wg_check was inserted before the existing guard and the guard got copied); the second copy was unreachable — removed.
2. stale comment: the validation comment left above node[++nn]=/bin/zsh in the node rule documented code that moved into wg_check(); removed.
3. busy banner: the state=failed branch now renders an informational banner — no red alert-message error style and no Unable to test node: prefix — so contention shows "Test in progress — Another test is running right now" instead of a failure notice.

74/74 tests pass; three platforms install OK via docker.