Fix #4: locate the WAV data chunk instead of inferring where it must be

[thorwhalen]

Closes #4.

The bug

decode_wav_bytes never found the audio — it inferred where the audio must be:

header_size = len(wav_bytes) - n_channels * width_bytes * nframes
wf = decode_pcm_bytes(wav_bytes[header_size:], ...)

That holds only if nothing follows the data chunk. LIST/INFO tags are routinely appended by ffmpeg, Audacity and iTunes, and those bytes were counted as header — so the read started too far into the buffer:

expected [0,  1,     -1,     2,      -2,     3, -3, 4,      -4,     5]
actual   [0,  20041,  20294, 21321,  21574,  6,  0, 24908,  26230,  14389]

Right number of samples, wrong samples, no exception. Separately, a data chunk declaring more bytes than the file carries (a stream writer that never patched the length back into the header) tripped assert header_size >= 44 — blaming the header for a problem in the data size. That assert is the "choke" in the original report.

soundfile reads all of these correctly, which is how the reporter noticed.

The fix

Walk the RIFF chunk list to find data, instead of inferring its position.

No behaviour change on valid input. A differential sweep over 576 well-formed containers (channels × width × nframes × fmt size × chunks before/after data) has the new code correct on 576/576 where the old was wrong on 336/576, with no case where the old was right and the new is wrong. This matters — decode_wav_bytes has eight consumers in this ecosystem (hum, know, odat, front among them).

Two subtleties, both surfaced by adversarial review of the first cut and neither obvious:

• Clamping an over-declared size to EOF re-creates the original bug. With a trailing LIST present, "read to the end" hands the metadata back as extra samples. So an overrunning data chunk is bounded by the next position from which the remainder parses as a chunk list landing exactly on EOF, falling back to EOF only when no such boundary exists.
• A short data chunk now warns (ShortWavData) instead of decoding silently. Clamping is what the issue asked for, but a caller who cannot distinguish a half-downloaded file from a complete one is no better off than before.

Also included:

• Sub-frame remainders are dropped rather than raising a struct-size error; a payload with no whole frame returns an empty waveform instead of surfacing an IndexError from inside the chunked decoder.
• header_size_of_wav_bytes returns the offset from the same walk, and loses its meta parameter, which it no longer reads (it has no callers anywhere).
• comptype in decode_wav_header_bytes is assigned unconditionally — it was bound only inside if params.comptype == "NONE", an unreachable branch that would have left the name unbound if stdlib wave ever widened.

⚠️ Contract change

Malformed input now raises ValueError with a message saying what is wrong. This unifies three previous outcomes — AssertionError, wave.Error, EOFError — depending on how the input was malformed. wave.Error is not a ValueError subclass, so an except wave.Error handler would no longer catch it. I checked all eight consumers plus the notebooks: none catches any of the three. Worth a release note.

CI was not running the tests

The pytest step passed root-dir: recode, and the isee action builds its file list with find <root-dir> -name '*.py' — which never reaches the repo-root test_recode.py. CI collected 25 doctests and zero tests. So the pre-existing test_decode_wav_bytes was not gating anything, and neither would a new regression test. Pointed at the repo root it collects 151. That change is in this PR.

Verification

• 151 passed on Python 3.10.13 — the CI interpreter — using the exact CI invocation (pytest -v --doctest-modules -o doctest_optionflags="ELLIPSIS IGNORE_EXCEPTION_DETAIL" $files).
• Every guard is mutation-checked. Seven single-line reversions each turn the suite red, including the three that matter most: restoring the old subtraction (22 fail), dropping the channel factor from the frame size (9 fail), and clamping an overrun to EOF (1 fail). The first two of those previously survived the entire suite — the tests were strengthened until they did not.
• Byte-level attack suite run against the walker: RF64/BW64, WAVE_FORMAT_EXTENSIBLE, data sizes of 0 / 0x7FFFFFFF / 0x80000000 / 0xFFFFFFFF, a LIST whose contents embed the literal data (mis-sync probe), odd-sized chunks with and without the alignment pad, chunk sizes declaring past EOF, truncation at 10 depths, two data chunks, and bytearray/memoryview inputs. No infinite loop, no negative slice, no signed/unsigned confusion, no mis-sync.

Known limitations (not addressed here, deliberately)

• WAVE_FORMAT_EXTENSIBLE decodes on 3.12 but raises on 3.10, because decode_wav_header_bytes delegates to stdlib wave, which only learned that format in 3.12. The new chunk walker handles such files fine; only the header read fails. Parsing the fmt chunk directly would fix it and drop the stdlib dependency — out of scope for decode_wav_bytes needs a more robust wav head parsing #4.
• 8-bit PCM is decoded as signed, but WAV 8-bit is unsigned per spec — silence (128) comes back as -128. Pre-existing, and it lives in mk_pcm_audio_codec/num_find_num_type_for, whose blast radius is every recode consumer rather than just the WAV path. Filed separately.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QDDDKZaseXu6NtpzAvGs2i