feat(archive-codec): write conformant [MS-CFB] compound files - #863
Merged
Conversation
Mearman
marked this pull request as ready for review
September 3, 2026 09:20
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Mearman
force-pushed
the
worktree-agent-a08a61dc31ae0de62
branch
2 times, most recently
from
September 3, 2026 09:38
8843f58 to
eca7409
Compare
writeCompoundFile takes the CompoundFileStream array readCompoundFile returns and emits a compound file from it, so re-writing what was read is a round trip rather than a translation between two vocabularies. It writes the header, the FAT, the DIFAT in both its spellings (the header's own 109-entry array and chained DIFAT sectors past that), the directory, and both allocation paths: a stream at or above the 4096-byte cutoff takes FAT-chained sectors, one below it a run of 64-byte mini sectors in the root entry's mini stream. Two parts are load-bearing beyond "a reader can parse it". The directory's sibling trees are genuine red-black trees rather than the right-sibling chain a purely structural reader would also accept. The sibling tree exists to be binary-searched by name, so a chain is not a smaller version of it, it is the wrong data structure. Splitting each sorted sibling list at its midpoint puts nodes at depths 0..D for D = floor(log2 n) and empty positions no shallower than floor(log2(n+1)), so colouring exactly the depth-D nodes red gives every root-to-leaf path D + 1 black nodes with no two reds adjacent and a black root -- every [MS-CFB] 2.6.4 constraint by construction, with no rebalancing pass. Sibling identity is the format's own ordering, not string equality: shorter names sort first, equal-length names compare by simple-uppercased UTF-16 code point. That makes 'Table' and 'TABLE' one name, which is why the same comparator decides both the sort and the uniqueness check. Chained DIFAT sectors are written rather than a size limit imposed, because the header array alone caps a version 3 file at 6.875 MB and a real .doc or .xls passes that routinely. The one ceiling that does throw is the version 3 per-stream limit of 0x80000000 bytes, past which the 64-bit size field would need a high half the spec forbids there. Because the directory's order is the format's name ordering rather than the caller's, the output depends only on the set of paths: two callers building the same file from differently ordered lists get identical bytes.
Adds cfb/write to the module table, documents what writeCompoundFile accepts and refuses, and records the two decisions a caller cannot see from the signature: the sibling trees are real red-black trees because the tree exists to be binary-searched, and the output is a function of the path set alone rather than of the order they were supplied in. Also restates CompoundFileStream's own comment as the package's compound-file vocabulary in both directions rather than the reader's alone, since the writer now takes the same shape.
… them Three places described the package as CFB-read-only, which stopped being true when writeCompoundFile landed: - the root package table's archive-codec row; - COMPARISON.md, where SheetJS's cfb was the closest counterpart partly because it wrote and archive-codec did not, which is no longer the distinction between them; - xls-codec's remaining-scope note, which listed a compound-file writer among the things #815 still has to build. The container half exists now, so what is left there is the BIFF8 record emission.
Mearman
force-pushed
the
worktree-agent-a08a61dc31ae0de62
branch
from
September 3, 2026 09:55
eca7409 to
360b588
Compare
Contributor
|
🎉 This PR is included in version 1.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
1 participant
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.
archive-codeccould read a classic OLE compound file but not produce one, which is what blocks write support inxls-codec(#815),doc-codec(#816), andppt-codec(#817): a.xlswriter emitting aWorkbookstream, or a.docwriter emittingWordDocumentand1Table, needs a container to put them in. This addswriteCompoundFileas the mirror ofreadCompoundFile— it takes the sameCompoundFileStream[], sowriteCompoundFile(readCompoundFile(bytes))is a round trip rather than a translation. That symmetry is also why nested storages are supported even though none of the four codecs needs one: the reader emits slash-joined paths, so a writer that refused them couldn't re-write what its own package had just read.Covers the header, the FAT, the DIFAT in both spellings (the header's 109-entry array and chained DIFAT sectors past it), the directory, and both allocation paths — FAT-chained sectors at or above the 4096-byte cutoff, 64-byte mini sectors below it.
Two things that aren't obvious from the signature
The directory's sibling trees are genuine red-black trees, not the right-sibling chain a purely structural reader would also accept. The tree exists to be binary-searched by name, so a chain isn't a smaller version of it, it's the wrong structure. Splitting each sorted sibling list at its midpoint puts nodes at depths 0..D for D = floor(log2 n) and empty positions no shallower than floor(log2(n+1)), so colouring exactly the depth-D nodes red satisfies every [MS-CFB] 2.6.4 constraint by construction — no rebalancing pass. The tests assert the invariants including black height, rather than taking the argument on trust.
Sibling identity is the format's ordering, not string equality (shorter names first, then simple-uppercased UTF-16 code point), so
TableandTABLEare one name and the same comparator decides both the sort and the uniqueness check.Validation
The round trip through this package's own reader is the bulk of the suite, but that only proves internal consistency, so it was checked against parsers that share no code with it:
DEFECT_INCORRECTmode accepts every fixture (minimal, empty, mixed mini+FAT, nested storages, 63 siblings, an 8 MiB DIFAT-chained file, version 4) and returns byte-identical stream content. Confirmed non-vacuous: deliberately corrupting the sector shift, mini-sector shift, cutoff, reserved bytes, or a sibling link makes it reject..doc(6 streams, control-prefixed names like\x01CompObjand\x05SummaryInformation) read throughreadCompoundFileand re-emitted throughwriteCompoundFile: every stream hash matches,file(1)identifies the result identically to the original, and LibreOffice Writer opens the rewritten file and reads the text back correctly.Scope and limits
.docor.xlspasses routinely.0x80000000bytes throw, since the 64-bit size field would need a high half the spec forbids there. This is the one validation branch with no test, because exercising it means allocating 2 GB.MAXREGSECTsector ceiling isn't checked: at ~2 TB it's unreachable inside a singleUint8Array, whose own allocation limit fails loudly first.Tests
37 new cases in
src/cfb/write.test.ts: header and sector layout asserted byte-for-byte against the spec's field tables (so a wrong offset fails here rather than surviving because our own reader makes the same mistake), round trips across mini/FAT/cutoff-boundary/ragged/zero-length/empty-file/nested/multi-directory-sector/multi-FAT-sector/DIFAT-chain/version-4 cases, red-black invariants parsed out of the directory directly, and the validation failures. Plus barrel, workerd, and smoke coverage for both directions.lint,typecheck(incl.attw),test,test:workers, andtest:smokeall pass, andturbo run _lint _typecheck _test --filter=...archive-codecis green across all 43 tasks, so the four consumers are unregressed.Infrastructure for #815, #816, and #817 rather than a fix for any one of them, so no closing keyword.