fix(hl2): preserve per-band gain across startup and overrides

[on8st]

HL2 startup could replay a band-agnostic display gain over the backend's restored per-band gain. For example, 20 m remembered at -12 dB was replaced by the legacy global +20 dB value. Startup now keeps the band's gain, and operator changes continue to save through the per-radio OperatingState document.

This also retains the original fix: an explicit lnaGainDb connection override controls the live gain without overwriting an existing start-band entry during a snapshot or band change.

Fixes #5400.

Scope

The operator directed that HL2 retain per-band gain and that this PR include the startup fix. The legacy-replay exception applies only to family == hl2. No Flex or Icom backend/model code or shared capability declarations changed. HL2 keeps its RfGain persistence domain, which is necessary for loading and saving its band map.

Files	Change
HL2 backend and band-memory policy	Preserve the stored entry during a temporary connect override; reset the pin on restore
MainWindow_Session.cpp / RfGainRestore.h	Skip the legacy global-gain replay only for HL2; preserve other families' existing behavior
HL2 gain tests / tests.cmake	Socket-free production startup, capture, band-hop, and store/load regressions
docs/HERMES.md	Document HL2 gain restoration and ownership

Validation

• macOS app and three selected test targets built with -j22, Qt 6.11.0, RelWithDebInfo, ENABLE_ASR=OFF.
• Headless CTest: 3/3 passed — hl2_gain_restore_test, hl2_band_memory_test, radio_state_memory_test.
• Mutation checks: removing the HL2 startup exclusion fails the startup/persistence regression; reverting the actual currentOperatingState() snapshot writer fails both override-preservation assertions. Restored fixes rebuilt and passed.
• Engine-boundary, test-registration, frozen CI-test-gate checks, and git diff --check passed.
• Automation bridge: isolated headless app connected to DEMO-0001, reported one slice/one pan with RF gain 0 and no transmit, then disconnected. This checks app startup, not HL2 firmware.
• No live-radio validation. The exact HL2 persistence assertions use the production backend and OperatingState store without starting transport. boardMaxRx bypasses UDP discovery, the test never pumps the event loop, and it cancels pending DSP setup before Metis can start. The original snapshot regression was moved out of hl2_state_restore_test, whose broader scenarios can open sockets; that target was not run.

Existing band classification and transmit-owning-receiver selection are unchanged. The unused restoreBandState() helper has no callers and is not a second active startup path. Explicit connection-parameter range handling and the no-op behavior when setting the already-live gain are unchanged.

Generated with OpenAI Codex (GPT-6 Astra)

[Ozy311]

Issue fit

There is no linked issue. The first commit identifies a real, narrow persistence defect on the direct RadioConnectRequest automation/embedder path: an explicit lnaGainDb can diverge from an existing start-band entry, and that session-only value should not silently replace the stored calibration. The shipped UI does not currently populate this parameter, as the PR correctly discloses.

Scope

Area	Fit
Connect-time LNA/write-back policy	Directly addresses the stated defect
Socket-free policy regression	Appropriate deterministic layer
Band-change read-back policy in commit 2	Separable record of already-correct behavior, not required for the fix

The second commit is clearly separated, but it adds production indirection and roughly 90 lines for a claim the PR says was already disproved. I would prefer it split or dropped; I am treating that as a scope nit rather than a blocker because the behavior is equivalent and independently asserted.

Blockers

1. The persistence fix only protects rememberCurrentBandState(). currentOperatingState() still unconditionally inserts m_lnaGainDb under m_currentBandKey at line 4460. A same-band tune, mode change, filter change, or other unrelated action schedules a store; that snapshot will therefore persist the session pin over the protected entry before the first band change. For example: restore 20 m = -12, connect with lnaGainDb=20, tune within 20 m, then the debounced capture saves 20 for 20 m. The snapshot path must apply the same preservation rule while m_lnaSessionPin is active, with a regression covering the production snapshot decision.
2. The test's field-evidence claim is false and directly contradicts the corrected PR body: the observed change was 40 m from -6 to -12, neither launch supplied lnaGainDb, and the PR states this mechanism did not cause it. Remove the causal claim rather than leaving inference presented as observation.

Nits

• The new policy/header and test use several unbraced single-statement if bodies; project style requires braces on all control flow.
• Consider splitting or dropping the second commit's already-correct band-change extraction so this persistence fix remains single-purpose.

Empirical evidence

• Reviewed exact head db988aa1af45b73665eb823f3737c96339479cf2; both commits are GitHub-verified.
• Configured the exact head in a disposable macOS worktree, built the touched Hl2Backend.cpp object and hl2_band_memory_test, and ran the test: all 16 assertions passed.
• git diff --check, tools/check_engine_boundary.py --strict, and tools/check_test_registration.py --strict passed (engine check emitted only existing tracked warnings).
• Merge-tree against the current local origin/main snapshot was clean.
• Socket preflight was clear: the PR adds only a pure, socket-free test.
• No app-driving test was useful: the defect is explicitly unreachable through the shipped UI, and the demo backend cannot exercise an HL2-only connect parameter.
• GitHub reports no checks on this head.

Recommendation

Request changes. Preserve the stored entry in both the band-change write-back and the operating-state snapshot, remove the unsupported field claim, and add regression coverage for the snapshot path.

[on8st]

Both blockers addressed; the branch is force-pushed as 1b9b6477 + b1f0576d (commit 2, the band-change assertions, dropped from this PR per your preference and kept on a separate record branch).

Blocker 1 — the snapshot path. Your example is exact: currentOperatingState() inserted m_lnaGainDb unconditionally at :4460, and the debounced store any unrelated action schedules reached the band map long before the first band change. Both call sites — the write-back and the operating-state snapshot — now ask the same policy function, so one rule is evaluated twice rather than two copies that can drift. Three regressions cover the snapshot decision, including your restore-20 m / connect-with-lnaGainDb / tune-within-band scenario, plus the two cases that must not change (no pin → the capture records the live value; an uncalibrated band still records). Mutation-checked: with the preservation removed, both the write-back and the new snapshot assertions fail; restored, 19/19 pass.

Blocker 2 — the field-evidence claim. Removed from the test, with the reason recorded in place. You were right that it contradicted the body: the correction had been made in the commit message and PR body and never propagated to the test file. This PR does not claim to explain the observed 40 m loss; that is #5400.

Nits: braces on every single-statement if in the header and test. Checkers: check_engine_boundary.py --strict 0 blocking (86 pre-existing tracked warnings), check_test_registration.py --strict OK, git diff --check clean, hl2_band_memory_test 19/19.

[on8st]

Environment correction, edited into the description above: the host is macOS 26 (Darwin 25.5.0), not macOS 15 — a wrong label on the same kernel. Nothing else changed.

[Ozy311]

Follow-up scope

Reviewed the full five-file PR and the incremental fix at exact head b1f0576d93b92f249b33e9a4f6b2d22861e13d24. The production correction requested in the first review is now present in both writers: rememberCurrentBandState() and currentOperatingState() use the same preservation policy. The unsupported causal attribution was corrected in the test, and the separable band-change record was removed from this branch.

Blocker

The requested regression for the production snapshot decision is still missing. The three new tests labelled snapshot call bandMemoryWriteback() directly, exactly as the original write-back test already did; they never execute Hl2Backend::currentOperatingState(). I mutation-checked the distinction: restoring the old broken snapshot line (lnaByBand.insert(m_currentBandKey, m_lnaGainDb)) compiled cleanly and hl2_band_memory_test still passed. That is the same bug this follow-up is meant to prevent. See the inline comment.

The policy itself is covered: mutating its preservation branch to return liveDb made both the write-back and labelled snapshot helper assertions fail. What is not covered is whether the snapshot writer actually invokes the policy with the backend's real band/map/pin state. Add a socket-free behavioral regression at the production capture seam (or extract a seam from that actual state construction) so reverting the currentOperatingState() line fails.

Nits / claims

• Two newly added production if statements remain unbraced; see the inline comment.
• The PR description is stale after the band-change commit was split out: it still says the second commit adds bandChangeLna() and five band-change assertions. It also reports eleven/19 checks in different places, while the current executable emits 15 checks. Please update the description so its scope and evidence match this head.

Evidence

• Exact-head macOS configure/build passed for hl2_band_memory_test, hl2_state_restore_test, and the touched backend object.
• Ran only the new socket-free hl2_band_memory_test: pass (15 checks).
• Positive mutation: changing bandMemoryWriteback() to return the live value made the intended preservation assertions fail; restoring it returned the test to green.
• Negative mutation: reverting only currentOperatingState() to the old unconditional live-value insert compiled successfully and the test remained green; restoring it rebuilt cleanly and the worktree is clean.
• git diff --check, tools/check_engine_boundary.py --strict, and tools/check_test_registration.py --strict passed (86 tracked warnings, zero blocking).
• Merge-tree against current origin/main (dafe441b194bb570fa44836c415a3ebae03280f8) is clean; current-main changes in the touched production files are unrelated capability declarations.
• Both commits are GitHub-verified; GitHub reports no checks on this head.
• No app or radio hardware was started or driven. No socket test was run; the PR's new test is socket-free.
• Trusted review-skill SHA-256: dc9bd117248079081ef9f5708e2ea3e9789b5c9f71c5efcf560054186fe309a4; canonical and mirror constitution SHA-256: d585c0383b782a8ade22a341134ab16b63f897446b1dc2fe1fc990590312d438.

Recommendation

Request changes. The code correction looks sound, but Principle XII requires the fixed snapshot contract to be demonstrated. Make a mutation at the production snapshot writer fail, then clean up the two style violations and stale PR claims.

[on8st]

Blocker fixed, at 5c1b35b4. You were right that the three snapshot cases proved nothing about production: they called bandMemoryWriteback() directly, so restoring the old lnaByBand.insert(m_currentBandKey, m_lnaGainDb) left them green — the same omission as the first review, one layer along.

The regression now lives in hl2_state_restore_test and drives the real Hl2Backend::currentOperatingState(). Reverting the production line fails it. It also carries a presence control, because every other assertion in that block is a value not changing, and a test made only of things that must not move passes just as well when nothing is wired up at all.

Braces are on both new ifs.

Two things a reviewer should not have to discover. This branch is based on 2add3555 and shows three red CW-passband checks in hl2_state_restore_test that are not from this change — they reproduce with it stashed, and main fixed them in d5d1953a by reading restoredStateForTest() instead of a pre-connect snapshot. A rebase clears them; say if you want that here rather than at merge. And CI has still not run on this head.

[on8st]

Head updated: subjects trimmed under the 72-character rule in docs/DEVELOPER-GUIDE.md, with the PR reference moved from the subject into a Refs: line in the body. Each replay was verified tree-identical to the commit it replaced — no content change from the push above this one.

[on8st]

A correction to our own commit metadata, disclosed rather than force-pushed a fourth time onto a branch you are reading.

The commit on this branch cites Principle XI. It should cite Principle VIII. CONSTITUTION.md says XI is demonstrated by CI re-running on the squash-merge commit on main, by the maintainer reproducing the bug pre-merge and confirming it gone post-merge, and that agent self-grading does not substitute — so citing XI on the strength of our own mutation check is the thing XI exists to refuse. What the change actually rests on is evidence replacing assertion, which is VIII.

XI's real gate is still outstanding here in any case: CI has never run on this head (action_required on the fork). Commits from here cite VIII unless that verification has actually happened.

[on8st]

@Ozy311 — your mutation is what made the last round's fix credible, and it found that the coverage was not. Head is 038a4b00. Mapping each item to the commit that acts on it; whether they answer you is yours to judge.

Blocker — the production snapshot decision — 038a4b00. You reverted currentOperatingState() to the old unconditional lnaByBand.insert(m_currentBandKey, m_lnaGainDb) and hl2_band_memory_test stayed green. You were right about why: the three checks labelled snapshot called bandMemoryWriteback() directly, exactly as the write-back checks already did. They proved the policy decides correctly. Nothing proved the snapshot writer asks it.

The regression has moved to tests/hl2_state_restore_test.cpp, which already builds an Hl2Backend socket-free against TEST-NET-1 and reads currentOperatingState() throughout. The new block runs your own scenario through the real capture: restore 20 m at -12, connect with lnaGainDb=20, then tune within 20 m. A same-band tune is not a band change, so rememberCurrentBandState() never runs and the debounced capture is the only writer reaching the map — which is why this path persisted the pin long before the first band change could. Reverting the currentOperatingState() line now fails on the first check.

It carries a presence control, because everything the fix asserts is an absence — a value that does not change. That would pass equally if the map were unwritable through the capture, or if the pin had never happened and the live value had been -12 all along. So the block ends the pin the way an operator does (band hop, return, setPanRfGain) and shows the same assertion move to 5. Preservation, not a dead writer.

Worth recording because it explains why the defect hid: a session pin is the only state in which the live gain and the stored entry legitimately differ. Every other path keeps them equal — which is exactly why a mutation test that did not construct a pin could not see the snapshot line's decision at all.

Nit — unbraced production ifs — 038a4b00. Both braced.

Nit — stale description. Rewritten. You were right on every count: it described db988aa1 as adding bandChangeLna() and five assertions, and that commit was split out of this branch at your first review's request; it reported eleven checks in one place and nineteen in another while the executable emits fifteen. The counts now come from running the binary. The commit table is the three that exist, and the section that described the split-out commit is replaced by what the two rounds actually produced.

One thing a reviewer will hit running this branch, so it is in the description too. hl2_state_restore_test reports three CW-passband failures here:

[FAIL] a pre-#4914 CW passband is dropped for one that contains the carrier
[FAIL] a one-sided passband under USB is untouched by the CW guard
[FAIL] a new-domain CW passband survives the guard unchanged

They are not from this PR — they reproduce with the change stashed — and they are already fixed on main by d5d1953a, which reads restoredStateForTest() instead of a pre-connect snapshot (#5031: those three were born red and shipped that way). This branch is based on 2add3555 and predates that commit. A merge or rebase onto current main clears them.

[aethersdr-agent]

Issue fit

There is no fixes/closes #NNNN in the body — this is a self-reported defect, reviewed against its own stated intent. Per GOVERNANCE.md that is fine for a bug fix with a clear root cause, and the root cause here is legible from the code rather than asserted: before this diff, rememberCurrentBandState() wrote m_lnaGainDb unconditionally into m_lnaDbByBand, and currentOperatingState() did the same into the persisted map, so a connect that set the live gain from the lnaGainDb param would consume the start band's stored entry on the next write. The fix addresses that, and I confirmed both writers now route through bandMemoryWriteback() — those are the only two sites that write the persisted per-band LNA (m_lnaDbByBand is written at Hl2Backend.cpp:2907 (operator), :4304 (restore), :4543 (remember), and the snapshot at :4473; the first two are correctly outside the rule).

Ozy311's four items all landed: the snapshot path is protected (:4473), the production writer is now exercised through the real backend rather than the pure policy, the fabricated field-observation provenance is removed and explicitly retracted in the test header, and the braces are on every new control-flow body I can find in all three files.

I also verified Ozy311's premise about the snapshot being the earlier writer, since the whole second commit rests on it: RadioModel::persistOperatingState() (src/models/RadioModel.cpp:439-456) calls currentOperatingState() off m_operatingStateSaveTimer, armed by scheduleOperatingStateSave() from operatingStateChanged — and notifyOperatingStateChanged() fires on setPanRfGain and on tune paths that never reach a band change. The premise holds, and force=true (the disconnect flush) goes through the same call, so disconnect is covered too.

Verdict: yes, it solves the defect it describes, with the caveats in Nit 1 below about how far the guarantee reaches in the assembled app.

Scope

File	What it changes	Claimed?	Verdict
Hl2BandMemoryPolicy.h	New header: connectLna(), bandMemoryWriteback(), clampDb()	Yes	In scope. Not dead — Hl2Backend.cpp:1694, :4474, :4545 all evaluate it, and the test includes it
Hl2Backend.cpp	Connect seeding, pin clear in setPanRfGain, both writeback sites, pin clear on band change	Yes	In scope
Hl2Backend.h	m_lnaSessionPin member + comment	Yes	In scope
tests/hl2_band_memory_test.cpp	New policy-level test	Yes	In scope
tests/hl2_state_restore_test.cpp	New backend-level block (lines 391–475)	Yes (review commit)	In scope
tests/tests.cmake	Registers hl2_band_memory_test	Yes	In scope

Everything in the diff is explained by the stated fix. No build/CI churn, no unrelated files, no CHANGELOG.md entry, no new public/protocol surface (m_lnaSessionPin is private; the persisted rfGain/lnaDbByBand JSON shape and extensionSchemaVersion = 1 are unchanged, so no migration is needed). No - line removes a guard: the only deletions are the four lines of the old unconditional connect seeding, replaced by a superset. Nothing here is a preference change — no default moves, no UI behaviour changes, and the connect-param precedence is explicitly preserved rather than reversed.

Socket-test surface (recorded, not a finding): the modified hl2_state_restore_test block drives Hl2Backend::connectRadio(), which constructs a MetisClient and starts the hl2-io QThread (Hl2Backend.cpp:351, :400-404). The target is 192.0.2.1 (TEST-NET-1, unroutable) and the block follows the pattern the file already uses at lines 215, 233, 251, 347, 376. No new socket-owning target, no fake HL2 peer, no listener bound. hl2_state_restore_test is already registered at tests/tests.cmake:3254. Worth knowing: ci.yml:311 filters HL2 ctest to ^hl2_am_dcblock_test$, so neither the new nor the modified test gates a merge — green CI on this PR says nothing about either.

Blockers

None.

Nits (non-blocking)

1. The pin's guarantee stops at the backend seam — a client-side global RF-gain replay can still consume the entry. src/gui/MainWindow_Session.cpp:1691-1708 replays a saved band-agnostic RF gain through m_radioModel.setPanRfGain() when the backend declares ClientSettingsDomain::RfGain — which the comment there says is currently HL2 only. MainWindow.cpp:10501 does the same from a profile snapshot. Both land in Hl2Backend::setPanRfGain(), which this PR makes clear the pin and which then writes the replayed value into m_lnaDbByBand[m_currentBandKey] — the entry the fix exists to protect. The write at :2907 predates this PR, so this is not a regression the diff introduces, but it does bound the promise, and the new backend-level test cannot see it because it drives Hl2Backend directly. Reasoned from code; I could not run the app to establish the ordering between that replay and connect. Worth a sentence in the body (the SCOPE note in commit 1b9b647 already gestures at "a separate global RF-gain replay" — this is where it lives).

2. applyRestoredState()'s full reset omits m_lnaSessionPin (Hl2Backend.cpp:4182-4198). That block's own contract is "Live members reset to the same virgin defaults a fresh backend construction would have. (PR #4619 review, Ozy311 finding 1)" and the new member is a live member. It is currently masked — connectRadio() unconditionally assigns m_lnaSessionPin = seed.sessionPin at :1706, and m_currentBandKey.clear() at :4198 makes both writeback sites no-op in the window between — so I could not construct a scenario where it misbehaves today. It is the documented reset seam and the omission is the same class Ozy311 already caught once.

3. setPanRfGain's early return skips the pin clear. if (clamped == m_lnaGainDb) return; at :2897-2898 sits above m_lnaSessionPin = false; at :2905. So an operator who sets the gain to exactly the pinned value never ends the pin, and the header's "Cleared the moment the operator sets a gain themselves" is not quite true for that one input. Concretely: 20 m stored at −12, connect pinned at 20, operator moves the slider to 20 — no-op, pin survives, and the band keeps −12 instead of adopting the operator's choice. The failure direction is conservative (an old calibration is kept rather than lost), which is why this is a nit and not a finding.

4. connectLna() returns the param branch unclamped (Hl2BandMemoryPolicy.h:58-62) while the restore branch clamps (:65). This exactly matches the pre-existing Hl2Backend.cpp:1673-1674, so it is not a behaviour change — but the header is now presented as the decision, it takes minDb/maxDb and ignores them on that path, and hl2_band_memory_test has a clamp case for stored values only. Either clamp it or say in the header why the param is deliberately exempt.

What I tried to break

• Does the new test actually fail against the unfixed writer? Yes, as far as I can reason: with the snapshot line reverted to lnaByBand.insert(m_currentBandKey, m_lnaGainDb), stored20m() returns 20 rather than −12 at line 452. The block is also honestly constructed against the "passes for the wrong reason" failure — the setPanRfGain(panId, 5) presence control at the end rules out an unwritable map or a pin that never happened, and .toInt(999) rules out a missing key reading as 0. This is the mutation Ozy311 ran, and the block is aimed at it correctly.
• Does the pin leak into a later band and make every subsequent band non-recordable? No. applyPerBandStateFor clears it at :4566, and — importantly — after rememberCurrentBandState() at :4562, so the preserved write happens first. Reversing those two lines would silently break the fix; the ordering is right.
• Can the live value diverge from what the pans show, or the dB reference drift? No. m_dbRef.setLnaGainDb(m_lnaGainDb) at :1838 runs unconditionally after all connect seeding, and the diff does not touch applyLnaGainDb() — the pin refuses persistence only, exactly as the header claims.
• Radio swap / second radio. applyRestoredState({}) clears the maps and m_currentBandKey, and connectRadio reassigns the pin, so radio A's pin cannot govern radio B's writeback (see Nit 2 for the residual).
• Sibling writers of the persisted map. I grepped every m_lnaDbByBand site; the two that needed the rule got it, and the operator/restore sites correctly did not. No third surface left carrying the defect.
• Both writeback call sites pass identical arguments to bandMemoryWriteback() — no transcription drift between :4474-4477 and :4545-4548, which is the failure mode a two-call-site rule usually dies of.
• Could not check: anything at runtime. I have no build and no way to drive the GUI, so every statement above is reasoned from the code in the head checkout, not reproduced. Nit 1's ordering in particular would be settled by an app-level run, not by reading.

Recommendation

Approve with nits. The mechanism is real, the seam is in the right place, the policy header genuinely is evaluated by production rather than copied, and the second commit fixes a real hole that the first one's test could not have found — the backend-level test now covers the production decision that the policy test cannot. The four nits are all either pre-existing behaviour surfaced by the new structure or robustness around a member that is currently always overwritten before use; none of them change the merge decision. Next step for the author: fold m_lnaSessionPin = false into the applyRestoredState reset block (Nit 2, one line), and add a sentence to the body about the global RF-gain replay boundary (Nit 1) so the fix's reach is stated rather than implied.

Thanks for the honest retraction of the field-observation provenance in the test header — "NO FIELD OBSERVATION IS CLAIMED FOR THIS MECHANISM" is a better artifact than the inference it replaced, and it made this review faster.

---

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

[aethersdr-agent]

Cisco CodeGuard — static analysis of this PR (18 finding(s))

• [MEDIUM] CG-PATH-001 — Potential path traversal in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5402/.github/workflows/appimage.yml:200
• [MEDIUM] CG-PATH-001 — Potential path traversal in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5402/.github/workflows/appimage.yml:399
• [MEDIUM] CG-PATH-001 — Potential path traversal in .github/workflows/macos-dmg.yml /tmp/aetherclaude/pr-5402/.github/workflows/macos-dmg.yml:107
• [MEDIUM] CG-PATH-001 — Potential path traversal in plugins/ulanzi-aethersdr/com.g0jkn.aethersdr.ulanziPlugin/plugin/app.js /tmp/aetherclaude/pr-5402/plugins/ulanzi-aethersdr/com.g0jkn.aethersdr.ulanziPlugin/plugin/app.js:12
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/core/QsoRecorder.cpp /tmp/aetherclaude/pr-5402/src/core/QsoRecorder.cpp:6
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5402/src/gui/MainWindow.cpp:8868
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5402/src/gui/MainWindow.cpp:8980
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5402/src/gui/MainWindow.cpp:9099
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5402/src/gui/MainWindow.cpp:9100
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5402/src/gui/MainWindow.cpp:9101
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MemoryDialog.cpp /tmp/aetherclaude/pr-5402/src/gui/MemoryDialog.cpp:1191
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MemoryDialog.cpp /tmp/aetherclaude/pr-5402/src/gui/MemoryDialog.cpp:1417
• [MEDIUM] CG-PATH-001 — Potential path traversal in tests/hl2_live_band_filter_probe.cpp /tmp/aetherclaude/pr-5402/tests/hl2_live_band_filter_probe.cpp:98
• [MEDIUM] CG-PATH-001 — Potential path traversal in tests/ulanzi_mapping_migration_test.cpp /tmp/aetherclaude/pr-5402/tests/ulanzi_mapping_migration_test.cpp:206
• [MEDIUM] CG-PATH-001 — Potential path traversal in tests/ulanzi_mapping_migration_test.cpp /tmp/aetherclaude/pr-5402/tests/ulanzi_mapping_migration_test.cpp:207
• [MEDIUM] CG-PATH-001 — Potential path traversal in tools/check_engine_boundary.py /tmp/aetherclaude/pr-5402/tools/check_engine_boundary.py:113
• [MEDIUM] CG-PATH-001 — Potential path traversal in tools/gen_touchpoint_manifest.py /tmp/aetherclaude/pr-5402/tools/gen_touchpoint_manifest.py:68
• [MEDIUM] CG-PATH-001 — Potential path traversal in tools/gen_touchpoint_manifest.py /tmp/aetherclaude/pr-5402/tools/gen_touchpoint_manifest.py:132

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

[jensenpat]

Reviewed 7be1edd54fc1b52b9462f63cbe04d0a0f2fc744c after the operator directed takeover and expansion to include #5400.

The combined fix preserves HL2 per-band RF gain both during normal app startup and when a direct connection request supplies a temporary gain override. No Flex/Icom backend or model code changed. The HL2-only legacy replay guard preserves every other family's previous decision. The RfGain capability stays enabled so OperatingState continues to load/store the band map.

Scope	Verification
HL2 connect, band writeback, snapshot, pin reset	Full production diff and callback/order tracing; real-backend socket-free tests
HL2-only GUI startup replay guard	Production restore helper called by MainWindow and exercised with the backend; non-HL2 compatibility controls
Persistence and regression tests	3/3 focused tests, registered in tests.cmake; atomic store/load into a recreated backend
Documentation	Updated existing HL2 notes; no changelog entry

The previous blockers are addressed: both persistence writers use the preservation policy; removing the production snapshot call fails the new test, rather than merely testing a copied policy; unsupported field attribution is corrected; the new conditionals are braced; the description now matches this scope. Removing the startup exclusion independently fails the original user-facing scenario. All mutations were restored, rebuilt, and the selected tests passed again.

Other thread notes: the pin resets in applyRestoredState, band-change clearing follows preservation, same-value setter no-ops and explicit connect-parameter range behavior remain unchanged and are disclosed. The suggested second replay in restoreBandState has no callers in the current tree.

Local verification: macOS/Qt 6.11.0, RelWithDebInfo, ENABLE_ASR=OFF; app plus hl2_gain_restore_test, hl2_band_memory_test, radio_state_memory_test built with -j22. Headless CTest 3/3 passed on the committed head. Engine boundary, registration, frozen CI gate, and whitespace checks passed. Current-main merge-tree was clean.

Bridge verification: isolated headless demo session, DEMO-0001, one pan/one slice, RF gain 0, no transmit, then disconnect. This is app-startup evidence, not HL2 hardware evidence. No physical radio was connected. The new HL2 regression skips discovery via boardMaxRx, never pumps the event loop, and cancels DSP setup before transport can start. The broader existing hl2_state_restore_test was not run because its other cases open UDP sockets.

Cost through local completion: approximately 15.7 minutes since scratch checkout creation; 2 configure calls, 6 build calls (one iconutil sandbox failure, one successful full build, two mutation builds, restoration build, commit-ID rebuild), 3/3 final tests, two expected mutation failures, 2 app launch attempts (one sandbox child exited before startup), 0 subagents. Compiler cache statistics and token counts unavailable. Prior reviews informed the scenarios; their build results were not reused.

CI: all five checks passed on the current PR head. The CI checkout is synthetic merge 664b5ca, whose parents are current main bc80177 and reviewed head 7be1edd. Linux/macOS compile the default graph; Windows builds the app and its platform gate targets. The three focused HL2/persistence tests above were run locally; they are not added to the frozen per-PR CI selection.

Technical verification is complete. Merge is withheld while the previous change-request reviews and discussion threads remain open. The prior review blocks have not been dismissed.

[Ozy311]

@jensenpat — coordination follow-up on my earlier change requests, at 7be1edd54fc1b52b9462f63cbe04d0a0f2fc744c. I am leaving your active assignment in place.

The correction and coverage I requested are present in the current source: both persistence writers preserve an existing start-band entry during a temporary pin, and hl2_gain_restore_test reaches the actual currentOperatingState() writer with the live +20/stored -12 scenario, a same-band tune, and a later operator write to +5. The unsupported causal attribution is corrected, and the current description explains the expanded startup fix. This answers the substance of my earlier findings.

My own fresh execution was limited to the standalone policy test: 15 assertions pass, and replacing the stored-value preservation return with the live value fails two assertions; restored code passes. I source-inspected the production regression and read your current-head review for its independently reported build, real-backend regression, and mutation evidence. I did not rerun that expanded suite or the GUI startup path. Visible current-head CI checks are successful.

This is a coordination/status comment closing the loop on my prior feedback, not a competing review, a claim takeover, a dismissal of the old review, or merge approval.

[Ozy311]

Re-reviewed a71edf6632cc0a288783284caee6b17983567fbb for the operator-requested release integration, after #5381 landed.

The fix preserves HL2 per-band RF gain across ordinary startup and explicit temporary connection overrides. Both production persistence writers honor the preservation policy, and the HL2-only startup replay guard leaves other radio families on their existing path. Our earlier snapshot, production-test coverage, provenance and brace findings are addressed. Jensenpat's completed technical review and assignment are preserved; the operator specifically directed this integration and approval/merge.

Scope	Assessment
HL2 connect/persistence and GUI restore helper	Same reviewed RF-gain fix and production-writer coverage
HERMES documentation	Matches the expanded startup/override fix
Includes and tests.cmake integration	Preserved DSP-setup, overload and band-memory policy includes, all test registrations, and both settings consumers; computed merge delta against current main contains only this PR's intended nine files

Blocking findings: none. Existing nonblocking same-value setter and explicit-parameter-range limitations remain disclosed; this integration does not alter them.

The final merge from landed main changes ancestry only: its complete tree59977d679a19af2e312e9a9e4cdbc97e871113c3 is identical to the locally tested54c737e6 tree.

Independent integration verification: built the combined production engine and focused targets on Apple Silicon macOS/Qt6.11.1, RelWithDebInfo (optional ASR/RADE/DFNR disabled). All four socket-free tests passed: hl2_gain_restore_test, hl2_band_memory_test, radio_state_memory_test and hl2_overload_policy_test. The gain regression uses the actual backend, supplies boardMaxRx to bypass discovery, does not pump events, and cancels queued DSP setup before Metis transport starts. The broader socket-owning hl2_state_restore_test was not run. Strict registration, engine-boundary and frozen-CI-gate checks and whitespace checks passed. No new mutation result is claimed; prior production snapshot/startup mutations are recorded in the earlier completed reviews.

Current-head platform CI and commit signatures were checked before approval. No fresh full application GUI session or live HL2/RF/TX test was performed for the mechanical integration. Every changed path, including the already-landed stack dependency, is reviewer-owned, and live Ozy311 membership is active.

Verdict: approve. This AI-assisted review through Ozy311 supersedes our previous change requests.

[on8st]

@Ozy311 — your negative mutation is the reason this fix is real rather than
merely green, so recording the two things it leaves open, since both were
disclosed rather than fixed and a merged PR is where that kind of note gets
lost.

The same-value setter. Hl2Backend::setPanRfGain returns early on
if (clamped == m_lnaGainDb), and both m_lnaSessionPin = false; and the
band-map write sit below it. So an operator who sets the gain to exactly the
pinned value neither ends the pin nor records their choice: 20 m stored at
−12, connect with lnaGainDb=20, and the operator — still on 20 m —
deliberately sets 20. The write is a no-op, the pin survives, and
currentOperatingState() persists −12. The direction is conservative, an old
calibration survives rather than dying, which is why it was a nit and not a
blocker and why it should not have held the merge.

It is uncovered as well as unfixed. hl2_gain_restore_test ends the pin with
session.backend.setPanRfGain(session.panId, 5);, a genuine change; there is
no case where the operator writes the pinned value itself. The window is also
narrow — after the first band change applyPerBandStateFor clears the pin, and
without a pin the writeback returns the live value, so it is the start band
only.

I will send a follow-up PR
that makes an operator write end the pin and record the band whether or not the
value moved, with that case added to the same test and failing before it
passes. Nothing else in the merged fix changes.

What nobody has run. You wrote that your fresh execution was limited to the
standalone policy test and that you did not rerun the expanded suite or the GUI
startup path; @jensenpat wrote that the bridge session was app-startup
evidence, not HL2 hardware evidence, with no physical radio connected. So the
half of this that closes #5400 — the startup replay — has landed on socket-free
tests and a demo backend. That is not a complaint about either review; it is
the seam neither of you could reach.

I filed #5400 from a real HL2
and still have it, so I will confirm the merged behaviour on hardware across a
genuine restart and report the result here whether or not it agrees with the
tests.

One thing that should not have to be re-found. The second global-gain
replay, MainWindow::restoreBandState, does not go through
restoreLegacyRfGain and would reintroduce #5400 exactly — but it has no
callers anywhere in src or tests, which is what @jensenpat reported and
what I see too. It is dead, not broken. Worth knowing that wiring it up would
be a #5400 regression rather than a restore.