Phase 1: new core API - #52
Merged
Merged
Conversation
The --names path was fixed for this; the archive-internal parser still used line.strip() as its truth test, so a name made entirely of whitespace was dropped. That left the oracle inconsistent with the library, which keeps such names, and no fixture exercises one.
First slice of the new core in systems.crigges.jmpq. Nothing uses it yet, so the existing API and tests are untouched. MpqSource is the single bounds-checked, little-endian read model (P1-3). A file-backed source maps into an Arena-scoped MemorySegment, which fixes the problem that forced the old design's hand: mapped byte buffers unmap only when the collector gets round to them, so on Windows the archive stayed locked after close() and the rebuild had to stage through a temp file. A test deletes the archive immediately after closing the source, which fails outright on Windows if a mapping is still live. Offsets are long, so a v2+ archive past 2 GiB needs no chunking, and out-of-range reads report JMpqException rather than IndexOutOfBoundsException because they mean a damaged archive. MpqHeader parses all four versions into an immutable record with the field layout documented against StormLib's TMPQHeader, verified from source rather than memory. It repairs instead of rejecting, following ConvertMpqHeaderToFormat4: a wrong declared header size is replaced by the version's real size and the archive is flagged malformed, which is what makes the protected maps of issue #46 readable and is groundwork for P2-5a. The same applies to a high byte in wSectorSize and to a block table larger than the file, which is clamped as StormLib clamps it. Every value that could drive an allocation is bounded at parse time: hash entries against StormLib's HASH_TABLE_SIZE_MAX, block entries against what the file holds, the sector shift against what keeps 512 << shift positive, and the declared archive size against the file, for every archive rather than only in legacy mode. Package is systems.crigges.jmpq, the audit's first suggestion, so the package carries no version number; the deprecated adapters will stay in systems.crigges.jmpq3. JMpqException is reused from there deliberately rather than duplicated, since it is what consumers already catch.
|
coverage: 77.234% (+0.03%) from 77.204% — phase1-core into master |
MpqArchive is now a working read-only archive, verified against the independent reference rather than against its predecessor. - MpqFileEntry replaces handing callers a raw block table row. A Block is a mutable row that does not know its own name; this is an immutable value object carrying name, locale, flags and sizes, so callers stop correlating the two tables themselves. Sector-offset-table presence is derived from the flags rather than from a sector count of 1. - MpqFileReader decodes a file straight out of the mapped segment, a sector at a time, so decoding never materialises the compressed form on the heap as the old path did. - MpqArchive opens from a Path, byte array or channel, enumerates via the list file, looks up by name and locale, and decodes whole or streaming. It never writes: producing a modified archive will be the writer's job with an explicit save, so that a read can no longer rewrite the file as a side effect of close(). - MpqOpenOptions carries forceV0 and the preferred locale, which starts P1-5. MpqOpenOptions.warcraft3() is the mode these fixtures need. The crypto, compression, hash table and list file code is reused rather than reimplemented: it was hardened in Phase 0 and a parallel copy would only be a chance to diverge. Verification, from the test report rather than by assertion alone: 171 files match the independent reference, every one it can decode, and 199 files match the existing implementation, which additionally covers the PKWARE, Huffman and ADPCM files the reference deliberately does not implement. A shared misreading of those codecs would show up as a divergence between the two. Self-review before pushing caught one thing the tests could not: the block table's byte count was computed as int * int, so a 2 GiB archive describing enough entries would overflow to a negative length and fail complaining about the wrong thing.
Frotty
marked this pull request as ready for review
August 20, 2026 18:11
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e6a605cd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The crigges namespace names a member who is no longer active; the code should carry the collective's name. Package root is org.inwc3, so it is a reverse-domain form that Maven Central would accept, unlike a bare inwc3 or a com.github.* group. This commit moves only the new core, which nothing depends on yet, so it is a rename with no behavioural surface. Also fixes the group and publication groupId, which Phase 0 had reconciled onto systems.crigges. The legacy implementation still sits in systems.crigges.jmpq3 and moves in the commit that introduces the compatibility adapter, since that is where the split between shim and implementation is actually decided. The end state, per the maintainer's call: everything real under org.inwc3, and systems.crigges.jmpq3 retained purely as deprecated shims so JMpqEditor stays importable and WurstScript compiles unchanged.
MpqArchiveWriter builds an archive in memory and writes it when told to. Saving is explicit (P1-2). The pre-2.0 JMpqEditor rebuilt the archive as a side effect of close(), so a missed flag or a throw on the way out could rewrite a file that was only meant to be read. Reading is MpqArchive, writing is MpqArchiveWriter, and the write happens where the caller asks for it. Format version is chosen, not inherited (P1-6). MpqWriteOptions accepts 0 or 1 and rejects the rest at construction, rather than emitting a 208-byte header carrying 32 bytes of meaning as the old writer did for a version 2 source. Verbatim copies are conditional on sector size. A file keeps its stored bytes only when the target archive keeps the source's sector size, because a sector offset table is expressed in that sector size. This is the bug the golden harness found in the old recompression path, encoded as a rule the writer applies rather than a case it can forget. MpqWriteOptions carries the P1-8 extension points: an explicit hash table capacity and extra unused block slots, enough for protection tooling to emit a maximised table without that being a concern of this library. Also listfile and prefix policy, since a Warcraft III map stops loading if its 512-byte prefix is dropped. save(Path) stages to a sibling file and moves it into place, so an interrupted save cannot leave a half-written archive where a working one used to be. It cannot target an archive that is still open: a mapped file cannot be replaced on Windows. The first version of this claimed otherwise and the test caught it, so the javadoc now spells out the build-then-write pattern instead. Verification: the writer preserves 179 files across all fixtures on the copy path, and the same 179 when re-encoding into a different sector size, which is the case the old writer corrupted. Plus reproducible output, explicit format selection, table capacity control, prefix handling, atomic save, and rejection of the internal names it generates. Self-review before pushing: block table sizing was int * int, and unlike the hash table its capacity is caller-influenced through extraBlockEntries, so it could overflow.
The golden harness already confirmed that something other than this library can read what the old writer produced. The new writer now gets the same treatment: the test exports its output plus the digests each archive should hold, and CI has tools/mpqref.py verify them. Result is 179 files across 10 archives with 0 skipped and 0 failures. Zero skipped is the part that matters: re-encoding leaves every sector as deflate or stored, which is exactly what the reference implements, so nothing is waved through as unverifiable.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 624579c7b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review finding, and a real corruption bug rather than a regression: it is present in master too. An encrypted file with no compression flag still occupies fixed-size sectors, and each is encrypted with its own key + index. The missing compression flag removes the sector offset table, not the sectors. StormLib decrypts inside the per-sector loop in ReadMpqSectors whatever the compression flags say; those only decide whether an offset table is consulted, and the whole-block decrypt at line 290 is the SINGLE_UNIT path, which is separate. Both implementations decrypted such a file as one block, so sector 0 decoded and every sector after it was returned as garbage. Fixed in the new core and in the compat layer. No shipped fixture has a file of this shape, which is how it survived a golden-file suite that checks 199 files against two other implementations, so the archive is built by hand in the test. Verified the test actually catches it by reintroducing the bug and watching it fail. The reference tool had the identical bug and would have confirmed the wrong answer, which is the second time a shared misconception has slipped past it. Fixed there too, and CI now has it verify the hand-built archives, so agreement between the two is real evidence. Also from the same review: an uncompressed SINGLE_UNIT entry now checks that its stored size equals its declared size, the invariant the stored path already enforced. Without it a damaged block table silently yielded the wrong number of bytes from an API that promises an exception.
MpqArchive.entries() resolves one locale per name, so an archive with several localised variants of a path misreports the others as locale 0 with no name, which also makes encrypted variants unreadable through the returned entry. Real, but unreachable with any current fixture, and fixing it needs the hash table to expose all its mappings rather than one lookup per name. Recorded against P1-5 so it is fixed where a multi-locale fixture can be built to prove it, rather than done blind on the strength of a P2.
Review finding, P1: a rebuild silently dropped every localised variant of a path but one, and relabelled the survivor as neutral. MPQ identifies a file by path and locale, but the writer keyed pending files on path alone and registered them all with locale 0. The same review raised a related P2 on the read side, where entries() named blocks by looking each path up once, so the other variants came back with no name and locale 0 -- misstating the locale and leaving an encrypted variant unreadable through the entry, because the key derives from the name. Both needed the same missing capability, so this does the P1-5 locale work rather than deferring twice. HashTable can now report all its live mappings, which is the only way to discover the variants of a path: a lookup resolves one by the format's preference order and cannot report the rest. On that footing: - MpqArchive.entries() correlates every mapping, so each block carries its real locale and every named variant keeps its name. - MpqArchive.localesOf(name) reports the locales a path is stored under. - The writer keys pending files on path and locale, carries every variant across in from(), and registers each under its own locale. put and remove gained locale-aware overloads; the neutral-locale forms are unchanged, so existing calls behave as before. - The rebuilt list file still names each path once, whatever its locale count, because a list file names paths rather than variants. Neither defect was reachable with any shipped fixture: every one is entirely neutral, which is how both survived a suite checking hundreds of files against two other implementations. The new test builds a three-locale archive and covers read, enumerate, rebuild and locale-aware removal. Master has the same write-side behaviour, so this is a defect rather than a regression.
Member
Author
|
All four findings addressed in
New tests build the two fixture shapes that were missing: a three-locale archive, and an encrypted uncompressed multi-sector file. |
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cfb60995f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review findings on the previous commit. The P1 is the same defect I had just fixed, in the sibling I did not check: the verbatim-copy path decrypted a stored encrypted file as one block, so it wrote a correct first sector and corrupt ones after it. It is worse there than on the read path, because the writer then clears the encryption flags, making the corruption permanent in the rebuilt archive rather than a bad read of intact data. Present in both the new core and the compat layer's copy path. This is the fifth time in this branch that fixing the instance I was shown left a sibling behind, so this time I enumerated every decryption site in both cores first. That turned up exactly the two remaining ones, both distinguishing SINGLE_UNIT from stored incorrectly. Archives carrying a hi-block table are now refused instead of misread. A hi-block table holds the upper 16 bits of each file position for archives whose data passes 4 GiB; the header was parsed but never applied, so a read would seek to the wrong place. Supporting it belongs with the v2-v4 read work, and refusing is honest in the meantime. Tests cover the rebuild path for both cores. The hand-built fixture now carries a list file, because without one there is nothing for a writer to carry over and the copy path is never reached; the first version of the test passed for that reason rather than because the code was right.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 1 of
AUDIT.md: the new core API, read and write, in a neworg.inwc3.jmpqpackage. The existingJMpqEditorAPI is untouched and its tests are green, so nothing downstream changes yet.Decisions the audit delegated
org.inwc3as the package root, replacingsystems.crigges. A reverse-domain form Maven Central would accept, unlike a bareinwc3orcom.github.inwc3. The artifactgroupand publicationgroupIdmoved too.systems.crigges.jmpq3is retained as the compatibility layer.JMpqEditorstays importable at its current fully-qualified name so WurstScript compiles unchanged; it becomes a deprecated adapter over this core in a follow-up.The new core
MpqSourceMemorySegment(P1-3)MpqHeaderMpqFileEntryBlockexposure (P1-2)MpqArchiveMpqArchiveWritersave(P1-2)MpqOpenOptions/MpqWriteOptionsFFM read layer. File-backed sources map into an
Arena-scopedMemorySegment, so the mapping is released deterministically. That is what forced the old design's hand: mapped byte buffers unmap only when the collector gets round to them, so on Windows the archive stayed locked afterclose()and the rebuild had to stage through a temp file. Three tests delete the archive immediately after closing, which fails outright on Windows if a mapping is live. Offsets arelong, and out-of-range reads raiseJMpqExceptionrather thanIndexOutOfBoundsException.Saving is explicit. The old
JMpqEditorrebuilt the archive as a side effect ofclose(), so a missed flag or a throw on the way out could rewrite a file that was only meant to be read. Reading isMpqArchive, writing isMpqArchiveWriter, and the write happens where the caller asks for it.Format version is chosen, not inherited (P1-6).
MpqWriteOptionsaccepts 0 or 1 and rejects the rest at construction, rather than emitting a 208-byte header carrying 32 bytes of meaning as the old writer did for a version 2 source.Verbatim copies are conditional on sector size. A file keeps its stored bytes only when the target keeps the source's sector size, because a sector offset table is expressed in that sector size. This is the corruption the golden harness found in the old recompression path, now a rule the writer applies rather than a case it can forget.
Header repair, not rejection. Following StormLib's
ConvertMpqHeaderToFormat4: a garbage header size becomes the version's real size and the archive is flaggedmalformed.listfileTooLong.w3xandspazzledMap.w3xparse, which is groundwork for P2-5a (issue #46).Verification
Numbers from the test report, not asserted in prose:
That last row is the strongest of them: zero skipped means every sector the writer emitted was decodable by an implementation that shares no code with this one. CI runs it on both platforms.
Also covered: streaming equals whole-file read; reproducible output; explicit format selection; forced table capacity; prefix preservation; atomic save; rejection of internal names the writer generates; and
putcopying its input.Remaining in Phase 1 (follow-up PR)
JMpqEditorreduced to a deprecated adapter over this core, which is also when the legacy implementation moves out ofsystems.crigges.jmpq3and leaves only shims; behaviour contracts documented and tested (P1-7); retiring thew3pbranch against the new extension hooks (P1-8).One known limitation is recorded as P4-3a in
AUDIT.md: a block may declare anynormalSizeand the codecs allocate their expected output up front. Fixing it means changing how the codecs produce output, which wants the Jazzer fuzz harness to validate, so it belongs with the Phase 4 coverage work.