Repository navigation
Fix Flex interlock source-list TX reconciliation. Principle II. - #6493
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 250f99922f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Yes for the stated defect. #6490 asks for exact comma-separated hardware-token matching with legacy single-source behaviour, ownership, operator intent and the existing VOX-enable path preserved, plus socket-free coverage and an isolated bridge diagnostic. src/models/RadioStatusOwnership.h:107-112 does exactly that, tests/radio_status_ownership_test.cpp:87-107 and tests/tx_operation_integration_test.cpp:387-493 cover list order, token boundaries, empty/unknown sources, foreign/unclaimed owners, release and stale SW, and the diagnostic is there. I walked the reconcileInterlockTx extraction branch-for-branch against the deleted inline block at src/models/RadioModel.cpp:11671-11678 — it is semantically identical, including the m_cwxActive = false side effect and the "only publish on edge" audio-gate guards, with m_lastInterlockSource clearing left in place below it. Parent #6437 correctly stays open.
Scope
| File / group | Change | Claimed? | Verdict |
|---|---|---|---|
src/models/RadioStatusOwnership.h |
token-list hardware-PTT match | yes | In scope |
src/models/RadioModel.cpp/.h (interlock handler + reconcileInterlockTx) |
shared reconciler, comment refresh | yes | In scope |
src/models/RadioModel.cpp/.h (automationInterlockProbe) |
isolated probe entry point | yes (#6490 asks for it) | In scope, but see below |
src/core/AutomationServer.cpp/.h |
new interlock verb + observe-only exemption |
yes | New public bridge surface — maintainer decision |
tests/* (4 files) |
socket-free regressions | yes | In scope |
docs/automation-bridge.md, docs/user/docs/* |
verb + user-facing behaviour docs | yes | In scope |
docs/user/src/components/LogAnalyzer/rules.json |
line: renumbering only |
yes | In scope — mechanical |
tools/automation_probe.py |
one MAPPERS entry |
yes | In scope |
Nothing in the diff is unexplained. Two notes rather than objections:
rules.jsonchanges only integers. I resolved all seven touched references against the head checkout (7721→else if (key == "model"),12717→"RadioModel: DAX TX create failed", etc.) — every one lands on its quoted text, and the ±88/±60 split matches the two RadioModel hunks.- The new
interlockverb is permanent public bridge surface (80 verbs → 81) arriving in a bug-fix PR. It is in the issue's acceptance list, but #6490 was filed by the PR author ~11 minutes before this PR, self-assigned, and labelledmaintainer-review— so the acceptance criterion and the implementation share an author. It follows the precedent set by #6470'sinvoke editText/commit. The code itself is well-fenced (token-gated, no aliases,staticentry point, nom_radioModeltouch, provably no command/keying intent), so this is a "does this belong in the shipped registry" call, not a correctness objection. Flagging it for the maintainer, not blocking on it.
Blockers
1. src/models/RadioStatusOwnership.h:107 — the live split is driven by an attacker-chosen remote field, and the field is retained so the cost replays on every later interlock line.
const QStringList sources = lastInterlockSource.split(QLatin1Char(','));What changed: three == compares became one QStringList materialisation. The input is not bounded anywhere upstream. src/core/backends/flex/RadioConnection.cpp:619-626 accepts any newline-terminated line up to the 16 MiB kMaxReadBuffer, CommandParser::parseKVs applies no per-value cap, and src/models/RadioModel.cpp:11657 stores kvs["source"].toUpper() verbatim. Because that value survives across interlock deltas until a terminal state clears it (src/models/RadioModel.cpp:11684-11686), one oversized comma-dense source= followed by a stream of tiny interlock state=PTT_REQUESTED lines re-materialises a multi-million-element list per status line. The 128-character bound the PR added for the probe (src/core/AutomationServer.cpp:8143,8160) does not cover this path.
Canon sentence, CONSTITUTION.md:191-198 (Principle VII), which names "the radio's VITA-49 status" and SmartLink/WAN as untrusted:
Every parser bounds-checks lengths, caps allocations, validates ranges, and fails closed on malformed input; it must not crash, hang, over-allocate, or act on bad data.
Validate at the boundary, as that principle requires — fail closed rather than truncate, since a .left(n) cut can slice ,MICROPHONE into ,MIC and manufacture a hardware-PTT match:
if (kvs.contains("source")) {
// Principle VII: FlexLib's token set is MIC/ACC/RCA/SW/VOX/TUNE, so a
// longer list is malformed — drop it rather than retain and re-split it.
const QString source = kvs["source"].toUpper();
m_lastInterlockSource = (source.size() <= 128) ? source : QString();
}Alternatively, keep arbitrary lengths working and remove the allocation instead (this also makes the new #include <QStringList> unnecessary):
const auto hasToken = [&lastInterlockSource](QLatin1String token) {
qsizetype from = 0;
for (;;) {
const qsizetype comma = lastInterlockSource.indexOf(QLatin1Char(','), from);
const qsizetype end = (comma < 0) ? lastInterlockSource.size() : comma;
if (QStringView(lastInterlockSource).mid(from, end - from) == token) {
return true;
}
if (comma < 0) {
return false;
}
from = comma + 1;
}
};
const bool hardwarePtt = hasToken(QLatin1String("MIC"))
|| hasToken(QLatin1String("ACC"))
|| hasToken(QLatin1String("RCA"));Calibration, stated plainly: this is a canon-citation blocker with a six-line fix, not a user-facing functional break — the peer is a radio the operator dialled, and the line length is capped at 16 MiB so the per-message allocation is bounded, as the security-audit pass established against the codex bot's "unbounded" framing. If the maintainer reads Principle VII as already satisfied because the 16 MiB field was being copied before this PR, this downgrades cleanly to a nit. Either way the fix is cheap enough to land here. Credit: raised by chatgpt-codex-connector and confirmed independently by security-audit (CodeGuard), which refuted the "unbounded" wording and located the retain-and-replay amplification.
Nits (non-blocking)
src/core/backends/flex/FlexPttStopTracker.cpp:286—(!source.isEmpty() && source != u"SW")→Failure::Ownership. #6490 explicitly rules this strictness in-scope-to-keep, and failing closed is the right default for an evidence certificate, so this is not a defect. But the consequence is worth recording: this tracker is live (src/core/backends/flex/RadioConnection.cpp:79), so on 4.3 firmware any software-keyed cycle whose interlock reports a second latched token (SW,VOX) now silently disqualifies the independent-stop evidence path, andtests/flex_ptt_stop_tracker_test.cpp:198-207pins that outcome. A follow-up issue for 4.3 list handling there would keep it from being forgotten.tests/radio_status_ownership_test.cpp:103-104—check(interlockKeepsLocalTxOn(true, false, false, false, false, source, true), "VOX enable preserves legacy source-independent reconciliation")is tautological:voxEnabledalone short-circuits the return, so this row passes for every string and cannot discriminate the token logic. Harmless, but it is not evidence. The discriminating rows (MIC,VOX→ true,MICROPHONE→ false) are the ones carrying the fix.src/models/RadioModel.cpp:1933-1936— the signal handlers are connected aftertransmit.applyChanges(delta), so the vox seeding is outside the capture window. Thecommands=[]claim still holds (applyChangesemits only*Changed, nevercommandReady), but connecting first would make the reply self-evidently complete.src/core/AutomationServer.cpp:2758vs:8140—interlock PROBEclears the observe-only gate (action is lowercased there) and then fails the exact-match indoInterlockwith a usage error rather than the observe-only error. Fails closed; mentioned only so it is not re-derived as a hole.
Socket tests
No new socket behaviour. tests/tx_operation_integration_test.cpp:170-175 constructs RadioConnection as an inert value with a recording m_commandSinkForTest and never binds, listens or connects; the added setClientIdentity only stores a handle into m_handle through the existing friend class TxOperationIntegrationTestAccess (src/core/backends/flex/RadioConnection.h:32). automation_boundary_core_test drives AutomationServer in-process via Access::request, as it already did. All four touched tests are pre-existing registered targets.
What I tried to break
- Refactor equivalence. Walked all three branches of
reconcileInterlockTxagainst the deleted inline block, including thecwxActiveclear, the audio-gate edge guards, and theREQUESTED/DELAYtransition predicate. Identical.m_txAudioGate/m_cwxActiveare plainboolmembers (src/models/RadioModel.h:2008,2012), so the reference binding is sound. - Sibling call sites. Grepped the whole head checkout for consumers of the wire
source=string. Only two exist: this predicate and the stop tracker above. The status-bar TX timer path (operatorTransmitActive) runs off locally trackedTransmitModel::PttSourceenums, not the wire text, so it needs no 4.3 list handling. - The regex. The diff renders it as
"\A...\z", which as a C++ escape would beA...zand would reject every legal source.od -con the head checkout shows"\\A[A-Z0-9_,]{0,128}\\z"— correct PCRE anchors, one flat character class, no backtracking shape. The tab, duplicate-key, unknown-key, bare-field and 129-character cases are all intests/automation_boundary_core_test.cpp:130-141. - Probe isolation and lifetime. The JSON arrays are declared before
TransmitModel transmit, so the by-reference lambda captures outlive the signal source; the context object is&transmit, so every connection dies with it.TransmitModel's constructor is: QObject(parent) {}— the "no hardware, settings, backend or transport" comment is accurate.TransmitDeltaisstd::optional-based, so onlyvoxEnableis applied. Work is fixed at five steps. - Gate ordering. Token compare at
src/core/AutomationServer.cpp:3975-3995(constant-time, onlypingexempt) runs before the observe-only gate at:4003, which runs before dispatch — so the observe-only exemption cannot be reached without the token, astests/automation_boundary_core_test.cpp:179-181asserts.interlockhas no aliases and no TX-permission surface because it emits nothing. - Do the tests discriminate?
MIC,VOX→ true fails against whole-field matching;MICROPHONE,RCAX→ false fails against substring matching; the foreign-owner rows fail against an ownership bypass. The four mutations the body claims are the right ones. - Docs regen. The hand-written
### interlock probeprose sits above<!-- BEGIN GENERATED VERB TABLE -->(docs/automation-bridge.md:4835), so the nextgen_bridge_docs.pyrun will not clobber it. - CodeGuard. All 40 flagged lines in
AutomationServer.cpp(300…13087) fall outside this PR's three hunks (2759-2762, 3321-3327, 8138-8171) — a whole-file scan, not a diff scan. Therules.jsonhits land on untouched log-matching regex literals in a pure data table. The"interlock-probe-test-token"literal is a value fed tosetAuthTokenby the test that asserts rejection. Refuted, not repeated. - Could not check. CI is green on
9dbfc5d9(build, check-macos, check-windows, Static checks, docs), but I cannot run the suite, drive the bridge, or reach 4.3 firmware: the 16-case bridge matrix, the four standalone mutations, and live SmartSDR convergence are all unreproduced here. Everything above is reasoned from the head checkout at/tmp/aetherclaude/pr-6493, not observed at runtime.
Recommendation
Needs maintainer decision. The fix itself is clean, correctly scoped and genuinely well tested — on the interlock reconciliation alone this would be "approve with nits". Two things need your call: whether interlock probe should become permanent bridge surface on an issue the author filed for themselves minutes earlier, and whether Principle VII's "caps allocations" applies to the live source= field the diff now splits. Concrete next step: land the six-line boundary cap (or the allocation-free scan) in this PR regardless of how the verb question goes, since it is cheap and the canon sentence is explicit. Thanks for the unusually legible diff — the shared reconciler is a real improvement over two copies of that branch chain.
🤖 aethersdr-agent · cost: $16.3307 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (373 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: $18.1410 · model: claude-opus-5 |
|
Fixed in signed commit Addressed the actionable review feedback:
The CodeGuard comment publishes 40 locations despite its 373-finding heading. I checked all 40 published locations against the reviewed head and base: every flagged line's text already exists in the base. Many hits are backticks in comments or strings, error-code/regex data, and the existing loopback TCI simulator. No added PR line establishes the reported command injection, SSRF, path traversal or PII claims. This does not claim an audit of unpublished findings or all pre-existing code. Maintainer decision for Jeremy/KK7GWY: please review inclusion of the isolated Validation: native ARM64 application build with RADE enabled, all six focused socket-free CTests, static/architecture checks, generated references and log-rule consistency checks pass. The disconnected Observe-only bridge proof on clean The PR remains ready for review; CI is rerunning for this commit. |
Fixes aethersdr#6490. Addresses the interlock compatibility item in aethersdr#6437. Preserve transmit ownership and intent gates, add an isolated production-reconciliation bridge probe, and cover exact token lists and state transitions offline.
Keep the isolated diagnostic available in observe-only mode without admitting other actions or bypassing authentication. Add regression coverage that fails on the prior gate and document the diagnostic contract. Refs aethersdr#6490.
da8ab17 to
32a06f4
Compare
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed exact head 32a06f4cda22b5785b15897bdabb1d32369c1d13 after the operator-requested signed rebase onto 5b43981a2. No blocking finding remains. Manual adversarial review completed; the harness code-review tool was unavailable.
RadioStatusOwnership.h:100 scans exact comma-separated hardware tokens without per-token allocations or truncation, keeps ownership authoritative, and preserves the existing VOX-enable and local-intent paths. The shared reconciler at RadioModel.cpp:1953 preserves the prior branch and edge-publication behavior. AutomationServer.cpp:8193 bounds and validates probe options; the Observe-only exception matches dispatch and follows authentication. The diagnostic remains within #6490's acceptance scope, as accepted by the operator's explicit request to review, rebase, and merge this PR. It creates no live model/transport and no transmit authority. The strict software-only stop tracker remains unchanged.
The earlier allocation finding is fixed in original da8ab1726, now rebased as 32a06f4cd; its thread already contains the author's evidence and resolution. I independently verified the allocation-free scan and large-input tests. The other review nits are addressed: intent capture precedes VOX seeding, Observe-only uses exact action matching, the source-independent VOX rows intentionally preserve legacy behavior, and #6503 records mixed-source qualified-stop work outside this fix. Published CodeGuard locations do not identify an added executable sink in this diff; the added handler parses options and invokes only the isolated reconciler.
Evidence on the clean reviewed head:
- ARM64 macOS app plus all six focused targets built successfully, with RADE enabled and no RNNoise x86 objects. All six focused CTests pass headlessly with the pinned Qt runtime selected.
- A scratch build restoring the original whole-field matcher fails nine ownership-test assertions; the current matcher passes.
- Running native app: 16 source/VOX cases plus two foreign-owner cases pass through the authenticated Observe-only socket with isolated settings, all IP networking denied, and TX disabled. Every case reports empty command/keying arrays; malformed-input, mutation/action and wrong-token refusals pass; CLI mapping works. Before/after attached radio/transmit snapshots are identical and disconnected.
pingidentifies32a06f4cd,dirty=false,readOnly=true,authRequired=true. The isolated process was stopped. - Four architecture gates pass on both refreshed main and head with identical findings except line offsets. Generated references/bridge docs, test registration, and 707 log-rule checks pass. Hosted Docs and Static checks pass on this head; Linux/macOS/Windows CI are still running and must pass before merge.
The only rebase conflict was generated log-reference locations, regenerated from the merged source. Contributor authorship is preserved, and GitHub verifies all three rewritten signatures. No user-visible UI or radio behavior beyond the reported source-list fix was added. Local runtime validation selected the pinned framework path to avoid the local QtKeychain installation's older Qt reference. This is offline client-state proof, not live firmware, GUI meter, audio-device, or RF proof.
jensenpat
changed the title
Fixes #6490. Addresses the interlock source-list item in #6437; the parent compatibility checklist stays open.
When Flex interlock status reports
source=MIC,VOX, whole-field MIC/ACC/RCA comparisons reject hardware PTT with VOX disabled and turn local TX meters/audio gating off during an owned transmission. Match exact comma-separated hardware tokens using a single-pass, non-owning view scan. Preserve legacy sources, ownership checks, the existing VOX-enable path, and explicit operator intent. A VOX token alone grants no intent. The qualified software-only stop tracker retains its strictSWrequirement; mixed-source stop qualification is tracked separately in #6503.The
interlock probe source=<tokens> [vox=0|1] [owner=local|other]automation diagnostic exercises the shared production reconciler with an isolatedTransmitModel. It constructs noRadioModel, backend, transport, or device and leaves the active session unchanged. Input is bounded and validated. Results expose state snapshots and captured command/keying intents. The probe works in Observe-only mode and still requires the configured authentication token. User and bridge documentation, the CLI mapping, and generated log-reference locations are updated.The protocol reference is FlexLib 4.3.16.42034
Radio.cs:8236-8261, from FlexRadio's published source archive.Validation on clean signed head
32a06f4cda22b5785b15897bdabb1d32369c1d13, rebased onto main5b43981a2:automation_boundary_core_test,radio_status_ownership_test,tx_operation_integration_test,flex_ptt_stop_tracker_test,flex_ptt_wire_session_test, andtransmit_model_test.commands=[]andkeyingIntents=[]. Malformed options, wrong tokens, unsupported actions, and model mutations are refused; CLI mapping passes. Attached radio/transmit snapshots remain unchanged. Bridgepingreportssha=32a06f4cd,dirty=false,readOnly=true, andauthRequired=true. The isolated process was stopped afterward.da8ab1726evidence additionally records four discriminating mutations and the large comma-dense allocation/truncation checks.All behavioral verification is offline client reconciliation. Live SmartSDR 4.3 firmware convergence, visible GUI meter rendering, audio devices, and RF transmission remain unverified. No radio was connected and no RF transmission was performed. Manual landing review found no remaining blocker; the harness
code-reviewtool was unavailable.Generated with OpenAI Codex (GPT-6 Astra)