Repository navigation
Fix CWX quote encoding and macro status decoding - #6494
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: 94315c8cd4
ℹ️ 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 protocol fix. #6488's acceptance list maps cleanly onto the diff: all three encode sites now route through one helper (src/models/CwxModel.cpp:17-24, consumed at :199, :281, :350), the single decode site gained the 0x1E case (:577), and tests/cwx_quote_test.cpp asserts exact command bytes through the real CommandParser rather than a hand-built kv map. The "queue character counts unchanged" claim is the one I most wanted to break and it holds structurally, not just by test: nChars is taken from the pre-encode text (seg.text.length() at :205, ch.length() at :286) and "→0x1E is 1:1, so cwxEndIndex, the sent= drain comparison and TX admission keep their arithmetic untouched.
The bridge-preview acceptance bullet is also satisfied — but see Scope: that bullet is self-authored and the issue still carries maintainer-review.
Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/models/CwxModel.cpp |
encodeCwxText helper; 3 encode sites; 0x1E decode |
Yes | In scope — the fix |
tests/cwx_quote_test.cpp |
New socket-free encode/decode regression | Yes | In scope |
tests/tests.cmake |
Registers cwx_quote_test, joins AETHER_SETTINGS_CONSUMERS |
Yes | In scope — matches the adjacent cwx_drain_watch_test idiom exactly |
src/core/AutomationServer.cpp |
New cwx preview / cwx preview-status bridge actions |
Yes | New public bridge surface → maintainer decision |
tests/automation_tx_watchdog_test.cpp |
Isolation test for the two new actions | Yes | In scope |
docs/automation-bridge.md |
Documents the two actions; corrects "Every action here emits a cwx verb" |
Yes | In scope |
docs/user/docs/cwx-panel.md |
One sentence on quote support | Yes | In scope |
Nothing in the diff is unexplained by #6488. No CHANGELOG.md entry — correct. Not updating docs/user/versioned_docs/version-stable/cwx-panel.md is also correct: that snapshot is re-taken at release prep (34363a3), and the only difference between the two files is this PR's new sentence.
Blockers
None.
Needs maintainer decision
1. src/core/AutomationServer.cpp:9136-9175 — the two new bridge actions return raw Flex wire text above the seam.
The reply hands the client the constructed command and its bytes:
{QStringLiteral("command"), command},
{QStringLiteral("utf8Hex"), QString::fromLatin1(command.toUtf8().toHex())},docs/agents/backends.md:9-20 (the #5554 standing notice) says, unqualified by file: "check that your change does not add to any item listed there — … no new raw Flex wire text above the seam." @chatgpt-codex-connector is reading that correctly in substance. Three things in tension, none of which I think I should resolve:
- I looked for precedent and found none.
tci send's{"command", command}(:12098) echoes caller-supplied TCI text, andlastCommandin the waveform snapshot (:5537) is a structured{action, name, code, body}object built at:6297/:6310— not thewaveform remove …wire string. So as far as I can tell these are the first bridge fields to carry Flex command bytes to a client. - Against treating it as a violation: the project's own command-plane ratchet does not register a new site, and legitimately so —
AutomationServerconstructs nothing. The strings come fromCwxModel, whose baseline intools/check_command_plane.py:79is unchanged at 9, as isAutomationServer.cpp's 6 at:103. Static checks are green. The exposure is new; the emitter is not. - Also against: #6488's acceptance list asks for exactly this preview, and the triage comment on that issue proposed this precise shape (disconnected-only, stack-local model, hex bytes). But the issue was authored by the PR author ~21 minutes before the PR and still carries
maintainer-review, so that bullet is self-certification rather than a ruling, and the triage bot is a lead.
I'd note for the record that Codex's "the socket-free test already exercises the production encoder, so keep this proof in the test" is the weaker half of its argument — cwx_quote_test is the mutation guard, but the preview is what produced the before/after byte captures in the PR body, which the test cannot. The question is whether that diagnostic belongs in the shipped bridge API or in an opt-in target.
Nits (all non-blocking)
src/core/AutomationServer.cpp:9145 — declaration order makes the capture lifetime fragile. preview is declared before the state its signal lambdas capture by reference:
CwxModel preview;…with commands, operation and capture declared at :9153-9160. Reverse destruction frees the captured locals before the QObject. This is safe today — I checked: ~CwxModel (src/models/CwxModel.cpp:31-34) only stores to m_queueValid and emits nothing, and both connect calls use &preview as context on one thread, so delivery is direct and synchronous and no lambda can fire after doCwx returns. It breaks silently the day something emits during teardown. Mechanical fix — hoist the three declarations above the model:
QJsonArray commands;
QString operation;
const auto capture = [&commands, &operation](const QString& command, int nChars) {
commands.append(QJsonObject{{QStringLiteral("operation"), operation},
{QStringLiteral("command"), command},
{QStringLiteral("utf8Hex"), QString::fromLatin1(command.toUtf8().toHex())},
{QStringLiteral("nChars"), nChars}});
};
// This temporary production model has no transport, audio or TX
// coordinator binding. Its signals are captured only into this reply.
CwxModel preview;(preview-status returns at :9148 before any of this, so the early-return path is unaffected.)
src/core/AutomationServer.cpp:2739-2805 — the previews are refused in observe-only mode. cwx is absent from isReadOnlyRequest's kSafe set and has no per-action clause, so the gate at :3992 blocks cwx preview whenever m_readOnly is set — even though it is pure introspection with no transport. Fail-closed, so not a defect, and the docs don't overclaim (they say "work with TX automation disabled", not observe-only). Worth matching the sibling anyway: #6493's body records "Review reproduced and corrected an Observe-only allow-list omission" for its isolated probe, i.e. the same omission was treated as worth fixing there. Drop-in beside the tci clause at :2796:
if (name == QLatin1String("cwx")) {
return normalizedAction == QLatin1String("preview")
|| normalizedAction == QLatin1String("preview-status");
}tests/cwx_panel_test.cpp:123-127 — a now-stale duplicate of the encoder.
QString sendCommand(const QString& text, int block)
{
QString encoded = text;
encoded.replace(' ', QChar(0x7f));This is the fourth encode site and it was not updated. It stays green only because no fixture in that file contains a " — so it is a silent stale copy, not a guard. The triage comment on #6488 expected it to adopt the shared helper. Small, and arguably its own cleanup.
docs/user/docs/cwx-panel.md:96 — scoped to 4.3 for an unconditional change. "On FlexRadio with SmartSDR 4.3 firmware, double quotation marks (") are preserved…" is accurate as far as it goes, but encodeCwxText has no firmware gate, and AGENTS.md:40-42 puts the project's reference radio at 4.2.18. I considered and rejected this as a regression: before the change a " closed the quoted argument early and corrupted the command, so pre-4.3 goes from broken one way to unverified, not from working to broken. Still, a 4.2 operator reading that sentence learns nothing about their own radio. One clause ("on earlier firmware the stand-in byte is sent but its handling is unverified") would close it.
What I tried to break
parseKVscomposition, which I expected to be the bug.closesQuotedValue(src/core/backends/flex/CommandParser.cpp:19-21) returns false for a self-closing token likemacro12="…$"(two quotes, not one), so I thought thewpm=25key would be swallowed. It isn't: the multi-token join at:26only fires when the opening token hascount('"') == 1, so a self-closed field is used verbatim and the helper never runs. The test'smacro12="…" wpm=25fixture plus itsmodel.speed() == 25assertion genuinely proves this, and the0x1Esubstitution is what keeps that quote-counting parser well-formed on inbound macros.- A character validator silently rejecting
".RadioModel::cwTextValidationError(src/models/RadioModel.cpp:4264-4286) walkscaps.cwTextAllowedCharacters, which would have made the fix dead on the CAT/TCI/bridge paths. OnlyIcomCivBackend.cpp:349sets it; Flex leaves it empty, so there is no character restriction. Refuted. - A second encoder or a pre-decode upstream. Grepped every
0x7f/0x1esite insrc/: the only CWX ones are the four inCwxModel.cpp(three now shared, one in the panel test).cwxstatus reachesCwxModel::applyStatuswith raw kvs viaRadioModel.cpp:11535-11537— nocleanClientText/decodeTextpre-pass intercepts it, so the decode is not double-applied. - Round-trip asymmetry. The decode maps
*→=but the encode does not map=→*, so client-saved macros send a literal=while the radio echoes*. Pre-existing, asserted deliberately by the test, and out of scope here — noting it only so it is clear I didn't mistake it for new. - The isolation claim on the preview. A default-constructed
CwxModelhas nullm_sendAvailability/m_transmissionAdmission/m_textSenderand an emptyTransmissionRoute, sodispatchCommandfalls to the two signals whose only receivers are the capture lambdas;transmissionRequested/transmissionDispatchedhave none. Both gates sit before the branch, so the disconnected check and the 1024-char bound coverpreview-statustoo. The token-auth gate at:3964and the observe-only gate at:3992both run on the canonical verb beforedoCwx, so no per-verb registration was needed. The new test case asserts the real keyer, macro store and transport stay untouched and would fail against the unfixed encoder (it pins43517f1e44581e). - CodeGuard's 111 findings. None intersect the change. The reported lines (300, 464, 841, 1760, 2399, 6197, 6987, 12290…) all fall far outside the changed regions (3391-3395, 9136-9175, 9226); the four I sampled are comment prose —
:6987is the// A directed broadcast (e.g. 192.168.50.255) …comment behind CS-VLN-SSRF-PRIV-192.toHex()at:9158is a Qt method name with no URL or network call indoCwx, and there is no shell,QProcessorsystem()in the function for the CMDI families to land on. Credit: security-audit (CodeGuard). The test fixtures'setAuthToken("rotated-test-authorization")strings are rotation-test inputs, not secrets.
Could not check:
- The
0x1Emapping itself — the Principle I citation. The in-tree reference atreference/FlexLib/FlexLib/CWX.csis 4.2.18-era and contains zero0x1Eoccurrences (I grepped it), andedge.flexradio.comis unreachable from here. Per the project's own rule a missing 4.3 key in a 4.2.x tree means stale reference, not fabrication — and #6437 cites specificCWX.csline numbers — but nothing in-tree corroborates the byte. What would settle it: installingreference/FlexLib_API_v4.3.16.42034/, which the triage comment on #6488 called step one and expected to precede implementation. Non-blocking; the client-side defect stands on its own regardless of the exact byte. - Runtime behavior. I have no build and no test run; every finding above is reasoned from the head checkout at
/tmp/aetherclaude/pr-6494, not reproduced. - CI does not run either new test. The only
ctest -Rsteps in.github/workflows/ci.ymlare the DV-waveform four (:819-820),cross_needle_meter_test(:832),mac_nr_filter_test(:1174) andasr_gpu_probe_test(:1189). Greenbuild/check-macos/check-windowsprovescwx_quote_testandautomation_tx_watchdog_testcompile; the PR body's local pass is the only evidence they execute.
Socket-test disclosure: none. tests/cwx_quote_test.cpp is socket-free. tests/automation_tx_watchdog_test.cpp contains no QLocalServer/QTcpServer/listen()/connectToHost()/bind() and links aethercore with Qt6::Network only transitively for the library; the new case drives AutomationServer through an in-process f.request(...) helper. The PR adds no socket-owning test and removes none.
Recommendation
Needs maintainer decision on the two new bridge actions; approve with nits on everything else.
The protocol fix is the good kind of small: one helper, four call sites collapsed to three plus a decode, a 1:1 substitution that provably leaves the drain arithmetic alone, and a test that fails against the unfixed encoder for the right reason. I tried the five most likely ways for it to be wrong and it survived all of them. The open question is not correctness but placement — whether a byte-level Flex diagnostic belongs in the shipped bridge API, given #5554's "no new raw Flex wire text above the seam" and that I could find no existing bridge field doing it. The acceptance bullet asking for it is the PR author's own, on an issue still labelled maintainer-review, so that call is Jeremy's. Concrete next step: rule on the preview (keep as-is / move behind an opt-in target / drop), and separately decide whether the 4.3.16 reference tree should land before this merges so the Principle I citation resolves in-tree. The three nits are safe to fold in either way.
Thanks for the unusually legible protocol work here — routing all four encode sites through one helper is what makes the quote case checkable at all, and the macro12="…" wpm=25 fixture is a well-chosen one.
🤖 aethersdr-agent · cost: $19.5906 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (111 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: $21.0783 · model: claude-opus-5 |
|
Antares Detector — candidate vulnerable file(s)
The quote decoding logic in src/models/CwxModel.cpp decodes quoted macro fields by stripping quotes and replacing control characters (e.g., decoded.replace(QChar(0x7f), ' ');). This involves reading untrusted input (the received body) and performing string manipulation without obvious bounds-checking or safe parsing. The code decodes quoted macro fields from FlexLib CWX.cs format, which can contain embedded quotes and control characters, and the implementation may be vulnerable to issues like unsafe string replacement or memory-safety problems in the quote parsing logic. Localized by Cisco Foundation AI Antares-1B running locally in the AetherClaude sandbox, seeded by the Cartographer security map. Advisory only — please verify before acting. 🤖 aethersdr-agent · cost: $21.0783 · model: claude-opus-5 |
Address PR aethersdr#6494 feedback by removing the production raw Flex preview API and its companion tests/docs. Replace the stale panel test encoder with independent byte fixtures covering Send, Live, macro save and F1 playback, and clarify that quote handling on older firmware is unverified.
|
Addressed the review feedback in signed commit 54fd6ef:
For protocol provenance, I directly downloaded the official FlexLib 4.3.16.42034 archive and compared it with 4.2.20. In I also checked the security advisories. CodeGuard's reported file, Validation: final native ARM64 application build with RADE, all four focused CWX tests, user-docs build, and relevant static/registration checks passed. The per-PR test filter remains frozen by repository policy; local execution and mutation evidence are recorded in the PR description. New-head CI is running. PR remains ready for review. Generated with OpenAI Codex. |
Implement the CWX quote stand-in item from aethersdr#6437 using the published FlexLib 4.3.16 source. Cover actual wire bytes, macro status parsing, TX guards and an isolated disconnected agent automation bridge preview. Fixes aethersdr#6488.
Address PR aethersdr#6494 feedback by removing the production raw Flex preview API and its companion tests/docs. Replace the stale panel test encoder with independent byte fixtures covering Send, Live, macro save and F1 playback, and clarify that quote handling on older firmware is unverified.
54fd6ef to
399a8af
Compare
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
No remaining blockers found at 399a8afa787f6f5e6b57d720359e32470209b435.
Rebased the two commits onto c54912ec4 without conflicts, preserved Robbie Foust's authorship, and verified both replacement signatures on GitHub. The final five-file patch retains the quote fix and removes the preview API; AutomationServer.cpp and bridge documentation have no PR delta. The earlier stale panel encoder and older-firmware documentation findings are addressed in this head; the existing inline thread has an evidence-bearing reply and is resolved. No blocking reviews remain.
Verified the protocol authority directly from the published FlexLib 4.3.16 archive (SHA-256 60978db51dac6ea781899dd80832ad4e7876ce96ed72532b57106376fc26463f): CWX.cs defines 0x1E, encodes quotes for sends/macro saves, and decodes macro status without a firmware gate. The helper preserves one-for-one character counts and leaves local-keyer text and admission paths unchanged. Macro indices remain bounded before array access; Qt string operations and the panel's signal blocker do not substantiate the advisory memory-safety claim. The reported AutomationServer findings concern code outside the final patch.
Local macOS validation on this exact head: all four focused CTests passed with QT_QPA_PLATFORM=offscreen (cwx_quote_test, cwx_speed_modifier_test, cwx_drain_watch_test, cwx_panel_test). Removing encoding caused five quote assertions to fail; removing decoding caused three to fail. Restored the exact patch, rebuilt all four targets, and reran them: 4/4 passed, clean working tree. Boundary/capability/command-plane checks have identical base/head findings; registration, frozen gate, generated-reference, bridge-doc drift, and log-rule checks passed. Hosted Docs and Static Checks passed. Platform CI is still running and must finish before merge.
Manual adversarial review performed; the separate automated code-review skill is unavailable in this session. Protocol evidence is socket-free model/parser and real-panel testing: the bridge has no applicable byte/status probe, and no app bridge session, live radio, firmware, or RF validation is claimed. Older firmware remains explicitly unverified.
Generated with OpenAI Codex (GPT-6 Astra)
Summary
Fixes #6488. Implements the CWX quote stand-in item from #6437; the remaining compatibility items stay open.
Embedded double quotes break CWX's quoted command argument, and macro status leaves the radio's quote stand-in undecoded. Batched sends, live characters and macro saves now encode
"as byte0x1E; macro status maps it back. Space encoding, BT/prosign handling, speed modifiers, local-keyer text, queue character counts and TX admission retain their existing behavior.The authority is the published FlexLib 4.3.16.42034 source, inspected directly and compared with the 4.2.20 source.
CWX.cs:30defines the stand-in; lines 48, 91, 100 and 153 encode it, and line 527 decodes it. The new reference applies substitution without a firmware gate. Text without quotes remains byte-identical; quotation-mark handling on older firmware is unverified and documented accordingly.Regression proof
Socket-free tests exercise the production command construction and full status-line parser. For
CQ "DX", the expected raw send-command bytes are6377782073656e64202243517f1e44581e222031; reply character counts continue to use the original text length. Macro fixtures cover quoted/unquoted status, spaces, BT, empty macros, resend and adjacent status keys.Panel tests drive Send, a Live quotation-mark keystroke, macro editing and F1 playback against the real model. Independent fixed byte fixtures replace the stale duplicate test encoder. The editor and local keyer retain literal quotation marks.
Protocol proof stays in the socket-free tests. This PR adds no production bridge API, synthetic firmware peer, socket-owning test, settings or changelog changes. No live firmware, RF, or Windows/Linux runtime validation is claimed.
cwx insert, the radio CW decoder and AutoDX have no existing applicable consumers here and remain outside this patch.Constitution principle honored
Principle I — FlexLib Is The Protocol Authority. The substitution comes from the officially published source. Principles VI and VIII: TX admission is preserved and regression tests reject removal of the mappings.
Validation
cwx_quote_test,cwx_speed_modifier_test,cwx_drain_watch_test,cwx_panel_test.Generated with OpenAI Codex.