Repository navigation
Fix radio nickname updates after acknowledged writes - #6470
Conversation
|
Thanks for this — the completion-callback nickname round-trip is a real improvement, and the failure here is a single line, not a design problem. What failedStatic checks → step Command plane above the seam (run 38588841668). Everything else in that job passed, and This is your code, not CI infrastructure — the #5262 M4 ratchet is working as designed. Reproduced locally at Why
flex->setNicknameCommandSink([this](const QString& cmd, FlexBackend::CommandReply reply) {
return sendCmd(cmd, std::move(reply)) != 0; // ← site #135
});Worth saying plainly: that lambda isn't wire text. The encode still lives in Two dead ends, so you don't spend time on them:
Suggested fix — widen the existing sink instead of adding a second one
I applied steps 1–3 to a scratch copy of your branch and re-ran the checker: If you'd rather keep the two sinks separate for clarity, that's a reasonable argument to make, but it needs a maintainer call on the checker rather than a baseline bump — the script documents its curation criterion precisely so that kind of change can be reviewed on its merits. No reviewer or Copilot comments were attached to this run, so nothing else to fold in. Nice work on the generation-guard in 🤖 aethersdr-agent · cost: $6.1460 · model: claude-opus-5 |
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: 62c3982b1f
ℹ️ 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.
Reviewed against #6469, the project canon, and the head checkout at 62c3982. Findings are reasoned from code — I have no build, no test run and no way to drive the GUI here, so nothing below was reproduced at runtime.
1. Issue fit — Partially, and the body says so
| #6469 item | Diff | Verdict |
|---|---|---|
1. Model/label/status bar stale after an R|0-only clear or rename |
FlexBackend.cpp:197-211 publishes RadioDelta{nickname} on a current-session code == 0; RadioModel.cpp:11859 commits it and infoChanged carries it to the label and status bar |
Addressed |
2. One edit sent radio clear_name twice, 24 s apart |
RadioSetupDialog.cpp:2123 consumes the edit and clears modified before dispatch, and :2182 adds !m_nicknamePending to the infoChanged re-seed |
Mechanically guarded, root cause still unknown |
| 3. No visible acknowledgment of a commit | Explicitly deferred as a cross-settings pattern | Out of scope, correctly |
Item 2 deserves a note rather than a demand. The issue's own hypothesis — that the infoChanged re-seed at :2168-2172 re-armed the field — can't be the mechanism: QLineEdit::setText() resets modified to false, so that path could never have made isModified() true for a second editingFinished. Something else did, and this PR doesn't find it. The body is straight about that ("Addresses #6469", "has not been reproduced or root-caused"), and the new "Return again and focus loss after an acknowledged edit send no second command" check pins the mechanical case. Leaving #6469 open is the right call; no honesty problem here.
2. Scope
| File(s) | Change | Claimed? | Verdict |
|---|---|---|---|
IRadioBackend.h, FlexBackend.{h,cpp} |
NicknameCompletion + reply-carrying command sink |
Yes | In scope. New seam surface (a defaulted param on a virtual); I grepped the head checkout — no other backend overrides requestRadioNickname, so there is no sibling implementation left behind |
RadioModel.{h,cpp} |
Sink swapped from sendCommand() to sendCmdPublic() |
Partly — the body says "the existing sink now carries optional replies through sendCmdPublic" |
In scope, one undisclosed side effect (nit 4) |
RadioSetupDialog.{h,cpp} |
Pending-edit latch, generation counter, disconnect reconciliation | Yes | In scope |
tests/radio_nickname_test.cpp |
Reply/ordering/teardown coverage | Yes | In scope |
docs/user/docs/radio-setup.md |
Documents the confirmed-update behavior | Yes | In scope |
docs/user/src/components/LogAnalyzer/rules.json |
"line" fields shifted +4 / +2 |
Yes ("Log Analyzer line references regenerated") | In scope, and verified: RadioModel.cpp:7637 is else if (key == "model") and :7646 is else if (key == "software_ver") in the head checkout, matching the new numbers |
No file is unexplained by the issue, no guard or early return is removed, no CHANGELOG.md entry, no new flat AppSettings key, no new raw Flex command verb (radio name / radio clear_name already existed on this path), no capability field. I found nothing to unbundle.
3. Blockers
None.
4. Nits (all non-blocking)
1. src/core/backends/flex/FlexBackend.cpp:199 — a malformed reply code reads as success. Confirmed from the parser; flagged by chatgpt-codex-connector and independently by security-audit (CodeGuard).
const bool accepted = current && code == 0;CommandParser.cpp:91-95 writes msg.resultCode only when the hex parse succeeds:
const quint32 resultCode = parts[1].toUInt(&ok, 16);
if (ok) {
msg.resultCode = static_cast<int>(resultCode);
}so R<seq> with no code field, or R<seq>|garbage|, leaves resultCode at its {0} default (CommandParser.h:22) and this gate reads it as an acknowledgment.
Why I'm calling it a nit and not a blocker, so the sizing is visible: the if (ok) gap is pre-existing and every code == 0 consumer in the tree shares it — sendCmd("info", …) at RadioModel.cpp:7620 and the client gui registration callback among them; the string published is the operator's own sanitized text, not anything the peer chooses; and an on-path party able to forge an R-line can forge a well-formed R<seq>|0| just as easily, so the parser default is no privilege gain. A conformant radio emits neither shape. The right fix is in CommandParser — a resultCodeValid flag, or a non-zero sentinel on an unparseable code — which closes it for all consumers at once rather than hardening one call site. Worth its own follow-up issue; I'd not hold this PR for it.
2. src/core/backends/flex/FlexBackend.cpp:206 — the status fence watches only one of the two nickname writers.
&& statusRevision == self->m_nicknameStatusRevision) {m_nicknameStatusRevision is bumped in decodeRadioStatus (FlexBackend.cpp:1685-1687), but that is not the only path that sets RadioModel::m_nickname: the info reply handler at RadioModel.cpp:7634 (else if (key == "name") m_nickname = val;) writes it directly, bypassing RadioDelta and so bypassing the revision counter. An info reply landing between dispatch and our R\|0 therefore does not fence the publish. In practice our write is the newer intent so the outcome is usually right — worth a comment saying the fence covers the status path only, or routing the info nickname through the same counter.
3. src/core/backends/flex/FlexBackend.cpp:1619 — the session fence rides a method about something else.
++m_nicknameSession;
// #4198: forget the amp/tuner encode handles on disconnect/reset so a stale
// handle can't survive into a reconnect (possibly a different radio).clearExtensionHandles() is named and documented for the #4198 amp/tuner encode handles; it now also fences nickname replies, with the bump placed above the comment that explains the rest of the body. onRadioSessionEstablished() (RadioModel.cpp:6822) is the declared per-session backend hook. A one-line comment at the bump, or a dedicated reset, would keep the next reader from deleting it as stray.
4. src/models/RadioModel.cpp:875-877 — the sink swap drops one debug line for every Flex verb.
flex->setCommandSink([this](const QString& cmd, FlexBackend::CommandReply reply) {
return sendCmdPublic(cmd, std::move(reply));
});sendCommand() logs qCDebug(lcProtocol) << "RadioModel::sendCommand:" << cmd << "connected:" … "wan:"; sendCmdPublic() does not. Both apply rejectsForeignPanWrite, so the pan-ownership guard the body claims is genuinely retained — only the log line differs, and it now goes missing for every FlexBackend-encoded command, not just the nickname. I grepped docs/, tools/ and .github/ for RadioModel::sendCommand and nothing consumes it, and RadioConnection.cpp:573 still logs TX: at lcConnection, so nothing diagnostic is actually lost. Flagging it only because it's a tree-wide behavior change the body doesn't mention.
5. tests/radio_nickname_test.cpp:341 — the last replyOrdering() assertion cannot fail.
check(!accepted, "retained reply callback survives backend destruction without applying state");accepted is already false from the preceding "a late reply from an ended session" check, so this passes whether the completion runs with false or never runs at all. Re-arm it so the check distinguishes the two:
accepted = true;
backend->requestRadioNickname(QStringLiteral("Destroyed"), [&](bool ok) { accepted = ok; });
backend.reset();
replies.last()(0, QString());
check(!accepted, "retained reply callback survives backend destruction without applying state");6. src/gui/RadioSetupDialog.cpp:2192 — guard asymmetry. The infoChanged lambda two lines up tests if (m_nicknameEdit && …); the new connectionStateChanged lambda dereferences it bare. Not reachable as a null deref — m_nicknameEdit is assigned unconditionally at :2097, before both connect calls, and never nulled — so this is consistency only.
7. src/gui/RadioSetupDialog.cpp:2097 — no setMaxLength on the nickname field (raised by security-audit), unlike the sibling edits elsewhere in this dialog. It matters a little more now than before: the R\|0 path publishes the client-sanitized string, so if the radio bounds the name server-side and still answers R\|0, the model and the status bar show the untruncated value until some later status or info refresh happens to carry the real one. That's the one Principle II edge an ack-without-echo approach can't close. Worth checking whether FlexLib's Nickname setter bounds length and mirroring it if so — I'm not asserting a number.
5. What I tried to break, and didn't
- Teardown ordering.
onDisconnected()runsexpirePendingCallbacks(RadioModel.cpp:7887) before the session bump inclearExtensionHandles(:7980), so I checked whether a reply could slip through on the way down. It can't: the drain passeskNoCommandPlaneCode(0x50000063,RadioModel.cpp:76), which failscode == 0. The other synthetic drop,kProfileLoadSuppressedCommandCode(0x50000061), is likewise non-zero. The dialog then converges throughinfoChanged(:8146) andconnectionStateChanged(false)(:8147), both afterm_nickname.clear()at:8056. There is a brief window where the expiry completion re-seeds the field from the not-yet-cleared nickname, but the two later emits settle it. - A dispatch that returns
false.WanConnection::sendCommandreturns 0 without invoking the callback whenm_connectedis false, and the one-argsetCommandSinkwrapper returnsfalsewithout invoking it — in both cases the completion never fires, so I checked the dialog can't latch forever:RadioSetupDialog.cpp:2144-2147clearsm_nicknamePendingon theUnsupportedreturn. Covered. - Thread affinity on the retained callback.
RadioConnection::commandResponseis emitted on the worker thread, butRadioModel.cpp:1408connects it withthisas the context object, so the reply — and them_nickname*counter reads, theemit radioChangedand theQLineEdit::setText— all run on the GUI thread.WanConnectioninvokes inline but is a direct GUI-thread member and is nevermoveToThread-ed. No race on the four new counters. - Lifetime.
QPointer<FlexBackend>is tested before every deref (:198),QPointer<RadioSetupDialog>beforeself->m_nicknameEdit(:2136), and the generation counter discards a completion whose edit has been superseded.replyOrdering()exercises reordered success, an interveningnickname=status, an ended session and a destroyed backend — the destroyed-backend assertion is weak (nit 5) but the guard it targets is correct. - Overload ambiguity on the two
setCommandSinksignatures. I checked all 19 call sites, including the explicitly-typedstd::function<void(const QString&)>intests/ModelControlWireSpy.h:17; every one is a one-arg or two-arg callable, none passes{}/nullptr, which would be the ambiguous case.build,check-macosandcheck-windowsare all green on62c3982, which confirms it on three compilers. - The pending latch with no timeout. A radio that stays connected and answers pings but loses this one reply leaves
m_nicknamePendingset, which suppressesinfoChangedrefreshes of the field (model and status bar still track) until the next edit or disconnect. Bounded in the usual case by the ping watchdog; I don't think it's worth a timer. - Sibling surfaces. Nothing else in
src/callsrequestRadioNickname, and no otherIRadioBackendsubclass overrides it, so there is no second nickname write path left on the old behavior. - Scanner findings I could not confirm. All four
rules.jsonhits are the regex/explanation text itself, untouched by this diff::88-89(CS-PII-DL-1,CS-VLN-SSRF-HEX) is the string0xf3000001in a user-facing error explanation;:744issm_\w+/denoiser_48k\.trtpkg;:1003isversion [\']GLIBC_2.\d+' not found. TheCS-VLN-CMDI-BACKTICKhits inIRadioBackend.handFlexBackend.cppare Markdown-quoted Flex command words in comments, the known lexical false positive here. TheCS-VLN-SSRF-HEXhits atFlexBackend.cpp:1523-1566` are pre-existing handle parsing outside the diff. Dropping all of them. - What I could not check. No runtime: the live 4.3 clear, the 24 s duplicate, and the "R|0 with no status echo" firmware behavior all rest on the trace in #6469 and on the author's hardware run. And
radio_nickname_testappears in noctest -Rfilter inci.yml, so green CI proves it compiles, not that it passes — the 4/4 is the author's local claim.
6. Recommendation
Approve with nits. The design is the right one for the reported defect: R\|0 really is the radio's confirmation, so taking it as authority is Principle II-compliant rather than optimistic, and the three fences (session, request ordering, status revision) plus the dialog's edit generation are each exercised by a test that would fail without them. Nothing I threw at it — teardown ordering, reordered replies, dropped dispatches, cross-thread replies, destroyed objects, overload ambiguity — produced a failure. Nits 1 and 5 are the two I'd most like to see land, nit 1 as its own CommandParser follow-up since it's tree-wide rather than this PR's doing. Worth saying plainly: the body's disclosure of what it did not reproduce, and its decision to leave #6469 open, is exactly the right shape for a change like this. Thanks for the thorough write-up and the wire trace.
🤖 aethersdr-agent · cost: $17.3347 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (195 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: $17.5203 · model: claude-opus-5 |
|
Additional changes and live testing are complete at signed head Agent automation bridge: added explicit Verification:
Remaining limits: the original second clear 24 seconds after the reported single edit was not reproduced or root-caused. This run tested the fixed head, without reconstructing the old revision's interaction. Native focus/Return triggers on the locked Mac were not exercised in this 4.3 run; explicit bridge commits exercised the production handler, while native trigger coverage comes from regression tests and the earlier live 4.2 rename test. Optional shared Saved/error and screen-reader feedback remains deferred. The unresolved reproduction gap is also recorded in the issue comment. The PR remains ready for review; build CI is still running on this head. |
NF0T
left a comment
NF0T
left a comment
There was a problem hiding this comment.
Thanks, @rfoust. Taking R|0 as the confirmation is the right design for this firmware, and the write-up made it easy to check. FlexLib 4.3.16 applies the nickname at send time (Radio.cs Nickname setter) and never waits for a status. This PR goes further and waits for the radio's reply, which is more conservative than SmartSDR itself and fits Principle II ("where a command has no status echo the client may update optimistically, but the radio's subsequent status is the truth"). You also answered the earlier review cleanly. Nits 1, 2, 3, 5 and 6 from aethersdr-agent are all addressed in code, and nit 7 is answered with the FlexLib StringHelper.Sanitize reference (no length cap there; I checked).
I drove the PR head on a FLEX-8400, firmware 4.3.16.42034, through the agent automation bridge. Everything the body claims held. I also found one ordering case where the new status fence does the opposite of what it intends, and I reproduced it on the radio. That is the one thing I'm asking to change, below. The branch also conflicts with main now and needs a rebase either way.
Blocker: the status-revision fence discards an accepted rename
FlexBackend.cpp:203 skips the R|0 apply if any nickname= status arrived between dispatch and the reply. The assumption is that such a status is newer than our write. On this firmware that assumption fails in a common case.
Every full radio status from 4.3.16 carries nickname=. I don't mean a rename echo. I mean the routine S…|radio slices=… panadapters=… lineout_gain=… nickname=NF0T-MAIN callsign=NF0T … status the radio sends whenever any MultiFLEX client creates or removes a slice or panadapter. When one of those is generated just before the radio processes radio name, it reaches us after dispatch but before the R|0, and it carries the old name.
Reproduced on the 8400 with slice add and a nickname commit sent back-to-back. Verbatim wire, from the instance's aether.connection log:
[13:54:00.652] TX: "C293|slice create pan=0x40000000 freq=14.200000"
[13:54:00.653] TX: "C294|radio name RaceName"
[13:54:00.677] RX: "S76BB182D|radio slices=0 panadapters=1 … nickname=Proof6470x …"
[13:54:00.697] RX: "R293|0|1"
[13:54:00.697] RX: "R294|0|"
The radio accepted RaceName. Afterwards the app disagreed with itself:
| Surface | Value |
|---|---|
Radio discovery broadcast (connect list) |
RaceName |
Radio tab (radioTab_1025-…) |
RaceName |
get radio → nickname |
Proof6470x |
| Status-bar label | Proof6470x |
| Radio Setup nickname field | Proof6470x. The field reverted after an accepted edit. |
The last row is the important one. A field snapping back to the old name right after the operator committed a new one is exactly the sequence the triage analysis on #6469 gave for the 24-second duplicate clear: the name reappears, so the operator enters it again. The state healed only at the radio's next full radio status (here, when I removed the slice). In a quiet single-client session that may not come for a long time.
Why I think the rule is inverted, not just racy. SmartSDR processes one client's commands in order on the TCP stream, and the R for a command follows its processing. So a status that arrives before our R|0 was generated either before the radio applied our write (stale, as above) or as an echo of it (same value as ours). Either way, applying our sanitized value on R|0 is correct. A status that arrives after the R|0 is applied after it anyway, so it still wins without any fence. That includes another client renaming the radio right after us: their command is processed after ours, so its status follows our reply. I couldn't construct an ordering where the fence helps, and I could make one where it hurts.
If that holds up, the fix makes the code simpler. Drop m_nicknameStatusRevision and its check, and keep the request-ordering and session fences, which are both sound. replyOrdering()'s "nickname status after dispatch supersedes the command reply" then flips: the sanitized value should win. Ideally add a case that mirrors the wire above, with a status carrying the old name between dispatch and R|0. Question for you: did you see a radio emit a nickname status after processing the write but before its R? If there's firmware that does, I'd like to understand it before suggesting the fence go.
Must-do before merge: rebase
mergeStateStatus is DIRTY. git merge-tree against current main conflicts in src/core/backends/flex/FlexBackend.h, src/models/RadioModel.cpp and docs/user/src/components/LogAnalyzer/rules.json. The rules.json line numbers are generated, so please regenerate them after the rebase rather than hand-merging. The green CI is from 2026-10-14, against a base 9 commits behind main.
What I tried to break, and what held
Live on the FLEX-8400 / 4.3.16.42034. Isolated offscreen instance, its own settings dir, AETHER_AUTOMATION_NO_TX=1, transmitting: false throughout. All edits went through your new editText / commit verbs on radioNicknameEdit.
| Attack | Wire | Result |
|---|---|---|
commit on an untouched field |
none | ✅ No write. |
Clear (editText "", commit) |
C131|radio clear_name → R131|0| (44 ms) |
✅ Model, field and discovery all "" with no reconnect. A tree search found no widget still showing the old name. |
Repeat commit; same-value editText "" + commit; dialog close and reopen |
none | ✅ Still exactly one command. |
Sanitized rename Proof 6470_x! |
C209|radio name Proof6470x → R209|0| (97 ms) |
✅ Model, field, status label and radio tab all Proof6470x. No nickname status echo followed. |
Rename racing a slice add |
see blocker | ❌ Split state until the next full radio status. |
Disconnect and reconnect (new handle 1024bae7) |
✅ Rename persisted on the radio. | |
Restore NF0T-MAIN |
C472|radio name NF0T-MAIN |
✅ Confirmed in the model and discovery. |
Four edits produced four commands over the whole session, so "one edit sends one command" holds on 4.3.
Locally (Windows, MSVC, Qt 6.12.0, PR head c5ef0abf9):
radio_nickname_test,automation_boundary_widgets_test,model_control_routing_test,backend_receive_contract_testandaetherd_amp_tuner_encode_testall pass. So do 46 neighbouring tests: the DVK / GUI-client registration / CWX consumers ofCommandParser,radio_setup_*, andautomation_*.automation_sensitive_line_edit_testandautomation_sensitive_grab_command_testfail identically on a base build, so they are environmental and not from this PR.- Mutations, each restored afterwards:
- Parser default put back to
0: all 9 malformed-reply cases fail. !m_nicknamePendingremoved from theinfoChangedrefresh: "unrelated identity refresh cannot replace a pending clear" fails.- The
R|0radioChangedemit removed: 6 assertions fail.
The tests pin what they claim.
- Parser default put back to
- Step 0: both test files drive
AutomationServerin-process. No sockets.
Read and checked:
- The
sendCommand→sendCmdPublicsink swap keepsrejectsForeignPanWrite. - A synchronous failure callback from
sendCmd's profile-load or no-command-plane drops runs the completion and then theUnsupportedbranch. That is harmless. QPointerguards sit on both the backend and the dialog.- The
deferInvokeActioncommit rechecks the opt-in, visibility, enabled and read-only state, observe-only, and the authorization epoch before it emits. - No #5554 prohibited pattern. The
dynamic_cast<FlexBackend*>in the diff is pre-existing context, and the command-plane ratchet is unchanged.
Scope
| Files | Claimed | Verdict |
|---|---|---|
IRadioBackend.h, FlexBackend.{h,cpp}, RadioModel.{h,cpp} |
yes | In scope. |
CommandParser.{h,cpp} |
yes (commit 71bd7ab) | In scope, but tree-wide (see guidance 1). |
RadioSetupDialog.{h,cpp} |
yes | In scope. |
AutomationServer.cpp, tools/aether_mcp.py, both bridge docs |
yes | Bundled but justified. ten9876 asked for exactly this commit path in #6451, and the live proof depends on it. New bridge surface, so see guidance 3. |
docs/user/docs/radio-setup.md |
yes | In scope. Satisfies the user-docs rule. |
rules.json |
yes | Generated. Regenerate after the rebase. |
tests/* |
yes | In scope. |
Preference vs fix: this is a fix. Every behavior change traces to #6469's acceptance criteria or to #6451. No default, layout or workflow changes. No CHANGELOG.md entry, correctly. All four commits are signed and verified. CODEOWNERS: everything here is @aethersdr/reviewers.
Forward guidance (non-blocking)
- The parser change affects every Flex reply, not just nickname. A malformed or missing result code now reaches every callback as
-1(logged asffffffff) instead of0. That fails closed, and it's the parser-level fix the earlier review asked for, so I'm glad it's here. Two notes. FlexLib'sParseReplydrops such a reply outright, so its callback never fires, whereas ours fires a failure. And GUI-client registration treats any non-zero code asDisconnectAndWaitForUser. A conformant radio never sends these shapes, so it's fine, but please give it a sentence in the body so a maintainer sees it isn't nickname-only. - The sink swap drops the
lcProtocolRadioModel::sendCommand:debug line for every FlexBackend verb. That was aethersdr-agent's nit 4, still unmentioned.RadioConnectionstill logsTX:, so nothing is really lost. One line in the body would do. - Bridge verbs. ten9876's #6451 proposal was a single
commitEditfor "most settings fields". This PR shipseditText+commitbehind a per-widget opt-in, used by one field. I think the opt-in is the safer shape. Since third-party agents will bind to these names, it's worth a nod from him on the naming before more fields opt in.
Once the fence question is settled and the branch is rebased, I expect this to be an approval. The squash subject I'd use: fix(flex): apply radio-confirmed nickname clears and renames without a status echo (#6470). Principle II.
Really nice work, especially the mutation testing and the plain statement of what you did not reproduce.
73, Ryan NF0T
👨🏼💻 Co-authored by Claude Opus 5.5
c5ef0ab to
2d3a214
Compare
Fixed in 2d3a214: removed the nickname status-revision fence, retained request/session/lifetime guards, added old-name full-status-before-R|0 rename and clear regressions plus a later-status authority case. The old fence fails those checks; radio_nickname_test and all six focused tests pass on the rebased head. Rebased onto bccd4b7 and regenerated Log Analyzer references; merge-tree is conflict-free. No live-hardware retest is claimed.
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Approved at 2d3a2147361d47952a59e523b86cf5a75cef3844 after a manual adversarial review; the automated code-review skill is unavailable in this session.
- Fixed NF0T's reproduced status-before-reply race in
2d3a2147361d47952a59e523b86cf5a75cef3844: an accepted rename/clear now supersedes the earlier full status carrying the old name. Request ordering, session reset and destroyed-backend guards remain; a later nickname status still wins. - Updated
replyOrdering()and the production-dialog test for that race. The old fence fails the new ordering assertions; the fixed test passes. Removed a duplicate fixture identity connection because main now wires injected backend state itself, retaining the exactly-one identity-notification check. - Rebased onto
bccd4b719, preserving upstream answered-transmit and setup sinks, regenerated Log Analyzer references, and checked the merge result. All five commits are GitHub-verified. - Current-head evidence: Qt 6.12 ARM64 app build with matching QtKeychain and RADE; 6/6 headless socket-free tests (
radio_nickname_test,automation_boundary_widgets_test,model_control_routing_test,backend_receive_contract_test,aetherd_amp_tuner_encode_test,radio_setup_control_routing_test). Isolated authenticatedDEMO-0001bridge exercised nickname edit, repeated commit and empty commit on the production field, identified this clean head, and reported TX disabled / not transmitting; owned PID 35975 was stopped. - Boundary/command-plane/capability/touchpoint findings are identical to main, with no baseline growth. Bridge/generated-reference checks, 707 Log Analyzer checks and diff checks pass. Hosted Docs (site/PDF) and Static checks pass. Platform CI is still running and remains a merge gate.
The prior parser finding is fixed at the shared boundary (invalid codes fail with -1 for every Flex reply consumer). The PR body now discloses that scope and the command-path logging behavior. The early command-plane finding is retired by the unified existing sink, not an exemption; the scanner's backtick/hex hits are comment/regex literals or unchanged handle parsing, with no executable command or URL sink added. All review threads have evidence replies and are resolved.
No new live-radio run: the ordering regression is grounded in NF0T's FLEX-8400 wire trace. The original 24-second duplicate cause remains unproven and #6469 stays open. No remaining actionable findings.
SmartSDR 4.3.16 can acknowledge a nickname clear or rename with
R|0|without sending a nickname status echo. Apply the sanitized nickname only after a valid successful reply so the model, radio tab, station label and status bar converge without reconnecting. Consume the field edit before dispatch, preserve newer uncommitted text, reconcile rejected values, and release pending state on disconnect.An unrelated full
radiostatus can carry the old nickname between dispatch andR|0. The successful reply confirms the requested name despite that earlier status; a nickname status received after confirmation remains authoritative. Request ordering, session reset and backend destruction still fence stale replies.Addresses #6469. The original second clear 24 seconds later remains unproven, so this PR does not close the issue. Related: #6460, #6437 and the bridge editing gap in #6451.
The Flex backend uses the existing guarded reply-capable command path and LAN/SmartLink transport, preserving foreign-pan ownership checks. Current main's answered-transmit and Radio Setup sinks are retained. Malformed or missing result codes now fail closed as
-1for all Flex reply consumers, rather than defaulting to success; valid unsigned 32-bit error codes retain their wire bit pattern. The unified sink usessendCmdPublic, which lackssendCommand's additional protocol debug line; transportTX:logging remains, and current main already routes the backend throughsendCmdPublic.The bridge adds explicit
invoke editTextandinvoke commitactions. OnlyradioNicknameEditopts in. Changed text, including empty text, arms the existing modified guard; commit queueseditingFinishedwithout re-arming repeated commits. Other fields, invalid input, hidden/disabled/read-only edits and observe-only requests are refused. Deferred commits recheck authorization and field state. ExistingsetTextandsubmitbehavior is preserved. User and bridge documentation and MCP descriptions are updated.Validation
bccd4b719; signed head2d3a2147361d47952a59e523b86cf5a75cef3844. GitHub verifies all five commits. Mechanical conflict resolution retains current main's answered-transmit and setup sinks; generated Log Analyzer references were refreshed.radio_nickname_test,automation_boundary_widgets_test,model_control_routing_test,backend_receive_contract_test,aetherd_amp_tuner_encode_test, andradio_setup_control_routing_test.REQUIRE_KEYCHAIN=ON; no RNNoise x86 sources. The authenticated isolatedDEMO-0001bridge identified the clean head and passed production-field rename, repeated commit and empty commit;txAllowed:falseandtransmitting:false. The owned instance was stopped. The final local build and repeat of all six tests and the bridge proof use a Qt 6.12-matched QtKeychain library; the runtime reports no duplicate Qt classes.Existing hardware evidence
The author tested original head
c5ef0abf9on a FLEX-6700 running 4.3.16.42034: bridge-only clear and sanitized rename receivedR|0|, all identity surfaces converged without reconnecting, repeated commits sent no extra commands, and the rename survived reconnect. The original nickname was restored; TX remained disabled. Earlier 4.2.18 evidence exercised native Return/focus loss.NF0T independently tested that original head on a FLEX-8400 running 4.3.16.42034 and reproduced the old-name status race documented in the review. The landing regression injects that exact status-before-reply ordering. The final rebased change has not been retested on live hardware.
Limits
Nickname/StringHelper.Sanitizesource contains no length cap, so no undocumented cap is added.Generated with OpenAI Codex (GPT-6 Astra)