Human-directed independent review (Claude / @NF0T)

Tier: All 7 changed files (IRadioBackend.h, FlexBackend.{cpp,h}, PanadapterModel.{cpp,h}, RadioModel.{cpp,h}) are plain src/ paths — Tier 3, default CODEOWNERS rule.

CI at review time: build, check-macos, and both static-analysis jobs green. check-windows/CodeQL were still pending — worth a final glance before merge.

Verified independently (not just re-asserting the PR body)

• Principle I: confirmed against FlexLib source (Panadapter.cs:918,951) — "center"/"bandwidth" are the exact wire status keys, both MHz doubles, matching this PR's decode exactly.
• Principle II: the new path only reads wire kvs and never writes back to the radio — one-way reconciliation preserved.
• Choke-point driving: decodePanCenterBandwidth is called from handlePanadapterStatus itself (not a live-only observer), so deferred/replayed status is genuinely covered as claimed.
• Pan-resolution consistency: the new ctor-wired lambda's m_panadapters.value(panId) → activePanadapter() fallback exactly matches the pre-existing fallback used elsewhere in the same function — no divergence between old and new pan resolution.
• Thread safety: the decode call happens synchronously from already-main-thread status processing, so the emitted signal resolves to a direct connection regardless of FlexBackend's own thread affinity.
• Touchpoint manifest: correctly left unconverted for models/PanadapterModel.h since this is a partial field-level conversion, consistent with the documented incremental methodology in aetherd-iradiobackend-design.md.
• Default member values (m_centerMhz{14.1}/m_bandwidthMhz{0.2}) never collide with the -1.0 "unchanged" sentinel.

Findings

1. (Moderate, non-blocking, pre-existing but undisclosed) The "single source of truth" claim is incomplete. PanadapterModel::applyWaterfallStatus() (PanadapterModel.cpp, right after the line_duration handling) still independently parses "center"/"bandwidth" straight out of kvs and writes m_centerMhz/emits infoChanged, entirely bypassing the new FlexBackend::decodePanCenterBandwidth() path. This code isn't touched by this diff, so it's not a regression — but it means center/bandwidth aren't actually single-sourced from the backend yet, and this second ingestion surface isn't mentioned anywhere in the PR body's field inventory (understandably, since it isn't even in applyPanStatus). Cross-checked against FlexLib's Waterfall.cs: "center" is genuinely live there (line 799); "bandwidth" is an explicit FlexLib no-op (line 1116, grouped purely to suppress a debug warning) — so this duplicate may be tracking a field FlexLib itself doesn't treat as meaningful in waterfall scope. Recommend disclosing this as a known gap or folding it into scope before calling this "the template" for the rest of 2.3.

2. (Minor, non-blocking) No test coverage for the new decode/normalize chain. FlexBackend::decodePanCenterBandwidth(), PanadapterModel::setCenterBandwidth(), and the new RadioModel wiring have zero unit tests in this diff, despite existing precedent for testing kvs-driven PanadapterModel setters directly (tests/panadapter_model_rx_antenna_test.cpp). Given this PR is explicitly the reusable template for the rest of RFC 2.3, shipping it without a test sets a weak precedent for the many follow-on conversions.

3. (Nit) ~RadioModel() doesn't null m_flexBackend. RadioModel.cpp nulls m_connection/m_panStream after m_backend.reset() as a defensive pattern against dangling non-owning pointers, but the new m_flexBackend alias isn't included. Harmless today (nothing reads it afterward), but inconsistent with the file's own convention.

Verdict

No functional regression confirmed — the core claim (byte-identical behavior + live GPU verification) holds up independently, and the design is sound for the slice it claims to cover. Posting as COMMENT rather than APPROVE/REQUEST_CHANGES pending your call on whether finding 1 or 2 should hold this before merge.

Human-directed independent review, update (Claude / @NF0T) — covers commit 836a8a6

Follow-up to the earlier review of 7897704 (decode facet only). This pass covers the newly pushed encode + extension facets. Local build verified clean on this commit; CI: build/check-macos/static-analysis green, check-windows/CodeQL pending at review time.

Verified against FlexLib (Principle I)

• wnb/wnb_level/wnb_updating are the correct wire keys (Panadapter.cs:1226,1244,1262).
• "slice set N mode=X" is the correct wire format (Slice.cs:295) — unchanged, correctly preserved through the move.
• sendSliceCommand(nullptr, cmd) correctly falls back to this->slice(txCommand.sliceId) for ID-based resolution — no null-deref, no dangling risk, behaviorally equivalent to passing a real pointer.
• extensionStatus dispatch guard and pan-resolution fallback match the established pattern from the decode facet — no divergence.

Findings

1. (Moderate — regression, independently confirmed against FlexLib) Dropped input validation on wnb_level. Old PanadapterModel::applyPanStatus parsed wnb_level via kvs["wnb_level"].toInt(&ok) and only applied it if (ok && ...) — mirroring FlexLib's own validation exactly (Panadapter.cs:1244: uint.TryParse + range check, "Invalid value" + skip on failure). The new FlexBackend::decodePanExtensions() uses kvs.value("wnb_level").toInt() with no ok check — a malformed/non-numeric wnb_level on the wire now silently becomes 0 and gets applied to the model (spurious wnbChanged/wnbStateChanged) instead of being rejected as before. Low-probability trigger, but a real Principle VII (Untrusted Input Validated At The Boundary) regression that both the prior AetherSDR code and FlexLib itself guarded against. Recommend restoring the ok-checked parse before merge.

2. (Minor — overstated claim, not a functional bug) "TX-inhibit interlock stays above the seam" doesn't actually change behavior for mode changes. TransmitInhibitPolicy::parseSliceTxCommand only recognizes commands containing a literal tx= kv. A "slice set N mode=X" command was already routing through the same guarded sendSliceCommand() before this PR (via SliceModel::commandReady), and the guard was already a no-op for mode changes both before and after. This commit is a pure architecture/plumbing change — the commit message overstates what the "guarded slice sink" accomplishes here specifically.

3. (Minor — quietly expanded scope, harmless) setSliceFrequency/setSliceFilter rerouted but unreachable. The diff also switches FlexBackend::setSliceFrequency()/setSliceFilter() to the new sendSlice() path, but neither has any caller yet (confirmed via git grep pinned to 836a8a6 — the only caller of any of the three is the new setSliceMode call). Dead code today, harmless, but not mentioned in the PR's facet description.

4. (Nit — documentation loss) The removed WNB block's comment — "FlexLib v4.2.18 exposes wnb_updating on display pan status while the radio normalizes the SCU-level WNB threshold; keep it distinct from the per-pan WNB enable flag" — is real, non-obvious wire-quirk knowledge that wasn't carried into decodePanExtensions/applyWnbExtension.

5. (Minor — recurring gap) Still zero test coverage. decodePanExtensions, applyWnbExtension, modeChangeRequested, and setSliceMode/sendSlice have no unit tests (confirmed via git grep pinned to 836a8a6), despite precedent (tests/slice_model_letter_test.cpp, tests/panadapter_model_rx_antenna_test.cpp). Same gap flagged in the original decode-facet review, now repeated across two more facets in what's explicitly meant to be the reusable template for the rest of RFC 2.3.

Assessment

No live-behavior-breaking bug found. Finding 1 is the one with real teeth — a narrow but genuine input-validation regression relative to both prior AetherSDR code and the FlexLib reference it was modeled on. Findings 2-5 are framing/completeness/hygiene, not correctness. Across both commits reviewed so far, the pattern is consistent: sound architecture, correct on the happy path, but validation rigor and test coverage slipping at the edges — worth tightening before this template gets copied three more times for the rest of 2.3.

Thanks @ten9876 — this is a clean, well-documented realization of the 2.3 template. I traced the whole path (wire → handlePanadapterStatus → decodePanCenterBandwidth/decodePanExtensions → normalized signals → setCenterBandwidth/applyWnbExtension) plus the slice encode seam, and the decode side is genuinely behavior-neutral. A few observations, none blocking. CI is green across all 7 checks (build, check-windows, check-macos, CodeQL, Engine/UI dependency direction).

Correctness — verified, no bugs found

• The sendSliceCommand(nullptr, cmd) sink is safe: parseSliceTxCommand classifies slice tune/set mode/filt as non-TX-enabling, so the guard is inert for these verbs and never dereferences the null slice. When a real TX-enable command does route here later, the guard resolves the target from the command string (this->slice(txCommand.sliceId)) exactly as intended.
• The ctor-wired pan lookup (m_panadapters.value(panId) → activePanadapter() fallback) mirrors handlePanadapterStatus's existing resolution, so the addressed-vs-active behavior is preserved. The legacy panadapterInfoChanged is emitted exactly once (the old inline block is replaced by the handler), so no double-emit.
• SliceModel::setMode → modeChangeRequested → setSliceMode produces a byte-identical slice set N mode=… string to the old direct sendCommand, and reaches the same sendSliceCommand. Behavior-neutral.

1. The body undersells the scope (worth a note for future reviewers/bisects). The description frames this as "the smallest genuine slice — two universal panadapter fields (center/bandwidth)," but the diff also lands (a) the full slice encode path — setSliceCommandSink/sendSlice/m_sliceSink, SliceModel::modeChangeRequested, and the setSliceFrequency/Mode/Filter rewiring to the guarded sink — and (b) the WNB extension facet end-to-end. The title ("decode + encode + extension facets") is accurate; it's just the "Why this scope" section that reads narrower than reality. No change needed to the code, but if a TX-inhibited-slice-mode regression ever surfaces, it traces to this PR, not a later one — good to have that on record.

2. Minor: wnb_level parse robustness regression. The old applyPanStatus guarded with toInt(&ok) and only applied the value when ok. The new FlexBackend::decodePanExtensions uses a bare .toInt(), so a malformed wnb_level=<non-numeric> now writes 0 (clamped) into the model instead of being ignored. Real Flex firmware won't send that, so severity is low, but preserving the ok-check when inserting the key keeps parity:

if (kvs.contains(QStringLiteral("wnb_level"))) {
    bool ok = false;
    const int lvl = kvs.value(QStringLiteral("wnb_level")).toInt(&ok);
    if (ok) wnb.insert(QStringLiteral("wnb_level"), lvl);
}

3. Latent (fine today): modeChangeRequested is only serviced when m_flexBackend is set. With Flex as the only backend this is correct, but once a non-Flex backend exists, setMode becomes a silent no-op on the wire while still emitting modeChanged. A // TODO(2.x): route via IRadioBackend once encode is backend-owned at the connect site would flag it for whoever generalizes the seam.

Nice work proving the template on the live RX plane. Points 2 and 3 are optional polish — I'd take this as-is.

_{🤖 aethersdr-agent · cost: $5.7085 · model: claude-opus-4-8}

Code review — high-effort recall pass (8 finder angles → verify)

Verdict: no confirmed functional regression. Ship-shape as a behavior-neutral seam step, modulo one narrow decode delta. Two independent finders (decode-half + encode/lifetime-half) plus a manual line-by-line pass agree the 2.3 template is behavior-preserving: the mode-change wire string is byte-for-byte identical, synchronous, and connected for every slice (incl. the stale-slice reclaim path); the -1.0/absent-field sentinels are correct; extension routing targets the right pan; and the emit-order change has no coupled consumer.

One real delta and two template-hardening items follow inline. I've kept them separate from @NF0T's earlier review, which already covers:

• Waterfall dual-parse of center/bandwidth — pre-existing (applyWaterfallStatus still parses independently); this PR neither introduces nor fixes it. Not a regression; worth a follow-up to converge both ingests on the new setCenterBandwidth path.
• No test coverage for the new decode/encode facets — agreed; the decode facets (decodePanCenterBandwidth, decodePanExtensions) are pure and cheap to unit-test and would have caught the wnb_level delta below.

Confirmed-safe, no action needed: m_sliceSink→m_sink fallback is dead-but-harmless in the wired RadioModel; setSliceFrequency/setSliceFilter are dead template code today (still on the old guarded sendCommand path via SliceModel) — when they convert, wire them like modeChangeRequested and mind the null-slice inhibit gap noted below.

Reviewed the full diff against main. This is a clean, well-scoped increment — the three-facet template (decode / encode / extension) is coherent, the choke-point driving is correct, and the test coverage pins the behavior that matters. Thanks for the thorough write-up and the self red-team, @ten9876.

The three self-review findings are all resolved in the current head

For the record, since these were flagged as open concerns — the final commit (478a31f0, "address 2.3 template red-team findings") already fixes them, so they're now no-ops against the current diff:

1. wnb_level guard — restored. FlexBackend::decodePanExtensions (FlexBackend.cpp:224) does toInt(&ok) and only inserts under if (ok), and aetherd_pan_decode_test pins the malformed-input drop. Confirmed the "decode is behavior-neutral" invariant holds. ✅
2. m_flexBackend alias teardown — ~RadioModel now nulls it immediately after m_backend.reset() (RadioModel.cpp:690), so the if (m_flexBackend) guards fail closed. No event processing runs between reset() and the null in the dtor, so the WAN-statusReceived-reachable window is closed. ✅
3. Null-slice TX bypass — accurately characterized as latent/not-reachable. Verified: parseSliceTxCommand("slice set N mode=…").txEnabled is false, so the inhibit block in sendSliceCommand is skipped entirely and the nullptr slice is never dereferenced (RadioModel.cpp:941). Correct to flag-for-record rather than change here.

Verification I ran

• Pan resolution is consistent. The ctor-wired panCenterBandwidthChanged / extensionStatus handlers resolve the target pan via m_panadapters.value(panId) → activePanadapter() fallback — byte-identical to handlePanadapterStatus's own lookup, so decode + WNB land on the same model as the old inline path. Behavior-neutral. ✅
• Encode path parity. Old setMode already routed through sendSliceCommand (via commandReady); the only delta is s → nullptr, and the guard re-resolves by sliceId from the command text. Wire string unchanged. ✅
• Dependency-direction CI check passed despite the transitional concrete FlexBackend* in RadioModel — good, the alias is documented as going away when status ingress becomes backend-owned.
• Scope is tight: all 11 files are on the stated touchpoint, test is registered in CMake, no stray files.

Minor (non-blocking) — defense-in-depth for the model sink

PanadapterModel::applyWnbExtension (PanadapterModel.cpp:97) does fields.value("wnb_level").toInt() with no ok check, relying entirely on FlexBackend having guarded upstream. That's fine for the single caller today, and the std::clamp(…, 0, 100) bounds the worst case, but as 2.3 adds more producers on the extensionStatus channel this becomes an implicit contract worth making explicit — either re-guard here or add a comment noting the sink trusts pre-validated input. Not a change I'd block on.

Overall: LGTM as a template. The narrow-but-end-to-end scoping is the right call for de-risking the rest of 2.3, and the "known gaps" disclosure (waterfall dual-parse, remaining Flex pan fields) is accurate against the tree. Nice work.

(CI still in progress at review time — build/macos/windows/analyze pending; dependency-direction and accessibility checks green.)

_{🤖 aethersdr-agent · cost: $2.3279 · model: claude-opus-4-8}

Human-directed independent review, final pass (Claude / @NF0T) — 478a31f

Pulled the branch locally, built it (clean), and independently re-verified rather than just trusting the write-up:

• wnb_level guard fix — confirmed in the diff: back to toInt(&ok) + if (ok), matching FlexLib's own uint.TryParse/skip-on-failure behavior.
• m_flexBackend teardown fix — confirmed: nulled immediately after m_backend.reset() in ~RadioModel().
• New test aetherd_pan_decode_test — built and ran it myself (all checks passed), and read the source: it specifically pins the malformed-wnb_level-must-be-dropped-not-zeroed regression, plus decode-sentinel and model-sink coverage for all three facets.
• PR body — now honestly discloses the waterfall dual-parse gap and reframes the TX-inhibit claim accurately (a plumbing move for now, not a live behavior change).

Every finding raised across our two reviews, the bot's two passes, and @ten9876's self red-team is either fixed in code or disclosed as a deliberate, pre-existing, or deferred item. One optional nit remains on record (defense-in-depth ok-check in PanadapterModel::applyWnbExtension, not blocking — the single caller already validates upstream).

We're good to go from our side. Given how load-bearing this template is for the rest of RFC 2.3, leaving the merge call to @ten9876 rather than approving ourselves.