Phase 3: hygiene, dependencies, readme

.github/workflows/build.yml

@@ -53,7 +53,7 @@ jobs:
         if: matrix.os == 'ubuntu-latest'
         run: |
           python tools/mpqref.py manifest src/test/resources/mpqs \
-            --names src/main/resources/DefaultListfile.txt \
+            --names src/test/resources/DefaultListfile.txt \
             -o /tmp/fixtures.tsv
           diff -u src/test/resources/golden/fixtures.tsv /tmp/fixtures.tsv
 
@@ -65,8 +65,15 @@ jobs:
           path: build/reports/tests/test
           retention-days: 7
 
+      # Telemetry, not a gate. continue-on-error as well as fail-on-error:
+      # the latter only covers the upload, so an outage while the action fetches
+      # its own reporter binary still failed the job -- which it did, with a 504
+      # from coveralls.io, on a commit whose build, tests, reference checks and
+      # manifest check had all passed. A third-party reporting service being
+      # down is not a reason to call this build broken.
       - name: Report coverage
         if: matrix.os == 'ubuntu-latest'
+        continue-on-error: true
         uses: coverallsapp/github-action@v2
         with:
           file: build/reports/jacoco/test/jacocoTestReport.xml

AUDIT.md

@@ -87,7 +87,9 @@ These are real bugs or hazards in behaviour that must not be carried into the ne
 - **P2-3 Sector CRC flag (0x04000000).** Verify per-sector ADLER/CRC on read when flag present; option to emit on write. Wire into extraction pipeline, not bolted onto `MpqFile`.
   - Done, in `MpqFileReader` on read and `MpqSectorWriter` on write, opt-in via `MpqWriteOptions.withSectorChecksums`. The checksums are Adler-32 **seeded with zero** -- not CRC32, and not standard Adler-32; see format notes 9 and 10, which also record what that distinction cost. Verification is on by default when reading and can be turned off to recover damaged archives.
 - **P2-4 `(attributes)` write support** *(issue [#11](https://github.com/inwc3/JMPQ3/issues/11))*. Honour the attributes bytemask properly on read (today hardcodes crc+timestamp layout and has a suspicious `-1` in the entry count — AttributesFile.java:38); regenerate CRC32+FILETIME on write when requested. Remove the dead commented block in `JMpqEditor.close`. Historical context from the issue thread: generation was disabled because CRC32 differed from StormLib for some `.wav` files — root cause is likely multi-compression handling (first sector of ADPCM-compressed wavs is not ADPCM-compressed since it holds the wav header). Fix alongside P2-6 and pin with a StormLib-golden CRC test. Acceptance for the issue itself: load + close `war3.mpq`-style archives without dropping `(attributes)` in a way the game rejects.
-  - Done. `MpqAttributes` reads every array the bytemask declares and accepts the entry counts StormLib tolerates; the unexplained `-1` is gone from the deprecated parser too. Generation is opt-in via `MpqWriteOptions.withAttributes`, with a pinnable timestamp so builds stay reproducible. The `.wav` CRC32 concern from the issue thread does not arise: the checksum is taken over decoded content, and the multi-compression ordering it depended on was fixed in P2-6.
+  - Done. `MpqAttributes` reads every array the bytemask declares and accepts the entry counts StormLib tolerates; the unexplained `-1` is gone from the deprecated parser too. Generation is opt-in via `MpqWriteOptions.withAttributes`, with a pinnable timestamp so builds stay reproducible.
+  - **Verified.** `tools/mpqref.py` now implements Huffman and ADPCM, transcribed from StormLib's `huff.cpp` and `adpcm.cpp` rather than from the Java, and decodes `wavTest.w3x`'s `ReviveNightElf.wav` to the same MD5 as JMPQ3 does: `6b131014f093fca5972bfc1a0477f1b1` over 144464 bytes. The file is a textbook instance of the issue thread's concern -- sector 0 is plain zlib because it carries the RIFF header, and the remaining 34 sectors are `0x41`, Huffman over ADPCM mono. The committed golden manifest now holds that digest, so `GoldenFileTests` checks the wav decode on every run instead of skipping it.
+  - Superseded note, kept for the reasoning: the argument used to be that the checksum is taken over decoded content, so it could only differ from StormLib if the decode differed, and the multi-compression ordering that would cause that was made table-driven in P2-6. The argument was right, but it was an argument.
 - **P2-5a Tolerant header parsing for real-world (protected) maps** *(issue [#46](https://github.com/inwc3/JMPQ3/issues/46))*. 59/857 sampled maps fail with "Bad header size": `readHeaderSize` hard-rejects `headerSize < 32 || > 208` (JMpqEditor.java:443) even though the game itself ignores the field for v0 archives. Mirror StormLib's leniency: derive the effective header size from the format version, clamp/ignore garbage values, and treat other header fields defensively (this also removes most of the need for consumers to pass `FORCE_V0`). Acceptance: the Forest Defense sample from the issue opens and extracts.
   - Done in Phase 0, as a side effect of modelling the header: `MpqHeader` repairs rather than rejects, following `ConvertMpqHeaderToFormat4`.
 - **P2-5b Fake-header protection resilience — nice to have** *(issue [#47](https://github.com/inwc3/JMPQ3/issues/47))*. Some protected maps plant decoy `MPQ\x1A` headers so `searchHeader` either accepts a bogus one or gives up. Approach: on finding a candidate header, validate it (plausible table positions/sizes within file) and keep scanning on failure instead of committing to the first match. Best-effort only — full protected-map support is explicitly not a goal; skip if it destabilises normal parsing.
@@ -98,24 +100,34 @@ These are real bugs or hazards in behaviour that must not be carried into the ne
 ## Phase 3 — Code hygiene & dependencies
 
 - **P3-1 Logging.** `logback-classic` + `logback.xml` ship in the library's runtime deps/resources and hijack consumers' logging config. Keep `slf4j-api` only; move logback + config to `testRuntimeOnly`. Remove `DebugHelper.appendData`'s `printStackTrace`.
+  - Done. `logback-classic` is `testRuntimeOnly`, no `logback.xml` ships, and `DebugHelper.appendData` raises `UncheckedIOException` instead of printing a stack trace.
 - **P3-2 Dependency audit.** `commons-compress` used only for `SeekableInMemoryByteChannel` (trivially self-implemented) — and will be needed for BZIP2 (P2-6), so decide once. Evaluate replacing unmaintained `jzlib` with `java.util.zip` (`Deflater`/`Inflater`) — the hand-rolled `zlibStoreLevel0` in CompressionUtil duplicates jzlib level-0 anyway; benchmark before/after. `xz` currently unused (see P2-6).
+  - Done. `jzlib` removed: `java.util.zip.Deflater`/`Inflater` do the same job through the JDK's bundled zlib, which is native and maintained. Verified byte-identical output on the same inputs before removing the dependency, so archives are unchanged. `commons-compress` stays -- it is what decodes BZIP2 sectors, so the P2-6 question the audit wanted decided once is decided: keep. `xz` is now used, by LZMA.
+  - Also removed `ZlibStore`, whose only caller discarded its output. `compress` with no recompression built a zlib stream of stored blocks -- necessarily larger than its input -- so every sector paid for a copy and an Adler-32 to produce something the caller always rejected by its own "did it shrink" test. It returns `null` now and the caller stores raw, byte for byte as before. Its hand-rolled Adler-32 also carried the signed-overflow bug found in P2-3.
 - **P3-3 Delete dead/duplicated code.** Commented-out `loadDefaultListFile` and attributes block; near-identical sector loops `extractCompressedBlock` vs `extractImplodedBlock` (MpqFile.java:94,150); triple keygen duplication (P1-4); `Either` union class → sealed interface or two-field record; unused `DefaultListfile.txt` decision (resource shipped but load path commented out).
+  - Done. `LinkedIdentityHashMap`, `GrowingBuffer`, `Either` and `ZlibStore` are gone; the commented-out listfile and attributes blocks with them. `DefaultListfile.txt` was decided rather than left: it is test-only, so it moved to `src/test/resources` and stopped adding a megabyte to the published jar.
 - **P3-4 Java 25 modernisation.** Records for `Block`/header/bucket models, sealed types where useful, **pattern matching for switch + record patterns** (final since 21) for compression dispatch and per-version header handling, `SequencedCollection` for the ordered file maps, FFM `MemorySegment`/`Arena` in the read layer (P1-3). Replace the `ThreadLocal` `STORE_BUFFER` in CompressionUtil with per-call allocation or a `ScopedValue`. The Vector API stays **out** (still incubator in 25 — not acceptable for a library). Set toolchain and `options.release` to 25.
+  - Done across the phases: records for the header, entry and options models, sealed `Content` hierarchy, pattern-matched switches for compression dispatch, `SequencedMap` for the ordered name maps, FFM `MemorySegment`/`Arena` in the read layer, and the `ThreadLocal` scratch buffer gone. Toolchain and `options.release` are 25. The Vector API stays out, as the audit requires.
 - **P3-5 Naming/typos sweep.** `getAllVaildBlocks`, "Invaild block position", `DegugHelperTests`, `FLAG_LMZA`, javadoc stubs like "the fc" / "the b" / auto-generated noise. Full javadoc on the new public API.
+  - Done. `getAllVaildBlocks` is deprecated in favour of `getAllValidBlocks`, kept for binary compatibility; `DegugHelperTests` renamed; `FLAG_LMZA` survives only as prose describing the old bug. Full javadoc on the new public API.
 
 ## Phase 4 — Tests & CI
 
 - **P4-1 Golden-file round-trip suite.** For each supported version/flag combo: StormLib-generated fixture → open → extract-all → compare hashes; rebuild → reopen with both jmpq3 and (in CI, optionally) StormLib CLI → compare. Today `testRebuild`/`testRecompressBuild` assert nothing (MpqTests.java:161,215).
 - **P4-2 Fix test infrastructure.** `getResource().getFile()` breaks on paths with spaces and inside jars — copy resources to a temp dir via streams. Tests currently mutate files inside `build/resources` and litter `out/` in the working dir. Remove `System.out.println`.
 - **P4-3a Bound decompression output allocations** *(found during Phase 0 self-review)*. Header and table validation now rejects implausible table geometry, and the write path no longer preallocates from a header-supplied archive size. One vector remains: a block may declare an arbitrary `normalSize`, and each decompressor allocates its expected output size up front, so a small crafted archive can still force large per-sector allocations. Fixing it properly means having the codecs grow their output instead of preallocating, which wants the Jazzer fuzz harness to validate it — hence grouped here with P4-3 rather than done blind in Phase 0.
+- **P4-3b ADPCM/Huffman verification — done** *(found while auditing issue [#11](https://github.com/inwc3/JMPQ3/issues/11))*. The fixture turned out to already exist: `wavTest.w3x` carries a 144 KB wav whose sectors are `0x41`, Huffman over ADPCM mono. What was missing was a second opinion, so `tools/mpqcodecs.py` implements both codecs from StormLib's C, and both implementations agree byte for byte. PKWARE remains the one codec the reference cannot read; JMPQ3's writer never emits it, so round-trip coverage is unaffected.
 - **P4-3 New coverage needed.** Concurrency (two archives in parallel — pins P0-2), locale API, encrypted-file round-trip (incl. ADJUSTED key), sector CRC, v2–v4 fixtures, malformed-archive rejection (pins P0-8), empty file, file > one sector exactly at boundary, listfile ordering determinism.
 - **P4-4 CI.** Only `gradle-publish.yml` exists (runs on release). Add a build+test workflow on push/PR (Windows + Linux matrix — path handling differs), publish jacoco report. Consider migrating TestNG → JUnit 5 while tests are being reworked (low priority, do only if touching most tests anyway).
 
 ## Phase 5 — Docs & packaging
 
 - **P5-1 README.** There is none. Cover: what/why, quick-start for new API, migration table `JMpqEditor` → new API, supported format matrix (read/write per version/feature), thread-safety statement.
+  - Done. Covers the format and feature matrix, the new API, integrity, thread safety and the limitations. `ReadmeExampleTests` compiles and runs every snippet in it, because the previous readme had drifted into claiming sparse and bzip2 were unsupported and that `(attributes)` could not be generated.
 - **P5-2 Publishing coordinates.** `group 'systems.crigges'` vs publication `groupId 'inwc3'` inconsistency; version bump to 2.0.0 with the new API; verify jitpack.yml still matches the Java 25 toolchain.
+  - Done. `group` and the publication `groupId` agree on `org.inwc3`, the version is `2.0.0-SNAPSHOT`, and `jitpack.yml` documents why `openjdk17` is still correct there: it only has to run Gradle, and the Java 25 toolchain is resolved by foojay.
 - **P5-3 Format notes doc.** Short `docs/mpq-format-notes.md` recording the spec interpretations chosen (with StormLib source references) — this is what makes it a *reference* library.
+  - Done, 15 sections with StormLib citations.
 - **P5-4 CLI tool — optional** *(issue [#10](https://github.com/inwc3/JMPQ3/issues/10))*. Small separate module/jar (list/extract/insert/rebuild, listfile + input/output dir options) wrapping the new API, per the consensus in the issue thread to keep it out of the library artifact. Do last; drop if time-constrained.
 
 ---