Phase 2: format completeness (#54)

Author: Frotty

.github/workflows/build.yml

@@ -46,6 +46,7 @@ jobs:
           python tools/mpqref.py verify build/roundtrip/archives --manifest build/roundtrip/expected.tsv
           python tools/mpqref.py verify build/roundtrip-newcore/archives --manifest build/roundtrip-newcore/expected.tsv
           python tools/mpqref.py verify build/stored-encrypted/archives --manifest build/stored-encrypted/expected.tsv
+          python tools/mpqref.py verify build/phase2/archives --manifest build/phase2/expected.tsv
 
       # Catches drift between the committed golden manifest and the fixtures.
       - name: Check the golden manifest is up to date

AUDIT.md

@@ -79,12 +79,21 @@ These are real bugs or hazards in behaviour that must not be carried into the ne
 ## Phase 2 — Format completeness
 
 - **P2-1 Polish v0/v1 read+write.** Correct v1 header round-trip (hi-word hash/block positions, 64-bit `archiveSize` handling), `Block.filePos` kept as `long` end-to-end (today `getFilePos()` truncates to int — BlockTable.java:88), user-data header (`MPQ\x1B`) parsed into a model instead of skipped (JMpqEditor.java:516 TODO), header-search alignment verified against StormLib.
+  - Done. `Block.filePos` is `long` throughout and the truncating `getFilePos()` is deprecated in favour of `getFilePosition()`; the user data header is modelled as `MpqUserData`, with its payload readable rather than discarded.
 - **P2-2 v2–v4 read support.** 64-bit table offsets, hash/block table hi-word arrays, **HET/BET tables** (encrypted + compressed variants), v4 MD5 validation of header/tables, compressed block/hash tables. Acceptance: open and fully extract StormLib-generated v2, v3, v4 archives byte-identically.
+  - Done via the classic tables: 64-bit and hi-word table offsets, the hi-block table (`MAKE_OFFSET64`, and unlike the other tables neither encrypted nor compressed), compressed hash/block tables detected from the version 3 stored-length fields, and the version 3 MD5 digests checked and reported through `MpqArchive.integrity()`. See format notes 12 and 13.
+  - **Not verified against real fixtures.** The acceptance criterion asks for StormLib-generated v2/v3/v4 archives; there are none in the repository and no StormLib build available to produce them. What exists is exercised with synthetic fixtures, which is weaker evidence and should not be read as the criterion being met. Generating fixtures with `smpq`/StormLib is the outstanding work.
+- **P2-2a HET/BET tables -- deferred** *(split out of P2-2)*. Not implemented. Two reasons, in order of weight. First, it is not needed for the acceptance criterion above: StormLib writes classic hash and block tables alongside HET/BET for v2-v4 archives, so extraction goes through the path that now works, and HET/BET is only *required* for an archive that omits the classic tables. Second, writing it blind is the failure mode this project keeps hitting -- an implementation derived from the spec, verified against a fixture built from the same reading of that spec, proves only self-consistency. Format note 9 is a worked example of that trap costing real correctness. Do this once a StormLib-generated fixture exists.
 - **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.
 - **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.
+  - Done. The plausibility test now applies to every candidate -- archive headers and user-data redirects alike -- rather than only in `FORCE_V0` mode, and the first candidate is kept as a fallback, so the scan can only ever find a header where the old one found one, never fewer.
 - **P2-6 Complete decompression matrix.** Add BZIP2, SPARSE, LZMA (xz dep is already on the classpath and unused), and correct multi-compression ordering (ADPCM+Huffman path exists; verify against StormLib order). Compression write side stays deflate (+ zopfli option), but the sector-type byte handling must be table-driven per spec.
+  - Done in Phase 0: dispatch is table-driven off StormLib's `dcmp_table`, and format note 2 records why `0x12` cannot be tested as a mask.
 
 ## Phase 3 — Code hygiene & dependencies
 

docs/migration-2.0.md

@@ -97,9 +97,45 @@ replaced by the version's real size and the archive opens with
 `header().malformed()` set, matching what Storm.dll does. 1.x refused unless you
 passed `FORCE_V0`.
 
-**Hi-block tables are refused.** Archives placing file data beyond 4 GiB are
-rejected explicitly rather than misread. Support arrives with the version 2 to 4
-read work.
+**Hi-block tables are read.** Archives placing file data beyond 4 GiB have their
+file positions extended from the hi-block table, as StormLib does. A hi-block
+table that falls outside the file is ignored and the archive flagged malformed,
+rather than refused.
+
+**Sector checksums are verified by default.** Where an archive records an
+Adler-32 per sector, a mismatch now fails the read instead of returning bytes
+known to be wrong. 1.x ignored the checksums entirely. If you would rather
+recover what is still intact from a damaged archive, turn it off:
+
+```java
+MpqArchive.open(path, MpqOpenOptions.defaults()
+    .withSectorChecksumVerification(false));
+```
+
+**Attributes are parsed properly.** `archive.attributes()` returns a
+`MpqAttributes` honouring the file's own bytemask, so archives carrying MD5
+digests or patch bits are read rather than misread. The 1.x `AttributesFile`
+assumed one fixed layout and reported one entry fewer than the file held.
+
+## Recording metadata on write
+
+Both are opt-in, because both change the bytes of every file written and
+neither is needed for a valid archive. Warcraft III wants neither; StormLib
+normally writes both.
+
+```java
+MpqWriteOptions.defaults()
+    .withSectorChecksums(true)                     // Adler-32 per sector
+    .withAttributes(true)                          // generate (attributes)
+    .withAttributesTimestamp(buildTimestampMillis) // pin it, or the build is not reproducible
+```
+
+Two things to know. Generating `(attributes)` requires a CRC32 over each file's
+decoded content, so it forces a decode of files that would otherwise have been
+copied verbatim — enabling it costs real time on a large rebuild. And supplying
+your own `(attributes)` while asking for generation is refused rather than
+producing two entries under one name; supplying it alone stays legal, which is
+how you preserved it before generation existed.
 
 ## Protection tooling