feat(aetherd): add observe-only typed resources - #5391
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
It has at least one correctness issue (slice ownership changes may not republish resources) and an API contract issue (advertising an unimplemented capability) that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Implements the next observe-only Stage 3 slice for aetherd by introducing typed control-protocol resources (server, radioSession, slice, panadapter) plus strict resource.get / resource.subscribe / resource.unsubscribe, with per-resource revisions and bounded, coalescing event delivery. This advances the RFC #3849 control-plane design while keeping the desktop UI on direct models.
Changes:
- Adds a typed resource store + per-session subscription/event-queue machinery to the control service.
- Introduces
RadioResourceAdapterto publish normalizedRadioModelstate as protocol resources and wires it intoaetherd. - Adds/extends tests and docs for the v1 observe-only resource surface.
File summaries
| File | Description |
|---|---|
| tests/tests.cmake | Registers the new socket-free control_resource_service_test. |
| tests/local_control_server_test.cpp | Expands handshake assertions to cover the resource methods/limits and adds a small resource.get/subscribe/event check. |
| tests/control_resource_service_test.cpp | New socket-free tests for revisions, atomic baselines, coalescing/resync, unsubscribe isolation, and SimBackend→model→resource publishing. |
| src/core/control/RadioResourceAdapter.h | Declares the observe-only model→resource adapter. |
| src/core/control/RadioResourceAdapter.cpp | Implements resource publishing for server/session/slice/panadapter based on model signals. |
| src/core/control/LocalControlServer.h | Exposes the resource store and adds session-output draining hook. |
| src/core/control/LocalControlServer.cpp | Integrates ControlResourceStore + per-client ControlSession output draining and overflow handling. |
| src/core/control/ControlSession.h | New per-client subscription state and bounded, coalescing output queue. |
| src/core/control/ControlSession.cpp | Implements subscription management, event coalescing, and resync-required signaling. |
| src/core/control/ControlService.h | Extends service to support resource get/subscribe/unsubscribe using ControlSession. |
| src/core/control/ControlService.cpp | Implements strict selector validation and resource method dispatch; expands advertised limits/capabilities. |
| src/core/control/ControlResourceStore.h | New authoritative in-process resource cache with revisions and snapshots. |
| src/core/control/ControlResourceStore.cpp | Implements upsert/remove, revisioning, and selector snapshots. |
| src/aetherd/main.cpp | Wires RadioSession + LocalControlServer + RadioResourceAdapter into the headless daemon skeleton. |
| docs/aetherd-control-resource-v1-catalogue.md | New v1 observe-only resource catalogue describing schemas, selectors, methods, and events. |
| docs/aetherd-control-protocol-v1-design.md | Links the design doc to the new fixed-schema resource catalogue. |
| CMakeLists.txt | Adds the new control-protocol implementation sources to aethercore. |
| AGENTS.md | Updates the Stage 3 progress/status section to reflect the newly-landed observe-only resources + resource methods. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Issue fit
There is no fixes/closes #N I could resolve (the PR body is not reachable from my credentials — I reviewed against the title, the catalogue the PR adds, and the AGENTS.md paragraph it rewrites). This is Stage 3 of RFC #3849, continuing #5109, so GOVERNANCE's RFC-first bar for architectural change is met and the RFC is open and assigned. Measured against its own stated intent — typed observe-only server/radioSession/slice/panadapter resources, resource.get, atomic snapshot+event subscribe/unsubscribe, per-identity revisions, bounded coalescing/resync — the diff delivers all of it, and the new control_resource_service_test pins every one of those claims with exact assertions rather than smoke checks. No TX method is advertised; canTransmit is a plain observation field and the end-to-end test asserts the simulator path stays RX-only.
One real defect and a handful of nits below.
Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
AGENTS.md |
Rewrites the Stage-3 status paragraph; adds "new fields belong in the adapter and the catalogue, never in a transport or via QObject reflection" | Yes | In scope |
CMakeLists.txt |
Adds ControlResourceStore/ControlSession/RadioResourceAdapter to CORE_SOURCES |
Yes | In scope |
docs/aetherd-control-protocol-v1-design.md |
3-line pointer to the new catalogue | Yes | In scope |
docs/aetherd-control-resource-v1-catalogue.md |
New normative protocol surface (4 resource types, 3 methods, 3 events) | Yes — it is the PR's subject | New public surface → maintainer decision, but squarely inside RFC #3849 step 2/3 |
src/aetherd/main.cpp |
Constructs a RadioSession + adapter against the server's store |
Yes | In scope |
ControlResourceStore.{h,cpp} |
New store: addresses, selectors, revisions | Yes | In scope |
ControlSession.{h,cpp} |
New; absorbs and replaces ControlSessionState |
Yes | In scope |
ControlService.{h,cpp} |
Ctor takes the store; capabilities() de-static'd; 3 new methods |
Yes | In scope |
LocalControlServer.{h,cpp} |
Owns the store, publishes the server resource, drains session output |
Yes | In scope |
RadioResourceAdapter.{h,cpp} |
New model→resource normalizer | Yes | In scope |
tests/control_resource_service_test.cpp, tests/tests.cmake |
New socket-free target | Yes | In scope |
tests/local_control_server_test.cpp |
Mechanical adaptation to the ControlSessionState → ControlSession rename |
Yes | In scope |
Everything in the diff is explained by the stated change. No unrelated files, no formatting churn, no CHANGELOG.md entry (correct). I read the - lines specifically for deleted guards: the only removals are the ControlSessionState struct and the static on capabilities(), both required by the rename — no guard, early return, or issue-citing comment was dropped.
Socket-test disclosure (§5254 reporting half). This PR does not add a socket test. It modifies tests/local_control_server_test.cpp, which is pre-existing and legitimate under the canon exception — the subject is our own LocalControlServer, it binds a QLocalServer under QStandardPaths::RuntimeLocation, and main() re-execs itself with --crash-server as a peer process for the stale-lock recovery case. The edits here are the type rename only. Worth flagging separately: neither local_control_server_test nor the new control_resource_service_test appears in any ctest -R filter in .github/workflows/ci.yml — both compile under the Linux all target but never execute in PR CI. That matches the existing precedent for this subsystem, so it is not a finding against this PR, but it does mean the four green checks prove compilation, not that the new assertions pass anywhere but the author's machine.
Blockers
1. slotOccupancyChanged can never refresh owned — the handler calls a function that early-returns on already-tracked slices. (RadioResourceAdapter.cpp:100-102, inline)
attachSlice() returns at line 141 when m_slices.contains(slice), so for any slice already published the occupancy handler is a no-op. owned is sourced from m_radio->isSlotOurs() (line 242), which reads a RadioModel-owned occupancy map — not a SliceModel property — so none of the 17 SliceModel signals wired in attachSlice can republish it either. RadioModel.cpp:10781 emits slotOccupancyChanged on exactly the transition this is meant to catch (// empty/foreign → ours), as do :10648, :10667, :10671, :10674.
Failure scenario, reasoned from the code (not reproduced at runtime): a second SmartSDR client releases slice 0, RadioModel flips isSlotOurs(0) false→true and emits slotOccupancyChanged(0), the adapter calls attachSlice(slice0), that returns immediately, no upsert happens, no revision advances, and every subscriber keeps "owned": false for the life of the connection. CodeGuard did not flag this; Copilot did, and it is correct.
panadapterReclaimed at line 106-107 has the same shape (second inline comment) — it is a re-attach signal routed into an add-only function. It is masked in the common case because clearDynamicResources() empties m_panadapters on disconnect, so a reclaim after a disconnect does republish; a reclaim without one would not. Worth resolving with the same helper rather than leaving one of the three call sites correct by accident.
The added test does not cover owned at all, which is why this survived — see the nit below.
Nits (non-blocking)
radio.sessionsis advertised with no corresponding method (ControlService.cpp:250, inline). Copilot flagged this as a hard defect; I'd downgrade it, becauseserver.readwas already in this list before the PR and is likewise not a method — the array is evidently feature tokens, not a method registry. But this PR now mixes both conventions in one array (server.read,slice.readalongside literal method namesresource.get,resource.subscribe), which is what makesradio.sessionsread as a missing method. Either split the two kinds or dropradio.sessions, whose only realisation is a wildcardresource.subscribe.- The catalogue's "All methods require the negotiated session ID and the
observegrant" overstates the code.handle()enforcesnegotiatedand the session-ID match (ControlService.cpp:117-151); no grant is checked anywhere, because every session getsobserveunconditionally. Harmless today, but the sentence will read as an implemented gate the moment a second grant exists. - The
serverresource value literal is duplicated verbatim three times (LocalControlServer.cpp:40-46,94-100,120-126), differing only inlocalTransport. A future field added to two of three would silently ship a resource whose shape depends on transport state. A smallserverValue(QStringLiteral("listening"))helper removes the drift. - Test gaps that would have caught the blocker: no assertion on
owned, none onresource.unsubscribewith an unknown ID returningresource.not_found(documented in the catalogue), none on thekMaxSubscriptions = 64cap or the 1–64 selector bound. The first is the one that matters — it's the only field in thesliceschema not derived from aSliceModelsignal, and it's the one that's broken. - Copilot's third comment (
-Wunused-variableonmain.cpp:30) is a false positive —RadioResourceAdapterhas a non-trivial destructor, and GCC/Clang do not warn for unused variables of non-trivially-destructible type. All four checks are green on00bb0093, consistent with that. No change needed. - CodeGuard CG-PATH-001 (
local_control_server_test.cpp:538) is a false positive. That line isrunEndpointValidationTest, which assertslisten("../shared/socket")is rejected — it is the traversal test, not a traversal.
What I tried to break
- The subscribe/event atomicity claim.
subscribe()inserts intom_subscriptions(line 43) before taking the snapshot (line 47), single-threaded with no re-entry between them, and returns the pre-incrementm_sequence; the nextenqueueResourceEventpre-increments, so a snapshot/event gap genuinely cannot open. The claim holds. - Use-after-free on the queued
outputReady→drainSessionOutput(socket)path. The lambda captures a rawQLocalSocket*withthisas context, so it is not auto-disconnected when the socket dies. I walked the three orderings: thedisconnectedhandler posts thedropClientmetacall beforedeleteLater(), so the FIFO'dDeferredDeletealways trails the erase, anddrainSessionOutput'sm_clients.find()compares (never dereferences) the pointer. If the drain does win the race,send()bails at line 250 onUnconnectedState. I could not construct a dangling dereference. The comment at line 173-176 shows this was reasoned about deliberately. - Member destruction order.
m_clientsis declared afterm_resourcesinLocalControlServer.h, so sessions (which holdconnects to the store) are destroyed before the store. Inmain.cppthe adapter is declared after the server and destroyed first, before the store it points into. Both correct. - Byte accounting in
enqueueCoalesced. The replace path computesm_pendingBytes - pending.bytes + bytesand only trips resync when the queue grows past the bound; the resync path itself resets both counters;unsubscribe's reverse-iteration prune decrements beforeremoveAtand correctly skips thestd::nullopt-resource resync message. I found no leak or double-count. waterfallRatesemantics. The catalogue insists this is the 1–100 rate, not milliseconds, while the accessor is namedwaterfallLineDuration()and its signal iswaterfallLineDurationChanged(int ms)— a plausible unit bug.RadioModel.cpp:5905carries the explicit commentwaterfallLineDuration() carries the 1..100 RATE, not milliseconds. The catalogue is right and the naming is the trap; no finding.- Slice lifetime without
sliceRemoved. TheQObject::destroyedhandler at line 163 drops the slice fromm_slicesbut leaves its resource in the store — a stale-resource leak if aSliceModelwere ever deleted silently. Every deletion path I read (RadioModel.cpp:1070-1076,10663-10664) emitssliceRemovedfirst, and disconnect goes throughclearDynamicResources(). Safety net, not a leak. - Selector parsing, fail-closed. Unknown keys, non-string
type, unsupported types, empty or >64-char identifiers,serverwith anid,radioSessionwith aradioSession, wildcardidonresource.get, andresourcesarrays of 0 or >64 are all rejected before reaching the store. The test pins the unknown-field case. - Could not check: the PR body's own claims (401 on the API from this environment), and anything requiring execution — no build, no
ctest, no daemon run. Every finding above is reasoned from source, not observed at runtime, and as noted in Scope, CI green here means "it compiles," since neither control test is in a-Rfilter.
Recommendation
Request changes — narrowly, for finding 1. Everything else is polish, and the store/session/adapter layering is the right shape: revisions live with the identity, coalescing is per-resource with byte accounting, resync fails closed, and the adapter is the single place model state becomes JSON (exactly what the new AGENTS.md paragraph promises). Fix the occupancy refresh, apply the same helper to panadapterReclaimed, and add an owned flip to testSimBackendEndToEnd so the field cannot silently go dead again. The catalogue is new third-party-visible protocol surface and deserves a maintainer's explicit sign-off before merge, but it is the declared purpose of the PR and sits inside the approved RFC, not smuggled scope. Nice work on the test — it is materially better than the surface area usually gets.
🤖 aethersdr-agent · cost: $8.5525 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (1 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $9.2637 · model: claude-opus-5 |
|
Addressed the review feedback in signed commit 7f9333e.
Local verification passed: ARM64 configure/build, the three focused control-protocol tests, strict engine-boundary and test-registration checks, the touchpoint manifest check, diff hygiene, and ARM64/RNNoise architecture checks. The CodeGuard CG-PATH-001 report is a false positive: the referenced ../shared/socket input is intentionally supplied by the endpoint-validation test, which asserts that traversal is rejected. |
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
No linked issue; the PR is the next milestone of the approved Stage-3 RFC (#3849), and I reviewed it against that and its own stated intent. It delivers what the body claims: four typed observe-only resources, strict resource.get/subscribe/unsubscribe, per-resource revisions, session-ordered events, bounded coalescing, and a model-only adapter wired into aetherd. No TX method, no grant beyond observe, no settings, no desktop adapter. Principle VI holds fail-closed and Principle VII's boundary validation is thorough — I went looking for a hole in parseSelector/onlyKeys and did not find one.
One blocker: the reconnect path is only correct for the Flex status path, and the resource surface loses every slice on a reconnect for HL2 / ANAN / Icom / RTL. Detail in comment 1.
Scope
| File / group | Claimed in the body? | Verdict |
|---|---|---|
ControlResourceStore.{h,cpp} (new) |
yes — "per-resource revisions" | in scope |
ControlSession.{h,cpp} (new) |
yes — "session-local ordered events, bounded coalescing/resync" | in scope |
RadioResourceAdapter.{h,cpp} (new) |
yes — "model-only adapter" | in scope |
ControlService.{h,cpp} |
yes — the three strict methods | in scope |
LocalControlServer.{h,cpp} |
yes — event delivery over the local transport | in scope |
src/aetherd/main.cpp |
yes — "wired into aetherd" |
in scope |
CMakeLists.txt (3 sources into CORE_SOURCES) |
implied | in scope |
docs/aetherd-control-resource-v1-catalogue.md (new) + design-doc link |
yes — "the v1 resource catalogue" | in scope |
AGENTS.md Stage-3 status prose |
yes — "and the Stage 3 status" | in scope |
tests/control_resource_service_test.cpp (new), tests/local_control_server_test.cpp, tests/tests.cmake |
yes | in scope |
Nothing in the diff is unexplained. The body's own checklist holds up: no CHANGELOG.md entry (correct — it is release-prep only), no flat-key AppSettings calls, no settings keys, no RadioCapabilities field additions, no CI edits. Both commit author dates are 2026-09-03 and both messages are on-topic. The new protocol surface is a public-API addition, but it is the ratified content of RFC #3849's Stage 3, not a side effect of a bug fix.
Test-boundary preflight (AGENTS.md)
The PR modifies the existing socket-owning local_control_server_test. That is the sanctioned carve-out — our own QLocalServer is the subject — and all three obligations canon puts on it are met: disclosed in the body, the tests.cmake block names the socket it binds (tests/tests.cmake:79-81), and it fails fast on a failed listen() rather than consuming its timeout. No new socket-owning target; control_resource_service_test is socket-free. The operator was notified before I ran anything.
Blockers
1. Slice resources are lost for the rest of the session after a reconnect on every non-Flex seam backend. See the inline comment on RadioResourceAdapter.cpp:83. Reasoned from code and confirmed step by step; not reproduced at runtime, because SimBackend cannot reach the path (proved below).
Nits — all explicitly non-blocking
Inline on the lines they concern: the resync path being unreachable through the shipped transport (LocalControlServer.cpp:229), localTransport: "stopped" being unobservable (:123), the waterfallRate -1 sentinel missing from the catalogue, sequence equality on subscribe, unbounded m_lastRevisions, and a confusing FLEX-6700 name in the sim fixture.
What I built, ran, and tried to break
Clean RelWithDebInfo build of the PR head in a scratch worktree (Debug does not link — -fsanitize=address is applied to aethercore but not to test targets; unrelated to this PR).
control_resource_service_test3x andlocal_control_server_test: pass, no flakes.- Mutation-tested both commit-2 fixes and they hold. Reverting
slotOccupancyChangedto the add-onlyattachSlicefails with "slot occupancy changes must republish RadioModel-derived ownership"; revertingpanadapterReclaimedtoattachPanadapterfails with "panadapter reclaim must republish canonical model state". These are real behavioural assertions, not implementation echo. - Instrumented the reclaim paths to check the reconnect leg. A probe on the non-Flex seam reclaim (
RadioModel.cpp:1246) never fires duringcontrol_resource_service_test, on first connect or on reconnect —SimBackendroutes slice state through the Flex texthandleSliceStatuspath, whose reclaim does emitslotOccupancyChanged(id)(RadioModel.cpp:10781). That is why the reconnect assertion passes, and why it cannot see blocker 1. I also mutatedSimBackend::disconnectRadio()to stop emittingsliceRemoved(matching HL2/ANAN/Icom) — the test still passed, which is what sent me to the probe. - Verified the catalogue's
waterfallRateclaim:PanadapterModel::setWaterfallLineDuration()really is the 1..100 rate, not milliseconds (#4606). The doc is right to say so. - Principle VI: no TX method or grant is advertised, and
canTransmit/txSlice/maximumTransmitWattsare read-only projections. No adapter path reaches a model setter or a backend intent. Held. - Principle VII: tried to get past the boundary with unknown params, non-string identifiers, over-long ids, wildcards where
resource.getforbids them, extra fields on theserverselector, and >64 selectors/subscriptions. Every one is rejected. - Lifetime:
LocalControlServer's member order destroysm_clients(owning eachControlSession) beforem_resources;drainSessionOutputnever dereferences a stale socket;outputOverflow -> abortis bound with the socket as context object. Held. - Coalescing reorder: moving the coalesced message to the queue tail keeps sequences monotonic, because the replacement sequence is always the highest. Held.
Not verified: blocker 1 at runtime — no socket-free seam backend can reach it, and HL2 hardware is out of scope for a review. Also worth knowing for anyone reading the green badge: ci.yml's per-PR ctest gate is a frozen -R list (DV tests, cross_needle_meter_test, mac_nr_filter_test, asr_gpu_probe_test) — neither control test runs in CI, so green here means "compiles on three platforms", not "the new tests pass".
7f9333e to
c69d7ed
Compare
PR #5391 — feat(aetherd): add observe-only typed resources (@rfoust)Second-opinion red-team review of Issue and proposed fix: RFC #3849 and its observe-only implementation slice call for typed model resources, atomic subscription baselines, ordered bounded delivery, and a SimBackend proof. This PR adds the resource store/session/service, a model-to-resource adapter, local transport delivery, and schema documentation. It implements that slice substantially, but one published field can miss a real model transition. Authentication, remote transport, control/TX, meters, packaging, and the desktop adapter remain outside this slice as planned. Scope and requirement fit
All 20 changed files are explained by the issue/PR. No unrelated feature, new dependency, UI redesign, per-feature settings, or changelog entry was added. Signed-commit, documentation and no-UI checklist claims hold; the author's three-test run and scanner claims are not substitutes for the narrower independent execution recorded below. Blocker
Non-blocking nit
What I tried to break
Independent verification and limitsIn an isolated detached scratch worktree at the exact head, macOS arm64/Qt 6.11.1 successfully built The local configuration disabled ASR, RADE, D-STAR, SpecBleach, DFNR, MQTT, RTL and GPU spectrum; this was focused protocol verification, not a full optional-feature/platform build. No sanitizer run or independent scanner run was performed. No standalone code-review skill was available; existing automated findings were inspected manually. The app GUI was not driven because this milestone is headless; the submitted resource test and reviewer probe drove the actual models with SimBackend. No operator app or radio was driven. All review test processes exited; no app instance was launched.
CI, governance and freshness
RecommendationRequest changes: fix finding 1 and keep its known-false regression before merging. The prior reconnect and sequence fixes withstand mutation checks, and the implementation otherwise fits the approved slice. After the update, re-review the new head, obtain the outstanding human owner decision, and keep the unrun socket-test boundary explicit; this comment does not change GitHub review state or rfoust's assignment. |
|
Addressed the latest review findings in signed commit
Focused verification passes: |
|
Follow-up: all five refreshed checks now pass on |
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
No closingIssuesReferences; this is the next observe-only Stage 3 slice of RFC #3849 (approved milestone, so GOVERNANCE.md's RFC-first requirement is satisfied). Reviewed against its own stated intent. Every claim in the Summary maps to a hunk: typed server/radioSession/slice/panadapter resources (RadioResourceAdapter.cpp), strict resource.get/subscribe/unsubscribe (ControlService.cpp), store-wide revisions (ControlResourceStore.cpp), session-local ordered events with bounded coalescing and resync (ControlSession.cpp), an independent local-socket cap (LocalControlServer::send), and the aetherd wiring (src/aetherd/main.cpp). The stated non-goals hold: no meter/transmitState resource, no auth, no remote transport, no desktop adapter, no mutation method, no TX verb — I grepped the diff for all of them.
Verdict: yes, it does what it says. No blockers.
Scope
| File / group | Claimed by title or body? | Verdict |
|---|---|---|
ControlResourceStore.{h,cpp}, ControlSession.{h,cpp}, RadioResourceAdapter.{h,cpp} |
Yes — the core of the slice | In scope |
ControlService.{h,cpp} |
Yes — the three new methods + capability/limit advertisement | In scope |
LocalControlServer.{h,cpp} |
Yes — session ownership, event drain, server resource lifecycle |
In scope |
CMakeLists.txt (+3) |
Implied — new CORE_SOURCES entries only |
In scope |
src/aetherd/main.cpp (+7) |
Yes — "a model-only adapter wired into aetherd" |
In scope |
src/models/RadioModel.cpp (+5) |
Partially — the reclaim occupancy edge is in the test plan, not the Summary | In scope, disclosure nit (nit 5) |
src/models/RadioModel.h (+13) |
Not explicitly — two …ForTest() seams + a comment |
In scope; matches the existing test-hook block directly above |
docs/aetherd-control-resource-v1-catalogue.md (new), docs/aetherd-control-protocol-v1-design.md |
Yes — "Documentation updated" | In scope |
AGENTS.md (+11/-4) |
Yes — Stage 3 status | In scope |
tests/control_resource_service_test.cpp, tests/local_control_server_test.cpp, tests/tests.cmake |
Yes — the test plan | In scope |
Nothing in the diff is unexplained. No CHANGELOG.md entry (correct — it is release-prep only). No settings keys, no credentials, no dialogs, no CI edits, no vendored churn. Commit author dates are 2026-09-03/04, all inside the PR's own window — no cherry-picked outlier. The body's checklist claims hold up against the diff.
Test-layer boundary (AGENTS.md, #5232/#5254)
tests/local_control_server_test.cpp is expanded, and it binds the production QLocalServer. That is a legitimate carve-out — our own server is the subject, not a synthetic third-party firmware peer — and it meets all three obligations canon puts on such a test: disclosed in the PR body, its tests.cmake block names the socket it binds, and it fails fast on a bind failure rather than consuming its timeout. Per the review protocol I notified the operator and did not build or run that target. control_resource_service_test is genuinely socket-free (links Qt6::Core only, no Qt6::Network) and is registered in AETHER_SETTINGS_CONSUMERS with an isolated TestSettingsProfile. No new socket-owning target enters the default graph.
Blockers
None.
Nits — all non-blocking
- Slice AGC/squelch resource fields go stale under external receive-audio replacement. See the inline comment on
RadioResourceAdapter.cpp:159. Unreachable fromaetherdtoday, but it is a latent trap for the desktop adapter slice. sliceRemovedunbinds by id, not by object identity — inline onRadioResourceAdapter.cpp:85.- Per-event cost on the main thread — inline on
ControlSession.cpp:101/:200. - "independent … cap" reads stronger than the code — inline on the catalogue.
- The Summary says "The desktop remains on direct models", which is true for consumption, but the new
emit slotOccupancyChanged(sliceId)in the non-Flex reclaim path does reach a desktop consumer:RxApplet::setRadioModel's handler callsupdateSliceButtons(...). That handler is idempotent and the refresh is arguably a latent-bug fix (the slice tab row previously never restyled after a non-Flex reclaim), so this is fine — but one sentence in the Summary saying the desktop gets one extra slice-row restyle on reclaim would make it reviewable later. Inline onRadioModel.cpp:1296. - Minor: in
testSimBackendEndToEnd,radio.slotOccupancyChanged(0),radio.panadapterReclaimed(...)andradio.connectionStateChanged(...)are emitted directly from the test, so those assertions prove the adapter's reaction rather than thatRadioModelemits them there. That is covered honestly elsewhere (testPureSeamReconnectRepublishesSlicedrives the real seam), so no change needed — just noting the boundary.
What I verified empirically vs. read
Built the PR head clean in a scratch worktree (control_resource_service_test, aetherd, AetherSDR — all exit 0) and ran control_resource_service_test → exit 0.
I did not trust the mutation claims in the test plan — I re-ran them, plus one the body did not claim. All four mutations are caught, with the specific assertion firing:
| Mutation | Result |
|---|---|
weightedAverageReported → weightedAverageChanged in the adapter |
FAIL — "the first known-false weighted-average report must publish an event" |
Delete emit slotOccupancyChanged(sliceId) from the reclaim path |
FAIL — "non-Flex slice reclaim must publish an occupancy edge" |
subscribe returns m_sequence instead of m_drainedSequence |
FAIL — "a baseline must stop at the last drained sequence when an older event is pending" |
Disable coalescing in enqueueCoalesced (mine, unclaimed) |
FAIL — "undrained changes to one resource must coalesce" |
| Restored tree | PASS |
The regression tests pin real behavior; none of them would pass against the unfixed code.
Drove the running GUI against the built-in demo simulator (DEMO-0001, offscreen, isolated AETHER_SETTINGS_DIR/HOME, AETHER_AUTOMATION_NO_TX=1, explicit socket) — never the operator's radio; get radio confirmed "serial": "DEMO-0001", "model": "AetherSDR Demo". Exercised the one desktop-visible change, the non-Flex reclaim edge: connect → sliceCount 1, panCount 1, slot 0 "ours" → disconnect → reconnect → sliceCount 1, panCount 1, slot 0 "ours", slice state (agcMode med, agcThreshold 65, audioGain 50, active true) identical before and after. No warnings or criticals in the app log. Instance closed.
What I tried to break and could not
- The snapshot/event gap the design doc newly promises. I tried to construct an ordering where a baseline is superseded by a stale queued event. Coalescing guarantees at most one pending message per resource address and it always carries the newest revision, so a pending event and a fresh baseline are necessarily the same revision — a duplicate, never a regression. The boundary holds.
requireResync→subscribeinterleaving. The undrained-resync-superseded-by-fresh-baseline path clearsm_pendingcorrectly andm_pendingBytesis reset with it; I checked every byte-accounting path (enqueueCoalescedreplace branch,unsubscribeprune,requireResync) and the counter never drifts from the sum ofm_pending.- Selector validation.
onlyKeysrejects unknown fields, wildcards are refused onresource.get,serverrejects any extra field,slice/panadapterrequireradioSession, ids are bounded and non-empty, unsupported types are rejected — the subscription and selector caps (64/64) both fire. The negotiated-session gate sits ahead of all three new methods. - Lifetime and ordering.
m_service(&m_resources)matches declaration order;RadioResourceAdapterinmain.cppis destroyed before theLocalControlServerthat owns the store it points at; a destroyedControlSessioncannot stranddrainSessionOutput, which re-looks-up the client and returns if it is gone. pruneStaleSessionModelsshared-id hazard (nit 2) — I looked for a path where the adapter unbinds a live slice and stays unbound. EverysliceRemovedsite that can carry a stale id pairs it with aslotOccupancyChangedon the same id, which re-attaches. It self-heals; that is why it is a nit and not a blocker.- TX / Principle VI.
canTransmitandmaximumTransmitWattsare exported as observation only; no method in the catalogue mutates a model, the capability list advertises notransmitorcontrol, andlocal_control_server_teststill asserts their absence.
Not verified: local_control_server_test (socket-owning; not run, per the test-layer boundary protocol), and the ARM64/macOS/Windows build claims (CI is green on all three; I built Linux x86-64 only).
…e II. Review follow-ups on the Stage 3 observe-only resource slice. RadioResourceAdapter published stale receive.agc.* and receive.squelch.* whenever external receive-audio replacement was active: the accessors publishSlice() reads switch to the external values, but SliceModel's AGC and squelch setters take an early-return branch in that mode and emit only their externalReceive* signals, which the adapter did not observe. Connect the four. audio.* needed no equivalent — those setters reuse the same signal in both branches, which is what made the gap easy to miss. The sliceRemoved handler matched by sliceId() alone, so a removal for a stale SliceModel whose id a live one had already reclaimed would unbind the live slice and publish resource.removed for a resource that still exists. Match the object instead, skipping whatever the model still serves for that id. observes() runs once per session per store change and swept every subscription and selector — up to the advertised 64x64 — on the model's publish path. Bucket the selectors by resource type. Events are now encoded once, at enqueue time, and the transport writes the stored frame instead of serializing the same message a second time in send(). The catalogue said the local transport's socket-output cap was "independent"; it is a separate check but the same maxQueuedOutputBytes budget, so a client would have budgeted it twice. Say so. Two socket-free regressions cover the adapter fixes, both mutation-checked: dropping the externalReceive* connects fails the AGC/squelch assertion, and restoring the id-only match fails the stale-vs-live removal assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Approving — every review thread on this PR is now resolved in code
Follow-up fixes pushed as 08da8be1 (signed), on top of 22d42ead. All eight of my open threads are addressed and resolved, each with the specific change and its mutation evidence recorded in the thread.
What changed since my review
| Finding | Fix |
|---|---|
receive.agc.* / receive.squelch.* stranded under external receive-audio replacement |
attachSlice() connects the four externalReceive*Changed signals; a comment records why audio.* needs no equivalent |
sliceRemoved unbinding a live slice by id match |
Resolves m_radio->slice(sliceId) and skips it; returns early rather than removing a resource that still exists |
observes() sweeping 64x64 selectors on the publish path |
Selectors bucketed by resource type in m_selectorsByType, rebuilt on subscribe/unsubscribe/resync |
| Each event serialized twice | PendingMessage stores the encoded frame; LocalControlServer::sendFrame() writes it, send() delegates so the socket cap check stays in one place |
| Catalogue calling the socket cap "independent" | Now a separate check, not a separate budget — budget maxQueuedOutputBytes once |
| The one desktop-reaching hunk undisclosed | PR body updated with the RxApplet slice-row restyle on non-Flex reclaim and why it is a correction |
Coverage added
Three socket-free regressions, all mutation-checked:
testExternalReceiveAudioRepublishesAgcAndSquelch— dropping the four connects fails "external receive AGC mode must reach the published resource".testSliceRemovalMatchesTheLiveObject— covers both directions (live id keeps the resource and stays attached; an id the model no longer serves is dropped). Restoring the id-only match fails "a removal for an id that still resolves to a live slice must keep it".testWireFramingIsCanonical— asserts the drained frame is one newline-terminated line, decodes as one JSON object, is byte-identical toQJsonDocument(...).toJson(Compact) + '\n', and carries the session-sequenced envelope. Dropping the newline fails it.
That last one still earns its place, but not for the reason I first gave: both local_control_server_test and control_resource_service_test are registered in tests/tests.cmake, and sanitizers.yml runs ctest --test-dir build unfiltered, so both already run there. ci.yml's -R filters are deliberately not where new tests go — new tests belong in tests.cmake and are exercised through the sanitizer lane. Disregard the suggestion in my earlier draft of this review to add these targets to the CI gate; that was wrong and no ci.yml change is wanted. The socket-free framing assertion is still the right call on its own merits: it pins the once-encoded frame deterministically, without depending on a socket-owning test I am not running under the AGENTS.md test-layer boundary.
Verification on the fixed head
- Clean build of
control_resource_service_test,aetherdandAetherSDR; no new warnings in any touched file. Test exits 0. - Six mutations caught, each by its own assertion — the three above plus re-checks of the sequence-boundary and coalescing paths I refactored (
subscribereturningm_sequence, coalescing disabled) and the selector-type bucket. - Drove the rebuilt desktop against the demo simulator (
DEMO-0001, offscreen, isolated settings,AETHER_AUTOMATION_NO_TX=1, explicit socket — never live hardware) through connect -> disconnect -> reconnect:sliceCount 1,panCount 1, slot 0"ours", slice state (agcMode med,agcThreshold 65,audioGain 50,active true) unchanged across the cycle, zero warnings in the log.
Scope stayed clean throughout — nothing in the follow-up commit touches anything outside the resource slice and its tests, and no CHANGELOG.md entry was added.
Approving; auto-merge armed behind CI.
## Summary Continues #3849 after merged #5391. This is the first, deliberately small sub-PR of authenticated non-TX control: establish an explicit per-session authorization boundary before any receive-control handler is exposed. - Default sessions are unauthenticated and fail negotiation closed. Only trusted embedding/transport code can select an authorization context; client names, JSON claims and session IDs cannot grant access. - The existing current-user local endpoint explicitly authorizes observers, preserving its seven observe-only capabilities. Authenticated sessions with no grants can negotiate and refresh capabilities, but cannot read or subscribe to resources. - Enforce observe permission at resource dispatch, direct subscription methods and event delivery. Keep negotiation identity private to the service/session. - Terminal, idempotent revocation clears subscriptions and queued observations before signaling the transport; the local server synchronously aborts unwritten output. Other clients are unaffected. - Share the production output binding with socket-free tests that independently buffer frames and verify synchronous transport purge, queued flush/drain ordering, and callback lifetime safety. - Update the protocol/catalogue and contributor status documentation. This does **not** complete the authentication/non-TX milestone. Credential verification/provisioning, typed receive intents, discovery/connect, remote transport, meters, desktop migration and Stage 4 TX arbitration remain separate work. There are no new control/TX grants, mutation methods, persisted settings, or dependencies. Revocation is an owning-thread lifecycle hook, not a new wire/admin operation; already-delivered bytes cannot be recalled. ## Constitution principle honored Principle VII — authorize at the protocol boundary and fail closed before resource processing. Principle VI — no transmit method or grant exists. Principle XI — socket-free behavioral regression tests are mutation-checked rather than relying on source-text assertions. ## Test plan - ARM64 macOS `RelWithDebInfo` build using the required local toolchain and RADE enabled: `AetherSDR`, `aetherd`, and the four focused test targets. Verify host/system ARM64, no RNNoise x86 sources, and ARM64 executables. - `control_protocol_codec_test`, `control_authorization_test`, and `control_resource_service_test`: socket-free checks for negotiation, grant denial, session isolation, queued/new event revocation, revocation before hello and during resync, and preservation of existing resource behavior. - Existing, unchanged `local_control_server_test`: exercises our own `QLocalServer` current-user transport; no fake radio. Requires a permitted local socket bind; passed outside the Codex sandbox. No new socket-owning test is introduced. - Mutation checks in an isolated worktree: unauthorized default observer, authentication implying observe, no-op revocation, and removed `resource.get` grant enforcement each make the new test fail. Restored production source passes. - Transport regression mutation checks: removing the production revocation hookup or changing it to a queued connection each fails the synchronous buffer-purge assertion. Restoring the exact production source passes. Coverage also stops batched writes on revocation, session/transport destruction, and write failure. - Strict engine boundary, test registration, frozen CI-gate integrity, touchpoint manifest, and whitespace checks. - An earlier focused security diff review found no findings in the authorization patch before the output-binding/test-coverage revision; that scan does not cover this later revision. No Windows/Linux runtime or live-radio proof claimed; this patch neither changes GUI wiring nor connects/controls a radio. - The new test is registered in `tests/tests.cmake` and enters the normal unfiltered main/weekly suites. The frozen per-PR test allow-list is unchanged. ### Review follow-up (`f7474b2c`) - Enforce single binding, non-null context/callbacks, and caller/endpoint thread affinity in release builds. Keep `AutoConnection`; do not force a cross-thread socket abort. - Deny unauthenticated parsed hellos before parameter/version negotiation; malformed envelopes retain protocol errors. - Clarify that revocation and the no-grants context are staged hooks without production callers. Keep that staging decision with the maintainer. - Document `observes()` and `takePendingFrames()` permission checks as defence-in-depth guards not independently mutation-pinned in any currently reachable state; defensively clear pending byte accounting on denied drains. - ARM64 app/daemon builds, all four focused tests, and static checks pass. Four follow-up mutations (remove once guard, caller-thread guard, context-affinity guard, or restore old hello ordering) each fail; restored source passes. - Focused security scan of this follow-up's three changed production files: no findings. This supplements, but does not replace, the earlier scoped reviews. No radio/TX or Windows/Linux runtime proof claimed. - Current-main merge tree is conflict-free; control sources and focused test sources are identical to the locally tested versions. CI must rerun for the new head. ## Checklist - [x] No new flat-key `AppSettings` calls (no settings changes). - [x] Clean-room implementation. - [x] No meter UI changes. - [x] Documentation updated; no `CHANGELOG.md` entry. - [x] Security-sensitive work reviewed; no GHSA applies to this preparatory authorization change. - [x] Signed commit verified locally (`f79ef219`). - [ ] CI passes. --------- Co-authored-by: Jeremy [KK7GWY] <kk7gwy@aethersdr.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Continues #3849 with the next observe-only Stage 3 slice. Adds typed
server,radioSession,slice, andpanadapterresources; strictresource.get,resource.subscribe, andresource.unsubscribe; atomic baselines, store-wide revisions that remain monotonic per resource identity, session-local ordered events, bounded session coalescing/resync, an independent local-socket hard disconnect cap, and a model-only adapter wired intoaetherd.The desktop remains on direct models. The one hunk that reaches it is the
slotOccupancyChangededge the non-Flex seam now emits when it reclaims a stagedSliceModel:RxAppletalready listens to that signal, so a non-Flex reclaim now also restyles the slice tab row once. That row previously never refreshed after a reclaim, so the effect is a correction rather than a change of behaviour, and the handler is idempotent. Nothing else on the desktop is touched.This PR intentionally adds no meter or transmit-state resources, authentication, remote/WebSocket transport, packaging, desktop adapter, mutation/control method, or TX surface.
Constitution principle honored
Principle VII — untrusted protocol envelopes, parameters, selectors, counts, and session identity are validated at the boundary. Principle II is preserved because resource events only observe radio/model-authoritative state; no status echo becomes an intent. Principle VI remains fail-closed because the protocol advertises only the
observegrant and no TX method.Test plan
AetherSDRandaetherdtarget build passes; both executables and the build graph are ARM64, with no RNNoise x86 sourcescontrol_protocol_codec_testandcontrol_resource_service_testpass; the pre-existing socket-owninglocal_control_server_testpassed before this model-only follow-up and was not rerunweighted_average=0report publishesweightedAverageKnown=true; an identical repeat is deduplicatedweightedAverageChangedconnection and passes with the report edgecontrol_resource_service_testuses an isolatedTestSettingsProfileand is registered as a settings consumerreceive.agc.*/receive.squelch.*;sliceRemovedmatches the object rather than the id;observes()is bucketed by resource type; events are encoded once and the transport writes the stored frameexternalReceive*connects, and restoring the id-only removal match, each fail their own assertion), and a socket-free framing assertion pins the once-encoded frame as byte-identical to the transport encodingThe existing local Unix-socket integration test is expanded; this PR does not add a new socket-owning test target.
control_resource_service_testis socket-free.Checklist
docs/COMMIT-SIGNING.md)AppSettingscalls — no settings were added (Principle V)MeterSmoother— not applicable; no meter or UI changesCHANGELOG.mdwas not changed