Repository navigation
test(hl2): name the fault, bound the envelope read. Principle VIII. - #6377
Conversation
Answers the two non-blocking notes on the approving review of aethersdr#6347. Test only; no tolerance or bound changes. run(): the branch taken when the modulator reports a fault that is not a stall drop failed with ": no fault but a stall drop". The FAIL line now states the expectation that broke: "every modulator fault is a stall drop", as the comment above run() describes it. onset(): firstAbove() returns 1e9 when a level is never reached, which puts halfAt far past the capture, and envDb then indexed c.iq out of range before checkKeyUp or checkMidOver could fail. envDb returns the -400 dB floor for any index outside [t0, c.iq.size()). Refs aethersdr#6347 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.
Issue fit
Yes. No linked issue — this is a Refs #6347 follow-up to review notes, which GOVERNANCE.md does not require an issue for (test-only, clear root cause, no architectural surface). Both stated changes are present and do what the body says, and I could reproduce the out-of-range path by reading: firstAbove() returns the 1e9 sentinel at tests/hl2_tx_passband_onset_test.cpp:273 when a level is never reached, halfAt at :279 becomes t0 + 4.8e10, and the pre-fix at < t0-only guard let that be cast to std::size_t and indexed. The length check at :256 passes first, so nothing caught it.
One claim I could not verify: "answers the two non-blocking notes left on the approving review of #6347." list_issue_comments returns issue comments only, not review bodies, and gh api .../pulls/6347/reviews is credential-blocked in this environment. The #6347 thread I can see carries an earlier round of notes (a)–(e), all of which landed in 9c715b47 on that PR, so the referenced notes are plausibly a later review I can't read. The diff stands on its own either way; the #6347 review thread would settle it.
Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
tests/hl2_tx_passband_onset_test.cpp:121 |
check() FAIL message reworded |
Yes, item 1 | In scope |
tests/hl2_tx_passband_onset_test.cpp:280-284 |
envDb upper bound + one comment |
Yes, item 2 | In scope |
Everything in the diff is explained by the stated intent. Three insertions, two deletions, one file, no product code, no tolerance or bound constant touched — I checked kMaxKeyUpHalfMs, kMinKeyUpRiseMs, kMaxKeyUpRiseMs, kMaxMidHalfMs, kMaxMidLeadMs, kFlatDb, kEdgeMinDb, kEdgeMaxDb, kStop100Db, kStop150Db and kTones against b3e0302ea; all unchanged. No new public surface, no deleted guard, no CHANGELOG.md entry. No sibling copy of this helper exists to leave behind.
On the harness's 98 CodeGuard findings and the ten-file audit list: nine of those ten files are not in this PR. The scan ran against a checkout whose base lags main by three commits (#6364, #6371, #6372) — which is why SpectrumOverlayMenu, SpectrumWidget and the AGC-T MainWindow hunks appear, and why git diff main --stat on the head reports 34 files. That is base lag, not scope creep. The one file this PR does change carries zero CodeGuard findings. I ran the required security-audit subagent over the full list; it returned no findings, confirming the added bound is complete and classifying all 36 cited lines as false positives — the CMDI-BACKTICK/-PIPE/-AMP hits are lexical matches on Markdown-quoted Flex verbs inside // comments and on C++ reference/&Class::signal tokens, the PII-IP/SSRF-LOCALHOST hits in docs/user/screens.json are RFC 5737 TEST-NET-1 documentation addresses, and screens.json:3877 is you@example.com (RFC 2606) plus a truncated tskey-auth-... format hint in alt text, not a key. Credit: security-audit (CodeGuard). The two real-code hits it did read out — MainWindow.cpp:6296 (QFile statusFile("/proc/self/status"), fixed literal) and the MainWindow.cpp:9920-9922 bundle walk feeding QProcess::startDetached list-args overloads with this process's own argv tail — are both false positives and both untouched here.
Blockers
None.
Nits
-
tests/hl2_tx_passband_onset_test.cpp:276— a sibling sentinel path still produces a vacuous pass. The PR hardensenvDbagainstfirstAbove's1e9sentinel, butleadMspropagates the same sentinel into a check that then passes for the wrong reason:o.leadMs = o.halfMs - o.firstMs;
When no level is reached at all,
firstMs == halfMs == 1e9, soleadMs == 0.0andcheckMidOver'so.leadMs <= kMaxMidLeadMspasses. The explicit short-capture early return at:257-259reaches the same state from the other side: it setso.halfMs = 1e9and returns withfirstMsleft at its0.0default, soleadMsstays0.0. In both caseso.halfMs <= kMaxMidHalfMsfails first and the test still exits 1, so this is cosmetic — the FAIL list just reports one fewer broken expectation than is actually broken. Pre-existing from #6347 and in the same sentinel-propagation family this PR is cleaning up, so worth naming; explicitly non-blocking and fine to leave for a later pass. -
tests/hl2_tx_passband_onset_test.cpp:121— "clearer", not "corrected". The old": no fault but a stall drop"was ambiguous rather than inverted: it reads either as "there was no fault, but a stall drop" (wrong) or "no fault except a stall drop" (which is the expectation). The new wording is unambiguous and matches the file's convention, sincecheck()at:35printsFAIL: %sand every other message in the file is phrased as a positive expectation. Worth a one-word softening in the body's framing; the code change is right.
What I tried to break
- The sentinel path the fix targets. With
halfMs == 1e9,halfAt ≈ t0 + 4.8e10and every call-site offset (1, 2, 4, 6, 10, 12 ms) lands pastsize(), so all six float to -400. The||short-circuits before thestatic_cast<std::size_t>(at), so there is no UB on the conversion itself, not just no bad read. Held. - Whether the guard changes any passing figure. It cannot: a finite
halfMscomes from then < c.iq.size()loop at:268-272, and everymsargument is positive, soat < halfAt < size()always and only the pre-existing lower bound is live. Checked numerically too — key-up (t0 = 0,halfAt ≈ 4586, capture 19200) and mid-over (t0 = 24000,halfAt ≈ 27500, capture ~43200) put all six offsets well inside. The body's "measured figures are the same before and after" holds by reading. - NaN reaching the cast. It can't: if
steadywere NaN,levelis NaN, every>=infirstAboveis false, and it returns the finite1e9sentinel.steady == 0.0exactly is also safe —levelis 0,firstAbovereturns 0 atn == t0for every threshold, soat < t0floors each read to -400. - Sibling unguarded reads of
c.iq.bin()at:138indexesiq[from + n]with no internal bound, but both callers passfrom = kSettlewithcount = ((size - kSettle) / kPeriod) * kPeriod, floor-aligned undersize - kSettleand behind the length check at:169. The loops at:262-264and:268-272are bounded byc.iq.size()directly. Nothing else computes an index. The audit subagent reached the same conclusion independently. - The other sentinel consumer,
rise1090Msat:275. Both-unreachable gives0.0, failing>= kMinKeyUpRiseMs; upper-only-unreachable gives ~1e9, failing<= kMaxKeyUpRiseMs; the reverse split is impossible because the thresholds are monotone (-20 dB is crossed no later than -0.9151 dB). No vacuous pass there — which is why onlyleadMsis a nit. - The early-return path.
Onsetdefault-initializes all four members at:230-233, socheckKeyUp/checkMidOverread no uninitialized doubles after:257-259. No UB; see the nit for the cosmetic half. - Socket tests: none. This PR adds and modifies no socket, no fake peer, no
bind/listen/connectToHost; I grepped the changed file and it has none. Nothing to record, and nothing removed either. - Could not check: anything requiring a build or a run. All five checks are green on
5a8a6eb, but notehl2_tx_passband_onset_testappears in noctest -Rfilter inci.yml— PR CI compiles it and never executes it, as the #6347 thread already acknowledged. So green here proves the file still builds on all three platforms, nothing more; the behavioural evidence is the author's local macOS/TXA run and their four ASan scratch variants, which I take at face value and did not reproduce. Every finding above is reasoned from the code, not observed at runtime.
Recommendation
Approve with nits. This is the rare follow-up that is strictly smaller than its justification: three lines, test-only, and the one substantive change converts an out-of-range read into a documented floor without moving any measured figure or weakening any check — the failure it unmasks (halfMs <= 106) now gets to fail cleanly instead of being pre-empted by UB. The disclosure discipline in the body is good, including the ASan caveat that only the test's own TU was instrumented. Next step is a maintainer read of the leadMs nit: either fold a one-line guard into this PR or let it ride as a known cosmetic gap from #6347, since the test exits 1 regardless. Thanks for chasing the review notes down to the sentinel rather than just reworading the message.
🤖 aethersdr-agent · cost: $11.3078 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (103 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: $12.0558 · model: claude-opus-5 |
Review of aethersdr#6377: when no level is reached, firstMs and halfMs are both the 1e9 sentinel, so leadMs was 0 and the mid-over lead check passed for the wrong reason; the short-capture return left it at 0 as well. Both now report the sentinel. The test exited 1 either way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Fixed in
The two notes this PR answers are jensenpat's inline comments on the approving review of #6347, which your tooling could not read:
Full ctest at 73, ON8ST |
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
The PR addresses the two review notes on #6347 and the subsequent leadMs sentinel note. No blockers found; approve with one non-blocking coverage suggestion.
Scope
| File/change | Claimed? | Verdict |
|---|---|---|
| tests/hl2_tx_passband_onset_test.cpp: non-stall fault message | Yes | In scope |
| Same file: bound envelope reads | Yes | In scope |
| Same file: propagate the failure sentinel into leadMs | Yes | In scope |
Everything in the diff is explained by the stated intent. No product-code, measurement-bound, public-surface, or unrelated changes.
Blockers
None.
Non-blocking nit
See the inline comment: retaining socket-free failure-path regression cases would preserve the scratch evidence in an automatically exercised form.
Verification
Reviewed head e726c40 against original base b3e0302 and GitHub merge ba73e3a. Built current main f753dbf plus the PR's sole changed file in an isolated scratch directory using Qt 6.12 and the prescribed ARM64 toolchain, with RADE enabled. Processor settings and test executable are ARM64; no RNNoise x86 sources were present.
Focused hl2_tx_passband_onset_test passed on both the PR version (8.52 s) and current-main baseline (7.34 s). Healthy onset output matched exactly: key-up 95.54 ms, rise 14.77 ms, mid-over 72.92 ms. All five reported CI checks are green; PR CI compiles this target but does not execute it. Main advanced after local validation only in HL2 discovery and its logging test, with no changes to this test or its DSP path.
What I tried to break
- ASan probes using the actual extracted onset function and injected captures reproduced the baseline out-of-range BUS when the onset is unreachable. The PR returned -400 dB safely, with halfMs and leadMs both 1e9.
- Short captures left leadMs at zero on the baseline; the PR returned the failure sentinel. Repeated both scenarios with the nonzero mid-over t0 under ASan.
- A healthy injected capture had identical before/after output; the real DSP test also preserved healthy measurements.
- Injecting a non-stall fault into the actual extracted run helper produced
FAIL: USB: every modulator fault is a stall dropand did not retry.
The ASan probes exercise extracted functions, not a fully sanitizer-instrumented DSP library. No GUI instance was needed for this test-only change, and no hardware or live TX was used. A separate automated review invocation was unavailable.
…6404) Refs #6377 Answers rfoust's optional note on the approving review of #6377, `tests/hl2_tx_passband_onset_test.cpp:286`: > the unreachable-onset and short-capture paths are demonstrated by scratch > variants, but remain absent from committed regression coverage. Consider > retaining socket-free injected-capture cases so removing this bound or the > leadMs sentinel guard fails automatically. ## Limits - Test only, one file. No product code, no tolerance and no bound changes. - Built and run on one Mac (macOS, Qt 6.12.0), TXA build. Per-PR CI compiles this test but does not run it. - "A level is never reached" is injected as a capture whose settled level is NaN. With finite samples it cannot be reached: the scan runs to the end of the capture, so it includes the settled tail, whose largest sample is at least its mean. - **Found while writing these cases, and fixed in the second commit (`683e6302`):** a silent capture is not that path either. Its settled level is 0, so every level was "reached" at the first sample and it read a 0 ms onset; an all-zero mid-over capture passed both mid-over bounds. The analysis now returns the sentinel when the settled level is zero, and an injected silent capture pins that every onset bound fails, at both `t0`. The passband legs already failed on a silent modulator; this closes the onset legs. - Without the envelope-read bound the test fails by crashing, not by a FAIL line: the read is at a wild address. - AddressSanitizer instrumented the test's own translation unit only, not the library it links. ## Change 1. `onset()` is split. `analyseOnset()` reads an IQ vector and a `t0`; `onset()` runs the modulator and hands it the capture. The short-capture return stays in the analysis and sets a `tooShort` flag; `onset()` turns that into the failed check it was. 2. The four onset bounds are predicates, so a case can assert that a result fails them without failing the test. `checkKeyUp` and `checkMidOver` call them with the same messages. 3. `checkInjectedCaptures()` runs with no modulator, at the key-up `t0` (0) and at the mid-over `t0`: - a step, as the control: the 50 % point reads 10 ms and passes; - a level never reached: `halfMs` and `leadMs` are the 1e9 sentinel, all six envelope reads are the -400 dB floor, all four bounds fail; - a capture one sample too short: `halfMs` and `leadMs` are the sentinel, all four bounds fail. ## Evidence The healthy run prints the same before and after: with the 16 lines of the injected cases left out, the test's output at `bbded449` and at this head is identical line for line (121 lines), exit 0 both times. | | key-up 50 % | key-up rise 10-90 % | mid-over 50 % | |---|---|---|---| | before (`bbded449`) | 95.54 ms | 14.77 ms | 72.92 ms | | this PR | 95.54 ms | 14.77 ms | 72.92 ms | Mutations, each a scratch build of the test file only, linked against the tree's library at this head; nothing of them is in this PR: | mutation | result | |---|---| | none | all checks passed, exit 0; under AddressSanitizer too, no report | | upper bound dropped from the envelope read | crash (SIGSEGV) in the first never-reached case, no check after it runs; under AddressSanitizer `BUS on unknown address` in the envelope read inside `analyseOnset` | | `leadMs = halfMs - firstMs` again | 4 checks failed, exit 1: "level never reached: halfMs and leadMs are the sentinel" and "every onset bound fails", at both `t0` | | `o.leadMs = 1e9` dropped from the short-capture return | 4 checks failed, exit 1: "too short: halfMs and leadMs are the sentinel" and "every onset bound fails", at both `t0` | From this PR's review (`46d88876`): `rise1090Ms` is the sentinel on every failure path too, and the checks assert it; the injected cases run before the modulator gate, so every build runs them, and the gate keeps a failure instead of skipping over it. Full ctest at `46d88876`, `-j8`: 770 of 772 passed (4 skipped: the `nr_stereo_independence_*` set), 2 failed: - `vkamp_connection_test`: failed at `-j8`, passed on the serial rerun (#5254). - `peripheral_auth_dialog_interaction_test`: "dialog 960x680, viewport 658, page minimum 784" (#6008); fails on clean main on this Mac, and this diff touches no dialog file. `hl2_tx_passband_onset_test` passed in 6.82 s. `check_test_registration.py --strict`, `check_wdsp_test_links.py --strict` and `gen_touchpoint_manifest.py --check` pass. 73, ON8ST 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
Refs #6347
Answers the two non-blocking notes left on the approving review of #6347.
tests/hl2_tx_passband_onset_test.cpponly.Limits
measured figures are the same before and after.
variants of the test that are not in this PR.
the library it links. The out-of-range read it reports is a BUS on a
wild address, not a redzone hit.
Change
run(): when the modulator reports a fault that is not a stall drop,the check failed with ": no fault but a stall drop". The FAIL line now
states the expectation that broke: "every modulator fault is a stall
drop", as the comment above
run()describes it.onset():firstAbove()returns 1e9 when a level is never reached.halfAtis then far past the capture, andenvDbindexedc.iqoutof range before
checkKeyUporcheckMidOvercould fail.envDbnowreturns the -400 dB floor for any index outside
[t0, c.iq.size()).onset(), from this PR's own review: when the 50 % point is neverreached,
leadMsis the 1e9 sentinel too, not1e9 - 1e9 = 0, and theshort-capture return sets it as well. So the mid-over lead check fails
with the others, where it passed for the wrong reason.
Evidence
Four scratch variants, the test file compiled with
-fsanitize=addressand linked against the tree's library at
5a8a6eb3:AddressSanitizer: BUS on unknown address, inenvDbcalled fromonset(); no check ranFAIL: USB: every modulator fault is a stall drop(and per leg), exit 1Full ctest at
e726c404,-j8: 755 of 758 passed, 3 failed, all threealso on the serial rerun. Besides the two below,
vkamp_connection_testfailed with only one of its two known checks (#5254).
peripheral_auth_dialog_interaction_test: fails on clean main on thisMac (feat(peripherals): authenticate direct TGXL, PGXL, and Antenna Genius connections #6008); this diff touches no dialog file.
unified_title_bar_test: fails on clean main on this Mac (Unified title bar (#6198): native-platform checks still open — Windows Snap Layouts, #4557 drift, macOS M1, xcb move #6224); onlyits two known checks failed; this diff touches no title-bar file.
hl2_tx_passband_onset_testpassed in 6.69 s.check_test_registration.py --strict,check_wdsp_test_links.py --strictandgen_touchpoint_manifest.py --checkpass.73, ON8ST
🤖 Generated with Claude Code