fix(hl2): drop setBandFilter()'s stale CONFIG one-shot

[on8st]

Summary

Refs #4579 — deliberately not Fixes, see Scope below.

MetisClient::setBandFilter() pushed a copy of m_ccConfig into m_oneShot. This deletes that push.

It was redundant: MetisClient::buildNextControlPacket() already puts live m_ccConfig in bank A of every EP2 frame, so the new filter pattern reaches the radio on the next frame regardless. The comment justified the push as getting ahead of "the rotation" — but the config register was never in the round robin at all. That rotation is the receiver NCOs, then gain, then the ADC assignment. There was nothing to get ahead of.

It was also harmful: a one-shot only ever fills bank B, the radio applies bank B after bank A, and the queued copy is a snapshot. setBandFilter() followed by setSampleRate() therefore put one frame on the wire carrying two disagreeing config banks with the stale one applied last.

The two comments that asserted the removed mechanism are corrected rather than left stale — the block in setBandFilter() and the declaration's doc comment in MetisClient.h.

Thanks to @ten9876 for the original analysis; this is the deletion suggested there, and the report's reasoning held in full when re-checked against current main.

Scope — what this does not close

m_oneShot is still cleared nowhere. MetisClient::setRxFrequencyHz, setTxFrequencyHz and setTxDriveLevel push without an m_running guard — unlike setIoBoardTxFrequencyHz, which has one — so a bank queued while disconnected can still ride the next session's first frames. This PR removes the snapshot, which closes the band-filter path; it does not add m_oneShot.clear() to start(), because that reads as in tension with stop()'s explicit "Preserve unrelated one-shot setup; only this board's writes are stale." That looked like a maintainer's call rather than something to fold into a one-line deletion, so #4579 should stay open after this merges.

Constitution principle honored

Principle VIII — Evidence Over Assertion. The defect is demonstrated by a test that fails before the change and passes after, rather than by the reasoning above. On the unfixed tree hl2_band_filter_frame_test reports bank A rate code 2, bank B rate code 0 — two config banks in one frame (2 = 192 kHz, 0 = 48 kHz) and fails 3 of 8 assertions.

Not claiming Principle XI: CI has not run on this head, and self-grading is explicitly not a substitute.

Test plan

• Local build passes — configured and built with the official Qt 6.8.3, RelWithDebInfo
• Behavior verified on a real radio — not done, and it is worth saying why. The test drives MetisClient::buildNextControlPacket() directly with no socket. Which of an EP2 frame's two sub-frames the gateware applies last is read from the HPSDR frame layout, not observed here. It does not affect the conclusion: after this change no frame carries two config banks at all.
• Existing tests pass — full ctest 364/364 passed, 0 failed, 4 skipped (all pre-existing: 1 quarantined, 3 GL/a11y). Rebased on main at 52c1ced5.
• Reproduction steps documented — the new test is the reproduction

Checklist

• Commits are signed
• No new flat-key AppSettings calls — no settings touched
• Code is clean-room
• Subject under 72 characters, imperative, ends with Principle <N>.
• Self-assigned on the originating issue — not possible: this account has pull-only access and gh issue edit --add-assignee returns 403

Note

One thing that could not have caught this, in case it is useful for #5457's inventory: tests/hl2_live_band_filter_probe.cpp carries what its own comment calls its load-bearing assertion — that the EP6 packet rate does not change across a filter write, because "a filter bit that leaked into the sample rate field would halve or double this". That is exactly this failure class, but the probe's step list only ever changes frequency and filter, never the sample rate, so the sequence that produces the stale bank never occurs in it. Not run here — it needs hardware.

🤖 Generated with Claude Code

https://claude.ai/code/session_014TtnKQu6QGrSeBirGeAsAs

[aethersdr-agent]

Issue fit

Yes. #4579 asks for exactly one thing — delete m_oneShot.push_back(m_ccConfig) in setBandFilter() — and the diff does that and nothing else to production code. I verified both halves of the issue's reasoning against the head checkout rather than taking them on trust:

• Redundancy. buildNextControlPacket() has a single ep2Packet() call site (src/core/backends/hl2/MetisClient.cpp:726), and bank A is withMox(m_ccConfig, keyed) unconditionally — on keyed frames too. That line is untouched by this PR and reads identically on the base (main), so the "bank A already carries it" premise holds before and after.
• The deleted comment's premise was false. The removed text claimed waiting for the round robin would leave relays on the old band "for up to three EP2 frames." The rotation (MetisClient.cpp:717–731) is m_ccRxFreq[…] → m_ccGain → kCcAdc. The config register was never a slot in it, so there was no rotation latency to get ahead of. This is a removal that deletes a comment naming a symptom, which is normally the shape I'd push back on — here the symptom the comment names does not exist in the code it guards.
• Staleness. setSampleRate() (MetisClient.cpp:414) rebuilds m_ccConfig wholesale, so the snapshot/live divergence the issue describes is reachable exactly as written.

Scope

File	What it changes	Claimed by title/body?	Verdict
src/core/backends/hl2/MetisClient.cpp	Deletes the one-shot push; replaces the comment	Yes — the entire fix	In scope
src/core/backends/hl2/MetisClient.h	Rewrites setBandFilter()'s doc comment, which asserted the deleted mechanism	Yes	In scope — a comment left asserting a removed mechanism would itself be a defect
tests/hl2_band_filter_frame_test.cpp	New socket-free frame-composition test	Yes	In scope
tests/tests.cmake	Registers the target	Yes	In scope

Everything in the diff is explained by the issue. No settings keys, no protocol/public surface, no CHANGELOG.md entry, no UI or default changed — nothing for the §6 preference check to classify. Signature verified on the single commit.

Socket-test disclosure: none to report. The added test opens nothing — no QUdpSocket/QTcpSocket, no bind/listen/connectToHost, no fake peer. It drives MetisClient::buildNextControlPacket(), which the header already declares public for this purpose ("Exists so the gate can be tested on the exact bytes that would go out"), and is the correct layer per AGENTS.md's test-layer boundary — wire encoding is a socket-free CTest.

Test-layer check. The regression witness is real: with the push present, the queued snapshot is consumed by the first buildNextControlPacket() call, so cases 2, 3 and 4 all fail on the unfixed code. Case 1 is honestly labelled as passing either way. isConfigBank() can't false-positive — kC0Config is 0x00 and every other bank the builder can emit has a nonzero even address (kC0TxFreq 0x02, kC0Rx1Freq 0x04, kC0AdcGain 0x14, kC0AdcAssignOrTxGain 0x1C, kC0I2c2 0x7A), none of which mask to 0x00 after ~kC0MoxBit.

Blockers

None.

Nits

• The other m_oneShot producers still have the cross-session hazard. #4579's second paragraph is broader than the fix: m_oneShot is cleared nowhere, and setRxFrequencyHz (MetisClient.cpp:400), setTxFrequencyHz (:631) and setTxDriveLevel (:647) all push unconditionally, with backend call sites guarded by if (m_metis) rather than by connected state (Hl2Backend.cpp:2412, :2980). stop()'s erase_if (:364) removes only IO-board banks. The issue's primary recommendation was the deletion this PR makes, so this is completeness rather than a gap in the fix — but the alternative it named (m_oneShot.clear() in start()) would have closed all of them at once. Worth a maintainer's call on whether that belongs in a follow-up. Inline note below.
• Case 4's label overstates what it exercises. It says "survives into the next session," but start() is never called — what it actually pins is that stop() leaves no config bank queued. Not wrong, just a broader claim than the code makes. Inline below.
• The new target is in no ci.yml -R filter, so CI proves it compiles in the full-tree build and does not prove it passes. That is this repo's standing property rather than anything this PR did, and I'm noting it so "5/5 green" isn't read as "the new test ran."

What I tried to break

• Attacked the premise, not just the diff. If bank A ever weren't m_ccConfig, the deletion would be a regression. Checked the keyed path, the CW path and the TX-IQ path: MetisClient.cpp:726 is the only packet construction, and m_ccConfig is bank A on every frame regardless of MOX. It survives.
• Looked for a lost ordering guarantee. setIoBoardTxFrequencyHz() queues five ordered banks that must land in sequence (:580). Removing the config push only lets them start one frame earlier; it cannot interleave or reorder them. It survives.
• Tried to make the test pass against unfixed code. It can't — the queued snapshot lands in bank B of the very first frame each case builds, and cases 2/3/4 all inspect that frame. Also checked isConfigBank for address collisions with every bank the builder can produce; none.
• Checked the disconnected/reconnect path. setBandFilter() before start() now mutates only m_params/m_ccConfig and queues nothing, so there is no snapshot left to replay. This is where the old code was worst and the new code is strictly better.
• Read the - lines for a deleted guard. The removal deletes a comment naming a symptom ("relays on the old band for up to three EP2 frames"), which normally I'd treat as a regression signal — I went and confirmed against the rotation code that the symptom it names is not reachable, since the config register was never in the round robin.
• What I could not check: I'm headless — no build, no ctest run, no radio. Every finding above is reasoned from the code in /tmp/aetherclaude/pr-5511, not reproduced at runtime. In particular I cannot confirm the protocol fact that the radio applies the second sub-frame last; the test's own header discloses that and, correctly, does not depend on it.

Recommendation

Approve with nits. The fix is minimal, matches the issue's own recommendation, and the reasoning in the new comments checks out line by line against the surrounding code — including the part where it contradicts the comment it replaces. The new test fails against the unfixed code for the right reason and is at the right layer. The two nits are labelling and follow-up scope, neither worth holding the merge. Next step is a maintainer decision on whether the remaining m_oneShot producers get the same treatment in a follow-up, or whether #4579 closes here.

---

_{🤖 aethersdr-agent · cost: $7.3038 · model: claude-opus-5}

[aethersdr-agent]

Non-blocking, and a maintainer call rather than a request.

This closes the cross-session replay for the band filter by queuing nothing at all. But #4579's second paragraph is broader — m_oneShot is cleared nowhere, and the other producers still push unconditionally:

• setRxFrequencyHz at line 400
• setTxFrequencyHz at line 631
• setTxDriveLevel at line 647

Their backend call sites gate on if (m_metis) rather than on connected state (Hl2Backend.cpp:2412, :2980), and stop()'s erase_if at line 364 removes only the IO-board banks. So a bank queued while disconnected can still ride the next session's first frames — the same shape this PR just closed here.

The issue's primary recommendation was exactly the deletion you made, so I read this as follow-up scope, not a gap in the fix. Flagging it because its alternative suggestion (m_oneShot.clear() in start() alongside m_txSeq / m_roundRobin / m_haveRxSeq / m_drops / m_linkUp) would have closed all four paths at once, and it would be easy for #4579 to be closed by this PR with that half still open.

[aethersdr-agent]

Labelling nit only — the assertion itself is sound and does fail against the unfixed code.

start() is never called here, so "the next session" is the framing rather than the mechanism. What this case actually pins is that stop() leaves no config bank queued, which is real: stop()'s erase_if (MetisClient.cpp:364) removes only the IO-board banks, so on the old code the config snapshot genuinely did survive it.

Might be worth saying that directly — e.g. "stop() leaves no config bank queued behind it" — so the case name matches what a future reader can verify from the code in front of them.

[aethersdr-agent]

Antares Detector — candidate vulnerable file(s)

• src/core/backends/hl2/MetisClient.cpp

setBandFilter in MetisClient.cpp accepts an int ocFilterByte from network packets and performs memory operations on that value (e.g., bit manipulation, memcpy-like copies of internal arrays), with no apparent bounds check or validation of the untrusted input before use.

_{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: $7.4523 · model: claude-opus-5}

[on8st]

Thanks — checked it, and I don't think this one holds. Taking the two halves separately, since neither depends on anything this PR changes.

"accepts an int ocFilterByte from network packets" — it does not. The only production caller is Hl2Backend, which passes ocFilterByteForHz(sliceFreqHz) — a pure function of the tuned frequency that returns one of a fixed set of kOc* constants. It is outbound control data derived from the operator's VFO, and it is never parsed out of a received datagram. Inbound Metis parsing lives in parseDiscoveryReply and the EP6 path, neither of which reaches this function.

"no apparent bounds check or validation" — the first statement in MetisClient::setBandFilter is

const std::uint8_t oc = static_cast<std::uint8_t>(ocFilterByte & 0x7F);

so the value is masked to 0–127 before anything uses it, and everything downstream reads oc rather than the parameter. That line is on main unchanged and is untouched by this PR.

"memory operations … bit manipulation, memcpy-like copies of internal arrays" — there are none on this value in this function. After the mask it is stored in a struct field and passed to ccConfig(). The only array copy anywhere near it was the m_oneShot.push_back(m_ccConfig) that this PR deletes — and that copied m_ccConfig, a built control block, not the parameter.

So: not attacker-controlled, bounds-checked on entry, and the one copy in the vicinity is being removed here rather than added.

No criticism of the tool intended — it says advisory, and I'd rather answer it on the record than leave it unaddressed on the PR. If I've misread which call path it has in mind, point me at it and I'll look again.

🤖 Generated with Claude Code

https://claude.ai/code/session_014TtnKQu6QGrSeBirGeAsAs