[on8st]

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
  (Replace retained socket fixtures with injected and opt-in coverage #5254).
• peripheral_auth_dialog_interaction_test: "dialog 960x680, viewport 658,
  page minimum 784" (feat(peripherals): authenticate direct TGXL, PGXL, and Antenna Genius connections #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

[aethersdr-agent]

Issue fit

No Fixes/Closes — this is Refs #6377, answering an optional note left on that PR's approving review asking for the unreachable-onset and short-capture paths to be retained as committed regression coverage instead of scratch variants. The diff does exactly that, and adds a third case (silent capture) found while writing them. A test-only follow-up to a merged PR needs no new issue or RFC under GOVERNANCE.md — bug fixes and test additions with a clear root cause explicitly do not. I could not verify the quoted note's attribution: read_issue/list_issue_comments return the bot and author threads on #6377 but not review comments, so the quote itself is unverifiable from here. Its substance matches the leadMs sentinel and envelope-read bound discussed in that thread's visible comments, so it is plausible; worth a glance from whoever wrote it.

Yes, it solves the ask. Each of the three failure paths is now pinned at both t0 values, and the mutation table in the body names which check dies for which mutation — which is the thing that makes "retaining these cases" worth anything.

Scope

File / group	What it changes	Claimed?	Verdict
tests/hl2_tx_passband_onset_test.cpp	onset() split into analyseOnset(what, iq, t0) + the modulator-running wrapper; the four onset bounds extracted as predicates; checkInjectedCaptures() added; a new steady == 0.0 early return in the analysis; <array> and <limits> includes	Yes, including the behaviour change	In scope

Everything in the diff is explained by the stated intent. One thing worth calling out explicitly because it is not pure added coverage: commit 683e6302 changes what the existing test asserts on a real capture. tests/hl2_tx_passband_onset_test.cpp:264-272 adds

    if (steady == 0.0) {
        o.halfMs = 1e9;
        o.leadMs = 1e9;

Before this, an all-zero settled tail made every level "reached" at the first sample, read a 0 ms onset, and passed both mid-over bounds — a silent modulator would have slipped through the onset legs. The direction of change is strictly stronger (a silent capture now fails rather than passes), it fires only on an exactly-zero tail, and the body discloses it under Limits. Not a finding; named so a maintainer sees it rather than reading the title's "injected captures" as additive-only.

No CHANGELOG.md entry (correct). No new public surface, no settings, no capability record, no socket or fake-peer anywhere in the file — the subject is Hl2TxDsp driven in-process through the event loop, which is pre-existing. No removed guard: the check(false, "the onset capture is long enough to read") moved from inside the analysis to the caller at :322-324, same message, same failure — it had to move so the injected too-short case can read the sentinel without failing the test.

CodeGuard

Both CRITICALs refuted. tests/hl2_tx_passband_onset_test.cpp:52 and :70 are:

// Run the event loop -- not sleep -- for `ms`, as the live I/O thread does.
// One fresh over: `audio` at 24 kHz, fed in 10 ms deliveries at real time,

Pre-existing // comments using Markdown identifier quoting, both outside every hunk in this diff (the hunks are the includes at 14-24 and the body from 231 on) — the usual whole-file-scan artefact. CS-VLN-CMDI-BACKTICK matches Ruby command substitution; C++ has no backtick operator. security-audit (CodeGuard) confirmed: zero matches for system|popen|exec|QProcess|fork|spawn|getenv in the file, and the only outward calls are processEvents, fprintf, sleep_for and the Hl2TxDsp API. It also returned no findings on the memory-safety questions I put to it, independently checking the std::fill range, never.back(), readable - 1, every iq[n] index and the envDb float-to-size_t cast.

Blockers

None.

Nits (non-blocking)

• tests/hl2_tx_passband_onset_test.cpp:358-360 — rise1090Ms is the one bound not sentinelled on any failure path, so the !keyUpRiseOk(…) leg of "every onset bound fails" is leaning on a default rather than on the sentinel. On the NaN capture it is 1e9 - 1e9 == 0.0; on silent and too-short it is the member default 0.0. The assertion holds because 0.0 < kMinKeyUpRiseMs, i.e. because the anti-click ramp's lower bound happens to be above zero. The comment

  // one sample too short to read. Both come back as 1e9 and fail every bound.

  is accurate about halfMs/leadMs, but a reader will take "fail every bound" to mean all four fail for the same reason. Either sentinel it alongside the others on both early returns (o.rise1090Ms = 1e9;) or narrow the sentence to name the two fields. Low stakes: if that lower bound ever moved to zero the injected case would fail loudly, not pass silently.

• tests/hl2_tx_passband_onset_test.cpp:414 — checkInjectedCaptures() sits after the return 77 modulator gate at :408-411, so on any build arm where modulatorName() != "wdsp-txa" the analysis paths are never exercised, even though none of them runs the modulator. Free coverage to move it above the gate, with the gate then preserving a failure rather than discarding it:

      checkInjectedCaptures();   // no modulator: valid in every build arm
  
      std::fprintf(stderr, "modulator: %s\n", Hl2TxDsp::modulatorName());
      if (std::string(Hl2TxDsp::modulatorName()) != "wdsp-txa") {
          std::fprintf(stderr, "SKIP: these figures are the TXA modulator's\n");
          return g_failures == 0 ? 77 : 1;
      }

  Counter-argument for leaving it: the step control asserts midHalfOk/midLeadOk, which are TXA figures, so there is a weak case for keeping the whole block TXA-scoped. Your call.

• tests/hl2_tx_passband_onset_test.cpp:375 — the step case's message says "the 50 % point reads 10 ms and passes", but it asserts only midHalfOk/midLeadOk; a step's 10-90 % rise is 0 ms, so it necessarily fails keyUpRiseOk. "passes the mid-over bounds" would read truer on a future red.

• tests/hl2_tx_passband_onset_test.cpp:254-258 — the tooShort return leaves envDb at its {} default (six zeros, i.e. 0 dB = full level), where the other two sentinel paths use the -400.0 floor. Nothing reads it (the diagnostic fprintf is downstream of the return) and the brief case does not assert it, so this is consistency only.

What I tried to break

Findings below are reasoned from the head checkout at /tmp/aetherclaude/pr-6404, not reproduced at runtime — I have no build and no test run.

• The NaN trick, hard. std::abs(std::complex<float>{NaN, 0}) is hypot(NaN, 0), which is NaN (C99 Annex F: only an infinite argument overrides a NaN), so steady is NaN; the NaN sits at readable - 1, which is ≥ settledFrom, so it actually reaches the mean; steady == 0.0 is false for NaN so the new early return does not swallow it; x >= NaN is false for every sample so all four firstAbove calls return 1e9; halfAt ≈ t0 + 4.8e10 puts every envDb read past iq.size() and onto the -400.0 floor. The one way this breaks is -ffast-math//fp:fast, which would let a compiler assume no NaNs and optimise the comparisons — I grepped CMakeLists.txt and cmake/ for ffast-math|fp:fast|ffinite-math|funsafe-math|Ofast and found none, so the case is sound on all three platforms.
• leadMs == 0.0 by luck? No — bit-exact. The step is zero up to t0+480, so firstAbove(-60.0) and firstAbove(-6.0206) both land on n == t0+480 and produce the identical expression static_cast<double>(480) * kMsPerOut; their difference is exactly zero, not within-epsilon. Similarly halfMs is 480 * (1000/48000), within the 1e-6 tolerance of 10.0 under any rounding.
• Every index. t0 + 480 ≤ readable (which is t0 + 12032) so the std::fill range is in bounds at both t0; never has ≥ 12032 elements so back() is valid; readable - 1 ≥ 12031 cannot wrap; brief is exactly one sample under the :253 guard, which is the point; envDb(12.0) on the step at t0 == 0 goes to at == -96 and hits the at < t0 branch rather than casting a negative double. Independently confirmed by security-audit.
• Would these pass against the unguarded code? I walked each mutation the body claims. Removing the steady == 0.0 return makes the silent capture read halfMs == 0.0, which fails z.halfMs >= 1e9 — the guard is genuinely under test, not asserted to itself. Removing the length guard makes brief compute steady == 0.1 and halfMs == 0.0, failing b.tooShort. Re-deriving leadMs = halfMs - firstMs makes the NaN case report leadMs == 0.0, failing both its sentinel and its bounds check. The cases discriminate.
• Could the new guard produce a false red on a healthy run? Only on an exactly-zero settled tail, which no working modulator produces; and analyseOnset is reached from onset() alone, so the passband legs (checkShape) are untouched. The body's before/after figures are identical (95.54 / 14.77 / 72.92 ms), which is consistent, though I cannot reproduce them.
• Policy and canon. Socket-free (no QTcpServer/QUdpSocket/bind/listen/connectToHost/Fake* anywhere in the file); no fake radio peer; registered target, present at tests/tests.cmake:6063-6067 with SKIP_RETURN_CODE 77 and no explicit TIMEOUT, so it takes the retrofit default; DSP behaviour is the socket-free-CTest layer under AGENTS.md's test-layer boundary, which is where this sits.
• CI state. get_check_runs on c4dfc61 returns zero check runs — none reported at review time, so there is no green to distrust either way. I also grepped .github/ and tools/ for hl2_tx_passband_onset and got no hits, which independently confirms the body's claim that per-PR CI compiles this test but never runs it: the new coverage only executes in the full suite.

Recommendation

Approve with nits. Single test file, one row in the scope table, every claim in the body that I could check held up, and the mutation evidence is specific enough to be checkable. The rise1090Ms nit is the only one with any substance and it is cosmetic today. The one thing worth a maintainer's eye rather than mine is the second commit's behaviour change — a silent capture now fails the onset legs where it previously passed mid-over — which is disclosed and strictly stronger, but is a change to an existing assertion rather than new coverage. Next step: land it, or take the three nits in one follow-up commit first; either is fine. Thanks for turning the scratch variants into committed coverage, and for the before/after output comparison — that is the thing that makes a test-infrastructure refactor reviewable.

---

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

[aethersdr-agent]

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

• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in tests/hl2_tx_passband_onset_test.cpp /tmp/aetherclaude/pr-6404/tests/hl2_tx_passband_onset_test.cpp:52
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in tests/hl2_tx_passband_onset_test.cpp /tmp/aetherclaude/pr-6404/tests/hl2_tx_passband_onset_test.cpp:70

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

[on8st]

Thanks for the review. All four taken, in 46d88876, a new commit on top (no rebase).

• rise1090Ms is sentinelled on the too-short return, on the silent return, and when the 50 % point is never reached. The three "sentinel" checks assert it, so "every onset bound fails" now holds on the sentinel and not on a default of 0 sitting under the lower bound.
• The injected cases run before the modulator gate, so every build runs them. The gate returns 1, not 77, when one of them has failed.
• The step case says it passes the mid-over bounds.
• The too-short return fills the envelope with the −400 dB floor, as the other two paths do.

The healthy figures are unchanged: key-up 50 % at 95.54 ms, rise 14.77 ms; mid-over 50 % at 72.92 ms.

For the maintainer, as you say: the second commit of this PR makes an existing assertion stricter. A silent capture now fails the onset legs, where it passed the mid-over bounds before.

Full ctest at 46d88876 on macOS: 770 of 772. vkamp_connection_test failed at -j8 and passed on the serial rerun (#5254); the other is the clean-main failure #6008.

73, ON8ST

[rfoust]

Issue fit

Approve. This follows the optional review note on #6377 by committing socket-free injected captures for unreachable and short onsets at both t0 values. The additional silent-capture case catches an all-zero tail that otherwise passed the mid-over bounds.

Scope

File	Change	Verdict
tests/hl2_tx_passband_onset_test.cpp	Split capture analysis from the modulator run; add injected failure-path checks and a silent-tail guard	Within the stated scope

No product code, tolerance, bound, settings, or public-surface change. The stricter silent-capture assertion is disclosed in the PR body.

Blockers and nits

None.

Verification

I reviewed head 46d8887627aac7518a61064ef3ebb26173b89c76 applied cleanly to current main d7a0802cd2e0423fef4c4e8a47908238e6413e56 in an isolated scratch worktree. The ARM64 hl2_tx_passband_onset_test target built and its focused CTest passed. Disabling the silent-tail guard in the scratch copy made the test fail four checks, two at each t0; restoring the source returned the test to passing. I checked the added test for socket ownership and synthetic peers: none. The PR's five reported checks passed on this head, and there are no unresolved review threads. I did not run the full suite or drive the GUI; this change is confined to a test file.

I tried the short-capture boundary, unreachable-level sentinel and envelope floor, zero settled tail, both onset positions, and the non-TXA gate path. The added checks cover these paths without changing the existing passband thresholds. The two automated backtick alerts refer to C++ comments at lines 52 and 70, not shell execution.