Stream XLSX and DOCX extraction with truncation reporting - #20
Merged
Merged
Conversation
Phase A — extract options. extractAttachment takes an optional second argument carrying maxOutputChars and trailer, and every 'extracted' result now reports `truncated`. The cap is plumbed through handler.extract() rather than read from the module constant, so the streaming handlers are written against their final signature. maxOutputChars clamps to MAX_OUTPUT_CHARS and may only tighten; the trailer sits outside cap accounting, so a truncated result may exceed the cap by the trailer's length. Omitting both options reproduces 0.3.0 behaviour. Phase B — .xlsx streams. workbook.xlsx.load() materialised every cell of every sheet as a live object before any cap could apply, which is what made a 4 MB in-cap workbook peak at hundreds of MB and OOM a 1024 MB worker. It is replaced by exceljs's stream.xlsx.WorkbookReader, stopping early on both the output cap and the handler deadline. That reader spools a worksheet to a temp file whenever the worksheet entry arrives before xl/sharedStrings.xml and xl/_rels/workbook.xml.rels, and awaits the spool while the zip stream is paused — which halts the stream and drops every later entry (exceljs #2790, #3064, #2147; all open, no release since 4.4.0). reorderForStreaming rewrites the entry order in memory so the reader never takes that path, injecting an empty shared-string table for workbooks that have none. A seen < expected backstop turns any other cause of a missing worksheet into 'failed' rather than a partial workbook reported as 'extracted'. The rewrite is also what keeps the decompression budget binding on this format: unzipper walks local file headers while the budget measures the central directory, so only the rebuilt archive is guaranteed to hold exactly the measured entries. An archive that cannot be rebuilt fails rather than being streamed as it arrived. Peak RSS on a workbook at the MAX_UNCOMPRESSED_BYTES ceiling, at concurrency 2 under a 1024 MB heap cap: 954 MB -> 192 MB. Output is byte-identical to load() across 18 workbooks, including date number formats and formula results.
Comment-only. Cuts restatement, narrative build-up, and explanations of what the adjacent line plainly does, keeping every load-bearing fact: the measured drop rates (~34%, 35/50), the 300-read verification, the upstream references (exceljs #2790/#3064/#2147, none since 4.4.0), the node_modules citations (workbook-reader.js:110, worksheet-reader.js:21, unzipper/lib/parse.js:51), and the reasoning that binds the decompression budget to unzipper through reorderForStreaming. Also corrects resolveCap's comment, which claimed a negative cap falls back to the ceiling. It clamps to 0, as its own test asserts. 197 -> 141 comment lines in attachment.ts, 117 -> 90 in the tests. Executable code is byte-identical to the previous commit.
The .docx handler is about to stop using mammoth.extractRawText and read word/document.xml with a streaming SAX parser instead. This is the harness that says the output did not change, landed BEFORE the swap so it is a real baseline: a fidelity suite that first appears alongside a new implementation proves nothing about either. Seventeen real Word documents vendored from mammoth 1.12.0's test/test-data, plus the three textutil-generated fixtures the suite already carries. Copied rather than read out of node_modules: the corpus and its expected output have to be visible in a diff for "no change" to be checkable, mammoth declares no `files` field so test/ ships by accident rather than by contract, and mammoth is about to become a devDependency kept solely as this oracle. Each case pins one golden from two sides. The snapshot records what we produce; mammoth is then asserted to produce the same string. A regression in our reader fails the snapshot, and `vitest -u` — which would launder that regression by rewriting the snapshot — then fails the mammoth assertion, because mammoth still produces the old value. Verified by injecting a trim into the current handler and running with -u: 14 of the 20 documents failed on the mammoth side with the snapshot already rewritten. The only way to change behaviour is an ACCEPTED_DIVERGENCES entry carrying a written reason, which is a reviewed, committed edit. That table is empty today. The oracle is imported at the top of the file with no skipIf. Guarding it would disarm the interlock silently, and green, in exactly the case it exists for: with mammoth absent the snapshot is pinned only by the code that generated it. Comparison is paragraph-array first, then exact string. vitest diffs the array element-wise, so a lost break reads as one changed line rather than a wall of escaped text; the string assertion then catches the trailing whitespace the split swallows.
mammoth.extractRawText built an xmldom tree and a document model over the
whole of word/document.xml before any cap could apply, then let the central
trim throw almost all of it away. Measured through dist/ at a 1024 MB heap on
a 45 MB document.xml inside a 3.65 MB archive — every gate cleared, nothing
skipped:
mammoth this
concurrency 1 812 ms / 607 MB 68 ms / 175 MB
concurrency 2 1552 ms / 970 MB 59 ms / 164 MB
concurrency 4 3512 ms / 1217 MB 58 ms / 168 MB
It built 43,420,044 characters every time and kept 250,000. Peak tracked the
document and multiplied by concurrency; this is flat, because the cap now
stops the read rather than trimming the result. The sharpest case is not the
big archive: a 0.41 MB attachment holding one 43 MB w:t peaked mammoth at
1270 MB against 199 MB here. That is the .xlsx OOM one format over, reachable
from half a megabyte of email attachment.
No new zip dependency. zipEntries already walks the central directory and
hands back each entry's still-compressed subarray, and zlib is already
imported and already stream-inflates one in inflateCounting, so the handler
locates word/document.xml and inflates exactly that region of exactly this
buffer. The only addition is saxes, which is already hoisted as a deduped
transitive of exceljs and word-extractor, so it costs nothing to install.
The output contract is mammoth's, reproduced rather than invented, and the
shape that makes it reproducible is a WHITELIST: an element mammoth has no
handler for has its entire subtree dropped, not recursed into. "Emit every
w:t, newline on </w:p>" over-extracts on any document with a text box or a
field. Adopting the whitelist also deletes work — mammoth's 20-entry ignore
list and its emit-nothing leaves are all just the default, and w:sdt keeping
only w:sdtContent and mc:AlternateContent keeping only mc:Fallback fall out
for free because w:sdtPr and mc:Choice are simply absent from the list.
Verified byte-identical to mammoth across all 20 documents of the fidelity
corpus on the first run, with no snapshot churn and an empty
ACCEPTED_DIVERGENCES table. That includes strict-format.docx (the ISO-strict
namespace URI, which a literal w: prefix match would silently return nothing
for), text-box.docx (mc:Fallback selection and w:pict hoisting, where the
picture's text has to land after the paragraph's break rather than inside
it), tables.docx and utf8-bom.docx.
Two things the port needs that the old reader got for free. Text is decoded
across inflate chunks with StringDecoder, since a chunk boundary lands
mid-sequence in any document with a non-ASCII character and a per-chunk
toString would emit U+FFFD pairs. And NEL/U+2028 are normalised to \n on the
way out: xmldom did that to the whole source before mammoth saw it, while
saxes does only the XML-standard \r\n and \r.
Malformed XML now stops the read rather than being recovered. saxes is
conformant where xmldom salvaged, so a document the old reader finished can
end short here; text already extracted is returned with truncated set, and
only a read that produced nothing stays a failure. Deliberately not a saxes
error handler that parses on — measured, that emits close-tag text as content
and descends into elements mammoth drops, which is silent wrong output. To
keep the common case off that path entirely, the OOXML prefixes are pre-bound
via additionalNamespaces: real producers emit stray o:/w10:/wps: markup
undeclared, and an in-document xmlns still shadows the pre-binding.
The comment rewrites are part of this commit because they would otherwise be
actively wrong. DECOMPRESSION BUDGET no longer has two foreign readers to
reason about: both OOXML formats now read through our own zipEntries walk, so
Invariant 2 becomes the proof that budget-ok implies zipEntries-ok for both,
and Invariant 1 is demoted to a structural check that no longer prevents
anything. findEocd's last-match choice is now internal rather than a match for
jszip's. The budget is also noted as knowingly stale: it measures every entry
while the handler inflates one, and on real documents word/document.xml is
2-28% of the archive. Loosening it is a permissiveness change that belongs
with the input caps, not smuggled in behind a parser swap.
The fidelity corpus proves the reader agrees with mammoth on real Word documents, but real Word documents do not carry every construct: across all 20 of them there is no w:tab, no w:br, no w:cr, no tracked change, no field, no w:sdt and no w:hyperlink, and none comes near the output cap or the deadline. This is what the corpus cannot reach. Every claim these tests pin was measured against mammoth, not read off the spec. Running all 22 synthetic constructs through both readers: 21 byte- identical, including w:pict hoisting (the picture's text lands after the paragraph's break, not inside it), the deleted paragraph mark (no break, text merges into the next paragraph), the deleted table row, w:instrText suppression across a complete complex field, the w:fldSimple subtree drop, CDATA vanishing, and NEL/U+2028 normalisation. The one divergence is w:sym, which is the declared one: mammoth maps it through dingbat-to-unicode and we drop it, since that table would be the only reason to keep any of mammoth's tree and the only place this reader would read an attribute at all. The load-bearing test is that an unrecognised element is dropped WITH its children rather than recursed into. A reader written the obvious way passes every other test in this file and still over-extracts on any document with a text box or a field. Three mutations confirm the tests are not vacuous: removing the NEL/U+2028 normalisation fails exactly one test, making unknown elements recurse fails exactly one test, and emitting one newline per paragraph instead of two fails 34 of 53. The first two failing precisely one test each is the point — each regression is caught by its own test rather than incidentally by another. Two contracts here are pinned specifically because they look like defects someone would later fix: a w:pict outside any paragraph is silently dropped, and a table flattens to one paragraph per cell with no tabs or row markers, so a 2x2 table is byte-identical to four consecutive paragraphs. Both are mammoth's behaviour and both are now stated in the README as limits.
Replacing a lenient DOM parser with a conformant streaming one moves the malformedness line in BOTH directions, so this measures where it lands rather than leaving it to be discovered. Against mammoth on twelve cases: eight agree, two we are stricter on, two we are more available on. Stricter: a raw U+000B in a w:t, and the same character written as . Neither is a legal XML 1.0 character; xmldom passed them through and saxes refuses. Both test cases put it in the FIRST text node, so nothing had been read and the result is `failed` — the same character further into a document keeps everything before it. Recorded as a deliberate coverage loss. More available: trailing junk after the root, and an unclosed root. mammoth threw away the whole document for both; we return the text read before the fault with truncated set. The policy itself is stated once at the top of the file: text read then a broken parse is `extracted` + `truncated`, nothing read is `failed`. What it deliberately is not is a saxes error handler that reports and parses on — measured, that recovery emits close-tag text as content and descends into elements mammoth drops, which is silent wrong output. The billion-laughs case asserts on TIME as much as status: saxes never parses entity declarations, its table is the five predefined ones on a null prototype and nothing writes to it, so the expansion is not representable and the reference is simply undefined. A parser that did expand it would not return at all. The fuzz is 1000 seeded iterations across two archives — a deflated one that mostly exercises the zip walk and preflight, and a stored one that puts the mutated bytes straight in front of saxes. Coverage measured rather than assumed: 516 of 1000 reach `extracted`, so the parser genuinely runs; the stored half alone reaches it 357 of 500 times; failure reasons span the whole pipeline, 352 from the zip walk, 21 from the handler's own w:body guard, 19 from routing. The property is not that the text is good but that the degraded state is always reachable, always labeled, always inside the deadline, and never the empty string — which is what makes shipping without a fallback reader safe. Two archives never reach the handler at all, pinned so that stays true: a renamed main part is unroutable because ooxmlKind requires the literal word/document.xml entry name (this is the one place we read the conventional path where mammoth resolved _rels/.rels, and routing already declined such a package before this change), and a corrupt deflate stream is caught by the preflight, which proves the handler's own inflate-error path is unreachable-but-safe.
Nothing in the shipped source calls mammoth any more — verified against the built output, where the 31 remaining occurrences in dist/attachment.js are all preserved comments and there is no require. It stays as the fidelity oracle, which is a test-time job, so it belongs in devDependencies. Deliberately NOT kept as a runtime fallback. A fallback would fire precisely on malformed input — the most likely to be adversarial — and hand it to the reader measured at 1270 MB on a 0.41 MB attachment, re-arming the exact hazard this change exists to remove. It would also forfeit the dependency win, keep an XML parser with CVE history loaded, and make "which reader ran" unobservable from the result. The contract already has a safe degraded state: `failed` with a reason, which a caller can see and retry. Ten packages leave the runtime tree, 3.61 MB: mammoth, underscore, @xmldom/xmldom, dingbat-to-unicode, argparse, xmlbuilder, lop, sprintf-js, duck, option. jszip, bluebird, base64-js and path-is-absolute stay, since exceljs and word-extractor need them regardless. Net dependency change for the package is one added (saxes, already hoisted as a deduped transitive of both exceljs and word-extractor, so it installs nothing new) and one moved. The README's resource-limits section claimed .docx was post-materialization with peak memory following the whole document, and listed it among the handlers that cannot honour the deadline. Both are now false. It gains the measured before/after table, and two residual bounds are stated rather than glossed: peak is not independent of INPUT size, since the API takes a Buffer, and saxes buffers one text node whole, so a single enormous run still costs about twice its own size. It also gains a scope bullet, because the port inherits limits that are easy to assume away: footnote, endnote and comment bodies are not extracted (they are separate zip parts), headers and footers are never opened, and a table flattens to one paragraph per cell — a 2x2 table is byte-identical to four consecutive paragraphs. All of that matched the previous reader exactly, so none of it is a regression, but a consumer reading an invoice should know the column a figure sat in is gone. Fidelity across everything available: 41 documents, 41 byte-identical, 0 divergences, 0 errors. That is the 17 vendored Word documents, the 3 repo fixtures, 8 real-world Word-authored templates, 10 textutil-dialect documents covering tables, lists, unicode and 200-paragraph bodies, and 3 more from disk. No saxes error was raised on any of them, which is the evidence that mattered most for shipping without a fallback.
OOXML_ASSUMED_PREFIXES was a hand-enumerated allowlist, and saxes fails the
whole parse on a prefix outside it — for ATTRIBUTES as well as elements
(saxes.js:1920-1925). Under the error policy that becomes `extracted` +
`truncated: true`, so everything after the offending element is silently gone.
That is not an exotic shape. Word 2013 and later stamp w15:paraId on EVERY
w:p, with w16cid:durableId beside it, so a document carrying those without
their xmlns — a fragment assembled by a templating tool, a repaired file —
lost everything after its first paragraph. Measured before the fix:
<w:p w15:paraId="12AB34CD"> ours "before\n\n" mammoth the whole document
<w:p w16cid:durableId="99"> ours "before\n\n" mammoth the whole document
<wne:acd/> ours "before\n\n" mammoth "before\n\nafter\n\n"
<w:p w14:paraId="1"> ours the whole document (w14 was in the map)
The w14 row is the tell: the allowlist was the entire difference. So was the
comment above the constant, which named wne: as a real-world stray prefix
while the map omitted it.
An allowlist cannot close this — there is always another prefix. resolvePrefix
closes the class: anything unclaimed resolves to a sentinel URI that
OOXML_PREFIXES does not map, so the element names {urn:agentextract:unbound}
local, misses the whitelist, and its subtree is dropped. That is exactly what
mammoth does with an unmapped namespace (xml/reader.js:53-66 produces the same
{uri}local shape, and no handler matches it), so this is the port getting
closer to its reference, not further.
saxes consults it last, after scope and after additionalNamespaces
(saxes.js:1845-1862), which is what lets the two compose. The map shrinks to
the three prefixes that still do something an allowlist has to do: w, mc and v
are the only URIs OOXML_PREFIXES maps, so an undeclared `w:` resolved to the
sentinel would drop the whole document body. The other eight entries resolved
to URIs this reader does not map either way, so guessing them and sentinelling
them were always the same outcome — they were maintenance with no behaviour
attached.
Fidelity is unmoved: 385 existing tests green with no snapshot churn, and the
41-document corpus still 41 byte-identical, 0 divergences, 0 errors. Six new
tests, and removing resolvePrefix fails five of them — including one that was
not written for this at all: the w:hyperlink container test uses r:id, an
undeclared prefix that had been passing only because `r` happened to sit in
the old allowlist. It now passes through the general mechanism.
The row named "an unbound namespace prefix" built its fixture with the offending element after `</w:document>`, so it was measuring trailing junk after the root — duplicating the case directly above it and passing for a reason unrelated to its name. It survived the resolvePrefix change for the same reason, which is how it surfaced. Replaced with the case that was actually missing: the same illegal character those two `failed` rows use, moved off the first text node. It returns "first\n\nsecond\n\n" with truncated set — everything before the fault, nothing after. That is what makes "stricter than mammoth" a bounded loss rather than a cliff, and until now it was asserted only in prose. Undeclared prefixes are covered in docx-streaming.test.ts, where they belong: they no longer break the parse, so the interesting assertion is that the whole document comes back.
A handler that stopped on its own deadline could never say so. `deadline` was `Date.now() + HANDLER_TIMEOUT_MS` — the same instant `withTimeout` was set to reject at — and the checks are `Date.now() > deadline`, so the earliest a handler could break was deadline + 1ms, after the timer had already fired. Ten seconds of successfully read pages came back as `failed`. The CPU-containment half worked (the parse really did stop); the reporting half was unreachable. HANDLER_DEADLINE_MARGIN_MS (1s) is the window a handler has to finish the unit in flight and return. It shrinks the handler's budget rather than extending the timer, so HANDLER_TIMEOUT_MS stays the outer bound the API side sizes EXTRACT_BUDGET_MS against. The existing deadline tests stub Date.now() and leave setTimeout real, which decouples the two clocks — structurally unable to catch this. The new test drives both off one synthetic clock, which is the real relationship, and fails on the old arithmetic with 'failed' instead of 'extracted'. Fake timers keep it at 5ms rather than a ~9s wall-clock test.
Streaming keyed order, names and membership off ZIP layout, where
workbook.xlsx.load() resolved xl/workbook.xml's <sheets> list through the
rels. Three divergences from main followed, all measured on a 3-sheet
workbook: a dragged tab came back in file order, an absolute rel Target
("/xl/worksheets/sheet1.xml") matched none of exceljs's single expected
spelling so every sheet came back "SheetN", and an orphan sheet7.xml no
<sheet> references was emitted as a sheet of its own. Cell content was
complete in all three; what differed was ordering, naming and membership.
reorderForStreaming now reads the two parts itself and becomes the one
authority: worksheets are laid out in tab order, unreferenced parts are
dropped from the rebuild, and it reports the sheets it wrote so the
handler names them positionally instead of asking the reader. Two things
the four-line summary of this fix does not cover:
- <sheets> also names chartsheets and dialogsheets, which have no
xl/worksheets part. Resolved targets are filtered to worksheets, or the
lost-worksheet backstop would false-fire on a legal workbook — the
reason that count used to be taken off the archive.
- exceljs dispatches worksheets on an UNANCHORED regex, so it treats
sheet1.xml.bak as one where WORKSHEET_PART does not. Harmless while
names came off the reader; with positional naming a single unresolved
emission shifts every later name. The rebuild drops every dispatchable
part it did not resolve, making emitted === laid out, and the backstop
moves from `<` to `!==` to cover the other direction.
Rel normalization handles absolute and '..' targets; it lives in our
resolver rather than rewriting the rels bytes, so nothing depends on
exceljs's matching any more. A workbook whose sheet list cannot be read
falls back to the archive's parts in entry order — measured byte-identical
to the previous behaviour, so no input that works today becomes a failure.
Also splits reorderForStreaming's two refusals: the 0xffff entry-count
bail reported "central directory could not be read" for a directory that
read perfectly well, sending an investigation at the wrong half of the
preflight.
Verified byte-identical on all 17 workbooks in the eval corpus. The three
new fixtures each reproduce their symptom against the previous code.
Two shapes were never byte-identical. load() proxied a merged cell's master value into every slave, so a horizontal merge repeated the label per column and a vertical one emitted trailing rows carrying nothing else; an error-valued formula stringified to "[object Object]". Streaming emits the merge once and the error cell as empty. Both read as improvements for a search index, so the behaviour stays. But merged workbooks do change row and column shape against main, so the comment says that instead of claiming identity, and both diffs get a test — an unpinned improvement is indistinguishable from an accident.
The accepting half measured 3660ms against vitest's 5000ms default in a full-suite run — 73% of budget. It failed a first clean run on another machine at 5.117s and passed on rerun, so the failure is load-dependent rather than a real regression. The fixture size is what the pair proves (65534 refuses, 65533 is accepted, so the refusal is provably the sentinel and not the size), so a cheaper fixture would cost the test its point. Explicit timeout instead.
exceljs 4.4.0 yields one Row per iteration, not an array —
worksheet-reader.js:275 pushes {eventType: 'row', value: row} and
:104-112 yields each value through, confirmed empirically as
['Row','Row','Row','Row','Row'].
The Array.isArray normalization stays: exceljs documents the batched
shape, which is why the inverted reading was believable in the first
place, and one predicate covers either.
zipEntryNames delegated to zipEntries, which bails on ZIP64 compSize/localOffset sentinels, a bad or missing local header, and data running past EOF. None of that matters for NAMING an entry, so format identification was inheriting refusals it has no stake in and falling back to the raw byte scan — the "fooled by storage order" path the entry walk exists to beat. It also seeked every local header and allocated a ZipEntry plus a subarray per entry, once or twice per extraction, before reorderForStreaming walked again. Names-only walk restored for identification; zipEntries stays the strict one for the rewrite, where the local header and compressed region are the point. Same findEocd, same stepping, so the two cannot disagree about which records exist.
resolveCap(-Infinity) returned the full 250k via the non-finite fallback while resolveCap(-5) returned 0 — the same request landing in opposite places. Gating the fallback on NaN instead sends both infinities through the clamp, so the normalizer is monotonic across its domain. No safety change: the function can only ever tighten, and +Infinity clamps to MAX_OUTPUT_CHARS the same as before. -Infinity was also the one input the cap tests did not cover, so it is covered now.
Both are the class that change was meant to close. 1. Duplicate entry names amplified the rebuild past the budget. Zip permits duplicate entry names, so a name is not a key — but sheet layout resolved through a name -> entry lookup, mapping every reference onto the FIRST entry carrying that name and re-emitting its bytes once per reference. archiveWorksheets had no dedup, so an archive of 21 entries all named xl/worksheets/sheet1.xml turned a measured 1.5 MB into a 31 MB rebuild: 21x, on a 111 KB input, all of it inside the existing gates. Scaled, a 5 MB attachment forces a 0.8 GB allocation — an OOM the API side cannot contain, since it is not a JS throw and its per-attachment catch never sees it. WorkbookSheet now carries the resolved ZipEntry instead of a part name, so the rebuild writes objects it was handed and duplication is unrepresentable rather than guarded against. 2. A declared-but-unresolvable sheet was silently deleted. Skipping a <sheet> whose relationship did not resolve removed it from the resolved list, and the rebuild drops every worksheet part not on that list — so the text vanished. Invisibly, because the backstop compares `seen` against that same list: both sides moved together and it could never fire. Reachable via a missing relationship, a percent-encoded Target, an empty name, or a '..' escape. Membership is now the archive's, not the workbook's. An unclaimed part is still dropped as an orphan when every declaration was placed — that is the original ask — but if anything went unplaced, orphan and victim are no longer distinguishable, so every unclaimed part ships after the named ones. It loses its tab position, never its rows. Targets are also percent-decoded, which narrows the trigger independently. The budget invariant stated only the drop direction, which is why the amplification read as impossible from the code's own reasoning. It now states all three: no measured record written twice, some deliberately not written, and one 153-byte literal of ours that was never measured. Corpus output stays identical to 847bfdd across all 31 fixtures. Each new test fails against 4a30ec8 — 31.4 MB vs a 1.5 MB budget, and two sheets where three were declared.
All three are the mistake the two before them were: reading "we could not account for this" as "there was nothing there", and reporting the result as a clean `extracted`. 1. A Target resolving outside xl/worksheets went uncounted. The branch exists for chartsheets, which legitimately have no worksheet part — but it also caught a Target that resolved to nothing at all, so the declaration was never counted unplaced, the real part stayed unclaimed, and the orphan rule deleted it. Reached by a mis-cased Target (OPC part names compare case-insensitively) or one written absolute without the xl/ prefix. It now stays uncounted only when the archive holds what we resolved to. 2. The rescue excluded entries by name. Layout stopped keying on names in 9066c53, but the rescue still filtered on them, so where two entries shared a claimed worksheet name the one nobody laid out was discarded along with the one that was — the same "a zip name is not a key" bug, surviving on the recovery path. `claimed` splits in two: `claimedNames`, which is what makes a part an orphan, and `placed`, which is what makes an entry redundant. An entry that is referenced but never placed is neither. 3. An empty declaration list was honoured as an answer. Returning [] rather than undefined skipped the archive fallback, so a <sheets> we could not read a single declaration out of dropped every worksheet part the archive held — the whole document, as an `extracted` carrying no text at all. A workbook that declares chartsheets and nothing else still returns [], so genuine orphans still drop. Each new test fails against 9066c53. The README stated the old rule as a guarantee; it now states the real one, and the invariant behind all five fixes: a sheet may lose its position or its name, never its rows.
Three defects, one root cause: each check keyed on something the PRODUCER writes — a target's path, an element's bare local name, a metadata part's size — rather than on what the format states. Two cost rows, silently, under a clean `extracted`. 1. A relationship's type was discarded, so classification ran on the target's path. ff40a18 replaced "does the part exist" with "does it exist and look like a chart sheet", but a Target is a string the producer chose, so the whitelist could be walked into: a Type=worksheet relationship redirected onto a planted xl/chartsheets/sheet1.xml read as an accounted-for chart sheet, and the worksheet part that really held the rows was left unclaimed for the orphan rule to drop. parseWorkbookParts now carries {target, type}, and NON_WORKSHEET_REL matches the type. Everything that is not a chartsheet or dialogsheet type is expected to be a worksheet, so an unknown or absent type is unplaced rather than waved through — the default inverts from permissive to safe. Macrosheets stay out deliberately: unplaced costs a rescued orphan, accounted-for could cost rows. 2. Both metadata parts were inflated whole and then copied into strings. One part may spend the entire 50 MB archive budget, and this pays it twice — as a Buffer, then as the UTF-16 string the parse needs. A 46 KB attachment whose workbook.xml inflates to 42 MB cost heap +89 MB and RSS 319 MB, against +49 MB and 205 MB before this resolver existed, on the path whose measurements claim 24 concurrent parses inside 662 MB. Now bounded at 4 MB through zlib's maxOutputLength, so the allocation never happens rather than being caught after: +48 MB and 216 MB. Over the cap degrades to archiveWorksheets, never fails. The comment justifying the old behaviour claimed exceljs holds these parts whole anyway. It does not — workbook-reader.js:157-166 streams both through saxes — so this was a memory class the read did not previously have. 3. Elements matched on bare local name, so a foreign one could impersonate a sheet. A <foo:sheet> planted in an <extLst> claimed a real sheet's part before the genuine declaration reached it, returning that sheet under the injected name and tab position. Both parsers now run xmlns and match on (namespace, local name), with a parent stack so <sheet> counts only as a child of <sheets> — which also closes the <sheet>-anywhere gap left open since the sheet-identity change. The cost: saxes with xmlns on treats an undeclared prefix as fatal. Contained — the caller already catches parse throws and degrades to archiveWorksheets, and exceljs's own parse of those bytes fails too — but a sloppy workbook that kept its tab names now falls back to Sheet1, Sheet2, …. All 31 corpus fixtures are unaffected. Each new test fails against ff40a18. The earlier cases were re-checked and hold: the four unplaceable-Target shapes, the empty <sheets/> fallback, the duplicate-name rescue, the chartsheet/orphan control, and the amplification bound at 3.1 MB against a measured 3.1 MB.
The previous commit moved classification off the target's path and onto the relationship type. That closed the path half and opened the mirror of it: a type is no more trustworthy alone than a path was. 1. A non-worksheet type was accepted without resolving its target. Setting sheet 2's relationship to Type=chartsheet while leaving Target=worksheets/sheet2.xml passed it as an accounted-for chart sheet, and the worksheet part that held the rows was dropped as an orphan — S1 and S3 only, status extracted, truncated false. A non-worksheet declaration now counts as resolved only when its target resolves to a part this archive holds that is NOT a worksheet. Neither signal is trusted alone, because each has now been wrong. 2. Only Transitional OOXML was recognized. ISO/IEC 29500 Strict re-homes the vocabulary under purl.oclc.org and is a legal .xlsx — Excel writes it as "Strict Open XML Workbook". No <sheet> matched, so a Strict workbook lost tab order and names to the archive fallback. Both namespaces are accepted for the spreadsheetml and officeDocument-relationships vocabularies. The .rels grammar is OPC, which Strict does not re-home; relationship TYPE values do move, and NON_WORKSHEET_REL already matched on the suffix. 3. Part URIs were compared case-sensitively. OPC requires ASCII case-insensitive equivalence, so a legal Target="Worksheets/sheet2.xml" names the entry stored as xl/worksheets/sheet2.xml. Matching case-sensitively made it unresolvable: the rescue kept the rows, but the sheet lost its name and tab position. Only the producer's target is folded — the archive side stays case-sensitive, because exceljs's dispatch regex is, and an entry it will never yield must not be laid out as though it would. 4. The 0xffff refusal counted entries the rewrite then dropped. The check ran on the pre-drop list while the EOCD is written from the post-drop one, so an archive rewriting to well under the sentinel could still be refused as "would rewrite to 65535 entries". It now counts what it is about to write. Each new test fails against e82bef6. The full set of earlier cases was re-swept and holds: every unplaceable-target shape, the empty <sheets/> fallback, the duplicate-name rescue, the chartsheet/orphan control, the foreign-element rejection, the metadata cap at +48 MB against a base of +49 MB, and the amplification bound at 3.1 MB against a measured 3.1 MB.
The pair check landed one notch too loose: a chartsheet-typed relationship counted as accounted-for whenever its target resolved to any present part that was not a worksheet. Pointed at xl/styles.xml it passed, and the worksheet part actually holding the rows was left unclaimed for the orphan rule to drop — S1 and S3 only, status extracted, truncated false. Base 847bfdd returns all three, so this was this branch's to fix. Type and target must now name the same KIND: a chartsheet relationship has to resolve to a present part under xl/chartsheets/, a dialogsheet one under xl/dialogsheets/. Every looser rule has failed the same way — path alone, type alone, and "not a worksheet" each let a contradiction through, and each cost the same rows. A path convention is load-bearing here where it could not be for the worksheet case, because the asymmetry inverted. A chart sheet stored somewhere unconventional now reads as unplaced, which costs a rescued orphan in the output. Guessing the other way costs rows.
Four cleanups, no functional change — every PoC and all 342 tests give
byte-identical results.
- `unplaced` was only ever tested `> 0`, so it is a boolean now.
- The duplicate-name story appeared in full twice, at WorkbookSheet and
in the DECOMPRESSION BUDGET block. The budget block is where the bound
is argued, so the interface comment states the invariant and points
there.
- The non-worksheet classification comment enumerated each rule that had
been tried and failed. The invariant is that neither the type nor the
target path is trustworthy alone, since both are producer-controlled;
which specific rules were wrong is in the history and in one fixture
per disagreement.
- Tests: `workbookWith` was duplicated verbatim across two adjacent
describes that assert on the same fixture convention, so it is hoisted
above both. The hand-rolled CRC-32 table is replaced by zlib.crc32,
already used elsewhere in the file.
Deliberately not done, both proposed and both declined for the same
reason — they would reopen what has already been reviewed and verified,
for no behavioural gain:
- Extracting a shared readCentralRecord from the three central-directory
walks. Haakam verified those bail-by-bail ("every zipEntries bail has a
budget counterpart"), and that verification is against their current
shape.
- Replacing path-based classification with a Content_Types-driven part
index and canonical worksheet renaming. That is the fix for
case-variant and arbitrarily-named worksheet parts, which base 847bfdd
loses identically — pre-existing on main, not this branch's, and it
reworks the rebuild the budget argument rests on.
NON_WORKSHEET_REL matched any URI ending /relationships/chartsheet, so https://invalid.example/relationships/chartsheet passed as authoritative. With a planted xl/chartsheets/fake.xml to point at, the target agreed and only the type's origin was wrong — the declaration read as an accounted-for chart sheet and the worksheet part holding the rows was dropped as an orphan. Base 847bfdd returns all three sheets, so this was this branch's to fix. A relationship type is an exact identifier the format defines. The regex becomes a map of the four exact URIs — chartsheet and dialogsheet, in both Transitional and Strict spellings — onto the part family each must resolve within. Membership of that map is the whole authority, so an unrecognized type is expected to be a worksheet and reaches the rescue rather than bypassing it. Every earlier version of this check trusted a pattern rather than an identity, and each cost the same rows: the target's path, then the type alone, then any non-worksheet target, then any type merely spelled like one. Fixture per disagreement, four now.
…n' into implement-streaming-extraction
This was referenced Aug 28, 2026
Closed
josephfeleke
approved these changes
Sep 2, 2026
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
3 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.
Summary
Supersedes #14 and #15 by integrating both stacked branches onto current
main, including every fix from the #14 review and resolving #15 against the final XLSX implementation.maxOutputCharsandtrailerextraction options plus programmatictruncatedreportingword/document.xmlthroughzlib+saxes, with corpus-verified Mammoth raw-text fidelity0.4.0Verification
npm run buildnpm test— 433 passingnpm auditandnpm audit --omit=dev— the accepted moderate UUID advisory described below; no other advisoriesexceljs@4.4.0 → uuid@8.3.2andhtml-to-text@10.0.1 → deepmerge-ts@8.0.2, matching the published manifest rather than root-only overridesnpm pack --dry-run—agentextract@0.4.0package builds successfully--max-old-space-size=1024, bothextractedwithtruncated: trueand 250,000 characters; peak RSS about 188 MiBFollow-up boundary
The API-side Phase D remains a separate
agentmail-apichange after0.4.0is published: bump the dependency from^0.3.0, pass the cap/trailer, carry{ text, truncated }, and writetruncatedinto companion-object metadata. The current API main branch does not yet contain that wiring.Consolidated-review follow-up
Commit
058740caddresses all six technical findings from the consolidated review:saxesnamespace resolution can become quadraticCommit
0b22743addresses all six findings from the follow-up review:The archive-wide decompression preflight remains outside the handler timer by design. It is bounded by the input, entry-count and inflate ceilings, and that boundary is now explicit in the README.
Commit
f5bd61eaddresses the final packaging and fidelity review:overrides, raiseshtml-to-textto^10.0.1, and refreshes the honest consumer graph to patcheddeepmerge-ts@8.0.2uuid@8.3.2advisory from ExcelJS; the advisory affects UUID v3/v5/v6 buffer writes, while this read-only path never calls themw:body, matching Mammoth on malformed siblingsVersion
0.4.0is deliberate: for a pre-1.0 package, the minor bump communicates the documented XLSX output changes more clearly than a0.3.xpatch while remaining the correct additive release for the new options/result field.