Repository navigation
fix(bridge): VOX can be switched off with TX permission. Principle VIII. - #6375
Conversation
The Phone applet's VOX button has no TX-keying endpoint, and its accessible
name ("VOX voice-operated transmit") matches the name fallback in
transmitControlMatch(). A name-matched control has no scoped action, so the
automation bridge refuses every action on it, including setChecked false,
with or without TX permission.
Register a disarm-only scoped action on the button. It stays a keying
control that requires bridge TX permission: arming (setChecked true, click,
toggle) and pointer verbs have no action and stay refused, and without TX
permission the gate blocks the button as before. The one new verb is
setChecked false with TX permission, which calls
TransmitModel::setVoxEnable(false).
The keyboard TX activation guard treats the button the same before and
after. The vox_toggle shortcut, MOX and the gate itself are not touched.
phone_applet_vox_disarm_test drives the real AutomationServer invoke and
clickAt paths and pins each unchanged refusal.
Refs aethersdr#6053
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
1. Issue fit
Partially, by design — and the body says so up front. This is Refs #6053, not Fixes, and the "What this does not do" section enumerates every part it leaves behind. I checked the body's claims against the code and they hold:
| #6053 triage item | this PR |
|---|---|
| 1. Declare the button at its creation site | done — registerTxKeyingAction at src/gui/PhoneApplet.cpp:174 |
| 2. Off-only must not need TX permission | not done, disclosed; requiresTxPermission stays at its default true |
3. Gate the ungated vox_toggle shortcut |
not done, disclosed; still keysTx=false at src/gui/MainWindow_Shortcuts.cpp:1258, firing tx.setVoxEnable(!tx.voxEnable()) at :1262 |
| 4. The general name-fallback dead end | not done, disclosed |
So the safety asymmetry in the issue's title survives: a bridge session without TX permission can still arm VOX via shortcut vox_toggle, and still cannot disarm it (src/core/AutomationServer.cpp:4478 blocks the disarm before preparation). The PR's own test pins that as "(unchanged)". That is the author's stated intent — #6053 says "this is a maintainer's call" and carries maintainer-review — so it is not a defect in the PR; it means #6053 must stay open after this merges.
One practical limit worth naming, which neither the issue nor the body states: doInvoke refuses an invisible widget at src/core/AutomationServer.cpp:4452, so the new disarm only works while the Phone applet is on screen. A runner that armed VOX through the ungated shortcut and never opened the applet still has no disarm path. The issue's own repro does invoke PHN click at step 2, so it is covered there — but it is a real caveat on "the bridge can now disarm VOX".
What this PR does not foreclose, which is why landing it ahead of the ruling is reasonable: whichever way item 2 is decided, the change is a one-line registration swap (registerReceiveControlAction, or a requiresTxPermission predicate), and the test's "TX not allowed: invoke setChecked false is refused" check simply flips to document the new policy.
2. Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/PhoneApplet.cpp:174-190 |
the disarm-only scoped action, plus core/TxKeyingMarker.h and <QPointer> includes |
yes | in scope |
tests/phone_applet_vox_disarm_test.cpp |
new test | yes | in scope |
tests/tests.cmake:8050-8066 |
registers the target | yes | in scope |
tests/tests.cmake:8413 |
adds it to AETHER_AUTOMATION_SERVER_TESTS |
implied | correct and required — the loop at :8423 links aetherdesktop_support, which owns AutomationServer.cpp |
docs/architecture/aetherd-touchpoints.md:166 |
core/TxKeyingMarker.h count 15 → 16 |
yes | in scope — generated, not hand-edited |
Everything in the diff is explained by the issue. Checks I ran in the head checkout rather than taking on trust:
python3 tools/gen_touchpoint_manifest.py --check→manifest up to date. The generator only scanssrc/gui/+src/main.cpp(tools/gen_touchpoint_manifest.py:41), so 16 is right even though 20 files tree-wide include the header.tools/check_test_registration.py,check_ci_test_gate.py,check_engine_boundary.py,check_qsettings.pyall exit 0.- No duplicate-symbol hazard:
aetherdesktop_support(CMakeLists.txt:1682-1690) does not compilePhoneApplet.cpp. target_include_directories(... PRIVATE src tests)— the addedtestsis needed forTestEventLoop.h/TestSettingsProfile.h; the sibling dexp target has onlysrcbecause it uses neither. The omittedAETHER_SOURCE_DIRdefine is also correct: nothing inPhoneApplet.cpp,DragValuePopup.cpporGuardedSlider.hreferences it.- Not in
AETHER_SETTINGS_CONSUMERS, and doesn't need to be:aethercorelinksaether_sqlite3PUBLIC (CMakeLists.txt:2446).
No new public surface, no deleted guard, no settings key, no default changed, no preference smuggled in — the one user-visible side effect ("keying": true in a widget dump, src/core/AutomationServer.cpp:476-477) is disclosed in the body. No CHANGELOG.md entry, correctly.
Socket tests: none. AutomationServer server; is constructed but never start()ed — the constructor at src/core/AutomationServer.cpp:2279-2282 is empty, and QLocalServer is created only in start() at :2304. The test drives handleLine(..., nullptr) through the AutomationServerTestAccess friend. Qt6::Network is linked for the bridge's headers, not for a socket. Nothing binds, listens or connects.
3. Blockers
None.
4. Nits (non-blocking)
-
src/gui/PhoneApplet.cpp:179-181— the disarm allowlist refuses the bridge's own readback vocabulary.docs/automation-bridge.md:509documents that a checkable button reads back"checked"/"unchecked", andparseBool(src/core/AutomationServer.cpp:1425-1431) treats"unchecked"as false. MOX's sibling lambda buildsonfrom the truthy set and lets everything else mean off (src/gui/TxApplet.cpp:520-523), sosetChecked uncheckedunkeys MOX and unchecks every ordinary button — but on VOX:const bool disarm = action == QLatin1String("setChecked") && (v == QLatin1String("false") || v == QLatin1String("0") || v == QLatin1String("off") || v == QLatin1String("no"));
it falls through to
transmit control has no scoped action for 'setChecked'and VOX stays armed. A runner that echoes back the state it just read is exactly the client this action exists for, so refusing it is the wrong-direction failure. Drop-in:const bool disarm = action == QLatin1String("setChecked") && (v == QLatin1String("false") || v == QLatin1String("0") || v == QLatin1String("off") || v == QLatin1String("no") || v == QLatin1String("unchecked"));
(The broader alternative — mirror MOX and treat anything not in the truthy set as a disarm — is also defensible, since every such value only lowers TX risk. Either way the spellings loop in the test gains an entry.)
-
tests/phone_applet_vox_disarm_test.cpp:139-144— the four-spelling loop reuses onecheck()label, so a failure on" FALSE "is indistinguishable from one on"0". Interpolating the spelling into the message would make the output self-locating. -
Worth one line in the body: the disarm requires the Phone applet to be visible (
src/core/AutomationServer.cpp:4452). The body's behaviour table reads as unconditional.
5. What I tried to break
Everything below is reasoned from the head checkout at /tmp/aetherclaude/pr-6375; I have no build and ran no test, so no runtime claim here was reproduced.
- Lifetime. The
Preparelambda captures rawthis, which I went after. It is stored as a property onm_voxBtn, a child of the applet, so its lifetime is bounded by the applet's;prepareTxKeyingAction(src/core/TxKeyingMarker.h:97)QPointer-guards the button, and the deferredPreparedre-checksQPointer<PhoneApplet> self. This is stronger than the MOX sibling atsrc/gui/TxApplet.cpp:525, which captures rawthisinto the deferred lambda. No dangling path found. - Accept-then-change-underneath.
deferInvokeAction(src/core/AutomationServer.cpp:7435-7439) drops the queued action if permission, the permission epoch or the radio model changed between accept and fire, so a revoked-permission or swapped-radio disarm never lands. Survived. - Signal re-entrancy.
setVoxEnable(false)→TransmitModel::phoneStateChanged→PhoneApplet::syncFromModelsets the button underQSignalBlocker(src/gui/PhoneApplet.cpp:561-563), so thetoggledhandler at:168does not re-fire and there is no write-back loop. This is also what makes the test's "the button follows" assertion true. - The disabled-widget hole.
prepareTxKeyingActionsilently no-ops on a disabled widget (src/core/TxKeyingMarker.h:103), which would have been a silent-failure finding — butm_voxBtnis never disabled anywhere in the tree (onlysetCheckable/setFixedSize/setCheckedtouch it), anddoInvokerefuses a disabled widget explicitly at:4466first. Not reachable. - The claimed-unchanged keyboard guard. On main the button was
NameFallbackwith no endpoint →txActionRequiresPermissiontrue →isTxKeyingButtontrue. After the PR it isMarkerwithrequiresTxPermission=true→ still true.src/gui/TxKeyActivationGuard.h:25-26gives the same answer, so the body's "unchanged" rows for Space/Enter are correct, not aspirational. - The pointer path.
TxPointerAction::prepare(src/gui/TxPointerAction.cpp:23) prepares with action"click", which the lambda refuses, soclickAtcannot synthesise a press/release — matching the test and the body's table. - Is the test load-bearing, or does it assert the implementation back to itself? Load-bearing. Flip
requiresTxPermissionto false andtxController()returns null without permission,prepareTxKeyingActionno longer bails onrequiresTxPermission && !controller, and the disarm would be accepted with TX off — caught by "TX not allowed: invoke setChecked false is refused". TheMarker-vs-NameFallbackcheck and thenameMatchWarnings == 0check both fail if the registration is removed. - The test's premise. The disarm needs
TxController::valid()on a bareRadioModelwith no backend.valid()(src/models/TxController.cpp:96-101) andRequest::valid()(src/core/TxCoordinator.cpp:302-312) are pure producer/epoch/session bookkeeping — no connection requirement — so the premise holds from code. - Fail-before. I could not run it, but the claimed shape checks out: on main the button has no endpoint, so the classification check, the name-fallback-warning check, and every disarm check (
prepareTxKeyingActionreturns{}→ "no scoped action") fail — the 12 the body reports. - What CI actually proved.
tools/check_ci_test_gate.pyreports 7 frozen-Rtests out of 745 registered, andphone_applet_vox_disarm_testis not among them. All five checks ona1949f5are green, including check-windows (the-DENABLE_ASR=OFFlane) — that proves the new target compiles and links on all three platforms, not that its 35 assertions pass. The 35/35 and 12-fail-before are the author's local macOS run. - security-audit (CodeGuard): no findings. The 383 hits on the listed files are lexical/fixture false positives —
CS-VLN-CMDI-*on Markdown-quoted protocol verbs inside//comments with no shell anywhere in those files,CS-PII-*on RFC 5737 TEST-NET-1 and loopback literals indocs/user/screens.json,CS-SEC-PASSWORDon keychain-backed fields that never reach a log sink. It independently confirmed the direction of this change: the only behavioural delta is a reduction in transmit capability, with no fall-through to the genericsetCheckedhandler (:4488returns intoinvokeTxAction). - Sibling surfaces. One VOX button exists tree-wide. The other
setVoxEnablewriters are the MIDI/knob controller (src/gui/MainWindow_Controllers.cpp:2171), the PSK beacon save/restore (src/gui/PskReporterMapDialog.cpp:1960,1992) and the ungated shortcut (src/gui/MainWindow_Shortcuts.cpp:1262) — none is a bridge-reachable widget, and the last one is the disclosed item 3.
6. Recommendation
Approve with nits. The change is narrow, monotonically safety-reducing (it can only switch VOX off), every refusal it claims to leave alone I verified is genuinely unchanged, and the body is unusually scrupulous — it claims Refs rather than Fixes, enumerates the four things it does not do, and discloses the widget-dump side effect. Nothing here forecloses either item-2 design.
Concrete next step: take the "unchecked" spelling, add one line to the body about the applet needing to be visible, and keep #6053 open — items 2 (off-only free of the gate) and 3 (gating vox_toggle) still need the maintainer ruling the issue asks for, and until item 3 lands an unpermissioned session can still arm VOX with no sanctioned way back.
Nice work on the fail-before table and on running the static checks — it made this a verify rather than an investigation.
🤖 aethersdr-agent · cost: $14.8629 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (388 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: $15.0262 · model: claude-opus-5 |
Review of aethersdr#6375: a checkable button reads back "checked" / "unchecked" (docs/automation-bridge.md), and a client that echoes the state it read was refused: setChecked unchecked left VOX armed. The disarm accepts it. The test names the spelling in each check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Fixed in
Agreed that #6053 stays open: items 2 and 3 need the ruling, and until item 3 lands a session without TX permission can still arm VOX through Full ctest at 73, ON8ST |
NF0T
left a comment
NF0T
left a comment
There was a problem hiding this comment.
Approve
Thanks, @on8st. This is a clean, honestly scoped piece of work. Where #6053 asked for "declare the button at its creation site" and a disarm that works, you delivered exactly that and nothing else, and the "What this does not do" section tells the reader which three of the four triage items are still open. That is what lets a Tier 3 reviewer approve it without pre-empting the maintainer ruling.
Why it is right
The incident behind #6053 was a keyed session that already held TX permission. The runner's setChecked false died at prepareTxKeyingAction() because the VOX button had no registered endpoint (TxKeyingMarker.h), so invokeTxAction() returned "no scoped action". Registering a disarm-only endpoint fixes that path and no other. I checked the paths that could have widened:
requiresTxPermissionstaystrue, sodoInvokestill blocks every verb whenm_txAllowedis false (AutomationServer.cpp:4478).- With
m_readOnlyset,txController()returns null, andprepareTxKeyingAction()returns empty for a permission-requiring endpoint with no controller. A read-only session still cannot disarm. TxPointerAction::prepareprepares with action"click", which the lambda refuses, soclickAtcannot synthesise a press.deferInvokeActiondrops the queued disarm if permission, epoch or radio model changed in between. It fails closed, which in this one case is the "wrong" direction for a disarm, but it is the existing gate and not something this PR should change.- JSON
falseand0reach the lambda as"false"and"0"(AutomationServer.cpp:3841-3846), so a client that sends a bool is covered.
Scope table
| File | Change | Claimed | Verdict |
|---|---|---|---|
src/gui/PhoneApplet.cpp |
disarm-only registerTxKeyingAction + two includes |
yes | in scope |
tests/phone_applet_vox_disarm_test.cpp |
new test | yes | in scope |
tests/tests.cmake |
target + AETHER_AUTOMATION_SERVER_TESTS entry |
yes | required, in scope |
docs/architecture/aetherd-touchpoints.md |
TxKeyingMarker.h 15 to 16 |
yes | generated, in scope |
No CHANGELOG entry (correct), no new public surface, no deleted guard, no preference change. The one visible side effect, "keying": true in a widget dump, is disclosed. I saw it live: the VOX button now reports "keying": true, "checked": true, "visible": true.
CODEOWNERS: every file is Tier 3 (*, docs/, tests/). @aethersdr/reviewers satisfies the gate.
What I tried to break
Built the PR head in an isolated worktree (MinGW 13.1, Qt 6.12.0, Windows 11, ENABLE_ASR=OFF).
- The new test on the PR head: 36 of 36 pass, 0 failures. CI built and linked this target on all three platforms but does not execute it (it is not in the frozen
-Rlist), so this is the first execution I know of outside the author's macOS run. - Fail-before, by reverting
PhoneApplet.cpptomain: 13 failures. That is your 12 of 35 plus theuncheckedcheck added afterwards. The failures are the marker classification, the name-fallback warning, every disarm spelling, and the knock-on "nothing armed VOX behind a refusal". - Mutation A,
registerReceiveControlActioninstead ofregisterTxKeyingAction: 6 failures. "requires bridge TX permission", "TX not allowed: setChecked false is refused", "an armed VOX stays armed", and the three keyboard-guard rows. So the test pins the permission requirement and the keyboard rows you saidregisterReceiveControlActionwould change. - Mutation B, letting
setChecked truethrough tosetVoxEnable(true): 2 failures. "setChecked true is refused" and "nothing armed VOX behind a refusal". The arming direction is pinned. - Live bridge against the built-in Demo radio (
DEMO-0001,family sim, isolatedAETHER_SETTINGS_DIR, own socket,txAllowed:false):shortcut vox_togglereturned{"fired":true,"ok":true}andget transmitreadvoxEnable: true. This is item 3 from #6053, untouched, as the body says.invoke "VOX voice-operated transmit"withsetChecked false,setChecked true,clickandtogglewere allblocked: ... transmit-keying control (TX-safety guard). These are the unchanged rows.- A widget dump showed
"keying": trueon the VOX button, as disclosed.
Not driven live: the permission-granted half. I tried to relaunch the Demo instance with AETHER_AUTOMATION_ALLOW_TX=1 and the session's auto-mode classifier declined it as a safety-bypass flag, so I did not pursue it. That half is covered by the unit test above, which drives the real PhoneApplet through AutomationServer::handleLine. I also did not test the "applet hidden" case live; that one I confirm from code only: the visibility check at AutomationServer.cpp:4452 runs before the TX guard.
Socket-test preflight (Step 0): clean. AutomationServer server; is constructed but never start()ed, and the QLocalServer is created only in start(). Nothing binds, listens or connects, and there is no fake peer.
Non-blocking, for future reference
- Principle citation. The branch commits cite Principle VIII (Evidence Over Assertion). That is defensible given the fail-before table, but the sentence this change leans on is Principle VI's "Any code path that can transmit fails closed", since the PR keeps arming refused. I used VI in the squash subject below; VIII on the branch commits is fine.
- #6053 must stay open. Items 2 (off-only without the gate) and 3 (gating
vox_toggle) still need the maintainer ruling, and until item 3 lands a session without TX permission can arm VOX with no way back. This PR saysRefs, notFixes, and the squash body below keeps it that way. - The disarm only works while the Phone applet is visible. You added that to the body, thank you.
Squash subject
fix(bridge): VOX can be switched off with TX permission. Principle VI. (#6375)
Nice work on the fail-before table and on running the static checks. It made this a verification and not an investigation.
73, NF0T
👨🏼💻 Co-authored by Claude Sonnet 5.5
2 participants
Refs #6053
What this does not do
This is the narrow part of #6053. It picks no policy.
invoke "VOX voice-operated transmit" setChecked falsewith "Allow TX via MCP" off is blocked exactly as on main. Whether an off-only action should be free of the gate is item 2 of the triage (a predicate onrequiresTxPermission, orregisterReceiveControlAction), and that is a design call for the maintainer.vox_toggleshortcut still arms VOX without the gate. It is registered withkeysTxat its defaultfalse(src/gui/MainWindow_Shortcuts.cpp:1258). Item 3 of the triage; untouched.setChecked falseon MOX stays gated; the UNKEY asymmetry the triage describes is not addressed.invokerefuses a widget that is not visible (src/core/AutomationServer.cpp:4452), so a runner that armed VOX through the shortcut and never opened the applet still has no disarm path.One visible side effect: the button now carries the
aetherTxKeyingmarker, so a widget dump reports"keying": truefor it (src/core/AutomationServer.cpp:476-477). On main it is refused without saying so.What was wrong
At
main16cfa80:src/gui/PhoneApplet.cpp:159-167creates the VOX button with the accessible name "VOX voice-operated transmit" and no TX-keying endpoint.transmitControlMatch()(src/core/TxKeyingMarker.h:182-200) finds no marker and falls back to the name deny-list, which containstransmit(:168). The button classifies asNameFallback.txActionRequiresPermission()is true (TxKeyingMarker.h:68-73), so without TX permissiondoInvokeblocks it (src/core/AutomationServer.cpp:4477-4485).invokeTxAction()(AutomationServer.cpp:7367-7386) asksprepareTxKeyingAction()for an action, and that returns nothing when there is no endpoint (TxKeyingMarker.h:83):transmit control has no scoped action for 'setChecked'.So the bridge refuses every action on the VOX button, including switching it off, in both permission states. A runner that holds TX permission and sees VOX chattering cannot disarm it.
The change
PhoneAppletregisters the VOX button withregisterTxKeyingAction(). The prepare callback returns an action only forsetCheckedwithfalse,0,off,noorunchecked(the word the bridge itself reads back for an unchecked button); the action callsTransmitModel::setVoxEnable(false). Every other action and value returns no action.The endpoint keeps
requiresTxPermissionat its defaulttrue, so the gate atAutomationServer.cpp:4478and the keyboard guard insrc/gui/TxKeyActivationGuard.h:25-26see the same answer as before: a TX control that requires permission.invokesetChecked falseinvokesetChecked falseinvokesetChecked true,click,toggleinvokesetChecked true,click,toggleclickAtclickAtregisterReceiveControlAction()is deliberately not used: it setsrequiresTxPermissionto false, which would also take the button out ofisTxKeyingButton()and change the keyboard rows above.docs/architecture/aetherd-touchpoints.mdis regenerated: thecore/TxKeyingMarker.hinclude count goes from 15 to 16.Tests
New
phone_applet_vox_disarm_test(offscreen, no radio, no socket): a realPhoneAppleton a bareRadioModel, driven throughAutomationServer::handleLineforinvokeandclickAt. 36 checks:setChecked true,click,toggleandclickAtrefused, VOX off and VOX on;click,toggleandclickAtall refused, model untouched;toggledsignal, a mouse click and an unfiltered Space each drive the model both ways;refuseTxKeyActivation()on the button: refused for a bound Space and a bound Enter with shortcuts off, not refused for an unbound Return, never refused with shortcuts on.Fail-before. With only the test and its registration applied to
main16cfa80, the test failed 12 of the then 35 checks (run before theuncheckedcheck was added, which makes 36): the marker classification, the name-fallback warning, and every disarm check (invoke setChecked false is accepted, the model follow-through, the four spellings, the out-of-step and already-off cases, and one knock-on check that finds VOX still armed). The 23 checks that pin unchanged behaviour pass on main and pass with the fix.Full suite, macOS, Qt 6.12.0,
-j8, failures rerun serially:phone_applet_vox_disarm_test,peripheral_auth_dialog_interaction_test,unified_title_bar_test34d81194)peripheral_auth_dialog_interaction_test,unified_title_bar_testThe two remaining failures are not from this change and fail the same way on unchanged main on this machine:
peripheral_auth_dialog_interaction_test: the Peripherals page is wider than the viewport because of a long serial-port name on this host (feat(peripherals): authenticate direct TGXL, PGXL, and Antenna Genius connections #6008).unified_title_bar_test: two checks, "Alt+F still opens the File menu with the menu bar hidden" and "opening a menu rounds the submenus added to it since" (Unified title bar (#6198): native-platform checks still open — Windows Snap Layouts, #4557 drift, macOS M1, xcb move #6224).This PR touches no dialog, title-bar or menu file.
tools/gen_touchpoint_manifest.py --check,check_test_registration.py,check_engine_boundary.py,check_ci_test_gate.py,check_wdsp_test_links.pyandcheck_qsettings.pypass.check_a11y.pyreports no finding on the changed lines.73, ON8ST
🤖 Generated with Claude Code