Repository navigation
test(hl2): pin the ADC pairing's age gate in the snapshot. Principle VIII. - #6441
Conversation
…VIII. healthSnapshot() gains a private overload taking the instant; the ADC rows take their age from it and read no clock, so the age row and the pairing gate come from one value. Hl2RxDsp::adcPeakObservedAgoMs() had no other caller and is removed. hl2_adc_age_gate_test reads the snapshot one nanosecond either side of the kSliceStaleMs boundary (aethersdr#6186). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
1. Issue fit
Yes. This implements the seam the triage on #6186 prescribed, point for point, and it closes the gap that issue names: nothing under tests/ read the adcPairing%1 or adcSliceObservedAgoMs%1 rows, so the freshness argument at the call site was product-only code.
Mapping the triage's requirements to the diff:
| Triage requirement | Where |
|---|---|
Clock on the snapshot, not an Hl2RxDsp fake clock |
Hl2Backend.h:304, Hl2Backend.cpp:6023-6028 |
Age from slicePeakAgeMs(adcPeakObservedAtNs(), nowNs), one value feeding both row and gate |
Hl2Backend.cpp:6108-6110, consumed at :6123 |
adcPeakObservedAgoMs() deleted |
Hl2RxDsp.h (−9); grep -rn adcPeakObservedAgoMs . over the whole head returns nothing, docs included |
friend struct Hl2AdcAgeTestAccess beside the existing *TestAccess friends |
Hl2Backend.h:300 |
Hold SliceSamplingGate open while varying only the age |
tests/hl2_adc_age_gate_test.cpp:139 asserts samplingGateOpen() on every read |
Reads at stamp + 150 ms + 999 999 ns and stamp + 151 ms |
:198-200, with firstStale - lastCurrent == 1 asserted |
| Land after #6164 so the helpers exist once | head is one commit on 47aa4098 |
The fifth row is the one worth calling out. The triage warned that a test asserting only "stale → Unknown" survives the true mutant if the sampling gate happens to be shut, because both gates return Unknown. This test asserts the gate open, the stamp unmoved, the flag row present and the peak row present on every read, so the two instants really do differ in the age alone. That is the difference between pinning the gate and pinning a coincidence.
The mutation table in the body is consistent with what I read: the age row and the pairing row are separate put() calls fed from one ago, so the true mutant can only be caught by the stale pairing check — thin, but correct, and the false mutant is caught from the other side.
2. Scope
Everything in the diff is explained by the issue and its triage.
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/backends/hl2/Hl2Backend.cpp |
public healthSnapshot() forwards to a new healthSnapshot(nowNs); age via slicePeakAgeMs |
yes | In scope |
src/core/backends/hl2/Hl2Backend.h |
private overload + one friend + comments | yes | In scope |
src/core/backends/hl2/Hl2RxDsp.h |
deletes adcPeakObservedAgoMs(), rewords the stamp comment |
yes | In scope — see the governance note under Nits |
tests/hl2_adc_age_gate_test.cpp |
the new test (+234) | yes | In scope |
tests/hl2_adc_pairing_test.cpp |
one comment, Hl2RxDsp → Hl2Backend forms the age |
yes | In scope, and now true |
tests/tests.cmake |
registers the target, joins AETHER_SETTINGS_CONSUMERS |
yes | In scope |
No new public surface: the overload is private, absent from IRadioBackend, and its only production caller is the public no-arg version. No row, key or label is added or renamed, so AutomationServer::doHealth()'s output is unchanged and no consumer can pick an instant. No settings key, no capability field, no protocol verb. No CHANGELOG.md entry — correct.
Socket tests: none added or modified. MetisClient's constructor creates one QTimer and no socket, and m_telemetryService is never constructed by Hl2Backend's constructor, so the "socket-free" claim holds for real and not just by intent.
Security audit (required by the harness; run by the security-audit subagent against the head checkout): all 64 CodeGuard findings refuted, no genuine finding in the additions. Credited as security-audit (CodeGuard).
- The ~60
CS-VLN-CMDI-BACKTICKhits on the C++ files are the scanner matching Markdown-style backticks inside//comments. A grep for a backtick on any line not starting//,*or/*returns zero matches across all four C++ files, and there is nosystem/popen/QProcess/exec*anywhere in the audited tree. The flagged lines are comment prose that exists verbatim onmain; the ~5-line shift is the new forwarder. .github/workflows/static-checks.ymldoes not intersect this diff at all —git diff --name-only 47aa4098d..9737ff2f5lists six files and that is not one of them. All 14 of its findings are whole-repo scan noise on untouched code, thecodeguard-file-list-can-be-main-tipshape. (Its line 496FILES=$(cat …)word-split weakness is real but pre-existing, and already replaced by the bash-array form onmainin #6439.)CS-VLN-CMDI-ENVVARatHl2Backend.cpp:321,330isqEnvironmentVariableIsSet()— a presence test that reads no value, pre-existing.- Independently of CodeGuard:
slicePeakAgeMs()'s(nowNs - peakAtNs) / 1'000'000cannot wrap in practice (both operands are ns counts from onesteady_clock), and the negative clamp runs before thestatic_cast<qulonglong>at:6113, so no negative-to-unsigned wrap is reachable.
3. Blockers
None.
4. Nits (all non-blocking)
tests/hl2_adc_age_gate_test.cpp:138— the second clock is a 2.5 s budget, not just "a clock the test does not set." The flag row only appears whiletelemetryLinkState() == Streaming, and that ism_rxAdvanceClock.elapsed() < kStreamStallDeclareMs(Hl2TelemetryCadence.h:64, 2500 ms), restarted by thelinkUphandler at:189. The three snapshot reads after it are cheap, and the WDSP feed happens beforelinkUp, so the real margin is large. But a >2.5 s stall betweenlinkUpand the last read — a sanitizer lane under load is the plausible one — fails on"the snapshot carries the overload flag, clear: the link reads Streaming", which reads as a link bug rather than as a slow machine. Your Limits section is right that it fails rather than passing as "stale"; a word in that check's text naming the 2.5 s window would save the next person the hunt.src/core/backends/hl2/Hl2Backend.cpp:6222—healthSnapshot(nowNs)is not clock-free as a whole. One later row still reads the wall clock:Your header comment is accurate as written — it scopes the claim to "the converter rows" — but the triage asked for a function that "reads the clock zero times", and a future reader injectingm_adcWindowClock.isValid() ? QVariant(qint64(m_adcWindowClock.elapsed()))
nowNsto pin some other row will find this one still onQElapsedTimer. Worth one clause in theHl2Backend.h:301-303comment saying the converter rows alone are onnowNs.src/core/backends/hl2/Hl2RxDsp.h:20— theHl2AdcPairing.hinclude is now referenced only from a comment. WithadcPeakObservedAgoMs()gone, no declaration in the header uses a symbol from it (:366is prose).Hl2RxDsp.cpp:3,Hl2Backend.cpp:16andHl2Backend.h:15all include it directly, so dropping it would not break those three — I did not walk every other includer ofHl2RxDsp.h, so check before you pull it. Pure tidiness either way.- Governance, for the maintainer rather than the author. #6186 still carries
maintainer-review, and its triage closed by asking for two rulings that have not been given: which test file this belongs in, and whether deletingadcPeakObservedAgoMs()is in scope or a separate cleanup. The PR answers both in its body (new file; deletion in scope, zero callers left) and the triage did say a new file was fair and that the accessor "can go", so this reads as reasonable rather than as jumping a gate. Flagging it only so the ruling lands on the record.
The retyped "agree — …" sentence is disclosed in your Limits and I checked it against Hl2Backend.cpp:6138-6141 character for character, .arg(headroom) included. I would not ask for a helper: the expected string is built from the measured peak, not a hardcoded dB figure, which is what keeps the test off platform-specific WDSP gain.
5. What I tried to break
Findings here are reasoned from source in the head checkout at /tmp/aetherclaude/pr-6441; I have no build and ran nothing but the Python static checks.
- Deadlock in the test harness.
Qt::BlockingQueuedConnectiononm_metis(:168,:176) is a self-deadlock if the target shares the caller's thread. It does not: the constructor movesm_metistom_ioThreadandstartStreamThread()callsthread->start()(AppActivity.h:25), so the event loop is live. The backend itself stays on the test thread, which is whysendPostedEvents(&backend, QEvent::MetaCall)is the right follow-up for the queuedlinkUp/telemetryUpdatedhandlers. - Empty receiver vector.
attachChain()returnsnullptrifm_rxis empty, which would fail the test immediately. It is not:m_rx.assign(1, Receiver{})runs in the constructor, andm_rx[0].dspisnullptrthere, so the injection overwrites nothing and leaks nothing. - Lifetime of the injected chain. The destructor
delete r.dspoutright after both threads are joined (:1749-1751), andm_ioDspsis empty because the test never connects, so there is no second owner and no cross-thread alias. The test'sHl2RxDspstays on the test thread —processIqBlockandhealthSnapshotboth run there, andm_adcPeakAtNsis an atomic read either way. - A rebuild stealing the chain at the link edge. This was my best shot at a use-after-free: if
linkUpqueued a receiver rebuild, the injecteddspcould be deleted mid-test.pushInitialState(),replayRestoredSession()andemitAllSliceState()contain no rebuild,beginDspSetup,deleteLaterorbuildReceiverscall — chains are built inconnectRadio(), which this test never calls. Your"the link edge kept receiver 0's chain"assertion at:192guards it anyway. - The two instants.
stamp + 150*1e6 + 999999truncates to 150 → current (<=inslicePeakIsCurrent);stamp + 151*1e6→ 151 → stale. The arithmetic checks out and the one-nanosecond separation is asserted rather than assumed. - The 16-block feed cap.
kMaxBlocksForOneSample = 16at:113looked like a magic number until I found the same constant and value inhl2_adc_sampling_seam_test.cpp:76, documented there as "~15 blocks at this rate and block size" — the same 48 kHz / 1024 configuration this test uses. Justified, and the loop early-exits once stamped. - A silently hot tone.
kToneAmplitude = 0.1frelies on WDSP's processing gain not pushing the slice insidekSliceHotHeadroomDb, which would change the verdict toSliceOnlyand the sentence with it. The test assertssliceHeadroomDb(*peak) > kSliceHotHeadroomDbexplicitly at:184, so that fails loudly instead of quietly changing what is being tested. - Does the test prove anything against unfixed code? Not in the usual sense — it cannot compile on
main(no overload, no friend), so "fails without the fix" is vacuous for a PR whose subject is the seam. The question that matters is whether it kills a broken gate, and it does in both directions: the pairing row is asserted present at exactlykSliceStaleMsand absent one nanosecond later, so flipping the freshness argument either way goes red. - CI, both halves.
build,check-macosandcheck-windowsare green on9737ff2f, so the new target compiles on all three platforms — your Limits section undersells this. Butci.yml'sctestcalls are-R-filtered and name nohl2_adctest, so the first Linux and Windows execution is the post-merge full suite.check_wdsp_test_links.py --strictandcheck_test_registration.py --strictboth pass locally on the head, and the target's linkage (aethercore Qt6::Core Qt6::Network) matches itsHl2RxDsp-instantiating siblings. - The overload itself. A private
healthSnapshot(std::int64_t)alongside a public virtualhealthSnapshot()does not hide the override — both are declared in the same class, so the zero-argument call resolves to the public one and access is checked after resolution. No ambiguity for existing callers.
6. Recommendation
Approve with nits. The production change is a clock parameter, one friend and a dead accessor deletion; the test is the careful one the triage asked for, and it guards the specific trap (SliceSamplingGate held open) that would otherwise have let the mutant live. The three nits are comment-and-wording work, and the only thing I would not merge without is the maintainer ruling #6186 is still labelled for — which is a signature, not a change. Thanks for the Limits section: every limit I set out to find was already in it, which is rarer than it should be.
🤖 aethersdr-agent · cost: $19.5589 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (64 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: $20.8728 · model: claude-opus-5 |
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
Addresses #6186: the socket-free test reaches healthSnapshot's ADC freshness gate while holding the stamp, sampling gate, peak, and overload flag steady. No blockers remain. The RAII setup nit is fixed in 0455180.
Scope audit:
| Files | Change | Verdict |
|---|---|---|
| Hl2Backend.cpp/.h | Private snapshot clock seam and test access | In scope |
| Hl2RxDsp.h | Remove unused age accessor | In scope |
| hl2_adc_age_gate_test.cpp | Boundary coverage and RAII setup | In scope |
| hl2_adc_pairing_test.cpp, tests.cmake | Comment, registration, settings isolation | In scope |
All changes are disclosed and explained by the issue. No public protocol surface, defaults, or UI behavior changed.
Verification: reviewed merge 082ff3d against main 47aa409 and original head 9737ff2. Built focused targets with the prescribed ARM64 toolchain, RADE enabled; both hl2_adc_pairing_test and hl2_adc_age_gate_test pass. Rebuilt and reran both after the RAII fix; both pass, with ARM64 executable verified. Registration and diff checks pass. The four engine-boundary checks have identical passing diagnostics on base and merge.
Adversarial checks: always-current fails the stale assertion; always-stale fails both fresh assertions; restoring the old wall-clock calculation fails the injected-age and stale-pairing assertions. Restoring production code passes again and matches the reviewed source hash. Examined no-stamp/no-link controls, fixed-stamp fresh/stale/fresh reads, unused accessor callers, ownership, and teardown.
Limits: no full suite rerun or GUI/live-radio session; demo mode does not reach this HL2-specific path. Verification was socket-free backend coverage with no TX. No invocable automated review pass was available; a second manual lifecycle/failure-mode pass was completed. New-head CI is queued and must pass before merge.
|
Antares Detector — candidate vulnerable file(s)
The pairing age logic uses (nowNs - peakAtNs) / 1000000 to compute age in ms, which can overflow std::int64_t and produce a negative age. The slicePeakAgeMs function returns ago < 0 ? 0 : ago, so a negative computed age is treated as 0 and deemed 'current', incorrectly pairing a dead peak with a live overload flag. This is a memory-safety/logic flaw in the ADC pairing age validation. 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: $20.8728 · model: claude-opus-5 |
Fixes #6186
The seam and the test the triage on #6186 describes. One production change:
Hl2Backend::healthSnapshot()takes its instant as an argument internally.Limits
QElapsedTimer. Every read asserts that its own snapshot carries the flag, so a link that stopped reading Streaming fails the test and cannot pass as "stale".Hl2RxDspis placed in receiver 0 and given IQ blocks by hand.healthSnapshot()and no helper exposes it, so a wording change there fails this test.adcPairing1and up are not read.Change
Hl2Backend: a privatehealthSnapshot(std::int64_t nowNs); the publichealthSnapshot()calls it withhl2::steadyNowNs(). The slice peak's age ishl2::slicePeakAgeMs(adcPeakObservedAtNs(), nowNs), and that one value feeds both theadcSliceObservedAgoMsrow andhl2::slicePeakIsCurrent().Hl2RxDsp::adcPeakObservedAgoMs()is removed: it has no caller left insrc/ortests/, and it was the clock read behind the snapshot's ADC rows.friend struct Hl2AdcAgeTestAccess, beside the existing test-access friends.hl2_adc_pairing_test.cppthat saidHl2RxDspforms the age.Test
New
hl2_adc_age_gate_test, socket-free. A realHl2RxDspstamps a peak from a tone 20 dB below full scale; link-up and telemetry with the overload flag clear arrive throughMetisClient's signals. The snapshot is read atstamp + kSliceStaleMs ms + 999999 nsand one nanosecond later, with the constant taken from the header.kSliceStaleMsand the pairing row carries the "agree" sentence.kSliceStaleMs + 1and the pairing row is not reported.SliceSamplingGate::applied()is true for the stamp, and the stamp has not moved. So the two reads differ in the age alone.Mutation check. Each mutant is a scratch copy of
Hl2Backend.cpp, compiled with the tree's own compile command and linked ahead of the library; none is committed.true(the mutant from the issue)falseNot run as a mutant:
<=changed to<inslicePeakIsCurrent()itself;hl2_adc_pairing_testowns that edge.Full ctest at
9737ff2f(-j8): 781 of 781 reported passed (ctest counts its skipped tests as passed), no rerun.check_test_registration.py --strictandgen_touchpoint_manifest.py --checkpass.73, ON8ST
🤖 Generated with Claude Code