openwrt-presence-agent: add package - #30094
Conversation
981ec55 to
11a1825
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit. Commit message matches the diff; comments inline.
Verified along the way (no action needed): upstream tag v0.1.1 exists, LICENSE and NOTICE are both present in the tarball so PKG_LICENSE_FILES is correct, main.version is the right -X symbol, and every UCI option in the shipped config maps to a real flag in internal/config with a matching default.
Generated by Claude Code
11a1825 to
0a3418e
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push (11a1825 → 0a3418e). All three findings from the previous round are addressed: base64 is gone in favour of a BusyBox-safe tr/head generator in both postinst and the init script, and test-version.sh was dropped so the generic version check applies again.
Commit checks
0a3418e"openwrt-presence-agent: add package" — the commit body still describes the service as publishing "normalized Wi-Fi client snapshots" only, but this revision adds the Ethernet/ARP path (newarpingdependency,arping_path/dhcp_leases_file/lan_interface/wired_reconcile_intervaloptions) and the package description was updated to "Wi-Fi and Ethernet". Worth updating the commit body to match, since it is the version bump's main user-visible change.
Two inline issues on the new Ethernet support, plus one nit.
Note: I could not verify the upstream v0.2.1 tag, the PKG_HASH, or that the four new --* flags exist in internal/config — theoabw/openwrt-presence-agent is outside the repository scope available to this session, so unlike last round those checks are unverified rather than confirmed.
Generated by Claude Code
68d5f08 to
7344414
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the new commit (0a3418e → 7344414). Both findings from the previous round are fixed correctly, and I could verify both against the tree this time:
+iputils-arpingis a real package in this feed and installs the binary to/usr/bin/arping(net/iputils/Makefile:84-86), so the newarping_pathdefault now points at a path something actually provides.PKG_RELEASEcorrectly stays at1across the0.2.1→0.2.3bump.- Spot-checked the other two hardcoded paths in the shipped config while I was here:
/bin/ubusmatches what theubuspackage installs (package/system/ubus/Makefile:62) and/var/run/hostapd/globalmatches the global control socketwpadopens (wpad.init:14).
Nothing blocking in the diff itself; two nits inline.
Commit checks
-
7344414"openwrt-presence-agent: address review feedback" — the subject names the process rather than the change, and the diff matches the body, but this commit only repairs things0a3418eintroduced in this same PR: aDEPENDSentry on a package that does not exist and a binary path that nothing installs. Merging as-is puts a commit adding a broken new package into the tree followed by a commit fixing it. CONTRIBUTING.md's PR advice points atgit rebase -i+git push -ffor exactly this case. Squashing back into a singleopenwrt-presence-agent: add packagecommit would also let you fix that commit's body, which still describes the service as publishing "normalized Wi-Fi client snapshots" only and never mentions the Ethernet path — the same gap I raised last round, still unaddressed because the fix landed in a second commit instead of an amend.Minor: the body's rationale "so package installation succeeds" does not match the actual failure mode. An unknown
DEPENDSentry is only warned about and then dropped by the metadata scanner (scripts/package-metadata.pl:465), so installation did succeed before —arpingwas simply absent at runtime and the wired probe path degraded silently. Worth rewording if you re-roll the commit anyway.
CI on 7344414 is still pending with no completed checks, so there is nothing to read there yet.
Not verified this round: the v0.2.3 tag, PKG_HASH, and whether the UCI options still map 1:1 to flags in internal/config at that tag — theoabw/openwrt-presence-agent is outside the repository scope available to this session and the tarball fetch is blocked, so the version bump itself is taken on trust.
Generated by Claude Code
7344414 to
649e25b
Compare
649e25b to
b2f7298
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed after the rebase (7344414 → b2f7298). The series is now a single openwrt-presence-agent: add package commit, which resolves the main point from last round — there is no longer an add-then-fix pair going into the tree, and the commit body now describes the Ethernet path (Fresh neighbor events or ARP replies confirm presence) instead of the Wi-Fi-only wording. The message matches the diff, so no commit checks this round.
Both nits from last round are also addressed, and the content diff since 7344414 is exactly those two fixes:
PKG_MAINTAINERmoved toTheo Wilenius <theo@tabw.dev>, which now matches the author andSigned-off-byon the single commit.- The package description dropped "only fresh ARP replies" in favour of "fresh neighbor events or ARP replies", agreeing with the PR body.
One inline issue, on the token_file UCI option versus the hardcoded bootstrap path in the init script. It predates this revision — I missed it in earlier rounds — but since the package has not merged yet it seems better to raise it now than as a follow-up.
Still not verified: the v0.2.3 tag, PKG_HASH, and whether the 22 UCI options map 1:1 to flags in internal/config at that tag. theoabw/openwrt-presence-agent is outside the repository scope available to this session, so the version bump is taken on trust as in the previous round.
CI on b2f7298: the three FormalityCheck runs are green; no build or test job has reported yet, so there is nothing to read there.
Generated by Claude Code
| start_service() { | ||
| umask 077 | ||
| mkdir -p /etc/openwrt-presence-agent | ||
| if [ ! -s /etc/openwrt-presence-agent/token ]; then | ||
| tr -dc 'A-Za-z0-9' < /dev/urandom | head -c 48 > /etc/openwrt-presence-agent/token | ||
| fi | ||
| chmod 600 /etc/openwrt-presence-agent/token | ||
| config_load openwrt-presence-agent | ||
| config_foreach start_instance observer | ||
| } |
There was a problem hiding this comment.
The token bootstrap hardcodes /etc/openwrt-presence-agent/token, but the path the agent actually reads is the UCI option token_file, passed through as --token-file (openwrt-presence-agent.init:31, default at openwrt-presence-agent.config:5).`` The two only agree as long as the user leaves token_file alone.
Change option token_file to anything else — a different filename, or persistent storage on a device where /etc is small — and start_service still creates and chmods the default file, then starts the agent with --token-file <other path>, which does not exist. Given readToken() refuses tokens shorter than 43 chars, the agent exits, procd burns its 5 respawn retries, and the operator is left with a populated token file at a path nothing reads. Since token_file is exposed as a knob and the whole point of the config file is that it is editable, that is a foot-gun rather than a theoretical case.
The end state to aim for: derive the path from the section before generating, rather than assuming the default. Note config_load currently runs after the bootstrap, so this needs a small reorder — either move config_load above the token block and read the path from the main section, or move the bootstrap into start_instance() so it runs per-section with that section's token_file. The latter is more consistent with config_foreach iterating over every observer section, so I have not put a suggestion block here.
postinst has the same hardcoded path, but there it is fine: at install time the shipped default is the only value that can be in effect.
Generated by Claude Code
Add an authenticated local service that publishes normalized Wi-Fi and Ethernet client presence for trusted local consumers. Combine low-latency hostapd and neighbor events with corrective snapshots and bounded ARP probes. DHCP leases provide candidate wired addresses. Fresh neighbor events or ARP replies confirm presence. Include procd and UCI integration, persistent on-device credentials, and dependencies on ubus and iputils-arping. Release: https://github.com/theoabw/openwrt-presence-agent/releases/tag/v0.2.3 Signed-off-by: Theo Wilenius <theo@tabw.dev>
b2f7298 to
78a3f43
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed after the amend (b2f7298 → 78a3f43). The series is still a single openwrt-presence-agent: add package commit and the message matches the diff, so no commit checks this round.
The only content change since b2f7298 is the token bootstrap moving out of start_service into start_instance, where the path now comes from the section's token_file — that is exactly the fix for last round's finding, and it also stops generating a token for sections with enabled 0. umask 077 stays in start_service, which still covers the generation because config_foreach runs in the same shell, and the mkdir/chmod failures now bail out before procd_open_instance instead of starting a broken instance.
One follow-on inline: the resolved path is not the one handed to the agent, because --token-file still goes through append_option.
Not verified, same as previous rounds: the v0.2.3 tag, PKG_HASH, and whether the 22 UCI options map 1:1 to flags in internal/config — theoabw/openwrt-presence-agent is outside the repository scope available to this session (get_tag there is refused), so the release is taken on trust.
CI on 78a3f43: the three FormalityCheck runs are green; no build or test job has reported, so there is nothing to read there.
Generated by Claude Code
| procd_set_param command /usr/bin/openwrt-presence-agent | ||
| append_option "$section" listen_address --listen-address | ||
| append_option "$section" port --port | ||
| append_option "$section" token_file --token-file |
There was a problem hiding this comment.
The bootstrap now resolves token_file with a default (line 27), but this line passes the option through append_option, which does a plain config_get with no default and drops the flag when the value is empty.`` So the two halves can still disagree, just in the opposite direction from last round: delete or blank option token_file in a section (or add a second `observer` section without it) and the script generates a token at `/etc/openwrt-presence-agent/token` while the agent is started with no `--token-file` at all and falls back to its own compiled-in default.
Since the resolved path is already in $token_file, passing it directly removes the second lookup and guarantees the agent reads exactly the file that was just created and chmod'ed:
| append_option "$section" token_file --token-file | |
| procd_append_param command --token-file "$token_file" |
(If the agent's built-in default is /etc/openwrt-presence-agent/token, the practical impact is nil for the single-section case — I can't check internal/config from here, see the review body. The multi-section case still shares one token file across observers either way.)
Generated by Claude Code
There was a problem hiding this comment.
Pull request overview
Adds a new OpenWrt net/ package for openwrt-presence-agent, a Go-based local presence observation service intended to publish authenticated Wi‑Fi/Ethernet client presence information to trusted local consumers.
Changes:
- Introduces a new
net/openwrt-presence-agentpackage Makefile for fetching/building/installing the Go binary and config/init assets. - Adds a procd init script to run the daemon with UCI-configurable flags and safe token-file permissions.
- Adds a default UCI config with sensible localhost binding and default paths for
ubus,hostapd, andarping.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| net/openwrt-presence-agent/Makefile | New package definition: source, Go build wiring, install steps, conffiles, and postinst token generation. |
| net/openwrt-presence-agent/files/openwrt-presence-agent.init | New procd init script that loads UCI and maps options to CLI flags. |
| net/openwrt-presence-agent/files/openwrt-presence-agent.config | New default UCI configuration for the service. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,76 @@ | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
Package details
Maintainer: @theoabw
This adds
openwrt-presence-agent, an authenticated, read-only local service that publishes normalized Wi-Fi and Ethernet client presence. The package follows the signed upstreamv0.2.3release.Wi-Fi presence combines low-latency hostapd events with authoritative corrective snapshots. Snapshot clients count as present only when hostapd reports them as both associated and authorized. Reconciliation also removes clients and providers that disappear between snapshots.
Ethernet presence treats DHCP leases only as candidate MAC/IP pairs. Fresh Linux neighbor events or bounded active ARP replies confirm presence; stale leases, neighbor entries, and bridge forwarding entries do not count as online.
The package installs the service binary, procd init script, and UCI configuration. It depends on
ubusandiputils-arping, using the packaged/usr/bin/arpingexecutable. Configuration and generated credentials are preserved during upgrades.Testing
0.2.3da79dce581a436d92bddcd94375b749f90f98fb16f3b98cc55678219a0c16b7dscripts/check.shv0.2.3release check and hardware-vendor package build pass; its full official SDK matrix is runningv0.2.1Formalities
This PR does not contain downstream source patches, so patch-specific formalities do not apply.