Testing harness #3

Activity

1. dtinth-claw commented on Jul 22, 2026

   

   dtinth-clawBot

   on Jul 22, 2026 – with dtinth-claw

   Last edited by dtinth-claw

   More actions

   I surveyed the codebase (as of upstream main @ 49f4f8d) to see what "get it under test" would build on. Summary and a proposed starting plan below.

   Current state: no automated tests at all

   • Zero QtTest/QTest usage in Jamulus's own sources; no test directory, no test target, no CMake (qmake-only via Jamulus.pro). All *test* hits are vendored deps (libs/opus, libs/oboe).
   • CI (autobuild.yml) is build/package/release only. The only quality gates are clang-format/shellcheck/pylint (coding-style-check.yml) and a JSON-RPC docs consistency check.
   • The one in-tree artifact is src/testbench.h (CTestbench): a manual fuzz tool that fires random protocol messages at a UDP port on a 1 ms timer. Not wired into anything (one commented-out line in main.cpp), no assertions — but it proves CProtocol runs standalone.
   • CONTRIBUTING.md's "Testing" section literally means "build it and try the feature."

   What's in our favor

   • Qt is already a dependency → QT += testlib (QtTest) costs nothing.
   • The build already supports GUI-free compilation (headless, serveronly/SERVER_ONLY), so core code compiles without widgets.
   • The best test targets are UI-free and deterministic:
     1. protocol.cpp (~3000 lines) — Create*Mes/Eval*Mes pairs are pure byte-buffer round-trips, and docs/JAMULUS_PROTOCOL.md is a spec to test against. Highest value: this is the wire format, and where malformed-input bugs live.
     2. buffer.h/.cpp — jitter/ring buffers (CNetBuf, CNetBufWithStats, CConvBuf), pure algorithmic.
     3. util.h/.cpp — CVector, CFIFO, CCRC, NetworkUtil, CHostAddress, etc.
     4. src/recorder/ — WAV + Reaper-project writers → golden-file tests.

   Main friction: monolithic qmake project with heavy header coupling (global.h everywhere, SERVER_ONLY ifdefs), so a test target has to pull in a cluster of core .cpp files rather than link a tidy library.

   Proposed first increment

   1. tests/ with a qmake tests.pro (stay in-family with the existing build; no CMake migration) that compiles protocol.cpp, buffer.cpp, util.cpp + deps with SERVER_ONLY-ish config and links QtTest.
   2. First suites: protocol message round-trip + malformed-input rejection (seeded by CTestbench's generators, but with assertions), CNetBuf put/get/overflow behavior, CCRC/NetworkUtil basics.
   3. A small test.yml CI job (ubuntu, Qt from apt) running the suite on PRs — separate from autobuild.yml so it stays fast.

   Open questions

   1. Destination: is this fork-only infrastructure, or do we shape it for eventual upstreaming to jamulussoftware/jamulus? Upstream maintainers are currently wary of high AI-generated volume, so if upstreaming is the goal I'd keep the first PR very small (harness + one suite) and discuss with them first.
   2. OK with qmake + QtTest (my recommendation, least invasive), or would you rather introduce CMake/Catch2 for the test side?

   @dtinth — blocked on question 1 (and 2 if you have a preference); happy to start on the harness + protocol round-trip suite on a branch here in the meantime.
2. dtinth commented on Jul 22, 2026

   

   dtinth

   on Jul 22, 2026

   OwnerAuthor

   More actions

   "and where malformed-input bugs live"

   Anything to back this up? Past issues, commits? Can you also check the issue tracker and past work on what has a large amount of churn, that would be a good first target?

   Re. questions:

   1. Yes, planning on upstreaming. Agree that first PR should be very small, but in our fork we can experiment as much as we want, we can decide the order of upstreaming later.
   2. As you recommended.

   Would /wayfinder be a good fit for this? If /setup-matt-pocock-skills is needed, please remember information in local memory so it (1) persists between Git branches (2) keeps the Git repo clean of agent files (for now).
3. dtinth-claw commented on Jul 22, 2026

   

   dtinth-clawBot

   on Jul 22, 2026 – with dtinth-claw

   More actions

   Evidence for "malformed-input bugs live in protocol parsing"

   Yes, this class has a documented track record here — and one member of it is open right now:

   • GetValFromStream uses ASSERT which is likely compiled out during release builds jamulussoftware/jamulus#3819 + PR Guard GetValFromStream against out-of-bounds reads in release builds jamulussoftware/jamulus#3821 (opened this week, open) — GetValFromStream in protocol.cpp validates its arguments only with Q_ASSERT, which compiles out in release builds; CVector uses std::vector's unchecked operator[], so a truncated/malformed network message can read past the end of the buffer in the builds people actually ship.
   • Check the size of PROTMESSID_ACKN messages jamulussoftware/jamulus#302 → commit 024ebb47 (2020) — an ACKN message with a valid checksum but no payload caused an out-of-bounds read (every other Eval* checked size first; the ACKN special case didn't). Commit message: "Found by fuzzing with afl-fuzz."
   • Fix two uninitialised reads jamulussoftware/jamulus#382 (2020) — "Fix two uninitialised reads. Found using valgrind with fuzzer-generated testcases."
   • jamulus 3.12.1 headless crash with out-of-bounds vector access jamulussoftware/jamulus#3747 → commit d545dbce (3.12.1, this year) — headless server crash from an out-of-bounds vector access in buffer.cpp.
   • Add bounds check iChanID before array access jamulussoftware/jamulus#3810 (merged last week) — bounds-check a network-provided iChanID before array indexing.

   So both the 2020 fuzzing wave and the 2026 bugs are the same shape: network-derived values used as sizes/indices without release-mode guards. None of those fixes came with a regression test, because there's nowhere to put one — which is precisely this issue.

   Also relevant precedent: upstream's own jamulussoftware#1413 "Run tests as part of build/CI" (2021) proposed running testbench.h in CI; it produced a useful explanation from atsampson (it's a template fuzzer generating valid-but-random messages) but was closed in 2022 without any tests landing.

   Churn analysis (commits touching each file since 2021-07, excluding sweeps that touched >10 files)

   file	commits	unit-testable?
   util.cpp / util.h	57 / 47	yes (pure-logic half)
   main.cpp	42	no (entry point)
   clientdlg.cpp / serverdlg.cpp	40 / 39	no (GUI)
   server.cpp / client.cpp	34 / 34	partially (orchestration)
   socket.cpp	31	parse path yes
   serverlist.cpp	26	mostly
   settings.cpp	21	yes (touches QSettings)
   protocol.cpp	11	yes, ideal

   Interesting split: churn and bug-severity point at different files. util.cpp/h is the churn champion (it's a grab-bag everyone touches), while protocol.cpp is low-churn but is where the security-class bugs recur — low churn there also means tests won't rot.

   Proposed first target

   1. protocol.cpp malformed-input + round-trip suite — every Create*/Eval* pair round-trips, and truncated/corrupted variants must be rejected without OOB access. This directly gives GetValFromStream uses ASSERT which is likely compiled out during release builds jamulussoftware/jamulus#3819/Guard GetValFromStream against out-of-bounds reads in release builds jamulussoftware/jamulus#3821 the regression test it currently can't have, which also makes a great small first upstream PR (fix + guard + test + harness in one story).
   2. util.h data-structure suite (CVector, CFIFO, CCRC, NetworkUtil) — covers the churn champion.
   3. buffer.cpp jitter-buffer suite (regression for jamulus 3.12.1 headless crash with out-of-bounds vector access jamulussoftware/jamulus#3747's class) as the third.

   Next step on my side: harness branch in this fork (qmake + QtTest as agreed) with target 1 as the seed suite.

   Re /wayfinder / /setup-matt-pocock-skills: neither is installed in my current environment, so I can't evaluate wayfinder's fit from here — if you install it on this box I'll pick it up. Your constraints are saved to local memory either way: agent/skill scratch files stay out of the git repo, and cross-branch state lives in local memory, not the working tree.
4. dtinth commented on Jul 22, 2026

   

   dtinth

   on Jul 22, 2026

   OwnerAuthor

   More actions

   I updated the skills in the dev box, please check.
5. dtinth-claw

   added

   wayfinder:mapWayfinder map issueWayfinder map issue

   on Jul 22, 2026
6. dtinth-claw

   added sub-issues on Jul 22, 2026

   • Prove the harness: QtTest seed suite builds and passes in the fork #4
   • Upstream contribution requirements for a test/CI PR #5
   • Track upstream #3819/#3821: GetValFromStream out-of-bounds guards #6
   • Scope the first upstream PR #7
   • CI design for the test suite #8
7. dtinth commented on Jul 22, 2026

   

   dtinth

   on Jul 22, 2026

   OwnerAuthor

   More actions

   I read this comment and this one already. Regarding behaviors that open PRs would fix but did not land in main yet, it is fine to add skipped tests for behaviors that proved failing and needs fixing, although do not do this needlessly. Just as a tool in your belt.
8. dtinth commented on Jul 22, 2026

   

   dtinth

   on Jul 22, 2026

   OwnerAuthor

   More actions

   FYI, I see this in runner log, please take care of that too.

   Node.js 20 is deprecated. The following actions target Node.js 20 but are being forced to run on Node.js 24: ilammy/msvc-dev-cmd@0b201ec. For more information see: https://github.blog/changelog/2025-09-19-deprecation-of-node-20-on-github-actions-runners/
9. dtinth-claw

   added a sub-issue on Jul 22, 2026

   • Smoke test spike: headless client/server connect in CI #9
10. dtinth-claw commented on Jul 22, 2026

    

    dtinth-clawBot

    on Jul 22, 2026 – with dtinth-claw

    More actions

    Node 20 deprecation warning: handled. Verified run 29928172118 — all 4 jobs green, Totals: 41 passed gate intact on every platform, and zero deprecation annotations (checked the full run log and the check-run annotations API).

    How: ilammy/msvc-dev-cmd has no Node 24 release (even its unreleased main still declares runs.using: node20), so the action was dropped entirely and the Windows job now bootstraps MSVC directly in PowerShell — running vcvarsall.bat x64 in a subshell, diffing the environment before/after, and applying the changed variables (the same technique the action used internally). A plain inline call vcvarsall.bat was tried first and does not work on the current windows-latest image (it ships a preview VS 2026 toolchain that leaves VCToolsInstallDir unset when called from a nested script) — that dead end and the workaround are commented in the workflow.

    Side benefit for the eventual upstream PR: one fewer third-party action to justify. Caveat noted in the workflow: if a future runner image goes back to a stable VS, the block may be simplifiable.
11. dtinth commented on Jul 22, 2026

    

    dtinth

    on Jul 22, 2026

    OwnerAuthor

    More actions

    I have a question about the Node 20 deprecation issue. Is the action that we use and stop using... like... is it an action that the upstream is already using or we just start using it here and then migrated away?
12. 135 remaining items

    Load more

13. dtinth-claw commented on Jul 24, 2026

    

    dtinth-clawBot

    on Jul 24, 2026 – with dtinth-claw

    More actions

    Upstream checks ran on #3828 (someone approved the workflows) — 18 of 19 green, including all six unit-test jobs on upstream's own runners, every style check, and notably the macOS/Windows autobuild jobs, which exercise our modified mac.sh/windows.ps1 in their full production context — the strongest validation those script changes can get.

    The one failure, Build for Linux .deb armhf (artifacts): infrastructure flake, not us. Evidence:

    • The job failed with no failed step recorded — the signature of a runner-level death (timeout or infra kill on the slow qemu-emulated armhf build), not a build error.
    • Our diff touches nothing armhf-adjacent, and the amd64/arm64 jobs running the same linux_deb.sh both passed.

    Action needed is a re-run, which needs upstream write access — PR authors can't re-run workflows. Suggested reply for the PR (or ask ann0see directly):

    The armhf .deb failure looks like a runner-level flake (job died with no failed step; the amd64/arm64 builds of the same script passed, and this PR touches nothing armhf-related). Could someone with write access re-run that job?
14. dtinth commented on Jul 24, 2026

    

    dtinth

    on Jul 24, 2026

    OwnerAuthor

    More actions

    As you recommend.
15. dtinth-claw commented on Jul 24, 2026

    

    dtinth-clawBot

    on Jul 24, 2026 – with dtinth-claw

    More actions

    Item 1 shipped as (a): commit 3b4612ae — a step parses QT_VERSION out of autobuild.yml's macOS main entry (legacy excluded via ARTIFACT_SUFFIX, ios can't match) and exports it; the duplicated pin is gone, and the parse fails loudly if it ever yields anything but exactly one plausible version (zero-match and multi-match both verified to hit the error path). Gate findings applied pre-push (pipefail-safe failure message, portable sed -E); bonus discovered during review: the old pin was invisible to bump-dependencies' Qt regex, so this also removes a latent silent-drift risk. Verification run 30079938743 in flight. Draft:

    jamulussoftware#3828 (comment)

    Addressed in 3b4612ae — took the parse approach for now: a step extracts QT_VERSION from autobuild.yml's macOS main build entry (the legacy/ios entries are excluded) and fails loudly if the parse doesn't yield exactly one plausible version, so a future autobuild.yml restructure breaks this workflow visibly rather than silently drifting. The composite-action refactor stays on the table as the cleaner follow-up, per the note in the PR description. One known residual: windows.ps1 hardcodes its own Qt version internally (autobuild.yml's Windows entry doesn't set one either) — documented at the PATH step; unifying that would fit naturally in the composite-action follow-up.
16. dtinth-claw commented on Jul 24, 2026

    

    dtinth-clawBot

    on Jul 24, 2026 – with dtinth-claw

    More actions

    Item-1 fix verified: run 30079938743 all 6 jobs green, with the live log showing Using Qt version 6.10.2 (from autobuild.yml) — parsed, not pinned. Tip 3b4612ae, 15 commits. Every review item on jamulussoftware#3828 is now addressed; the draft above awaits your paste, then it's armhf re-run + re-review on the maintainers' side.
17. dtinth commented on Jul 24, 2026

    

    dtinth

    on Jul 24, 2026

    OwnerAuthor

    More actions

    Can you rebase our working branch (not PR branch) based on the current PR's branch?

    Also please spike a refactor of the actions. This way we can try explore both at once: (1) exactly what the follow-up PR will look like, and (2) explore how "doing it as a sibling PR that autobuild.yml adopts first, then this PR consumes the action" might look like (in which we extract the common action and submit as a sibling PR).
18. dtinth-claw commented on Jul 24, 2026

    

    dtinth-clawBot

    on Jul 24, 2026 – with dtinth-claw

    More actions

    Both explorations dispatched in parallel (isolated worktrees):

    1. Working-branch rebase — really a port: the branches diverged architecturally during review (fluent DSL vs the reviewed event-log design), so a textual rebase would be conflict soup. The agent archives the old head as archive/testing-harness-dsl, resets testing-harness onto unit-test-harness (3b4612ae), and re-expresses the old suite's extra coverage (~27 more cases: the full round-trip family, CLPing incl. new CL-side tester wiring, extra goldens, invalid-body rejection) in the reviewed style. CI-verified before it reports.
    2. Composite-action spike on spike/setup-qt-action: commit A models the sibling PR (action + autobuild.yml migration, legacy-mac override, the windows.ps1 env question), commit B models this PR's consumption (parse step and per-workflow setup deleted). Verification via the unit-test workflow plus the autobuild/<branch> trick for autobuild's mac/windows jobs, with cache-key continuity analysis (does the shared cache survive the transition?) in a writeup, so both PR shapes land as concrete diffs + stats for you to compare.

    Reports here when each is green.
19. dtinth-claw commented on Jul 24, 2026

    

    dtinth-clawBot

    on Jul 24, 2026 – with dtinth-claw

    More actions

    Working branch rebuilt — testing-harness is now d5e587f0: one commit on top of the PR branch (3b4612ae) porting the old suite's extra coverage into the reviewed architecture. Old DSL-era head preserved at archive/testing-harness-dsl.

    • 45 test cases, all green locally (incl. an ASan/UBSan run) and in CI: run 30086203850, all 6 jobs.
    • Ported in new style: the full round-trip family (jitBufSize/clientID/chanPan/muteState/chatText incl. emoji/NetwTranspProps/licenceRequired/recorderState) as ReceivedLog() one-liners, goldens for ClientID and CLPing, RejectInvalidMessageBody via SendRawBytes (accepted-at-frame-level + zero signals, same shape as the ACKN regression).
    • One architecture addition the port forced: minimal connection-less wiring (WireCLMessages() + CLSentFrames()) — the CL path (CLMessReadyForSending/ParseConnectionLessMessageBody) is separate plumbing the tester had never needed before; wired only as far as the two CLPing tests require.
    • Deliberately not ported: the old ParseValidFrame/RejectTruncatedFrame — already covered by AcceptValidFrame and the truncation row in RejectInvalidFrame_data; porting them would duplicate, not add.
    • Nice verification detail: the 4-decimal FormatFloat precision was empirically confirmed to absorb pan/gain's 1/32768 wire quantization — checked against actual production output, not assumed.

    Net effect: the working branch is again strictly ahead of the PR branch (base + 26 extra cases), ready to become the follow-up coverage PR after jamulussoftware#3828 merges. Composite-action spike still in flight.
20. dtinth-claw commented on Jul 24, 2026

    

    dtinth-clawBot

    on Jul 24, 2026 – with dtinth-claw

    More actions

    Composite-action spike done — both PR shapes exist as concrete, CI-verified commits on spike/setup-qt-action (tip 57e5bf1c), full writeup in research/setup-qt-action-spike.md.

    The two shapes, in numbers:

    • Sibling PR (27732846): .github/actions/setup-qt + autobuild.yml's mac/windows jobs migrated — +154/−18 across 3 files.
    • Consumption follow-up (639d9657): unit-tests.yml drops its parse/cache/install/PATH steps for one uses: — +14/−72. The version literal ends up in exactly one place: the action's default input.

    The spike earned its keep — two real bugs found, one nearly invisible:

    1. Nested powershell.exe silently drops an empty -BuildOption '' arg (caught by the unit-tests run).
    2. The important one: an unconditional PATH export made windeployqt resolve a dependent DLL against the wrong Qt install, breaking both Windows autobuild entries — caught only by a genuine autobuild/* push running the full build/package matrix (failing run → fixed, 10/10 green). The fast workflow_dispatch loop never saw it. Fix: PATH export is opt-in (add-to-path, default false) — only unit-tests' bare qmake needs it; the build scripts use absolute paths.

    Key design decisions (details in the writeup): action-owned cache key (hashes action+scripts+version instead of hand-mirrored workflow keys — one deliberate cache repopulation at transition); two mutually-exclusive macOS setup steps because composite-action defaults are unreachable once with: sets the key at all; Xcode pinning stays outside (iOS shares the job template with a different version).

    Top-priority companion finding: bump-dependencies.yml's Qt6 job can neither reach action.yml (file glob) nor match its key: value syntax (regex — verified directly). Post-refactor, the version literal that matters would silently go stale on the next Qt bump unless that job is updated in the same PR. Flagged as must-ship-together, not deferrable.

    Recommendation: sibling-PR-first, with the add-to-path fix and the bump-dependencies update included. Verification loop lesson for that PR: dispatch runs aren't enough — it needs at least one real autobuild/* push before review.

    (Also: autobuild/spike-setup-qt exists for re-running the full matrix; quirk noted — a NEW autobuild/* branch's first push doesn't trigger the workflow, only subsequent pushes do.)
21. dtinth-claw commented on Jul 25, 2026

    

    dtinth-clawBot

    on Jul 25, 2026 – with dtinth-claw

    More actions

    New question from ann0see on PR jamulussoftware#3828 (not a finding — a placement question, so no fix pushed; your call):

    jamulussoftware#3828 (comment)

    No strong preference — happy to move them if you'd rather have `/test/`.
    
    `src/test/` was picked because it stays inside the existing tooling coverage with zero config changes. A new top-level `/test/` dir would need three spots extended:
    
    - [`tools/update-copyright-notices.sh` line 56](https://github.com/jamulussoftware/jamulus/blob/main/tools/update-copyright-notices.sh#L56) — the `find` list (`android ios linux mac src windows tools .github`) wouldn't visit it, so the yearly copyright bump would skip the test files.
    - [`Jamulus.pro` line 1194](https://github.com/jamulussoftware/jamulus/blob/main/Jamulus.pro#L1194) — `make clang_format` filters to `^(android|ios|mac|linux|src|windows)/`, so local formatting would skip them (CI's style check scans everywhere, so it would still flag them — with no convenient local fix).
    - The `paths:` filters in the new unit-tests.yml, which rely on `src/**`.
    
    There's also mild precedent: `src/testbench.h` already lives under `src/`.
    
    All three are one-line changes though, so if `/test/` fits the project layout better, say the word and I'll move it.
22. dtinth commented on Jul 25, 2026

    

    dtinth

    on Jul 25, 2026

    OwnerAuthor

    More actions

    I already made a reply (before I saw your message). What's your take?
23. dtinth-claw commented on Jul 25, 2026

    

    dtinth-clawBot

    on Jul 25, 2026 – with dtinth-claw

    More actions

    My take (full detail in the draft one comment up, verified against current main): your guess is close but the build system itself is actually indifferent — test.pro reaches the sources by relative path, which works the same from src/test/ or /test/. What does care is the sidecar tooling, which covers src/ but not a new top-level dir:

    1. Copyright bump — tools/update-copyright-notices.sh line 56's find list (android ios linux mac src windows tools .github) would skip /test/.
    2. make clang_format — Jamulus.pro line 1194 filters to ^(android|ios|mac|linux|src|windows)/. CI's style check scans everywhere regardless, so /test/ files would still get flagged — just with no local formatting target to fix them.
    3. This PR's workflow triggers — unit-tests.yml's paths: filters use src/**.

    Plus mild precedent: src/testbench.h already lives under src/. All three are one-line changes, so no strong preference here — /test/ is fine if ann0see wants it; it just costs three extra config touches in the PR.

    Follow-up draft for the PR thread, picking up from your "will get back":

    Checked with the agent ([context](https://github.com/dtinth/jamulus/issues/3#issuecomment-5077580774)). The build system is indifferent — `test.pro` uses relative paths either way. What ties it to `src/` is the sidecar tooling: [`tools/update-copyright-notices.sh`](https://github.com/jamulussoftware/jamulus/blob/main/tools/update-copyright-notices.sh#L56)'s `find` list and the [`make clang_format` target](https://github.com/jamulussoftware/jamulus/blob/main/Jamulus.pro#L1194) both cover `src/` but not a new top-level dir, and this PR's workflow `paths:` filters use `src/**`. There's also `src/testbench.h` as precedent.
    
    All three are one-line changes though, so no strong preference. If you prefer `/test/`, I'll move the files and extend those three spots in this PR.
24. dtinth commented on Jul 25, 2026

    

    dtinth

    on Jul 25, 2026

    OwnerAuthor

    More actions

    Let's move.
25. dtinth-claw commented on Jul 25, 2026

    

    dtinth-clawBot

    on Jul 25, 2026 – with dtinth-claw

    More actions

    On it — moving src/test/ → /test/ on unit-test-harness (append-only), extending the three config spots, then shepherding CI. Will post the draft reply when green.
26. dtinth-claw commented on Jul 25, 2026

    

    dtinth-clawBot

    on Jul 25, 2026 – with dtinth-claw

    More actions

    Move done and verified.

    • Commit bc226653 (append-only): src/test/ → test/, +13/−12 across 7 files — test.pro relative paths (.. → ../src), runner-script .pro paths, workflow paths: filters gain test/**, gcovr's now-dead --exclude src/test/ dropped, and the two sidecar configs extended (update-copyright-notices.sh find list, Jamulus.pro clang_format regex). coding-style-check.yml / .clang-format-ignore verified as needing nothing (extension-glob / exclude-only).
    • Pre-push gate (4 reviewers): clean on all axes; the spec reviewer verified ../src resolution with a real out-of-tree qmake run.
    • Fork CI: 6/6 matrix green. Upstream: style checks (C++/Python/shell) already green on the commit; autobuild jobs still running.

    Draft reply to ann0see's comment:

    Moved in bc226653 — `src/test/` → `test/`, plus the three knock-ons (`update-copyright-notices.sh` find list, `Jamulus.pro`'s `clang_format` regex, the workflow `paths:` filters) and the relative-path fixes in `test.pro` / the runner script. Full test matrix green on [my fork](https://github.com/dtinth/jamulus/actions/runs/30150926477).

    @dtinth done — paste when ready?
27. dtinth-claw commented on Jul 25, 2026

    

    dtinth-clawBot

    on Jul 25, 2026 – with dtinth-claw

    More actions

    Copilot finding on the moved harness — convincing (TruncateBy documents that an oversize truncation yields an empty frame, so chaining into CorruptCRC is a legal path to indexing [-1]), fixed under the standing protocol. Matrix 6/6 green.

    jamulussoftware#3828 (comment)

    Addressed in 33a89721. Real: `TruncateBy` can legally produce an empty frame, so chaining it into `CorruptCRC` would index the last byte of nothing. It's now a guarded no-op on empty frames.