Scope the first upstream PR #7

Activity

1. dtinth-claw

   added

   wayfinder:grillingWayfinder grilling ticketWayfinder grilling ticket

   on Jul 22, 2026
2. dtinth-claw

   added a parent issue on Jul 22, 2026

   • Testing harness #3
3. dtinth-claw

   marked this as blocked by 3 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
4. dtinth-claw commented on Jul 22, 2026

   

   dtinth-clawBot

   on Jul 22, 2026 – with dtinth-claw

   Author

   More actions

   🔒 Claimed (orchestrator, 2026-07-22) — this is a grilling ticket: it resolves through @dtinth's answers, one question at a time.

   All inputs are in: harness proven (#4), upstream requirements researched (#5), jamulussoftware#3819 assessed (#6), CI verified green on all platforms (#8).

   Question 1 — the framing of PR 1. Upstream's still-open #2428 asks for a client/server smoke test in CI — that's their pre-agreed spec, but it is not what we built (a unit-test harness). Three honest framings:

   • (a) Unit-harness PR, Automatic Client/Server Smoke Test in CI jamulussoftware/jamulus#2428 referenced as context — ship what exists (2 files + workflow, proven green). Risk: a reviewer notes "this isn't the smoke test Automatic Client/Server Smoke Test in CI jamulussoftware/jamulus#2428 asked for". Mitigation: you comment on Automatic Client/Server Smoke Test in CI jamulussoftware/jamulus#2428 first, proposing the unit harness as step 1 with the smoke test as follow-up, and let a maintainer nod before the PR opens. My recommendation — it satisfies the "agree before coding" gate in spirit and in letter, with the artifact already de-risked.
   • (b) Build Automatic Client/Server Smoke Test in CI jamulussoftware/jamulus#2428's smoke test first — maximum upstream-alignment, but it's new unbuilt work (headless client+server, connection verification), a bigger and flakier artifact than a 26–66s unit job, and delays the merge of what's ready.
   • (c) Bundle both — one PR with unit harness + smoke test. Directly contradicts the research finding that small single-purpose PRs merge friction-free and big ones get hammered. Not recommended.

   Which framing? (Or a different one.)

   On deck after Q1 (one at a time): commit authorship for the PR branch you'll front (current testing-harness commits are bot-authored with AI trailers — rebase/re-author vs. keep as-is with disclosure); whether the ASan/UBSan variant belongs in PR 1 or later; and the exact PR title/description draft for your review.
5. dtinth commented on Jul 22, 2026

   

   dtinth

   on Jul 22, 2026

   Owner

   More actions

   Let's not rush to create a PR yet!

   I think we should eventually have both smoke tests and unit tests. They provide different benefits and have different limitations, and give us different levels of confidence.

   So I would like to explore the smoke testing aspect of this too, but I doubt they would be in the same PR. So smoke testing should be on our map too as that is a part of getting the software under test - while the first PR would be setting up a simple unit test harness with CI wired up, but I wouldn't call that the destination. In fact, I think this map doesn't have a fixed destination. We can explore more as we go (I find the mapping part of wayfinding more useful than reaching a fixed destination)
6. dtinth-claw commented on Jul 22, 2026

   

   dtinth-clawBot

   on Jul 22, 2026 – with dtinth-claw

   Author

   More actions

   Acknowledged — no PR yet. Recorded from your answer:

   • Q1 settled: the first upstream PR will be a simple unit-test harness with CI wired up; smoke tests are separate work in a separate eventual PR. Neither is "the destination".
   • The map is now open-ended: I've rewritten the map's Destination section accordingly (no fixed endpoint; milestones, not destinations) and removed the out-of-scope lines that assumed the effort ends at the first merged PR.
   • Smoke testing is on the map: new prototype ticket Smoke test spike: headless client/server connect in CI, seeded from upstream Automatic Client/Server Smoke Test in CI jamulussoftware/jamulus#2428's sketch. It's on the frontier, undispatched for now.

   This ticket stays open for the remaining first-PR decisions when you're ready to move (commit authorship for the branch you'll front, whether an ASan/UBSan job rides along, PR title/description draft). No rush from this side.
7. dtinth commented on Jul 22, 2026

   

   dtinth

   on Jul 22, 2026

   Owner

   More actions

   ASan/UBSan please elaborate (I am new to C++ world). Authorship of commits can be you as the agent as it is the agent who did the implementation work.
8. dtinth-claw commented on Jul 22, 2026

   

   dtinth-clawBot

   on Jul 22, 2026 – with dtinth-claw

   Author

   More actions

   Recorded: commit authorship stays with the agent (it did the implementation work) — so no re-authoring; at most a mechanical squash of the CI iteration commits into a tidy branch before the PR, which I'd still recommend for reviewability.

   ASan/UBSan, from zero:

   C++ has no safety net: reading past the end of an array, using freed memory, or overflowing a signed integer isn't guaranteed to crash — it's undefined behavior. Often the program just quietly reads garbage, appears to work, and misbehaves somewhere far away later. That's why bugs like the ones in Jamulus's history (the 2020 ACKN out-of-bounds read found by fuzzing, the jamulussoftware#3747 out-of-bounds vector crash, the jamulussoftware#3810 out-of-range channel ID) can sit unnoticed for years: a test could execute the buggy line and still pass, because nothing visibly breaks at that moment.

   Sanitizers fix exactly that. They're compiler flags (-fsanitize=address,undefined, supported by gcc/clang) that rebuild the program with instrumentation around every memory access and arithmetic operation:

   • ASan (AddressSanitizer) — catches memory errors the instant they happen: out-of-bounds read/write, use-after-free, double-free. The program aborts with a report pinpointing the exact line and the memory involved.
   • UBSan (UndefinedBehaviorSanitizer) — catches undefined behavior: signed overflow, invalid shifts, misaligned access, etc., again at the exact moment.

   So "41 tests pass" means the observable behavior was right; "41 tests pass under ASan/UBSan" additionally means no test execution path touched memory illegally, even invisibly. For a protocol parser fed hostile network bytes, that's the guarantee that actually matters — it's the compile-time-flag equivalent of what the afl-fuzz work was hunting for by brute force. Cost: roughly 2× compile+run time, which at our scale (tests run in milliseconds) is nothing.

   We already ran the suite once under both sanitizers locally during the prototype (clean). The pending question — one decision: should PR 1's workflow include a fifth matrix entry, "Linux Qt 6 + ASan/UBSan" (~10 extra lines of yml, ~40s job), or should PR 1 stay at the minimal 4-platform matrix with sanitizers as a follow-up?

   My recommendation: include it — it's the cheapest possible insurance against precisely the bug class that motivated this whole effort, and it makes the PR's value proposition sharper ("tests + memory-safety net"), not bigger in any way a reviewer would push back on.
9. dtinth commented on Jul 22, 2026

   

   dtinth

   on Jul 22, 2026

   Owner

   More actions

   I'm kinda wondering why you propose having both, testing with and without ASan/UBSan? Wouldn't just doing it with ASan/UBSan be a better choice? Not a directive. Just want your opinion on this because I noticed you didn't surface that as an option.
10. dtinth-claw commented on Jul 22, 2026

    

    dtinth-clawBot

    on Jul 22, 2026 – with dtinth-claw

    Author

    More actions

    Fair challenge — "sanitize everything" is a real option and I should have surfaced it. Why I still land on plain platform jobs + one sanitized job rather than sanitizers-everywhere:

    1. A sanitized build is a different program. The instrumentation changes code generation, memory layout, and links in a runtime that intercepts every allocation. It's brilliant at catching UB, but it means you're no longer testing the configuration you ship. The clean split is: plain jobs answer "does the shipped build behave correctly on this platform/Qt combo?"; the sanitized job answers "does any test path touch memory illegally?". Sanitizing everything would leave the first question unanswered — and it's the sharper question here, because of the Q_ASSERT theme from GetValFromStream uses ASSERT which is likely compiled out during release builds jamulussoftware/jamulus#3819: asserts vanish in release builds, so a suite that only ever runs in an instrumented debug-flavored build can quietly diverge from what users actually run.
    2. It's not uniformly possible anyway. UBSan doesn't exist on MSVC, and MSVC's ASan, while real, is fiddly with qmake/nmake (runtime DLL plumbing). So "all jobs sanitized" in practice becomes "Linux/macOS sanitized, Windows plain" — the axes get muddled: platform coverage and instrumentation would be conflated instead of orthogonal.
    3. Jobs are nearly free at our scale, so we don't have to choose. The 4 plain jobs cost 26–77s; one sanitized Linux job adds ~40s. This is also the pattern the big C++ shops use (Qt, Chromium): platform bots plain, sanitizer bots as dedicated configs.

    That said, your instinct is right in one narrow sense: if we were forced to have a single Linux job, sanitized would beat plain — more bugs caught per CI-second. The argument for both is that they answer different questions and the marginal cost is seconds.

    So the refined proposal for PR 1's matrix: the current 4 plain platform jobs + 1 Linux Qt 6 ASan/UBSan job = 5. Still your call.
11. dtinth commented on Jul 22, 2026

    

    dtinth

    on Jul 22, 2026

    Owner

    More actions

    I see. Then "Linux Qt 6 + ASan/UBSan" as you recommend.
12. dtinth-claw commented on Jul 22, 2026

    

    dtinth-clawBot

    on Jul 22, 2026 – with dtinth-claw

    Author

    More actions

    Sanitizer job landed: run 29930520844 — 5/5 jobs green, the ASan/UBSan job runs in 28s, and the build log confirms -fsanitize=address,undefined on every compile and link line (with halt_on_error so a report can't slip past the gate).

    That completes every open decision on this ticket except the last: the PR text you'll front. Draft below — edit ruthlessly, it's written in your voice.

    ---

    Title: Add a QtTest unit test harness with a protocol seed suite and CI

    This adds the beginnings of an automated test suite, as discussed in jamulussoftware#2428 (and offered on the maintainers' communication channels): a minimal QtTest-based unit test harness, a first suite covering the protocol layer, and a small CI workflow that runs it on Linux, macOS and Windows.

    What's in the PR (no production code is modified):

    • src/test/test.pro — qmake test target; compiles protocol.cpp + util.cpp in a headless server-only configuration and links QtTest. qmake src/test/test.pro && make check.
    • src/test/tst_protocol.cpp — 41 tests: the ParseMessageFrame wire-format contract (valid frames parse; short/bad-tag/bad-CRC/length-mismatched/truncated/junk inputs are rejected without crashing — the frame bytes are built independently of the production generator, so the on-wire format itself is pinned); round trips of representative messages through two connected CProtocol instances (incl. UTF-8 chat text, gain/pan quantization, the ACK flow, and a connection-less ping); and a regression test for the ACKN out-of-bounds read fixed in 024ebb4.
    • .github/workflows/unit-tests.yml — 5 quick jobs (26–77s): Linux Qt 5.15 (the supported minimum's era, distro packages), Linux Qt 6, macOS, Windows/MSVC, and Linux Qt 6 under AddressSanitizer+UBSan. Qt is installed the same way autobuild.yml does (aqtinstall + cache); no third-party actions beyond those autobuild already uses. Every job requires the QtTest Totals: line in captured output, so a job can't pass without the tests actually running.

    Deliberate choices: tests live under src/ so the existing clang-format/copyright tooling covers them; the code is written against Qt 5.12/C++11 (CI proves 5.15 and 6.x); the suite pins only stable public behavior — e.g. it asserts nothing about the GetValFromStream semantics currently under discussion in jamulussoftware#3819, and it deliberately doesn't assert the ACK-on-invalid-body behavior (noted in a comment) since that's a protocol decision, not a test's.

    This is intentionally a small first step. If the approach is welcome, natural follow-ups are more suites (jitter buffer, util) and the client/server smoke test sketched in jamulussoftware#2428 — separately, to keep each PR reviewable.

    Disclosure: the implementation was done by an AI coding agent (Claude) working under my direction in my fork, with research into this repo's contribution conventions beforehand; commits are agent-authored and I've reviewed and take responsibility for the result.

    ---

    Remaining mechanical prep once you approve the text (no PR opened until your explicit go): squash the ~13 iteration commits into 2 clean ones (harness+suite, workflow), reshape the workflow triggers from the fork-iteration shape (push on testing-harness) to pull_request/push on main with the same paths: filters, rebase onto latest upstream main, and hand you the branch.

    @dtinth — over to you on the draft (and whether the "offered on the maintainers' communication channels" phrasing is one you want, since only you can speak to that).
13. dtinth commented on Jul 22, 2026

    

    dtinth

    on Jul 22, 2026

    Owner

    More actions

    Thanks for the draft, but I will write the PR myself once I am ready for that. You only need to prepare the branch.
14. dtinth-claw commented on Jul 22, 2026

    

    dtinth-clawBot

    on Jul 22, 2026 – with dtinth-claw

    Author

    More actions

    Understood — the draft is yours to ignore. Preparing the clean branch now: squashing the iteration history into two commits (harness+suite, workflow), reshaping the workflow triggers to the upstream shape (push/pull_request on main + workflow_dispatch, same paths: filters), rebasing onto latest upstream main, and re-verifying all 5 jobs green via a manual dispatch. Will post the branch name here when it's verified — testing-harness stays untouched as the working history.
15. dtinth-claw commented on Jul 22, 2026

    

    dtinth-clawBot

    on Jul 22, 2026 – with dtinth-claw

    Author

    More actions

    Branch ready: unit-test-harness

    unit-test-harness — based on upstream main (49f4f8d6), 2 commits, 3 files, +909/−0, testing-harness left untouched as the working history.

    1. 51c1ceaf — Add QtTest unit test harness with protocol seed suite (src/test) — src/test/test.pro + src/test/tst_protocol.cpp
    2. 7a7218c3 — Add unit test CI workflow — .github/workflows/unit-tests.yml, triggers in final upstream shape (workflow_dispatch + push/pull_request on main, paths: scoped to src/** + the workflow itself, mirroring check-json-rpcs-docs.yml); otherwise byte-identical to the proven fork version

    Re-verified on this exact branch via manual dispatch: run 29933317459 — 5/5 green (Linux Qt 5.15, Linux Qt 6, Linux Qt 6 ASan/UBSan, macOS, Windows/MSVC), Totals: 41 passed confirmed in every job log, zero annotations.

    @dtinth — over to you: write the PR when you're ready and open it from this branch against jamulussoftware/jamulus:main. If you want anything changed on the branch first (commit wording, file layout, test trims), say the word here. Reminder from the research for when you open it: expect the "first-time contributor" CI-approval gate, two maintainer approvals needed to merge, and the AUTOBUILD: Please build all targets tag is only needed if you want the full platform builds on the PR.
16. dtinth-claw commented on Jul 23, 2026

    

    dtinth-clawBot

    on Jul 23, 2026 – with dtinth-claw

    Author

    More actions

    Resolution

    The first upstream PR is open: jamulussoftware/jamulus#3828 "Add unit testing harness" (3 commits, 7 files, +1029), fronted by @dtinth from the unit-test-harness branch after a comment-by-comment review on the map that reshaped the branch through: imperative tests, the event-log and sent-frame-log observation design, receiver-side acceptance API, golden frames as annotated += literals, gate-in-runner, upstream-script Qt install with shared cache, the restored jamulussoftware#302 ACKN regression, and two comment-philosophy audits. Everything this ticket asked — scope, packaging, narrative, and the decisions along the way (agent authorship, ASan job, trim + restore) — is settled and executed.

    Remaining work happens on the PR itself (upstream review), tracked from the map. Closing.
17. dtinth-claw

    closed this as completedon Jul 23, 2026