feat(aetherd): establish the Stage 3 control protocol boundary - #5109
Conversation
There was a problem hiding this comment.
Pull request overview
This PR establishes the first Stage-3 slice of the aetherd control-protocol boundary by introducing a bounded, transport-neutral v1 JSON envelope/handshake implementation, a current-user local IPC transport, and build-system/CI changes that enforce a QtWidgets-free engine/daemon.
Changes:
- Add the v1 control protocol codec + observe-only handshake/capability service, plus a
QLocalServertransport and focused negative/limit tests. - Split desktop-only QtWidgets-dependent code out of
aethercoreintoaetherdesktop_support, and introduce a QtCore/QtNetwork-onlyaetherdexecutable. - Extend engine-boundary checks and refresh/CI-enforce the generated aetherd touchpoint catalogue.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/check_engine_boundary.py | Expands EB3 scanning to include src/ app-shell files and refreshes the vendor-include baseline. |
| tests/tests.cmake | Registers new control-protocol tests and ensures desktop-only tests link through aetherdesktop_support. |
| tests/local_control_server_test.cpp | Adds transport/handshake/limits regression coverage for LocalControlServer. |
| tests/control_protocol_codec_test.cpp | Adds envelope/limit/response-shape tests for ControlProtocolCodec. |
| src/core/EibiClient.cpp | Removes a GUI-only dependency include from core. |
| src/core/control/LocalControlServer.h | Introduces the current-user local IPC server API and limits struct. |
| src/core/control/LocalControlServer.cpp | Implements local IPC listen/endpoint hardening, handshake deadline, client limits, and backpressure disconnect. |
| src/core/control/ControlService.h | Defines the transport-neutral handshake/capabilities service interface. |
| src/core/control/ControlService.cpp | Implements observe-only hello + capabilities.get flow and fail-closed dispatch. |
| src/core/control/ControlProtocolCodec.h | Defines the v1 envelope limits, request/parse result, and response helpers. |
| src/core/control/ControlProtocolCodec.cpp | Implements bounded JSON validation + envelope parsing/serialization. |
| src/core/AutomationServer.h | Moves setters out-of-line to support dependency decoupling. |
| src/core/AutomationServer.cpp | Adds out-of-line setter definitions. |
| src/aetherd/main.cpp | Adds the initial headless aetherd executable with local socket option and server start. |
| docs/architecture/aetherd-touchpoints.md | Updates touchpoint tagging completeness and expands tag notes. |
| docs/architecture/aetherd-touchpoint-tags.json | Updates/extends the generated tag catalogue. |
| docs/aetherd-control-protocol-v1-design.md | Adds the normative v1 contract document for Stage 3. |
| CMakeLists.txt | Builds new control components, adds aetherdesktop_support, introduces aetherd, and enforces “no QtWidgets” for engine/daemon. |
| AGENTS.md | Updates Stage-3 status and clarifies “no QtWidgets” in aethercore/aetherd. |
| .github/workflows/static-checks.yml | CI-enforces touchpoint-manifest freshness via gen_touchpoint_manifest.py --check. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
There is no fixes/closes issue — the body says "Refs #3849", and GOVERNANCE.md's RFC-first rule is satisfied by that RFC plus the normative contract this PR lands. Reviewed against the PR's own stated intent, and the six summary bullets all map to hunks I could verify in the head checkout: the contract doc, the codec, the observe-only service, the local transport, the aetherd/aetherdesktop_support split, and the touchpoint catalogue (195/195 tagged — python3 tools/gen_touchpoint_manifest.py --check prints "manifest up to date" on the head, so the CI gate is real and not aspirational).
The safety claim is the one that mattered most and it holds under reading: ControlService::handle has exactly two reachable methods (hello, capabilities.get), capabilities() hard-codes grants: ["observe"] and capabilities: ["server.read"] with no path that widens them, and any auth key in hello is rejected with auth.invalid rather than accepted-and-ignored. No TX verb exists to advertise.
Scope
| Files | What it changes | Claimed? | Verdict |
|---|---|---|---|
docs/aetherd-control-protocol-v1-design.md |
New normative v1 contract | Yes | In scope. Both relative links (aetherd-headless-engine-design.md, ../SECURITY.md) resolve. |
src/core/control/* |
Codec + observe-only service + local transport | Yes | In scope |
src/aetherd/main.cpp, CMakeLists.txt (aetherd/aetherdesktop_support targets, Widgets removal, GPU-define move) |
Headless boundary | Yes | In scope |
AGENTS.md |
Step-3 status + target table | Yes | In scope, and accurate — see "what I tried to break" |
docs/architecture/aetherd-touchpoint* |
51 new tags | Yes | In scope |
.github/workflows/static-checks.yml |
Manifest freshness gate | Yes | In scope. src/** already in paths, so include-only changes do trigger it. |
tests/*, tests/tests.cmake |
Two new suites + relink sweep | Yes | In scope |
tools/check_engine_boundary.py |
Widens the EB3 baseline by 8 rows/stems; adds src/ root to the scan |
Not in the summary bullets | Needs maintainer decision — see #1 |
CMakeLists.txt RNNoise x86 include-dir hunk |
Drops src/x86 from the include list on non-x86 |
Only under "Validation" | Out of scope, harmless — see #4 |
src/core/AutomationServer.{h,cpp}, EibiClient.cpp |
Touched per files changed |
Implied by the split | In scope |
CMakeLists.txt trailing-newline removal |
Whitespace | No | Trivial |
Blockers
None. Two items below need a maintainer call rather than a code change; the rest are nits.
Needs maintainer decision
- The EB3 baseline grows, and the checker's own comment forbids that.
tools/check_engine_boundary.py:158says "NEVER add a stem or a row to make a build pass", and the diff adds four new rows (ConnectionPanel.cpp,DemoApplet.cpp,MainWindow_Session.cpp,src/main.cpp) plus new stems on four existing ones. I checked every one againstmainand they are all genuinely pre-existing — e.g.src/gui/ConnectionPanel.cpp:3,6,7,8onmainalready includeHl2Discovery.h,IcomModels.h,MetisProtocol.h,SimBackend.h. They became visible only because this PR tags those headersvendor(hl2|sim|icom)in the JSON the checker derives its vocabulary from. So the ratchet is being re-baselined to a wider vocabulary, not loosened to launder new coupling, and the added comment says so. That is a legitimate reading — but it is the ratchet that guards the whole Stage-3 burndown, so it is your call, not mine. Inline attools/check_engine_boundary.py:168.
Nits (non-blocking)
out.requestIdis populated before it is validated (ControlProtocolCodec.cpp:341), so the server echoes an id the design doc says is impossible:idis specified as "1–64 printable ASCII characters", but a 100 KiB id, or one containing an escaped�, is copied intoout.requestIdand then reflected verbatim in thetransport.limit_exceeded/protocol.invalid_envelopeerror frame. Bounded bymaxQueuedOutputBytesso it is not a DoS, but it is a spec violation and an unvalidated echo. One-line fix suggested inline.capabilities()advertises limits the service does not enforce.maxSubscriptions: 64andmaxPendingRequests: 128are returned in everyhelloresult (ControlService.cpp:85-88), and §8 of the new doc calls the defaults "part of the v1 contract", but nothing in this slice enforces a pending-request count or the declared 100/s request rate —readClientdrains every newline-framed request in the buffer with no cap. Given §11 stages subscriptions later this is defensible; consider saying so in the doc, or omitting the fields until they bind.- The codec test covers three of the six declared limits.
kMaxMessageBytes,kMaxNestingand duplicate keys are pinned;kMaxStringChars,kMaxArrayEntries, non-finite numbers, trailing-data-after-value, and thevalidRequestId/validMethodsyntax rules are not. The nesting case also only asserts the reject side, so an off-by-one that set the effective limit to 1 would still pass. §11 step 1 asks for "schema fixtures/tests for envelopes, errors and limits" — this is most of the way there. aetherdis built but never installed. There is noinstall(TARGETS aetherd ...)next to the twoAetherSDRrules, so the binary is produced on every dev machine and CI run and shipped nowhere. Fine for a skeleton; worth a line in the body confirming it is deliberate.- The CMake
FATAL_ERRORassertion sits ~20 lines beforeinclude(tests/tests.cmake)(CMakeLists.txt:2762). Nothing after it touchesaethercore's link list today, so it holds — but moving it to the very end of the file would make it airtight against a later edit, which is exactly the failure mode its own comment describes. - CodeGuard CG-PATH-001 refuted (both hits).
tests/local_control_server_test.cpp:390is the negative test asserting../shared/socketis rejected;tools/check_engine_boundary.py:116is a repo-relative constant. Neither is a traversal.
What I tried to break
- "remove QtWidgets from
aethercore" — held. Grepped every QtWidgets include undersrc/coreandsrc/modelson the head: the only hits areAutomationServer.cpp,ShortcutManager.{h,cpp},ThemeManager.cpp,SettingsHelpers.cpp,TxKeyingMarker.h. The four.cppfiles all moved toaetherdesktop_support; the two headers are included by nothing that compiles intoaethercore(onlyThemeSeedGenerated.cpp, which moved with them, and by tests that compile the sources directly). Soaethercoreneeds neither Widgets links nor Widgets include paths.check_engine_boundary.py --stricton the head: 99 findings, 0 blocking, 24 of them the expected EB2 source-location warnings on the moved files — which is exactly what the new AGENTS.md wording claims. - Hunted for an ODR split from moving
AETHER_GPU_SPECTRUMoffaethercorePUBLIC. This was my best blocker candidate: a define that used to be global going per-target normally means two TUs disagreeing about a class layout. It does not here — the only#ifdef AETHER_GPU_SPECTRUMoutsidesrc/gui/isAutomationServer.cpp:90,1173, and that file compiles intoaetherdesktop_support, which the diff gives the define. The four test targets that compileContainerManager.cpp(the one GUI file with a GPU ifdef reachable from tests) never linkedaethercore, so they never had the define to lose. - Hunted for tests left with unresolved symbols by the library split.
tests.cmakeuses an explicit 9-target list plus aBUILDSYSTEM_TARGETSsweep keyed onaethercore + Qt6::Widgets; I confirmed noadd_executableappears after that sweep (line ~3817 of 3952), so it sees every target, and thattheme_manager_test/theme_seed_test/shortcut_manager_testare unaffected because they compile the moved sources directly rather than pulling them from the archive. Static-archive semantics mean the doubled cases don't duplicate-define. CI is green onbuild(Linux),check-windowsandcheck-macosfor626d55b, and the Linux job compilesall— that is the strongest evidence here, and it is what turned this from a suspicion into a non-finding. - Attacked the JSON scanner. Trailing commas in objects and arrays,
{"a":1,},[1,], empty input, unterminated string/object/array, control characters inside strings, and unicode-escaped duplicate keys ({"a":1,"a":2}— caught, because the raw key is round-tripped throughQJsonDocumentbefore theQSetcompare). Recursion is depth-capped at 32 before it can smash the stack. I could not construct an input that reachesQJsonDocument::fromJsonhaving bypassed the structural pass. - Attacked the socket lifecycle.
send()can callabort(), which emitsdisconnectedsynchronously from inside bothreadClient's loop and the handshake-timer slot — deleting aClientthere would destroy the veryQTimermid-emission. The deferredQTimer::singleShot(0, this, …)atLocalControlServer.cpp:141closes that, and usingthisas the context object means the deferred call is cancelled if the server dies first.close()double-erasing by pointer key is safe. The one thing I'd note:runBackpressureTestasserts a disconnect, which a synchronousdropClientwould also produce, so that test may not actually falsify the deferral it is named for. - Verified two factual claims inside the new normative artifacts rather than taking them on trust: the
AutomationBridgeSettings.htag note assertingtxAllowed()is a TX gate read by both the bridge and HL2 (confirmed —Hl2Backend.cpp:15, andMetisClient.cpp:563,593,604,685gate keying on it), and theQLockFilecrash-recovery path (setStaleLockTime(0)still permits PID-based stale removal, sorunCrashRecoveryTestis not relying on a timer that never expires). - Could not check: anything requiring a build or a run. Every finding above is reasoned from the head checkout at
/tmp/aetherclaude/pr-5109plus the three Python gates, which I did execute (gen_touchpoint_manifest.py --check,check_engine_boundary.py --strict,check_test_registration.py --strict— all pass). I did not exercise the socket server, so the handshake-timeout, backpressure and stale-endpoint behaviours are read, not observed.
Recommendation
Approve with nits, with the EB3 baseline widening flagged for an explicit maintainer ruling. This is a well-shaped seam: the observe-only surface is genuinely closed rather than merely undocumented, the boundary is asserted at configure time instead of discovered in a release artifact, and the doc is honest about what Stage 3 does not buy. The one structural item — re-baselining EB3 to a vocabulary that now includes HL2/Sim/Icom — is a consequence of the tagging work rather than new coupling, but it touches the ratchet the whole burndown depends on, so it should be ruled on rather than absorbed. Concrete next step: take the requestId one-liner, then decide on the EB3 row additions; the rest can ride follow-ups. Nice work.
🤖 aethersdr-agent · cost: $13.6201 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (2 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: $14.4827 · model: claude-opus-5 |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
No linked issue, but this is RFC Stage 3 work and the body scopes it honestly: a first implementation slice that advertises only observe, exposes no model resources, subscriptions, non-TX mutations, remote transport or credentials, rejects supplied authentication until a verifier exists, and keeps TX unavailable until the Stage 4 arbiter. Declaring what a protocol slice deliberately does not do is the right shape for a boundary PR.
Verified empirically on macOS/arm64: the full AetherSDR target and the new aetherd target both build clean, and control_protocol_codec_test and local_control_server_test both pass.
No blockers on correctness. One finding, one maintainer call the PR itself asks for, and a rebase.
The ratchet re-baseline is honest — I checked it against its own new rule
check_engine_boundary.py gains a re-baseline of previously-untagged HL2/Sim/Icom surfaces, and the PR writes the rule for when that is allowed:
every added include must be proven to predate the classification against the merge base, the change must document that evidence, and it requires explicit maintainer review. After classification, the expanded set is shrink-only again.
So I did the proving rather than accept the framing. Sampling the added entries against origin/main:
| entry | occurrences on main |
|---|---|
ConnectionPanel.cpp → Hl2Discovery / IcomModels / MetisProtocol / SimBackend |
12 / 1 / 1 / 8 |
MainWindow.h → Hl2Discovery / SmartLinkClient / WanConnection |
3 / 2 / 3 |
src/main.cpp → Hl2EmergencyStop |
2 |
Every one pre-exists. This freezes coupling that was already there and was simply outside the audit's vocabulary — burndown targets, as the comment says, not waivers. Writing the permitted-re-baseline rule into the script in the same PR that exercises it is the right order to do those in.
The maintainer call the PR asks for is that re-baseline, by its own terms — flagging it to @ten9876 rather than treating my spot-check as sufficient. The set is large enough that a full audit is a maintainer's call, not mine.
AGENTS.md: Tier 2, and the edits stay inside it
I checked this because a 2708-line PR editing project canon deserves it. AGENTS.md §14-24 puts itself at CODEOWNERS Tier 2 (infrastructure) precisely so "editing build conventions is not a governance act and should not need a maintainer's approval", reserving Tier 1 for CONSTITUTION.md and GOVERNANCE.md. These edits are the build-target table and the step-3 status line — descriptive, squarely Tier 2, and they do not touch the one passage that restates policy. No governance issue.
Finding
aetherd links QtGui, and the AGENTS.md row it adds says "Qt Core/Network". (inline: CMakeLists.txt:1288) Measured on the built binary:
$ otool -L build/aetherd | grep Qt
…/QtGui.framework/Versions/A/QtGui
…/QtNetwork.framework/…
…/QtCore.framework/…
QtWidgets is genuinely absent — that claim holds, and it is the harder one. But the target's own declaration (aethercore + Qt6::Core + Qt6::Network) is not what the binary ends up with, because aethercore links Qt6::Gui at CMakeLists.txt:1981 and that propagates.
For a headless service shell QtGui is the dependency that actually matters. On a minimal Linux container it is QtGui, not QtWidgets, that drags in the windowing-system stack — a libQt6Gui.so load needs fontconfig and, depending on build, xcb/GL present. So aetherd as built today may not start on the kind of box it exists to run on, while satisfying every "never QtWidgets" check.
Not a blocker for a skeleton, and the fix is Stage 3+ work rather than this PR's — but the new AGENTS.md row will be read as a statement of fact, and it should either say Qt Core/Network/Gui (Gui transitively via aethercore — burndown target) or the transitive edge should be broken before that row lands.
Also
- 43 commits behind
main, 2 conflicts —CMakeLists.txtanddocs/architecture/aetherd-touchpoint-tags.json. Both are files this PR restructures heavily, andCMakeLists.txtis contended by several other open PRs right now, so the longer this sits the worse that gets. - The
EibiClient.cppchange is a single deleted line in a 2708-line PR — worth a word in the body, since it is the one hunk nothing in the summary accounts for.
Verified vs. read
Built and ran: the full app target, the new aetherd target, and both new test suites. Measured: otool -L on aetherd; the re-baselined symbols against origin/main; the conflict set. Read: the AGENTS.md tier rule, the codec's bounds and the observe-only handshake's fail-closed posture. Not verified: the protocol against a real client — there is none yet — and no Linux container to confirm the QtGui load-time claim in the finding, which is why I have framed it as a dependency observation rather than a reproduced failure.
|
Thanks for pushing this one so far, @rfoust — the Stage 3 slice itself looks healthy, and CI is red for a narrow, self-contained reason: the new What failedAt Which commit did itWalking the check runs on the branch heads:
One honesty note: I can read step-level status through the API but not the raw log text, so I'm not quoting the error line. The Linux and macOS Why the walk is likely over-reachingThe useful signal from the failed fix is that skipping imported targets wasn't enough. So either the flagged edge runs through a project-owned target, or the walk is erroring rather than asserting. Two things in the implementation make the first easy to hit:
Both are the same underlying issue: this is a generate-time question being answered with configure-time string data. Suggested way outCheapest diagnostic first — swap the For the check itself, two options that don't depend on parsing genexes:
Worth saying: the flat check you replaced was green, and the boundary it guards is real. If the graph walk keeps fighting you, landing this slice with the flat check and filing the recursive version separately would be a reasonable call — it isolates a build-infrastructure problem from an otherwise-passing protocol slice. On the review commentsNone of the open review comments are related to this failure. The The bisect above should make this a single-file fix. Nice work on the slice. 🤖 aethersdr-agent · cost: $6.4873 · model: claude-opus-5 |
|
Final review fixes are now on the branch. The touchpoint manifest check rejects missing or invalid live tags, and the QtWidgets boundary assertion walks concrete project-owned link targets without interpreting unrelated generator-expression metadata as dependency edges. The deliberate indirect helper-library leak still fails configure. Final CI is green on Linux, Windows, macOS, and static checks at 9cdd86a. This PR is ready for maintainer review; the EB3 vocabulary re-baseline conversation remains intentionally open for the explicit maintainer ruling required by the checker. |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
No closingIssuesReferences; the PR says Refs #3849 (the accepted aetherd RFC). Measured against the Stage 3/4 implementation plan you recorded on that issue (2026-08-20), this is landing sequence items 1 and 2 — contract/catalogue + headless boundary — and it does exactly that, no more. hello/welcome negotiation exists; typed resources, resource.subscribe, auth and every TX verb do not. The RFC's non-negotiable ordering ("Stage 3 must not expose a mutation/keying surface without Stage 4's arbiter") is honoured: the only two methods in the registry are hello and capabilities.get, grants is ["observe"], and supplied auth material is rejected outright rather than ignored. Yes, it solves what it claims to solve.
Verified locally (Linux, Arch, Qt 6.10, fresh worktree at 9cdd86a1)
cmake -G Ninjaconfigures clean — the newaether_assert_no_qtwidgets()graph walk runs and passes on bothaethercoreandaetherd.ninja control_protocol_codec_test local_control_server_test aetherdbuilds clean; both tests pass (exit 0).ldd build/aetherd→ nolibQt6Widgets. The headline claim holds.python tools/gen_touchpoint_manifest.py --check→manifest up to date;python tools/check_engine_boundary.py --strict→103 finding(s), 0 would block.- Branch is 0 commits behind
origin/main, so the green CI rollup is against current main, not a stale base. - I checked the
AETHER_GPU_SPECTRUMPUBLIC→PRIVATEmove for silently-dropped coverage and found none: I configuredorigin/mainin a second build tree and diffedbuild.ninjaper object. Every test target that compilesContainerManager.cpp(container_manager_test,container_nesting_test,workspace_controller_test,workspace_container_mode_test) already compiled it without the define on main — they link Qt directly, neveraethercore. No regression.
Scope audit
| File / group | Claimed in title or body? | Verdict |
|---|---|---|
src/core/control/*, src/aetherd/main.cpp, tests/control_protocol_codec_test.cpp, tests/local_control_server_test.cpp, docs/aetherd-control-protocol-v1-design.md |
yes | In scope. The Stage 3 slice itself. |
CMakeLists.txt — aetherd, aetherdesktop_support, Qt6::Widgets off aethercore, aether_assert_no_qtwidgets() |
yes | In scope. |
src/core/AutomationServer.{h,cpp} out-of-lined setters |
implied by the split | In scope. Forced by the library split (the step-1 dry run flagged this exact incomplete-type/jumbo-moc failure). |
src/core/EibiClient.cpp — drop ThemeManager.h |
yes (bullet 6) | In scope. ThemeManager.cpp leaves aethercore, so an engine TU can no longer include it. |
tools/gen_touchpoint_manifest.py, docs/architecture/aetherd-touchpoint*, static-checks.yml |
yes ("refresh and CI-enforce") | In scope, and it's the audit's own item 1. |
tools/check_engine_boundary.py — EB3 vocabulary widening + baseline expansion |
not in the body | Needs maintainer decision — see 1 below. |
tools/check_engine_boundary.py / AGENTS.md — EB3 scan widened to src/main.cpp + MacStartupAbortGuard.{h,cpp} |
not in the body | Tightens the ratchet (good), but it is a separate ratchet change that immediately grants src/main.cpp an Hl2EmergencyStop waiver. Please name it in the body. |
AGENTS.md — backend table 4 → 6 implementors (ANAN, RTL rows) |
not in the body | Unrelated docs de-drift. Harmless, non-blocking; worth a body line. |
CMakeLists.txt — RNNOISE_INCLUDE_DIRS ARM64 hunk |
yes (bullet 8) | Out of scope for a protocol PR. Disclosed and CI-verified, so non-blocking — but see 5. |
On the - lines: nothing removes a guard, an early return, or a comment naming a fixed symptom. The CORE_SOURCES deletions are relocations into aetherdesktop_support, and the aethercore PUBLIC Qt6::Widgets deletion is the point of the PR. No CHANGELOG.md entry — correct, that file is release-prep only.
Findings
1. EB3 baseline widening — maintainer call (unchanged from your open thread, restating with the canon citation). Non-blocking from me; it is @ten9876's to rule on. But whichever way it goes, the PR currently ships a tool whose documented rule contradicts AGENTS.md, and AGENTS.md is the canonical file. AGENTS.md:533-536 still reads: "The set only shrinks — never add a stem or a row to make a build pass. If EB3 blocks you and the include is genuinely unavoidable, that's a design conversation for a maintainer, not a baseline edit." This PR edits the EB3 bullet directly above that sentence and leaves the sentence itself untouched, while check_engine_boundary.py:159-168 now carves out a classification re-baseline. Please mirror the ruling into AGENTS.md in the same PR so canon and enforcement agree.
I did verify the substance of your claim independently and it holds: the diff touches zero src/gui/ files and zero src/main.cpp, so every added stem describes coupling that already exists on origin/main — it cannot be new. And the change is net-tightening, since hl2/sim/icom/anan/rtl wire headers were previously unenforced by EB3 rather than clean.
2. aetherd drags in far more than QtGui. The body says "QtWidgets-free, but not yet QtGui-free". On Linux it is also not free of QtMultimedia, QtSerialPort, QtWebSockets, QtDBus or qt6keychain:
$ ldd build/aetherd | grep -o 'libQt6[A-Za-z]*\|libqt6keychain'
libQt6Multimedia libQt6Concurrent libQt6Gui libQt6SerialPort
libQt6WebSockets libQt6Network libqt6keychain libQt6DBus libQt6Core
Not a defect — they all arrive through aethercore, which is the whole engine — but the burndown note in the body and in AGENTS.md reads as if QtGui were the only remaining edge. Worth stating accurately now, while the note is being written.
3. v1 can never negotiate with a v2 client (inline on the design doc).
4. Transport-level error frames carry "id": "", which the contract forbids (inline on LocalControlServer.cpp).
5. The ARM64 RNNoise hunk cites a contract I cannot find (inline on CMakeLists.txt).
Nits (all explicitly non-blocking)
LocalControlServeris aQObjectsubclass with noQ_OBJECT— inline.tests/tests.cmake's desktop-support retrofit loop is position-dependent — inline.- Automated
/code-reviewpass: attempted and did not run against this PR — it resolved the target to the invoking checkout's branch instead of #5109 and reviewed unrelated NR2 code. Everything above is my own reading plus the local build/test run described at the top. No automated findings are folded in.
The protocol code itself I read closely and have nothing to add to it: the two-pass parse (bounded structural pre-scan, then QJsonDocument) is the right shape, the pre-scan's depth cap is what keeps QJsonDocument's own 1024-deep recursion off the stack, the deferred dropClient comment explains a real synchronous-abort() hazard rather than restating the code, and the transport test covers handshake timeout, timeout-under-backpressure, client cap, output overflow, stale endpoint, crash recovery and endpoint-name traversal. That is a stronger test surface than most of what lands here.
# Conflicts: # CMakeLists.txt
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Approved.
All seven findings from my previous review are addressed and verified against the current head: the AGENTS.md carveout (:526-530) with the "Removing coupling" bullet cross-referencing it, the hello envelope pinned to v1 with params.versions carrying negotiation, errorResponse omitting the correlation id when none can be correlated, Q_OBJECT plus a meta-object assertion, the tests.cmake retrofit loop moved below the final target declarations, the widened QtGui/runtime-surface burndown inventory, and the RNNoise architecture guard promoted from workspace config into canon.
The EB3 re-baseline is approved as a maintainer ruling on the thread — narrow, scoped to this classification pass, shrink-only afterward.
CI green on build, Static checks, check-windows and check-macos against a head that is current with main. Independently verified on Linux earlier in the review: clean configure including the aether_assert_no_qtwidgets() graph walk, both new tests passing, and no libQt6Widgets in the linked aetherd.
Summary
aetherdexecutable and remove QtWidgets fromaethercoreaetherdesktop_supportEibiClient.cpp's now-unusedThemeManagerinclude as part of that desktop dependency splitIRadioBackendimplementorsSafety and scope
This is the first implementation slice of Stage 3, not the complete thin-client protocol. It deliberately advertises only
observe; it exposes no model resources, subscriptions, non-TX mutations, remote transport, credentials, or transmit method. Supplied authentication is rejected until a verifier exists. TX remains unavailable until the Stage 4 arbiter and authorization funnel are implemented.The desktop continues to consume models directly.
AutomationServerremains the desktop test bridge and is not promoted into the permanent protocol.aetherdis a build/test skeleton in this slice; it is not installed, packaged, or auto-started yet.aetherdis QtWidgets-free, but it inherits the engine's current runtime surface: Qt Concurrent, Gui, Multimedia, SerialPort, WebSockets, DBus, and qtkeychain when enabled, in addition to Core and Network.Qt6::Guiis currently required becauseBandPlanManagerandDxccColorProviderexposeQColor. Removing that edge and narrowing the other transitive dependencies are explicit later burndown targets rather than part of this skeleton PR.The EB3 baseline additions freeze vendor includes already present on the relevant
mainrevision before this PR classified those headers. The current-main ANAN/RTL additions are documented againstb1499334.AGENTS.mdnow matches the checker's narrow rule: a vocabulary-classification rebaseline must prove every added include predates the classification, document that evidence, and receive explicit maintainer review; the exact set is shrink-only afterward. That maintainer ruling remains open.Refs #3849.
Validation
upstream/main(f41dc755) per the repository stale-branch policy; preserved both the Stage 3 QtWidgets graph assertion and upstream's new CPack configuration in the only conflictcontrol_protocol_codec_testlocal_control_server_testautomation_device_diagnostics_testlocal_control_server_testbinds the productionQLocalServercurrent-user socket and covers observe-only capability negotiation, invalid JSON, unsupported versions, handshake timeout, client cap, oversized input, backpressure, stale endpoints, and crash recoveryidon uncorrelated errors fails the codec testLocalControlServer::Q_OBJECTfails the server meta-object testaetherdesktop_supportimplementation after the library splittools/check_engine_boundary.py --strict(0 blocking findings)tools/check_test_registration.py --stricttools/gen_touchpoint_manifest.py --check(211/211 current touchpoints classified)--checkinstead of silently regenerating an incomplete catalogueQt6::Widgetsfails configure through the recursiveaethercore/aetherdtarget-graph assertiongit diff --checkAetherSDRandaetherdMach-O outputsaetherdlinks the documented engine runtime modules and does not link QtWidgetsreadOnly=true,txAllowed=false, radio disconnected, and MOX/tune/transmit all false0a006f1a: Linux, Windows, macOS, and Static ChecksNo live-radio control or TX proof was performed.