Feature: adsbexchange-stats: add new package - #29424
billmcilhargey wants to merge 5 commits into
Conversation
636fcfa to
983abb9
Compare
|
Marked as ready for review, I was waiting for the main one to be reviewed and approved first as I was not sure the process but in case this one was holding the main one up, I marked this as ready for review too Appreciate it again! |
|
Feel free to add label "Depends on PR in other repo" as this package depends upon approval of PR -> #29411 or basically package -> readsb-wiedehopf the package here, adsbexchange-stats is an optional package that is referenced by the package readsb-wiedehopf which is currently in the final review stages for getting approved. This package adsbexchange-stats is ready for review and all set unless any reviewers have any concerns. Also since copilot ran into an overall error on this, although the session itself looked good, I re-requested a review by copilot since it appeared to let me, just in case a reviewer had any concerns with copilot running into an error earlier. Thank you again! |
4ff524b to
b13b2a5
Compare
|
Resolved GitHub Copilot review requests and all ready for any final reviews and approvals! Thank you again |
|
Working on other PR change requests and just wanted to ensure this stayed active Finally catching up here and let me review the work here requested and work to implement! Thank you! |
b13b2a5 to
789198c
Compare
|
Signed commit with key |
789198c to
c65a8f5
Compare
9d38cf7 to
e75ab51
Compare
|
Now successfully verified and signed and following repros guidelines and ready for review and the dependency package is being reviewed now too |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (e75ab51, fresh review — no prior automated review at this head).
Two findings look merge-blocking to me, both reproducible without hardware:
patches/010-openwrt-paths.patchis not a parseable unified diff —patchaborts withmalformed patch at line 110, so the package cannot build. Hunk headers 4 and 5 disagree with their bodies, and the removed-sideperlline is wrapped mid-token._clamp()in the init script never matches, because|coming out of a variable is notcasealternation.dns_cache,dns_ignore_localandlog_levelare all silently pinned to their defaults regardless of UCI.
The info action is a third real (if harmless) defect: procd's rc.common overrides it, so the banner and the Makefile's @PKG_*@ substitutions are unreachable.
The remaining two comments are marked nit: and are not blockers. Everything else — procd usage, conffiles, PKG_MAINTAINER, scriptlet guards, PKG_RELEASE — reads fine, and the shell in functions.sh / json-status-helpers.sh is carefully written. Note that CI here only ran the three FormalityCheck jobs (all green); there was no build job, which is why the patch breakage went unnoticed.
I did not review against upstream ADSBexchange/adsbexchange-stats — that repo is outside this session's access, so the patch's content against the pinned commit 11647f27 is unverified; only its diff structure is.
Generated by Claude Code
Companion integration update: explicit opt-in and selected feeder identity
Published 138780946 — adsbexchange-stats: require opt-in and follow feeder identity and 7bc1b3898 — restore embedded patch metadata, coordinated with a05026d2c in openwrt/packages#29411. The uploader remains a separate optional package; ordinary feeding does not require it. Summary update
Final behavior of this update
Existing review fixes included
Implementation/validation replies were added to the five addressed review threads. The current default-off and feeder-associated behavior supersedes the original PR description's auto-start and main-UUID-only statements. VerificationThe uploader suite has 41 passing cases under each of dash, Bash, and BusyBox ash, including consent/identity guards, explicit activation, generated environment, option values, install/upgrade framework behavior, reload, and end-to-end calls from the decoder setup helper. Together with decoder suites, 1,011 shell regression executions passed; 42 real-UCI integration checks also passed, including companion-package updates not committing pending decoder edits. Companion scripts/new tests are ShellCheck-clean, decoder changes add no new baseline findings, and changed scripts pass syntax checks under all three shells. The patch applies without fuzz to upstream No full OpenWrt SDK package build, router install/upgrade test, physical SDR test, or live statistics upload was performed. These are local mocked/native checks, not renewed hardware certification. Remaining CI blockerThe current patch passes the required Git-header validation, but Code Patches checks every commit and still flags the intermediate |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
- 7bc1b38 "adsbexchange-stats: restore embedded patch metadata": this only puts back the patch headers that 1387809 dropped. For a new package, squash both follow-ups into e75ab51 so the series is one "add new package" commit.
Generated by Claude Code
Companion-only review follow-up
Published 2b4a37201 — adsbexchange-stats: align boolean guards and main section startup on Code recommendations addressed
Six failing cases were reproduced before these changes. The expanded companion suite now passes 69 cases under each of dash, Bash, and BusyBox ash: 207 passing executions, including the existing decoder-to-uploader activation checks. Source/test ShellCheck, cross-shell syntax, and whitespace checks pass. No live upload, router install, or full SDK build was performed for this follow-up. Implementation replies have been posted to the two new review threads. Their code fixes are complete and the conversations are being marked resolved. Remaining history/CI recommendationThe reviewer also requests one clean “add new package” commit. That is still needed: Code Patches scans every PR commit and continues to reject the intermediate The appropriate history cleanup is to squash the original package commit and its three follow-ups into one signed addition, then publish with an exact force-with-lease on this branch only. Explicit permission for that rewrite was requested but was not available, so the tested code changes were published as a normal follow-up instead. I have not modified checks or rewritten history to hide the failure. The CI/history blocker remains open pending that authorization. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 5
Open (6)
feederis… · Newfeederis… · Newfeederis… · Newenvanduuidare removed. The… · Newaircraftandbytesare used in arithmetic expansions without validation/coercion. If either… · New
Resolved since last review (5)
printf(pr+ newline +…mktempfails (e.g., low space/permissions on/tmp),errfilefalls back to/dev/null,…TEMP_DIRis env-overridable, leaving$TMPFILEunquoted intouchcan break if the…TEMP_DIRis env-overridable, leaving$TMPFILEunquoted intouchcan break if the…TEMP_DIRwith a trailing/, while the patchedjson-statusthen appends…
Clamp startup log settings consistently with the uploader environment so notices report effective verbosity and summary intervals rather than raw invalid UCI input. Document the activate action using the service command form, bump the release, and cover effective settings with focused regression tests. Signed-off-by: Dr Bill Mcilhargey <contributor@mcilhargey.com>
|
Addressed both outstanding review recommendations in signed, signed-off commit 9e58191 on
Validation: all 101 stats checks passed, including the integrated companion-consent flow against the updated readsb package. Shell syntax checks passed; ShellCheck introduced no new findings versus the pre-change baseline. No full OpenWrt SDK build or device test was performed. Both previously open review threads have been resolved. Thank you for the review. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (3)
Print errors explicitly on stderr as well as sending them to syslog. Keep showurl in the extra-actions section instead of daemon control, and start the newly introduced package at release 1. Add focused tests for production logger output and package help. Signed-off-by: Dr Bill Mcilhargey <contributor@mcilhargey.com>
|
Additional recommendations addressed in signed, signed-off commit e3d462d on
Validation: all 105 stats regression checks passed under Linux sh, including the production logger stderr/syslog test and package-help assertions. Shell syntax checks passed, with no new ShellCheck findings versus the previous commit. SDK/device validation was not performed. All three new review conversations are resolved. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (1)
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Enforce mode 0600 before writing UUID content, including when a runtime file already exists. Abort startup if permissions cannot be secured. Normalize log levels at upload-helper entry points before numeric comparisons, with a quiet fallback for invalid overrides. Cover pre-existing UUID permissions, failed chmod, and valid/invalid manual log-level overrides with focused regression tests. Signed-off-by: Dr Bill Mcilhargey <contributor@mcilhargey.com>
|
Addressed the latest recommendations in signed, signed-off commit f61dfa1. UUID files are restricted to mode 0600 before writing, including pre-existing files; failed permission changes prevent startup. Upload helpers normalize log-level overrides before numeric comparisons, preserving valid levels and safely handling invalid values. Also corrected the PR description to document showurl, about, and activate rather than info. Validation: all 117 stats checks passed under Linux sh, syntax checks pass, and ShellCheck has no new findings. SDK/device testing was not performed. All three new review threads are resolved. Ready for another review. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 3
Open (3)
ADSBX_*values, which makes controlled overrides… · NewADSBX_LOG_LEVEL>=3, curl-voutput is forwarded verbatim into syslog. This verbose output… · NewADSBX_LOG_LEVEL>=3, curl-voutput is forwarded verbatim into syslog. This verbose output… · New
Resolved since last review (3)
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 7
Open (7)
PKG_LICENSEshould be expressed as an SPDX license expression.MIT GPL-2.0-onlyis ambiguous… · Newadsbx_errwrites to both stderr and syslog. Since the procd service is configured with `stderr… · Newlog_level >= 3, the code logs fullcurl -voutput to syslog.curl -vwill include request… · Newlog_level >= 3, the code logs fullcurl -voutput to syslog.curl -vwill include request… · NewADSBX_LOG_LEVEL>=3, curl-voutput is forwarded verbatim into syslog. This verbose output…ADSBX_LOG_LEVEL>=3, curl-voutput is forwarded verbatim into syslog. This verbose output…ADSBX_*values, which makes controlled overrides…
Redact station UUIDs, authorization headers, and cookies from curl diagnostics on success and failure. Omit UUIDs from startup notices. Preserve configured runtime paths and disable duplicate daemon stderr logging while retaining useful error output for manual callers. Remove brittle metadata assertions, align package help, and document runtime overrides and logging privacy. Add focused behavior tests. Retain the repository convention for space-separated license IDs. Signed-off-by: Dr Bill Mcilhargey <contributor@mcilhargey.com>
|
Published signed, signed-off commit 0ab2b8e. Curl diagnostics redact station UUIDs and sensitive authorization/cookie headers on success and failure; startup notices omit the UUID. Runtime path overrides are preserved, and procd disables duplicate stderr error logging while manual callers retain it. Removed brittle metadata/help assertions, aligned package help, and documented privacy and override behavior. Retained the repository convention for whitespace-separated license identifiers with an explanation in the thread. All 121 stats checks passed under Linux sh, including redaction and logging modes, with no new ShellCheck findings. SDK/device testing was not performed. All nine reviewed threads are resolved. Ready for another review. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (2)
Resolved since last review (7)
log_level >= 3, the code logs fullcurl -voutput to syslog.curl -vwill include request…log_level >= 3, the code logs fullcurl -voutput to syslog.curl -vwill include request…adsbx_errwrites to both stderr and syslog. Since the procd service is configured with `stderr…PKG_LICENSEshould be expressed as an SPDX license expression.MIT GPL-2.0-onlyis ambiguous…ADSBX_LOG_LEVEL>=3, curl-voutput is forwarded verbatim into syslog. This verbose output…ADSBX_LOG_LEVEL>=3, curl-voutput is forwarded verbatim into syslog. This verbose output…ADSBX_*values, which makes controlled overrides…
| if [ "$errfile" != /dev/null ] && [ -s "$errfile" ]; then | ||
| if [ "$rv" -ne 0 ]; then | ||
| logger -t "$ADSBX_LOG_TAG" -p daemon.warn < "$errfile" | ||
| elif [ "$ADSBX_LOG_LEVEL" -ge 3 ]; then | ||
| logger -t "$ADSBX_LOG_TAG" -p daemon.debug < "$errfile" | ||
| fi | ||
| fi |
| uuid=$(printf '%s\n' "$snapshot" | awk -v feeder="$feeder" ' | ||
| function enabled(value) { return value ~ /^(1|on|true|yes|enabled)$/ } | ||
| { | ||
| separator = index($0, "=") | ||
| if (!separator) next | ||
| key = substr($0, 1, separator - 1) | ||
| value = substr($0, separator + 1) | ||
| if (value ~ /^\047.*\047$/) value = substr(value, 2, length(value) - 2) | ||
| settings[key] = value | ||
| } | ||
| END { | ||
| uploader = "adsbexchange-stats.main" | ||
| selected = "readsb." feeder | ||
| if (settings[uploader] != "adsbexchange-stats" || | ||
| !enabled(settings[uploader ".enabled"])) exit 1 | ||
| if (settings[uploader ".feeder"] != feeder) exit 2 | ||
| if (settings[selected] != "feeder" || | ||
| settings[selected ".preset"] != "adsbexchange" || | ||
| !enabled(settings[selected ".enabled"])) exit 3 | ||
| uuid = settings[selected ".uuid"] | ||
| if (uuid == "") uuid = settings["readsb.main.uuid"] | ||
| print uuid | ||
| } | ||
| ') |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
| PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION)-$(PKG_SOURCE_VERSION).tar.xz | ||
| PKG_MIRROR_HASH:=b16c3c708daea4389f1d850aca1c899cfd9827785038c332c4bf28e541ae5992 | ||
|
|
||
| PKG_LICENSE:=MIT GPL-2.0-only |
ADSBexchange.com statistics uploader for OpenWrt.
Periodically reads
aircraft.jsonfrom readsb (viareadsb-wiedehopf),aggregates per-aircraft RSSI and counts, and POSTs the result to
https://adsbexchange.com identified by the station UUID stored at
readsb.main.uuid-- shared with readsb's BEAST connectors so a singlereadsb-uuidconfigures both.📦 Package Details
Maintainer: @billmcilhargey
Description:
New leaf package
utils/adsbexchange-stats. Procd-supervised bashuploader that talks to ADSBexchange.com's per-station ranking endpoint.
Pure shell payload, no compile step. Configuration is UCI-only
(
/etc/config/adsbexchange-stats); the station UUID is shared withreadsb-wiedehopfviareadsb.main.uuidso a single identity is usedby the BEAST feed connector and this uploader. Logging goes to syslog
under the
adsbexchange-statstag at user-selectable verbosity.Files added:
utils/adsbexchange-stats/Makefilepostinst/prerm/postrmutils/adsbexchange-stats/files/adsbexchange-stats.configutils/adsbexchange-stats/files/adsbexchange-stats.initshowurl,about,activate)utils/adsbexchange-stats/files/adsbexchange-stats.functions.shutils/adsbexchange-stats/files/adsbexchange-stats.json-status-helpers.shutils/adsbexchange-stats/patches/010-openwrt-paths.patchjson-status(paths, env file, scratch dir, perl-version gate, metrics)utils/adsbexchange-stats/README.mdUpstream: https://github.com/ADSBexchange/adsbexchange-stats pinned at
commit
11647f27de3eef51fb19bcb39f0dc0b8500a6671. Upstream licensepreserved as
MIT; OpenWrt packaging files areGPL-2.0-only.Dependency chain
This PR consumes three artifacts shipped by #29411:
readsb.main.uuid— shared station identityreadsb-uuidCLI — manages the UUID for both packages/usr/lib/readsb/functions.sh— providesreadsb_is_uuid(8-4-4-4-12 hex validator)DEPENDS:=+bash +jq +curl +coreutils-stat +readsb-wiedehopf🧪 Run Testing Details
openwrt-23.05branchgit-25.163.46318-26086b5, kernel 5.4.164)ipq60xx/genericBuild verification (snapshot SDK,
aarch64_cortex-a53):Runtime verification on the GL-AXT1800:
Tested behavior:
postinstauto-start path: withreadsb.main.uuidalready set → service starts; without UUID → banner printed, service stays stopped (verified both).service adsbexchange-stats {start|stop|restart|reload|status|enable|disable}. Extra actions:showurl,about, andactivate <feeder>.uci commit adsbexchange-statsand onuci commit readsb(UUID /write_jsonchanges propagate without manual restart).log_level0 / 1 / 2 / 3 — error-only / +summary / +per-cycle / +curl-v all observed inlogread.dns_cache=0(default) anddns_cache=1with a non-loopback resolver — both function;127.0.0.0/8resolver auto-disables the cache as documented.json_paths_overrideempty (derived fromreadsb.main.write_json=/var/run/readsb) and explicit override both resolve correctly; unsafe-token rejection logs atwarn.prerm/postrmclean removal:/var/run/adsbexchange-statscleared,/etc/config/adsbexchange-statspreserved (conffile),readsb.main.uuiduntouched.✅ Formalities
If your PR contains a patch:
git am(the patch only adds OpenWrt-specific defaults and an opt-in
$ADSBX_ENV_FILE source hook; both are no-ops when the env file is
unset, so the change is upstream-friendly).