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.
| {QStringLiteral("capabilities"), QJsonArray{ | ||
| QStringLiteral("server.read"), | ||
| QStringLiteral("radio.sessions"), | ||
| QStringLiteral("radioSession.read"), | ||
| QStringLiteral("slice.read"), | ||
| QStringLiteral("panadapter.read"), | ||
| QStringLiteral("resource.get"), | ||
| QStringLiteral("resource.subscribe"), | ||
| QStringLiteral("resource.unsubscribe")}}, |
| connect(m_radio, &RadioModel::slotOccupancyChanged, this, [this](int sliceId) { | ||
| attachSlice(m_radio->slice(sliceId)); | ||
| }); |
| AetherSDR::control::RadioResourceAdapter resources( | ||
| &radioSession.radioModel(), &server.resourceStore(), | ||
| QStringLiteral("radio-1")); |
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
| connect(m_radio, &RadioModel::slotOccupancyChanged, this, [this](int sliceId) { | ||
| attachSlice(m_radio->slice(sliceId)); | ||
| }); |
There was a problem hiding this comment.
Blocker — owned can never update after a slice is first published.
attachSlice() returns at line 141 when m_slices.contains(slice), so this handler is a no-op for every slice that already exists. owned comes from m_radio->isSlotOurs() (line 242), which is RadioModel state, not a SliceModel property — so none of the 17 SliceModel signals wired below can republish it either.
RadioModel.cpp:10781 emits this signal on precisely the transition that matters (// empty/foreign → ours), as do :10648, :10667, :10671, :10674. A foreign client releasing a slot flips isSlotOurs and every subscriber keeps the stale "owned": false for the life of the connection.
The root cause is routing a refresh signal into an add-only function; panadapterReclaimed at line 107 has the same shape.
| connect(m_radio, &RadioModel::slotOccupancyChanged, this, [this](int sliceId) { | |
| attachSlice(m_radio->slice(sliceId)); | |
| }); | |
| connect(m_radio, &RadioModel::slotOccupancyChanged, this, [this](int sliceId) { | |
| SliceModel* slice = m_radio->slice(sliceId); | |
| if (slice && m_slices.contains(slice)) { | |
| publishSlice(slice); | |
| } else { | |
| attachSlice(slice); | |
| } | |
| }); |
(CodeGuard did not flag this; Copilot did, and it is correct.)
| connect(m_radio, &RadioModel::panadapterAdded, | ||
| this, &RadioResourceAdapter::attachPanadapter); | ||
| connect(m_radio, &RadioModel::panadapterReclaimed, | ||
| this, &RadioResourceAdapter::attachPanadapter); |
There was a problem hiding this comment.
Same shape as the slotOccupancyChanged finding above: panadapterReclaimed is a re-attach signal, but attachPanadapter() returns early at line 171 when the panadapter is already tracked.
This is masked in the common case — clearDynamicResources() empties m_panadapters on disconnect, so a reclaim following a disconnect does republish, which is why testSimBackendEndToEnd's reconnect leg passes. A reclaim without a preceding disconnect would silently not republish.
Worth resolving with the same contains-then-publish helper so all three call sites (sliceAdded, slotOccupancyChanged, panadapterReclaimed) are correct by construction rather than two-of-three by accident.
| {QStringLiteral("capabilities"), QJsonArray{QStringLiteral("server.read")}}, | ||
| {QStringLiteral("capabilities"), QJsonArray{ | ||
| QStringLiteral("server.read"), | ||
| QStringLiteral("radio.sessions"), |
There was a problem hiding this comment.
Copilot flagged radio.sessions as an unimplemented method. I'd downgrade it rather than repeat it: server.read was already in this list before this PR and is likewise not a method, so the array is clearly feature tokens, not a method registry.
The real issue is that this PR now mixes both conventions in one array — server.read / slice.read / panadapter.read (tokens) sitting next to resource.get / resource.subscribe / resource.unsubscribe (literal method names). That ambiguity is what makes radio.sessions read as a missing method to a client author.
Either split them into two fields, or drop radio.sessions — its only realisation is a wildcard resource.subscribe on {"type":"radioSession"}, which resource.subscribe already advertises.
Non-blocking.
|
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 |
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, per-resource revisions, session-local ordered events, and bounded coalescing/resync; and a model-only adapter wired intoaetherd.The desktop remains on direct models. 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
/opt/homebrew/bin/cmake --build build-codex-arm64 -j8)control_protocol_codec_test,control_resource_service_test, andlocal_control_server_testpassThe 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