Skip to content

feat(aetherd): add observe-only typed resources - #5391

Open
rfoust wants to merge 1 commit into
aethersdr:mainfrom
rfoust:codex/aetherd-stage3-read-resources
Open

feat(aetherd): add observe-only typed resources#5391
rfoust wants to merge 1 commit into
aethersdr:mainfrom
rfoust:codex/aetherd-stage3-read-resources

Conversation

@rfoust

@rfoust rfoust commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Continues #3849 with the next observe-only Stage 3 slice. Adds typed server, radioSession, slice, and panadapter resources; strict resource.get, resource.subscribe, and resource.unsubscribe; atomic baselines, per-resource revisions, session-local ordered events, and bounded coalescing/resync; and a model-only adapter wired into aetherd.

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 observe grant and no TX method.

Test plan

  • Local ARM64 build passes (/opt/homebrew/bin/cmake --build build-codex-arm64 -j8)
  • control_protocol_codec_test, control_resource_service_test, and local_control_server_test pass
  • SimBackend drives authoritative radio, slice, and panadapter resources through connect, disconnect, and reconnect, with exact-schema and ordered-event assertions
  • Slow-client overflow requires resync; multi-client sequencing and unsubscribe isolation are covered
  • Engine-boundary, test-registration, manifest-freshness, and diff checks pass
  • Focused security diff scan completed with no findings
  • Behavior verified on a real radio if applicable — not applicable to this receive-only, Sim-backed protocol slice; no live-radio or TX test was performed
  • Existing tests pass (CI)
  • Reproduction steps documented if user-reported bug — not applicable; this is an approved RFC milestone

The existing local Unix-socket integration test is expanded; this PR does not add a new socket-owning test target. control_resource_service_test is socket-free.

Checklist

  • Commits are signed (docs/COMMIT-SIGNING.md)
  • No new flat-key AppSettings calls — no settings were added (Principle V)
  • Code is clean-room — not decompiled, disassembled, or reverse-engineered from a proprietary binary (Principle IV)
  • All meter UI uses MeterSmoother — not applicable; no meter or UI changes
  • Documentation updated — the v1 resource catalogue and Stage 3 status; CHANGELOG.md was not changed
  • Security-sensitive changes reference a GHSA if applicable — not applicable; this is not a vulnerability fix

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 RadioResourceAdapter to publish normalized RadioModel state as protocol resources and wires it into aetherd.
  • 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.

Comment on lines +248 to +256
{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")}},
Comment on lines +100 to +102
connect(m_radio, &RadioModel::slotOccupancyChanged, this, [this](int sliceId) {
attachSlice(m_radio->slice(sliceId));
});
Comment thread src/aetherd/main.cpp
Comment on lines +30 to +32
AetherSDR::control::RadioResourceAdapter resources(
&radioSession.radioModel(), &server.resourceStore(),
QStringLiteral("radio-1"));

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ControlSessionStateControlSession 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.sessions is advertised with no corresponding method (ControlService.cpp:250, inline). Copilot flagged this as a hard defect; I'd downgrade it, because server.read was 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.read alongside literal method names resource.get, resource.subscribe), which is what makes radio.sessions read as a missing method. Either split the two kinds or drop radio.sessions, whose only realisation is a wildcard resource.subscribe.
  • The catalogue's "All methods require the negotiated session ID and the observe grant" overstates the code. handle() enforces negotiated and the session-ID match (ControlService.cpp:117-151); no grant is checked anywhere, because every session gets observe unconditionally. Harmless today, but the sentence will read as an implemented gate the moment a second grant exists.
  • The server resource value literal is duplicated verbatim three times (LocalControlServer.cpp:40-46, 94-100, 120-126), differing only in localTransport. A future field added to two of three would silently ship a resource whose shape depends on transport state. A small serverValue(QStringLiteral("listening")) helper removes the drift.
  • Test gaps that would have caught the blocker: no assertion on owned, none on resource.unsubscribe with an unknown ID returning resource.not_found (documented in the catalogue), none on the kMaxSubscriptions = 64 cap or the 1–64 selector bound. The first is the one that matters — it's the only field in the slice schema not derived from a SliceModel signal, and it's the one that's broken.
  • Copilot's third comment (-Wunused-variable on main.cpp:30) is a false positiveRadioResourceAdapter has a non-trivial destructor, and GCC/Clang do not warn for unused variables of non-trivially-destructible type. All four checks are green on 00bb0093, consistent with that. No change needed.
  • CodeGuard CG-PATH-001 (local_control_server_test.cpp:538) is a false positive. That line is runEndpointValidationTest, which asserts listen("../shared/socket") is rejected — it is the traversal test, not a traversal.

What I tried to break

  • The subscribe/event atomicity claim. subscribe() inserts into m_subscriptions (line 43) before taking the snapshot (line 47), single-threaded with no re-entry between them, and returns the pre-increment m_sequence; the next enqueueResourceEvent pre-increments, so a snapshot/event gap genuinely cannot open. The claim holds.
  • Use-after-free on the queued outputReadydrainSessionOutput(socket) path. The lambda captures a raw QLocalSocket* with this as context, so it is not auto-disconnected when the socket dies. I walked the three orderings: the disconnected handler posts the dropClient metacall before deleteLater(), so the FIFO'd DeferredDelete always trails the erase, and drainSessionOutput's m_clients.find() compares (never dereferences) the pointer. If the drain does win the race, send() bails at line 250 on UnconnectedState. I could not construct a dangling dereference. The comment at line 173-176 shows this was reasoned about deliberately.
  • Member destruction order. m_clients is declared after m_resources in LocalControlServer.h, so sessions (which hold connects to the store) are destroyed before the store. In main.cpp the adapter is declared after the server and destroyed first, before the store it points into. Both correct.
  • Byte accounting in enqueueCoalesced. The replace path computes m_pendingBytes - pending.bytes + bytes and only trips resync when the queue grows past the bound; the resync path itself resets both counters; unsubscribe's reverse-iteration prune decrements before removeAt and correctly skips the std::nullopt-resource resync message. I found no leak or double-count.
  • waterfallRate semantics. The catalogue insists this is the 1–100 rate, not milliseconds, while the accessor is named waterfallLineDuration() and its signal is waterfallLineDurationChanged(int ms) — a plausible unit bug. RadioModel.cpp:5905 carries the explicit comment waterfallLineDuration() carries the 1..100 RATE, not milliseconds. The catalogue is right and the naming is the trap; no finding.
  • Slice lifetime without sliceRemoved. The QObject::destroyed handler at line 163 drops the slice from m_slices but leaves its resource in the store — a stale-resource leak if a SliceModel were ever deleted silently. Every deletion path I read (RadioModel.cpp:1070-1076, 10663-10664) emits sliceRemoved first, and disconnect goes through clearDynamicResources(). Safety net, not a leak.
  • Selector parsing, fail-closed. Unknown keys, non-string type, unsupported types, empty or >64-char identifiers, server with an id, radioSession with a radioSession, wildcard id on resource.get, and resources arrays 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 -R filter.

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

Comment on lines +100 to +102
connect(m_radio, &RadioModel::slotOccupancyChanged, this, [this](int sliceId) {
attachSlice(m_radio->slice(sliceId));
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@aethersdr-agent

Copy link
Copy Markdown
Contributor

Cisco CodeGuard — static analysis of this PR (1 finding(s))

  • [MEDIUM] CG-PATH-001 — Potential path traversal in tests/local_control_server_test.cpp /tmp/aetherclaude/pr-5391/tests/local_control_server_test.cpp:538

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants