Repository navigation
fix(pan, icom): suppress unsupported-control notices during band-state display replay. Principle VIII. - #6672
Conversation
…e display replay (aethersdr#6667). Principle VIII. - In RadioModel.cpp, pass /*announce=*/false to requestPanDisplay in setPanWnb and setPanWnbLevel, matching their display replay siblings; these setters are only invoked during connect/band-state restore, where refusal on radios lacking WNB (such as Icom) should not raise an operator-facing dropped-command notice. - In MainWindow.cpp:restoreBandState, pass /*announce=*/false when restoring LevelRange to prevent spurious unsupported-control notifications on radios that do not declare level range controls. - Fixes the icom_family_test regression introduced in aethersdr#6628 where replaying stored display state triggered commandDropped. Fixes aethersdr#6667.
NF0T
left a comment
NF0T
left a comment
There was a problem hiding this comment.
Review and Verification of PR #6672
Issue fit: Resolves the new regression failure on main reported in #6667:
FAIL: Icom display-state sync does not emit Flex-only pan or waterfall commands
Scope & Analysis
| File | Change | Verdict |
|---|---|---|
src/models/RadioModel.cpp |
Pass /*announce=*/false to requestPanDisplay in setPanWnb and setPanWnbLevel during band-state restore |
In scope |
src/gui/MainWindow.cpp |
Pass /*announce=*/false to requestPanDisplay for PanDisplayField::LevelRange in restoreBandState |
In scope |
Verification
- Static Checks Passed:
python tools/check_command_plane.py --strict(125 flat, 0 added).python tools/check_capability_records.py --strict(70 frozen).python tools/check_ci_test_gate.py --strict(frozen per-PR selection unmodified).python tools/check_engine_boundary.py --strict(0 blocking findings).python tools/gen_seam_probe_table.py --check(up to date).python tools/gen_touchpoint_manifest.py --check(up to date).python tools/check_network_timeouts.py --strict(0 violations).python tools/check_qsettings.py --strict(0 violations).python tools/check_theme_seed.py --strict(clean match).
- Auto-squash merge configured:
- Auto-squash armed via
gh pr merge 6672 --squash --autociting Principle VIII. - Awaiting green CI checks and peer codeowner review.
- Auto-squash armed via
—
🤖 Reviewed by Antigravity (Gemini 3.8 Flash (Medium))
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for turning this around fast, @NF0T — the change is the right shape and it matches the house pattern exactly. I could not break the code. I did break two claims in the body, and there's a merge-order trap worth fixing before this lands. No blockers.
1. Issue fit
Yes, for the one failure it targets. This implements the #6667 triage handoff verbatim: announce=false on both WNB setters plus the LevelRange sibling the triage named at MainWindow.cpp:11422. tests/icom_family_test.cpp:326-334 calls setPanWnb(false) and setPanWnbLevel(50) and then asserts connectTimeDroppedCommands == 0, so there is a test that fails without this fix — that's the right coverage layer (socket-free model test, a refusal/non-event). Bug fix with a named root cause, so no RFC is needed per GOVERNANCE.md.
Worth stating explicitly: the refusal class is every non-Flex family, not just Icom. Wnb, WnbLevel and LevelRange are declared by FlexBackend.cpp:552-557 alone — HL2 (Hl2Backend.cpp:1843), ANAN (AnanBackend.cpp:423), RTL (RtlSdrBackend.cpp:245) and Icom (IcomCivBackend.cpp:295) all omit all three, and Sim (SimBackend.cpp:279) declares Geometry only. So the flag flip is correct for more backends than the one test exercises.
Two body claims did not survive checking — see findings 1 and 2.
2. Scope
| File | What it changes | Claimed in the body? | Verdict |
|---|---|---|---|
src/models/RadioModel.cpp (2 hunks) |
announce=false on setPanWnb / setPanWnbLevel, plus a comment each |
Yes, item 1 | In scope |
src/gui/MainWindow.cpp (1 hunk) |
announce=false on the LevelRange push in restoreBandState |
Yes, item 2 | In scope |
Everything in the diff is explained by the issue. No new public surface, no deleted guard, no config or CI churn, no CHANGELOG.md entry (correct — don't add one). One commit, signed. Three of the five setters icom_family_test calls already passed announce=false; this closes the remaining two.
3. Blockers
None.
4. Findings and nits (all non-blocking)
1. Fixes #6667 will close a still-red issue — please change to Part of #6667.
#6667 tracks three failures: transmit_seam_drop_notice_test, show_tx_in_waterfall_client_flag_test and icom_family_test. This PR addresses only the third; the first two are #6665's job, and the triage comment says so ("Merge #6665 → clears 2 of 3"). If #6672 merges before #6665, GitHub auto-closes #6667 while two of its three assertions still fail on main — and the next full-suite run will reuse the same failed-name fingerprint and find a closed issue. One-line fix in the body:
Part of #6667. The remaining two failures are fixed by #6665.
2. Every production path this touches is currently unreachable, so the "spurious notices on band switches" symptom does not reproduce.
The body says replaying WNB state and LevelRange "raise spurious notices on band switches". I went looking for the band switch that does it and there isn't one:
MainWindow::restoreBandState(src/gui/MainWindow.cpp:11394) andcaptureCurrentBandState(:11371) have zero callers anywhere in the tree — a repo-wide grep finds only the definitions, the declarations atMainWindow.h:1199-1200, and mentions inCHANGELOG.md/docs/architecture/recenter-policy.md:506. Noconnect, noinvokeMethodby name.BandSettings::saveBandState/loadBandState(src/models/BandSettings.h:50-51) likewise have no callers; onlysetCurrentBand/currentBand/bandForFrequencyare used.setPanWnb's only non-test caller isMainWindow.cpp:11427, inside that dead function.setPanWnbLevelhas no caller at all.
So the whole per-band snapshot feature is unwired, and the real effect of this PR is to make icom_family_test honest and to harden a path that is correct for whenever it gets wired. That's still worth merging — I'd just rather the body said that than claimed an operator symptom, because the next person to read #6667 will go looking for the band change too. (docs/architecture/recenter-policy.md:506 listing restoreBandState(...) as a live tune path is stale for the same reason — not this PR's job to fix.)
3. src/gui/MainWindow.cpp:11426 — the RfGain push three lines below your fix still announces.
m_radioModel.setPanRfGain(snap.rfGain);
m_radioModel.setPanWnb(snap.wnbOn);setPanRfGain → setPanRfGainFor → requestPanDisplay(..., {RfGain, gain}) with announce defaulting true. RfGain is declared by Icom/HL2/ANAN/RTL, so the issue's radio doesn't refuse it — but Sim declares Geometry only, so on the demo backend this is the same spurious notice in the same replay block. Moot today for the reason in finding 2, and the flag must not be flipped inside setPanRfGainFor (that has genuine operator callers at MainWindow_Wiring.cpp:5692, MainWindow_Shortcuts.cpp:1150, MainWindow_Controllers.cpp:1726, RigctlProtocol.cpp:1680). If you want the block internally consistent, move it into the pan scope you already have:
if (auto* pan = m_radioModel.activePanadapter()) {
// Don't push center or bandwidth — slice tune recenters the pan and
// the radio determines bandwidth. Pushing stale saved values causes
// FFT/waterfall misalignment during the transition (#279, #291).
// Only restore dBm scale (client-side display preference).
m_radioModel.requestPanDisplay(pan,
{PanDisplayField::LevelRange, PanLevelRange{snap.minDbm, snap.maxDbm}},
{}, /*announce=*/false);
m_radioModel.requestPanDisplay(pan,
{PanDisplayField::RfGain, snap.rfGain}, {}, /*announce=*/false);
}
m_radioModel.setPanWnb(snap.wnbOn);Equally fine to leave it and say in the body that the replay block has one announcing push left.
4. No test pins the new behavior where it belongs. icom_family_test covers the WNB pair incidentally (its subject is Icom family materialization), and the LevelRange site at MainWindow.cpp:11422 has no coverage at all — GUI layer, so a nit rather than a blocker. tests/pan_display_drop_notice_test.cpp already builds Icom and HL2 fixtures and asserts the operator path still announces (:245 "icom WtrFall Gain, manual black: still raises commandDropped", :253). It is the natural home for the replay counterpart, and it would make the policy split explicit in one place. Suggestion, not a request.
5. announce=false suppresses the log line too, not just the signal. RadioModel.cpp:10336 emits qCWarning(lcProtocol) << "Backend refused operation" << operation before the operatorOrigin gate, but requestPanDisplay skips the whole reportReceiveDispatch call when announce is false — so these refusals now leave no trace in a support bundle. That is exactly what applyWaterfallAutoBlack (:6753) and RadioModel_PanControls.cpp:132 already do, so I'm not asking you to change it; flagging it so the maintainer knows the refusal is fully silent, not just un-toasted.
6. One question I could not settle. The comment at MainWindow.cpp:11419-11421 calls the dBm range a "client-side display preference", but the push is gated on a backend capability, and nothing in the block applies the saved minDbm/maxDbm to the widget locally — only setSpectrumFrac / setRfGain / setWnbActive run after it. If the range really is client-side, a non-Flex radio gets no dBm restore at all and suppressing the notice makes a no-op silent. Possibly fine, possibly a follow-up issue; I didn't chase it far enough to assert either way.
5. What I tried to break
- Does
announce=falsesuppress a real send? No. InRadioModel_PanControls.cpp:234-239the dispatch tom_backend->requestPanDisplayhappens first and the flag is read afterwards, only to decide whether to callreportReceiveDispatch. On Flex (whereWnb/WnbLevel/LevelRangeare declared) the command still goes, the return value is unchanged, and no model state is skipped. - Does this silence an operator who moved a WNB control? No. Every operator-origin WNB path calls
requestPanDisplaydirectly with the announcing default and is untouched:MainWindow_Wiring.cpp:5676,5683(WNB panel) andMainWindow.cpp:5702,5705(band-stack recall). I checked that the setters aren't reachable by name from the automation bridge, rigctld, CAT or TCI either —AutomationServer.cpp:1701-1702only readswnbActive/wnbLevel. So putting the flag inside the setter rather than at the call site is contained today. It is still the more brittle seam — a defaultedbool announce = trueparameter would let a future operator-facing caller opt in — but that's taste, not a defect. - Band-stack recall, which looks like the same stored-value replay.
MainWindow.cpp:5700-5707pushese.wnbOn/e.wnbLevelwith the announcing default, and the triage told you to leave it alone. I tried to show the triage wrong and couldn't: both pushes are guarded against the pan's current value, andBandStackSettings.cpp:74defaults a missingwnbLevelto50— the same default asPanadapterModel.h:315m_wnbLevel{50}— so on a backend that never reports WNB the guards hold and nothing is sent. Correct today, but only because those two defaults agree; if either moves, that path starts announcing. - The body's sibling citation. It checks out:
setWaterfallColorGain(:6697) andsetWaterfallBlackLevel(:6713) passreportDrop=falseoutright and carry the comment "The connect-time replay of the stored value: no operator moved a control", which your new comments deliberately mirror. The capability-derivedreportDropI first suspected lives in the…Forvariants (:6701,:6718), which are the operator paths. The precedent is real. - Attacking the test.
icom_family_testwould also pass withsetPanWnb's body emptied — it proves "no notice", not "still dispatches on Flex". Nothing anywhere pins Flex WNB replay dispatch, so the flag flip can't regress it, but the gap is real and finding 4 is where I'd close it. - Could not check: anything at runtime. No build, no test run, no GUI. Every finding above is reasoned from the code in the PR head checkout, not reproduced. CI on
5f82cd6is 5/5 green (build, check-macos, check-windows, Static checks, Sanitizer configure) — note thatctestin those lanes is-R-filtered, so green does not meanicom_family_testran; the full suite is the only thing that will confirm the fix.
6. Security audit
Required by the harness (CodeGuard reported 100 findings across the two changed files). Ran it: 38 leads examined, all refuted, no findings. They are all on pre-existing lines — the flagged unit is the file, not the hunk — and the diff itself is three compile-time boolean literals with no input boundary. For the maintainer skimming red check marks: the CRITICAL CS-INJ-role_override at MainWindow.cpp:11694 is a DSP comment containing the word "override" ("CNN override: if carrierScore > 0.70, treat as narrow carrier…") documenting a carrier-vs-voice classifier threshold — no instruction to any reader and no prompt injection. The CS-VLN-CMDI-BACKTICK cluster is backticks inside // comments naming firmware verbs; CS-VLN-CMDI-AMP is C++ reference declarators; CS-VLN-PATH-PROC at :6321 is the constant /proc/self/status read for the status-bar RSS; CS-VLN-SSRF-HEX hits are internal sentinel response codes and Flex stream ids; CS-PII-IP at :6949 is the WSJT-X link-local multicast default 224.0.0.1 for a local UDP bind. Credited: security-audit (CodeGuard).
7. Recommendation
Approve with nits. The code is correct, minimal, consistent with the established announce=false replay pattern, and it makes a genuinely red test green without touching any operator-origin path — I tried four ways to find a silenced operator and found none. Concrete next step: change Fixes #6667 to Part of #6667 (finding 1) so merging this doesn't close an issue that two still-failing tests belong to, and add a sentence to the body saying the symptom is latent because restoreBandState is unwired (finding 2). Findings 3-6 are maintainer's-choice follow-ups. Merge order matters: #6665 first, or #6667 stays open until it lands.
🤖 Generated with Claude Code
🤖 aethersdr-agent · cost: $17.2955 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (105 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.4921 · model: claude-opus-5 |
|
Antares Detector — candidate vulnerable file(s)
PanadapterStream.cpp parses VITA49 UDP packets by reading a fixed-size header (VITA49_HEADER_BYTES = 28 bytes) and then using raw pointer arithmetic to access packet payload fields. The processDatagram() function reads the packet header and extracts stream ID, packetClassCode, InformationClassCode, vitaSeq, and hasTrailer, then uses these values to index into stream-specific structures (m_daxStreamIds, m_iqStreamIds, m_knownPanStreams, m_knownWfStreams). The code defensively checks data.size() against VITA49_HEADER_BYTES before processing, but the packet payload is accessed via reinterpret_cast<const uchar*>(data.constData()) and pointer arithmetic (raw pointers) rather than a safe memory-safe API. This can lead to reading beyond the intended buffer if the packet is malformed or if data.size() is not strictly VITA49_HEADER_BYTES, potentially causing a buffer overrun or use-after-free-like memory corruption. 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: $17.4921 · model: claude-opus-5 |
|
@aethersdr-agent Thanks for the review and inputs! We have updated the PR body and the auto-squash merge configuration accordingly:
The code and branch remain untouched to preserve the completed green CI checks. — |
There was a problem hiding this comment.
Issue fit
Partially — and the gap is in the commit trailer, not the code.
The code change is right, and I verified the root-cause story independently rather than taking the body's word for it. Pre-#6628, setPanWnb was if (m_activePanId.isEmpty() || !hasCommandPlane()) return; followed by raw sendCmd("display pan set … wnb=…") — on Icom hasCommandPlane() is false, so it returned silently. #6628 replaced that with requestPanDisplay(activePanadapter(), {PanDisplayField::Wnb, on}) and the announcing default. Icom declares panDisplayControl = {Geometry, RfGain} (src/core/backends/icom/IcomCivBackend.cpp:295), so supports(Wnb) is false → Unsupported → reportReceiveDispatch → commandDropped (src/models/RadioModel.cpp:10341-10343). announce=false restores the pre-#6628 silence without restoring the old family-shaped guard. That is the correct seam, and it matches the sibling convention at src/models/RadioModel.cpp:6715 (setWaterfallBlackLevel, "the connect-time replay of the stored level"). I also checked git show 8f00c0563 -- tests/icom_family_test.cpp: empty, so the assertion at tests/icom_family_test.cpp:333-334 pre-dated #6628 — #6628 broke a standing contract, it did not move the goalposts.
The change also implements the triage handoff in #6667 verbatim (both WNB setters plus the LevelRange site at src/gui/MainWindow.cpp:11422), so there is a prior ruling behind it.
But #6667 has three failing legs and this PR fixes one. The PR body honestly says "Refs #6667"; the commit message says Fixes #6667. Single-commit PR → the squash default is the commit message → merging this auto-closes #6667 while transmit_seam_drop_notice_test and show_tx_in_waterfall_client_flag_test are still red. Their fix (#6665) is still open as of this review. I confirmed the other two legs are unrelated to this code path: neither test touches requestPanDisplay or the announce flag; they are radio-setup refusal labels and typed txwaterfall dispatch. See Blocker 1.
Coverage: no new test, and none is needed — tests/icom_family_test.cpp:333 is the regression test, and it fails against the unfixed code (that is how #6667 was filed). Correct layer per AGENTS.md "Test-layer boundary": a socket-free CTest for a non-event/refusal. No test files are added or modified, so there is no socket test or fake peer to record in this PR.
Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/models/RadioModel.cpp:6637-6647 |
announce=false + a comment on setPanWnb / setPanWnbLevel |
Yes (body §1) | In scope — and matches the #6667 triage handoff |
src/gui/MainWindow.cpp:11422-11424 |
announce=false on the LevelRange replay in restoreBandState |
Yes (body §2, disclosed as latent) | In scope — named explicitly in the #6667 triage |
Everything in the diff is explained by the issue. Six lines, two files, no CHANGELOG.md entry, no build/CI churn, no new public surface, no capability or declaration edits, no deleted guards (the only - lines are the two calls being re-emitted with the extra arguments). {} is the positional default completion needed to reach the fourth parameter — not a behavior change. No preference content: this restores prior behavior under a documented convention, so it classifies as a fix, not a preference.
Blockers
-
Fixes #6667.in the commit message will close a still-red issue. The commit trailer reads:Fixes #6667.but the PR body says "Refs #6667", and #6667's own triage comment states: "Merging #6665 closes the
transmit_seam_drop_notice_testandshow_tx_in_waterfall_client_flag_testhalves of this issue." #6665 is open. If this merges first, #6667 closes with two deterministic failures still onmain, and the next full-suite run files a fresh issue whose fingerprint has merely lost one name from the failed-test set — the exact "closed but unfixed" pattern the fingerprint reuse is supposed to prevent. Fix is one word in the commit message:Refs #6667.Alternatively, land #6665 first and leave the trailer — but that makes merge order load-bearing, and a maintainer has to enforce it. Changing the trailer is cheaper.
Nits
-
src/models/RadioModel.cpp:6643-6647— the new comment onsetPanWnbLevelreads "The band-state replay of the stored value", butsetPanWnbLevelhas no production caller at all.grep -rn "setPanWnbLevel" src/ tools/ hal-plugin/returns only the declaration, the definition, andtests/icom_family_test.cpp:329. The change is correct and consistent, but the comment describes a call site that does not exist. Worth a word ("no production caller today; the replay contract applies if one appears") or a follow-up to decide whether the setter should exist. -
src/gui/MainWindow.cpp:11394-11434— same for the hunk's own function. The body is upfront thatrestoreBandStateis unwired, and I confirmed it:captureCurrentBandStateandrestoreBandStateare declared atsrc/gui/MainWindow.h:1199-1200, defined, and never called. So this hunk, andsetPanWnb's only caller, are both dead today. That means #6667's triage claim — "An Icom operator now gets a dropped-command notice on every band change" — is not reachable in a live session. The fix is still right (it restores a contract and clears the test), but nobody should expect a user-visible symptom change from it. Worth correcting in the body so the next reader doesn't go looking for the band-change repro. -
src/gui/MainWindow.cpp:11426— the next line in the same replay block still announces:m_radioModel.setPanRfGain(snap.rfGain);
setPanRfGain→requestPanDisplay(panadapter(panId), {PanDisplayField::RfGain, gain})atsrc/models/RadioModel.cpp:6656, announcing default. Harmless on Icom (it declaresRfGain), but inconsistent inside the very function this PR is making notice-free, and it would announce on a backend that declares neither. Not a setter-flag fix —setPanRfGainis shared with real operator paths (src/gui/MainWindow_Session.cpp:2074,src/gui/MainWindow_Shortcuts.cpp:1150), so the replay would have to callrequestPanDisplaydirectly. Follow-up, not this PR. -
src/gui/MainWindow.cpp:5700-5707— the band-stack recall path replays the same two fields with the announcing default:if (e.wnbOn != pan->wnbActive()) { m_radioModel.requestPanDisplay(pan, {PanDisplayField::Wnb, e.wnbOn}); } if (e.wnbLevel != pan->wnbLevel()) { m_radioModel.requestPanDisplay(pan, {PanDisplayField::WnbLevel, e.wnbLevel}); }
#6667's triage describes these as "the operator-origin WNB path … already correct. Leave those alone" — I read it differently and want to flag the disagreement rather than bury it. This is inside the
BandStackPanel::recallRequestedlambda opened atsrc/gui/MainWindow.cpp:5655, andeis a storedBandStackEntry. The operator clicked recall, not a WNB control; by the same principle the PR applies at:11422, this is stored-state replay. UnlikerestoreBandStateit is genuinely wired, and the live operator WNB controls are elsewhere (src/gui/MainWindow_Wiring.cpp:5676,5683) and correctly still announce. So this is plausibly the one reachable spurious-notice site — gated by the inequality checks, so it only fires when the recalled value differs from the pan's current one. A maintainer call whether it belongs here or in a follow-up; I would not block on it. -
CodeGuard's 40 lead lines on these two files are all whole-file lexical noise, hundreds-to-thousands of lines from the six-line diff. The security-audit (CodeGuard) pass returned no findings and I spot-checked the three worst-sounding myself:
src/gui/MainWindow.cpp:11694is// CNN override: if carrierScore > 0.70, …(the rule fired on the word "override"; no bidi or invisible characters anywhere in either file),:5558is a comment containing the Flex response code50000053matched as a bank-account shape, and:6949iscs.value("WsjtxAddress", "224.0.0.1")— a link-local multicast default for the local WSJT-X listener, never logged. Nothing to act on; recording it so the scan result is not mistaken for "not checked".
What I tried to break
- "These were refactored in #6628 with default
announce = true." Verified againstgit show 8f00c0563: the pre-image was thehasCommandPlane()-guarded rawsendCmd, so the claim holds and the fix restores the prior contract rather than inventing a new one. - Does suppressing the notice lose legitimate operator feedback? I traced every caller of both setters.
setPanWnb: one (dead) caller.setPanWnbLevel: none. The live operator WNB toggles go throughrequestPanDisplaydirectly atsrc/gui/MainWindow_Wiring.cpp:5676,5683and still announce. The dBm-range operator path isRadioModel::setPanDbmRange(src/models/RadioModel.cpp:6627), untouched and still announcing. So no operator-origin notice is suppressed by this diff. - Does the change break a test that asserts the notice is emitted?
tests/pan_display_drop_notice_test.cppandtests/pan_control_routing_test.cppdriverequestPanDisplaydirectly (e.g.pan_control_routing_test.cpp:187,334,341), never through these setters, so the flag flip is contained — same conclusion the #6667 triage reached, reached independently. - Does
announce=falsesuppress the radio write too? No.src/models/RadioModel_PanControls.cpp:234-239: the dispatch happens unconditionally;announcegates onlyreportReceiveDispatch. AndreportReceiveDispatchonly emits onUnsupported, so a successful write was never announcing anyway. - Do the other two #6667 legs share this root cause (i.e. should this PR have covered them)? No —
show_tx_in_waterfall_client_flag_testfails on radio-setup refusal text andtransmit_seam_drop_notice_teston line-output capability pairing; neither referencesrequestPanDisplayorannounce. They need #6665, which is a separate open PR. - Could green CI here mean the fix is proven? It cannot.
icom_family_testappears in noctest -Rfilter in.github/workflows/ci.yml— the only filtered steps are the DV, cross-needle, mac-NR and ASR-probe ones. The five green checks prove this compiles on three platforms; they do not run the test this PR fixes.
Everything above is reasoned from the head checkout at the PR tip. I have no build and no test run here, so I did not execute icom_family_test or exercise any GUI path; the "this makes the assertion pass" conclusion is a code reading of the five setters that test calls (the three waterfall ones already pass announce=false; the WNB pair now does too, and nothing else in that block emits commandDropped).
Recommendation
Approve with nits, once the commit trailer changes from Fixes #6667. to Refs #6667. — that one word is the only thing I would hold the merge for, because it closes a red-main issue two-thirds unfixed while #6665 is still open. The code itself is a minimal, convention-matching, correctly-seamed fix that implements the triage handoff exactly and carries a prior ruling behind it; I tried five ways to find a suppressed operator notice or a broken sibling test and found none. Concrete next step: amend the trailer and force-push, then let a maintainer decide whether the band-stack recall site at src/gui/MainWindow.cpp:5700-5707 joins this PR or becomes a follow-up. Thanks for the careful body — disclosing that restoreBandState is unwired, instead of implying a user-visible fix, made this much faster to verify.
🤖 aethersdr-agent · cost: $11.1869 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (105 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: $11.4373 · model: claude-opus-5 |
Ozy311
left a comment
Ozy311
left a comment
There was a problem hiding this comment.
Reviewed 5f82cd658eade87403480845185b104af61325ad against merge base 69a500779f9d5b8ffeeb48114e3d6e0611339827; trusted governance/current main was 6bb29bb7c7b1988d7f3300e41fc596f62902c65b. Governance manifest: PASS, with the applicable backend, settings, GUI and test-boundary rules read.
Issue fit: This addresses the Icom display-replay notice regression reported in #6667 after #6628. It addresses one of that tracker’s three failing tests; it does not address the other two failures. I independently traced the original setters, the #6628 conversion and the current callers.
Scope
| Changed area | Claimed behavior | Assessment |
|---|---|---|
src/models/RadioModel.cpp:6637–6647 |
Replay WNB and WNB level without unsupported-operation notices | In scope; empirically verified through the production setters. |
src/gui/MainWindow.cpp:11422–11424 |
Silence unsupported LevelRange replay | In scope as latent cleanup. restoreBandState has no production caller at this head, as the updated PR body acknowledges. |
No unrelated feature, TX, thread, persistence, capability or default changes found. announce=false skips both the refusal warning and commandDropped; it does not change capability admission, the dispatch result, ownership/session fencing or supported writes.
Blockers
No new blocking code findings. Existing reviews already cover the tracker-closing wording and the latent-helper scope; I am not duplicating those findings.
Nits and evidence limits
No new independent nit. The existing icom_family_test was not run: its connectToRadio path attempts actual UDP transport to 192.0.2.1. The focused evidence below uses injected state and an unopened command sink, with no bind, connect, listening socket, synthetic firmware peer or radio session. It therefore verifies the notice policy and encoding, not Icom firmware convergence or a live reconnect/band-switch workflow.
What I tried to break
- Built the exact-head
pan_display_drop_notice_testand its sharedaethercoredependencies from a fresh source archive with pinned Qt 6.12 on macOS; the registered test passed. Compiled the complete changedMainWindow.cpptranslation unit with-fsyntax-only; it passed with one unrelated, pre-existing unused-lambda-capture warning. No current-main application objects were mixed into the build. - Extended a private copy of that test with production
setPanWnb/setPanWnbLevelcalls using actual Icom and HL2 capability declarations. Both replay calls were silent; equivalent explicit operator WNB, WNB-level and LevelRange requests still returnedUnsupportedand emitted the three expected notices. Explicit LevelRange replay remainedUnsupportedwithout a notice. The real Flex encoder still emitted both WNB commands to the selected second pane through the unopened sink. - Reverted only the two changed RadioModel setters to their exact merge-base source and rebuilt. The private probe failed exactly four replay assertions: WNB and WNB level for Icom and HL2. Operator-refusal and supported-Flex controls continued to pass. Restoring the exact head made the probe pass again. The unchanged registered pan-display test passes both versions, so I do not count it alone as WNB regression coverage.
- Checked the production caller graph: direct operator controls still call the announcing request API; the altered MainWindow replay helper is unwired. No app was launched, and no hardware/RF, full-suite, sanitizer or additional local platform run is claimed. Eight relevant static gates passed on both head and merge base with identical output: engine boundary, capability records, command plane, touchpoint manifest, generated reference, test registration, CI test gate and QSettings.
CI and recommendation
The required checks and sanitizer-option configure check are green for this head, and its sole commit has a valid GitHub-verified signature. CI run 39635881761 was created at 2026-10-21T16:33:48Z, before current main’s 2026-10-21T19:54:00Z commit. These checks describe the then-current merge result, not a tested merge with today’s main. The inspected filtered jobs do not execute icom_family_test; green CI is not proof that the full suite passes.
Recommendation: COMMENT — no new blocking code findings. The silent-replay policy survived the before/after mutation and explicit operator/Flex controls. Current-main integration, the existing tracker-closing discussion, and the required human CODEOWNER decision remain outside this offline review’s demonstrated evidence; this is not an approval or merge action.
Reviewed with OpenAI Codex.
Keep RF-gain restore and band-stack WNB recall silent when the backend lacks these optional display controls. Explicit operator control requests retain their notices. Add socket-free replay and supported Flex dispatch coverage, document bookmark recall, and refresh generated log references.
|
Landing remediation is in The operator explicitly requested RF-gain replay and band-stack recall be fixed in this PR and recorded in the comments. This also addresses the coverage suggestion: Demo bridge proof (private settings/socket; The tracker-closing concern is retired without rewriting the contributor branch: the live repository settings specify The scanner comments do not identify a changed input boundary: this diff changes typed replay requests, constant announce flags, tests and documentation; it changes no VITA parser, path construction or external command execution. The cited whole-file lexical leads are unchanged code and comments, as prior reviews documented. No blocking review or unresolved inline thread was present. macOS build, focused test, mutation check, eight static gates and 707 Log Analyzer checks passed. Hosted CI/Docs results are awaited for this new head. No live hardware, full-suite or post-merge result is claimed. The automated code-review skill was unavailable; a manual adversarial pass was performed. |
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Verified final head 9b7abe5e1cddbd01719f7164c5b556395fd20197.
The original WNB replay fix remains intact. Signed remediation 9b7abe5e1 addresses the requested RF-gain replay and band-stack WNB recall at src/gui/MainWindow.cpp:5702-5707 and :11424-11428, with code comments and user-guide coverage. This scope extension was explicitly requested by the operator. Direct operator controls still announce unsupported requests. Supported Flex WNB/RF-gain writes still reach the selected second pane.
Pre-merge evidence: macOS app build passed with Qt 6.12 and -j22; headless pan_display_drop_notice_test passed 1/1 on this head. Removing the WNB replay guards fails exactly four assertions (on/off and level, Icom and HL2), while operator-notice and Flex encoder controls remain green. Restoring them passes. Eight static checks pass with identical merge-base/head output; generated log references are refreshed and 707 Log Analyzer checks pass.
The final app binary was driven only through a private offscreen demo bridge, with isolated settings and TX disabled. DEMO-0001 recalled a bookmark containing WNB on/73: observed frequency 14.175 MHz, filter 300–2700 Hz, unsupported WNB still off/50, transmitting false, no unsupported-control status notice and no pan-display refusal log. The instance exited cleanly. The unwired restoreBandState helper is verified by source and dispatch-policy coverage, not a GUI invocation.
The PR comments answer the prior findings. No unresolved review threads or CHANGES_REQUESTED reviews remain. The tracker concern is retired by the independently confirmed PR_BODY squash setting and an explicitly supplied squash message using Refs #6667, without rewriting the contributor's history. Fully silent unsupported replay (warning and signal) follows the existing contract. The stale client-only dBm comment is corrected without changing persistence or local display behavior.
Hosted Docs validation passed. Required CI checks are verified separately immediately before merge. The automated code-review skill was unavailable; this is a manual adversarial pass. No live-radio/RF, full-suite, sanitizer runtime, or post-merge result is claimed.
Problem and behavior
Restore the silent optional-display replay contract broken by #6628.
RadioModel::setPanWnbandsetPanWnbLeveldispatch stored values without reporting unsupported WNB controls. Explicit operator requests retain their existing unsupported-control notices; supported writes and capability/session fences are unchanged.Band-stack bookmark recall also replays WNB on/off and level silently. The unwired
MainWindow::restoreBandStatehelper silently replays both LevelRange and RF gain; this helper has no production caller at this head, so no live band-switch symptom is claimed for it. The operator explicitly requested the RF-gain and band-stack additions during landing. Code comments explain the replay policy, and the user guide documents unsupported WNB bookmark recall.announce=falsesuppresses both the refusal warning and the dropped-command signal; it does not turn an unsupported request into an accepted one.Validation
pan_display_drop_notice_testbuilt with Qt 6.12 and-j22.DEMO-0001, TX disabled: recalled a bookmark storing WNB on/73, reached 14.175 MHz with filter 300–2700 Hz, retained unsupported WNB off/50, and produced no unsupported-control status notice or pan-display refusal log. The instance was closed.icom_family_testwas not executed because its connection path attempts actual UDP transport to a test address; socket-free injected coverage verifies this notice policy instead.Refs #6667. This PR addresses its display-replay leg only.
Generated with OpenAI Codex (GPT-6 Astra)