Repository navigation
Conversation
A scanned PDF, a zero-byte file and a whitespace-only file all returned the
identical result object — {status:'extracted', truncated:false} with no
extraction — so "we found no text" and "there is no text" were the same answer.
A consumer could not decide whether OCR was worth trying, which meant the one
outcome most worth acting on was the one the API could not express.
`emptyReason` is now set on exactly the results that carry no text, and never on
the ones that do, so a successful result always has exactly one of `extraction`
and `emptyReason`:
- 'no-text-layer' — the document HAS content, none of it text. OCR is the next
step. Only a handler that can PROVE this may report it, and only PDF can today:
pages were visited and not one yielded a text item. Deliberately requires a
page we actually read, so a zero-page file and a deadline stop draw no
conclusion.
- 'no-text-content' — read, and genuinely holds nothing. The honest default for
every format that cannot tell the two apart; it claims no more than it knows.
Also stops calling a password-protected Office file 'unrecognized attachment'.
Office wraps an encrypted package in an OLE container rather than a zip, so
routing saw OLE where it expected PK. The bytes were unrecognized; the file was
not, and a caller who knows a document is merely locked can go ask its sender
instead of treating the attachment as unreadable. Detected by the OLE directory's
UTF-16LE EncryptedPackage stream name, bounded to the head like every other
sniff, and pinned by a test that an ordinary .doc is NOT relabeled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Footnotes, endnotes, comments, headers and footers each live in their own zip part, so reading only the body dropped every one of them silently — a footnote carrying the substance of a clause, a header carrying a confidentiality marking — with nothing in the result to say anything was missing. One corpus fixture extracted to nothing at all: footnote-hyperlink.docx's entire content is a hyperlink inside a footnote. All five parts carry the same paragraph/run vocabulary as the body and differ only in the element that contains it, so the existing reader reads them with one new parameter: a DocxPartShape naming the root, the container, its depth, and whether it repeats. The main-document shape is written exactly as it was hard-coded, so the body path is unchanged rather than re-derived — the whole fidelity corpus is byte-identical except the four documents that have auxiliary content, each now an ACCEPTED_DIVERGENCES entry recording what mammoth still produces. Decisions worth naming: - ORDER IS THE CAP'S PRIORITY ORDER, not reading order, which cannot be reconstructed: a footnote's reference is inline while its body is elsewhere, and a header is repeated per section rather than positioned once. Body, then notes, then margins — so a document that runs into MAX_OUTPUT_CHARS loses page furniture before a footnote, and a footnote before a paragraph. - THE BODY IS LOAD-BEARING, THE REST IS BEST-EFFORT. A malformed footnotes part costs its own text and sets `truncated`; it does not fail the extraction. Trading a readable document for a fragment over its margins would be worse than the behaviour this replaces. - Separator and continuation-separator notes are skipped. Word writes them into every footnotes part to draw the rule above the note area; emitting them puts a blank paragraph into every document that has one footnote. - A header repeated across sections is emitted once. Word writes up to three parts per section, usually identical, and repeating a letterhead spends the cap. - Parts are concatenated, not joined with a separator: the reader already ends every paragraph with one. Matched by conventional path, as the main part already is. A part missed that way costs its own text and nothing else, unlike the main part. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
25 distinct reason strings became 10 codes. The strings mostly interpolated
values the caller already had — its own byte count, its own content type,
constants this package exports — so they read like diagnostics while carrying
nothing a consumer could act on without a regex. This suite was the proof: it
matched `reason` with two dozen patterns, because that was the only way to
assert which refusal had happened.
`${byteSize} bytes exceeds ${MAX_INPUT_BYTES}` -> 'too-large'
`unsupported type ${type}` -> 'unsupported-format'
`decompresses to over ${cap} bytes` -> 'expands-too-large'
`handler exceeded ${ms}ms` -> 'timed-out'
Handlers now throw ExtractionFailure carrying a code rather than prose, so the
entry point reads a value instead of turning a message into a reason.
Two codes exist for reasons beyond tidiness. 'timed-out' is the only retryable
failure, and was previously reachable only by matching the timeout's message
against HANDLER_TIMEOUT_MS. 'internal' separates our invariant tripping or a
pinned dependency moving (`yielded 2 of 3 worksheets`, `no _parseStyles;
expected the 4.4.0 internals`) from the file being at fault — routine failures
and a paging-worthy one were previously the same `failed` with a different
sentence.
TWO COSTS, both accepted rather than hidden:
- Seven `malformed zip: ...` variants collapse into 'malformed', so six tests
that used the prose to prove WHICH guard fired now only prove that one did.
A consumer's action is identical for all seven ("broken, do not retry"), and
the public contract is what the field is for.
- An unrecognized throw is attributed to the bytes, so a genuine bug of ours
escaping into the entry point is reported as 'malformed' and blames the file.
The message used to name it. Our own invariant sites throw 'internal'
explicitly; what is lost is the throws we did not anticipate, which were never
branchable either way. Noted at failureReason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Haakam21
changed the title
All six reproduced before fixing, and every regression test here was confirmed
to fail against the unfixed code — two of my first attempts passed either way
and were rewritten with deterministic clock mocks rather than kept.
.docx auxiliary parts:
- ONE BROKEN PART DROPPED ALL LATER ONES. The catch set the same `truncated`
flag the loop breaks on, so an unreadable footnotes.xml also cost the
endnotes, comments, headers and footers. The comment beside it claimed a
broken part "costs its own text and nothing else" — the intent, not the
behaviour. Now a separate `partFailed` flag: it still surfaces as truncation,
because text IS missing, but it no longer ends the walk.
- A PART'S TRUNCATION FLAG COULD BE LOST. `continue` for empty text ran before
`read.truncated` was read, so a part the deadline cut off before it emitted
anything reported the document as complete.
- COMPLETE DOCUMENTS COULD BE REPORTED TRUNCATED. The deadline was checked once
per zip ENTRY, above the name match, so a clock expiring while walking parts
we never read marked a whole document truncated and had the entry point append
a "continues past this point" trailer to it. Checked below the match now.
- Duplicate entry names are resolved to the last record, as the main part
already does, instead of reading and emitting both.
Result contract:
- AN UNREAD DOCUMENT COULD BE REPORTED AS HOLDING NOTHING. `emptyReason` is a
claim about the whole document, and was being set on truncated reads: a PDF
full of text with a zero cap said 'no-text-layer', telling a caller to OCR a
document that has a text layer, and a .docx said 'no-text-content', telling
them to discard one that was never read. Only set on a complete read now; a
truncated result with no text carries neither, which is the honest answer.
- ONE FACT HAD TWO STATUSES. 'unsupported-zip-feature' came back `skipped` from
the budget and `failed` from the rewrite. Status is now derived from the
reason through one total table, so the pairing cannot be got wrong at a
return site.
- LOCKED FILES COULD BE REPORTED 'malformed'. The password check sat inside the
no-handler branch, so an encrypted package named .doc (an OLE container, which
carries the legacy handler's magic) reached word-extractor and came back as
never-retryable. Hoisted above routing, and pdf.js's PasswordException is
mapped too.
Also deleted a test of mine that asserted `REASONS.has('internal')` — a property
of the literal set declared three lines above it, which can never fail. The real
coverage for that code already exists in the ExcelJS-hooks test; a second copy
would have been a worse version of it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second review round on this branch. All eight reproduced first, and each regression test was confirmed to fail against its own unfixed code. The first finding is the one that matters: the previous commit's `partFailed` flag only covered a part that threw, and readPart does NOT throw when a part breaks AFTER producing text — it returns with a flag. Half-readable is the common shape of a broken part, so the original defect survived through the path the fix did not cover. readPart now reports `failed` separately from `truncated`, and only running out of room or time ends the walk. .docx auxiliary parts: - A part that broke partway through its text still stopped every later part. - `remaining <= 0` assumed something was left to lose, so a body of exactly maxOutputChars got a "continues past this point" trailer over parts holding nothing. Word writes empty headers and separator-only footnotes routinely, so this was the ordinary case. Reading at a budget of zero answers the question instead: the reader charges for text it finds, an empty part charges nothing. - continuationNotice, the third structural note type in ECMA-376's ST_FtnEdn, was emitted as a stray blank paragraph — the defect the separator skip fixed. - Headers and footers are read in numeric order, so which one survives a cap is a property of the document rather than of how the archive was written. Classification: - A BLANK PDF WAS REPORTED AS A SCAN. `pagesRead > 0` treated any page without text as proof of image content, so an empty document was sent to OCR. pdf.js names its image operators, so this now asks instead of assuming: on an otherwise-empty read, walk the visited pages until one paints an image. Stops at the first hit, which for a real scan is page one, and only ever runs on a document that produced no text. - ANY ERROR MENTIONING "password" BECAME password-protected. The message match relabelled a broken file as an intact locked one for quoting a field or tag. Matched on pdf.js's exception class alone now. - THE ENCRYPTED-FILE CHECK WAS A RAW BYTE SEARCH, and .doc stores body text in the same UTF-16LE encoding — so a document ABOUT encrypted packages was declined as one. Matches a CFB directory RECORD now: name, byte length at +64 and stream type at +66, at a 128-byte boundary. - DecompressionCheck no longer carries its own status, so REASON_STATUS really is the only place the status/reason pairing lives. Decided rather than fixed: a failed dynamic import is now 'internal' rather than 'malformed'. A parser that could not load never ran, so the attachment is not what is broken. Its test pins the mapping and not the path that produces it — vitest's doMock will not propagate a throwing factory to the importer — and the comment says so rather than implying wider coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third review round on this branch. All ten reproduced first, and each regression test was confirmed to fail against its own unfixed code. Truncation, all three from the same mistake — deciding it from the CAP rather than from text actually lost: - An empty <w:p/> charges 2 characters like any other output, so a header holding one, read at a budget of zero, came back over cap and marked a COMPLETE document truncated. Word writes those routinely. - A duplicate header did the same and was then discarded anyway. Text-level dedupe could not help: with no budget the reader stores nothing, so the copy comes back as '' and reads as new text. Identical parts are now recognized before reading, by stored CRC and size. - readPart merged "out of time" with "out of room". They differ: running out of time means we stopped and cannot know what was left, running out of room means the reader knows whether any of it was text. Split. Also: the body's text is '\n\n' when it holds nothing, so a document whose only text is in a footnote began with a blank paragraph. Dropped when the body has no text of its own. The PDF image probe, added last round, was too eager: - It ran when `truncated` was already set, where its answer is discarded — on a long blank PDF it could spend the rest of the deadline for nothing. - getOperatorList decodes a page's images, which is work this handler did not previously do, and per page it could push a large scan over HANDLER_TIMEOUT_MS — turning a file that used to extract into one that fails. First page only now: a scan's first page is an image, and the cost of being wrong is a blank cover page reported as 'no-text-content', which understates rather than misdirects. Reason codes: - Workbook metadata over our own MAX_METADATA_BYTES was 'wrong-document-shape' (failed). It is our cap declining to materialize an intact file, so it is 'expands-too-large' (skipped); an unreadable stream on the same path is 'malformed'. - The encrypted-file check now seeks the directory the CFB header points at, instead of scanning for records that look like one. Scanning was wrong twice: bounded to 64 KB it missed a real encrypted file whose directory sits later, and unbounded it matched the directory of an OLE object EMBEDDED in another file — so a .doc carrying an encrypted attachment was declined as locked while its own text went unread. EVERY EMPTY REASON IS NOW A PROOF. 'no-text-content' was the default for any handler that could not tell, which told callers "there is nothing here" about image-only documents nobody could read — suppressing OCR exactly where it is wanted. Handlers now report only what they can show: PDF from its image operators, .docx from a w:drawing/w:pict frame, HTML from an <img>, text and .xlsx because they decode everything. .doc reports NEITHER, because word-extractor says nothing about what it could not read, and the absence now means "we cannot tell" instead of a false claim. Cleanup: `corrupt` and `declined` had become one function under two names, now `refuse`. The `error.name === 'ERR_MODULE_NOT_FOUND'` arm could never match — Node puts that on `.code` — and is gone. NOT fixed, and the README no longer claims otherwise: legacy .doc encrypted with the fEncrypted FIB flag carries no EncryptedPackage stream and is still reported 'malformed'. Detecting it means reading the FIB out of the WordDocument stream — real work, and after three rounds of byte-poking heuristics being wrong, not something to bundle into a fix commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourth review round. Nine of ten findings confirmed; five were the same mistake, and it is the mistake this branch's own README warns against: reporting an emptyReason as a proof where the code had only a default. 'no-text-content' means "there is nothing here", so every false one tells a caller to discard a document — and each site below was reached by a document that wanted OCR. The PDF probe, added two rounds ago, was wrong three ways at once because it returned a BOOLEAN and `false` silently became that claim: - Page 1 only. A scan behind a blank cover page reported 'no-text-content'. One page can PROVE an image; it cannot disprove one for a longer document, which now answers "unknown". - The probe not running at all — no OPS, deadline already passed — also reported it. Every scanned PDF would have read as truly empty. - No try/catch around getOperatorList, so a document that used to come back empty could come back FAILED because the thing labelling its emptiness threw. It now returns EmptyReason | undefined, so "cannot tell" is representable. The same over-claim in four more places: - .docx counted any w:drawing, which wraps every DrawingML object — charts, text boxes, a letterhead's decorative shapes. An empty template claimed to be a scan. Only an actual image reference counts now (a:blip / pic:pic / v:imagedata), matched AHEAD of the skip gate because those elements live under wp:inline, a subtree the reader drops — checked where the extraction logic sits, it would never have fired at all. - .docx decided the body's contribution from text EMITTED rather than kept, so a suppressed vMerge cell could put back the leading blank paragraph this branch removed. Taken from the body's own text now. - .xlsx claimed 'no-text-content' while never opening the parts a pasted screenshot lives in. It no longer answers. - HTML decided it with /<img/ over raw markup: matching inside comments, <script> and tracking pixels, and missing <svg>, <picture>, <canvas> and CSS backgrounds. Answering properly needs the DOM this handler does not build, so it no longer answers. Two more, unrelated to emptiness: - looksEncryptedOffice read only the FIRST directory sector. A 512-byte sector holds four entries and a real encrypted package has about ten, so EncryptedPackage usually sits in the second — the previous fix did not work on real files, and the tests missed it by putting the entry at index 1. The directory chain is now walked through the FAT. - The xlsx metadata failure picked between 'expands-too-large' and 'malformed' from the entry's SELF-DECLARED uncompSize, which a hostile header sets freely. inflateEntry reports which failure it actually hit. NOT a defect: finding 10 claimed XLSX_CANONICAL_CONTROL_PARTS is unused. It is live at the control-part canonicalization in reorderForStreaming; deleting it would break the rebuild. SEPARATE PRE-EXISTING BUG, found while checking a test that broke when fixture bytes shifted: a 4-byte character straddling an inflate chunk boundary decodes to U+FFFD at every ODD byte offset of word/document.xml. Reproduced on origin/main, so it is not from this branch, and the existing test passes only because its fixture happens to land on an even offset. Left alone here and reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eight of ten findings hold; two do not reproduce. Every regression test was confirmed to fail against its own unfixed code — the Strict-image one passed on the first attempt and was tightened until it did not. CFB directory walk, three faults in the code that fixed the last one: - `readSector(fatSectors[...] ?? -1)` was meant to end the chain. at(-1) is 0, so it returned the file HEADER and the walk followed header bytes as a FAT into arbitrary sectors. Ends the chain properly now. - Every storage in a compound file shares one directory, so matching an entry anywhere also matched an encrypted object EMBEDDED in a readable document — declining the carrier as locked while its own text went unread. Only the root storage's own children count, walked through its sibling tree. - The child ID is at +76, not +68, which I had wrong until the fixtures caught it. Empty-reason claims, again — the same over-claiming as last round, in places last round did not reach: - DOCX_IMAGE_ELEMENTS listed only Transitional DrawingML while the reader accepts ISO Strict, so an image-only Strict .docx read as holding nothing. - A header or footer logo counted as the document's own picture, so an otherwise empty document on headed paper claimed to be a scan. Body only now. - A PDF page with no image operator was called 'no-text-content'. It is not: tests/fixtures/blank.pdf yields no text items and 564 DRAWING operators, which is what text outlined as vector paths looks like. Only a page that draws nothing at all proves emptiness; anything else is "cannot tell". The test that asserted otherwise was wrong in both of its previous forms. Silent text loss: - A part cut short was compared against earlier headers by its stored PREFIX, so a clipped "ACME Corp\n\nDRAFT..." matched the letterhead before it — dropping the rest AND suppressing the truncation. Reproduced at one exact cap value; a part we could not read to the end is no longer judged a duplicate. - Header dedupe trusted the CRC the archive declares. A writer that defers it to a data descriptor leaves zero there, which made every later header a copy of the first. Only a committed checksum counts. The PDF probe is now raced against the deadline as well as guarded by it: one slow image decode could outlast the time left and have withTimeout reject the whole extraction, turning a document that read fine into 'timed-out' because the thing that LABELS its emptiness was slow. Also: the README claimed exactly one of extraction/emptyReason is present on a complete read, then said four handlers report neither. The second is right. DID NOT REPRODUCE, and left alone rather than changed speculatively: finding 6 (header text after a text-free drawing going unreported) and the first shape of finding 7 — both report `truncated: true` as they stand. The second shape of 7 did reproduce and is fixed above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It produced a defect in every review round it existed for, and always the same one: a value that could not be proven was reported as though it had been. Four rounds, five sites — PDF page-one probes, .docx drawings, .xlsx rows, HTML markup — each time a "cannot tell" collapsed into 'no-text-content', which tells a caller there is nothing to find. The documents that reached those paths were exactly the scanned ones the field was added to rescue, so every bug in it made the library worse at the job it was for. The underlying problem is not any of those sites. It is that "why is this empty" cannot be answered from a text extractor's vantage point: the evidence lives in the parts these readers deliberately do not open — drawing relationships, display lists, the DOM — and the honest answer is unavailable often enough that the field was absent on most results anyway. A tri-state nobody can rely on is worse than no field: it invites branching on a value that is usually missing and was, several times, wrong when present. `extraction` is omitted for an empty read, as it always was, and the README now says plainly that we do not distinguish a scan from a blank page — so a consumer who cares about OCR treats every empty result as a candidate. Removed with it: the pdf.js operator probe and its race, the .docx picture tracking (DOCX_IMAGE_ELEMENTS, sawPicture), the per-handler claims, and the scan.pdf fixture. Net -440 lines. KEPT, because they stand on their own and none of them depended on this: password-protected detection and its CFB directory walk, the .docx auxiliary parts, the reason codes, and every truncation fix from the last three rounds — those are about `truncated`, which is a fact the handlers actually observe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Haakam21
changed the title
All nine hold. The first three lose text; two of them are the same rule broken in a place the earlier fixes did not reach. - A PART STOPPED BEFORE THE CHUNK HOLDING ITS TEXT lost that text silently. `sawText` only covers what was read before the stop, so a header that charges its way past the cap on empty paragraphs in an early chunk reports sawText: false and the result says the document is complete. Reproduced with a 54 KB header. readPart now distinguishes ABANDONING a part mid-read — where what follows is unknown, so any stop counts — from reading it to the end and clipping, where the reader knows whether any of what it clipped was text. That second case is what keeps an empty <w:p/> from marking a document truncated. - THE BODY'S OWN FAILURE ENDED THE WALK. body.failed was folded into `truncated`, which breaks the loop, so a document that broke mid-body lost every footnote, comment and header behind it — the exact rule the comment beside partFailed states. Body failure now feeds partFailed, like any part's. - HEADER DEDUPE TRUSTED THE ARCHIVE'S OWN CRC AND SIZE. Nothing verifies those, so a crafted package could give header2 header1's checksum and have its different text dropped without a word. Compares the stored bytes instead, which is what the parts actually are. Two labels for one fact, and one label for two facts: - inflateEntry called an unsupported compression method 'malformed' while the decompression budget calls the same archive 'unsupported-zip-feature' — different statuses for one file, which REASON_STATUS exists to prevent. - 'expands-too-large' was documented as the MAX_UNCOMPRESSED_BYTES zip-bomb signal, but also reports MAX_METADATA_BYTES, so an ordinary .xlsx with a 5 MB workbook.xml read as a bomb. The code covers both ceilings and now says so. Dead conditions removed: `!duplicate` could never be false where it was tested (a clipped part is never judged a duplicate), and `!read.stoppedForTime` repeated `read.truncated`. The condition is now the two cases above and nothing else. Behaviour for a clipped possible-duplicate is unchanged and still deliberate — we cannot tell, so we report the possible loss. Cleanup: the docx return comment still described emptyReason machinery that "Drop emptyReason" removed; SaxesAttributes was redeclared inline at three sites; `used()` recomputed every section's length per part, quadratic in the number of headers, now a running total; and dedupe fingerprints were recorded for groups that never dedupe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round seven. Six of ten fixed, one does not reproduce, and three are left open on purpose — see below. Fixed: - WORDS MERGED ACROSS PARTS. Sections are concatenated because the reader ends every paragraph with a break, but a part that broke MID-paragraph does not — so a body ending "Body text" followed by a footnote "Note" read as "Body textNote", a word present in neither. A separator now goes BETWEEN sections that need one and never after the last, because the final section may legitimately end unterminated: a document whose last paragraph carries a deleted mark keeps its text without a trailing break, and closing it broke two tests on the first attempt. - INTERNAL FAILURES WERE SWALLOWED by the auxiliary parts' best-effort catch. Best-effort is about the FILE being unreadable; an invariant of ours tripping or a parser failing to load is not, and hiding it as 'extracted' with a truncation flag meant the 'internal' spike the README says should page someone could never fire. - The xlsx metadata reason depended on which part was inspected first, so a corrupt rels alongside an oversized workbook reported 'expands-too-large' — `skipped`, which tells a caller the bytes are fine. 'malformed' wins now. - Header dedupe held every part's compressed bytes as a string; a digest answers the same question at 32 bytes each. - The deadline and cap checks were split under a comment saying they mean different things. They no longer do; merged. - Ordering and duplicate-matching ran for footnotes, endnotes and comments, which only ever have one part each. NOT REPRODUCED: item 4. A header of only empty paragraphs read at a budget of zero returns truncated:false today — the round-six fix covers it. NOT FIXED, and this is the honest part: items 2, 3 and 6 share one root cause the reviewer identified correctly — truncation is judged by "did we see text in this part" rather than "was content actually dropped from the output". The right fix is to measure the dropped characters, and I tried it: counting them where text is appended conflicts with five existing invariants around speculative and deferred text (vertical-merge continuations, deleted-mark stashes, hoisted picture frames), each of which deliberately appends text that may never be output. Making the count agree with charge()'s notion of committed text is a change to the reader's accounting, not to this loop, and it belongs in its own change with its own tests rather than as a seventh patch to a condition. The symptom is a false truncated:true on a complete document, which overstates rather than hides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round eight, and the reviewer's design finding is what unlocked this. Items 1-5
all came from reading a part into WHATEVER BUDGET WAS LEFT, which made the
part's own text depend on how much room happened to be free — and every question
asked afterwards inherited that:
- A duplicate header was compared as its own clipped PREFIX, failed to match the
first, and was emitted as new text. Visible in the output as
"ACME Corp\n\nACME ".
- The truncation flag was set BEFORE the duplicate check, so a copy we were
always going to discard reported the document as continuing past this point.
- "Was anything cut" could only be guessed from whether the part contained text
at all, which is a different question — a footer returning every character it
had still set the flag.
- The separator between sections was never charged, so the joined text could run
past the cap and be cut by the central trim, which then set the flag itself.
Each part is now read with the full cap as its budget and fitted afterwards. All
three questions become facts: the text is the part's own, duplicates compare
exactly, and what is lost is `length` minus `room` — a substring we hold rather
than an inference. The byte-hash dedupe goes with it; it was there to catch
copies before reading and rarely fired, because Word tags each section
differently. Peak is bounded as the body already was: one part at most
maxOutputChars, the ceiling the whole extraction is held to anyway.
Whitespace-only overflow is trimmed here rather than passed up. A footer ending
one paragraph break past the cap has everything a reader would see; handing it
on had the entry point slice it and set `truncated` independently.
This is the restructure I said last round belonged in its own change and then
failed to land — my attempt measured dropped characters inside the reader, which
conflicts with five invariants around speculative and deferred text. Fitting
after a whole read needs none of that.
Cleanups: opcKey ran on every entry once per part family (five times over, each
building a URL) and now runs once; a `refusal()` helper replaces five hand-written
{ status, reason } pairs; docxMainPartChunks is docxPartChunks, since it has read
every auxiliary part since this branch began.
NOT FIXED, and I think correctly so: item 1. A blank 16 KB header exhausts the
cap on paragraph breaks, so the read stops and we never see what follows it.
Reporting truncation there is conservative but honest — "this header held
nothing" is only knowable by reading past the cap, which is the one thing the
cap exists to prevent. It overstates rather than hides.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round nine. Findings 1, 3 and 7 are one problem seen from three sides, and the
reviewer named it: blank content was charged against the budget and treated as
if it were text.
- A part's trailing run of empty paragraphs was charged in full. The empty and
duplicate checks used the TRIMMED text while the untrimmed text was what got
appended and counted, so 30 empty paragraphs in a footnote bought nothing and
pushed a later footer out of the budget entirely. Collapsed to the single
break that ends any part. Auxiliary parts only — the body's exact breaks are
the contract the mammoth fidelity corpus pins.
- Running out of room ENDED THE WALK, so a blank header that exhausted its own
read limit dropped every later part, silently, because a part never read
cannot report what it held. Every part is reached now; the fitting decides,
and a part with text that cannot fit both records the loss and stops the walk,
since nothing after it would fit either.
- Output was tracked three ways — an array, a running count, and a final join
with the separator rule written twice. The two copies had already drifted:
one asked endsWith('\n') where the separator is '\n\n'. One string now, so
the count cannot disagree with the text.
Also: the sort built a fresh collator on EVERY comparison. Measured here at
146 ms against 6 ms shared for 20k keys, and it runs before any deadline check —
a crafted package of tiny header entries would block the event loop with
withTimeout unable to fire. Built once.
And readPart's own catch turned our failures into ordinary truncation once any
text existed, which is the same hole as the caller's catch one level in: both
now ask failureReason(error) === 'internal' rather than repeating the test, so
they cannot drift.
NOT REPRODUCED: finding 2, a body overflowing on blank paragraphs alone. The
construction returns truncated:false with the footer intact today.
NOT FIXED: the body still asks `overCap && sawText`, which is the weaker
question. Left alone deliberately — the body's output is pinned byte-for-byte
against mammoth over a 20-document corpus, and trimming its breaks to match the
auxiliary rule is a fidelity change, not a bug fix.
Two tests written for this round were deleted rather than kept: one for the
separator rule, which passes either way because no path reaches a single-'\n'
ending, and one that duplicated the blank-tail coverage. The scenario test that
remains says which fix actually pins it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round ten. The P1 is mine: the blank-run collapse added in "Collapse blank
tails" was anchored as `(\n\n)\s+$`, which backtracks from every position when a
run of blank paragraphs is followed by one character. Measured here at 4 ms for
2k newlines, 63 ms for 8k and 252 ms for 16k — quadratic, and the reviewer's
709-byte archive with 60k of them blocked the event loop for 14 s. It runs
synchronously inside the parse, so neither the per-chunk deadline nor
withTimeout could interrupt it: a working timeout was no defence because the
thing overrunning never yielded.
`\n{3,}` scans once. It also fixes what the anchored form never reached —
LEADING and INTERIOR runs were still appended and charged, so the same header
produced 120,007 characters of mostly newlines and pushed later parts past the
cap. Same file now: 98 ms, 9 characters.
That is two rounds running where my fix for a budget bug introduced a worse one.
The lesson I am taking is that an anchored regex over parser output is a
liability whatever it looks like, because the input is attacker-shaped by
construction.
Also fixed:
- Parts asked `overCap` alone where the body asks `overCap && sawText`, so a
header of empty paragraphs reported a truncation that cut nothing.
- Every remaining part was still decompressed and parsed after the output was
full. Skipped now, but only once the loss is ALSO recorded — with room gone
and nothing recorded yet, a later part holding text is exactly what we still
need to learn, and skipping it there is what dropped parts silently in round
nine.
- The separator rule was written twice again, in append() and at the fitting.
One function now.
- `partFailed` was kept apart while the walk branched on truncation. It no
longer does, so the second name for "text is missing" was two things to keep
in step for no difference.
- A comment describing the body's text sat above the partKeys map.
NOT FIXED, deliberately:
- #5, appending auxiliary parts after a body that broke. For a body cut by the
CAP this is already moot — there is no room, and the skip above covers it. For
a body that FAILED, dropping the footnotes as well trades real text for a
tidier prefix, and `truncated` already says the document continues. That is
the same call round six made in the other direction; I think more text wins
for a search index, but it is a judgement, not a defect.
- #9, the leading space on footnote text. It is in the source: the fixture's
own footnotes.xml reads " A tachyon walks into a bar." Dropping it would be
us editing the document rather than extracting it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Two limitations removed and one contract tightened. An earlier
emptyReasonfield was part of this branch and has been dropped — see the final commit for why.Footnotes, endnotes, comments, headers and footers were silently dropped. Each lives in its own zip part, so reading only
word/document.xmllost all of them — one corpus fixture,footnote-hyperlink.docx, extracted to nothing at all, because its entire content is a hyperlink inside a footnote. All five carry the body's paragraph vocabulary, so the existing reader reads them with one new parameter naming the container; the main-document shape is written exactly as it was hard-coded, and the fidelity corpus is byte-identical except the four documents that have auxiliary content, each recorded as anACCEPTED_DIVERGENCESentry. Parts append in the output cap's priority order (body, notes, margins) rather than reading order, which cannot be reconstructed; the body is load-bearing while an auxiliary part is best-effort, so a malformed footnotes part costs its own text and setstruncatedrather than failing a readable document.reasonis now a code, not a sentence. 25 strings become 10 values. The strings mostly interpolated things the caller already had — its own byte count, its own content type, constants this package exports — so they read like diagnostics while carrying nothing branchable; this suite was the proof, matchingreasonwith two dozen regexes.statusstays at three values and is now derived from the reason through one total table, so one fact cannot come back under two statuses.timed-outis the only retryable failure and was previously reachable only by matching a message againstHANDLER_TIMEOUT_MS;internalseparates our invariant tripping or a pinned dependency moving from the file being at fault. Password-protected files are also named rather than called "unrecognized", detected by walking the CFB directory tree to the root storage's own children.emptyReasonwas dropped after five review rounds. It reported a value that could not be proven as though it had been — four rounds, five sites, each time a "cannot tell" collapsing intono-text-content, which tells a caller there is nothing to find. The documents reaching those paths were the scanned ones the field existed to rescue. "Why is this empty" cannot be answered from a text extractor's vantage point: the evidence lives in the parts these readers deliberately do not open. The README now states plainly that a scan and a blank page are indistinguishable here.564 tests, typecheck and build pass. Reviewers found 40 defects across five rounds, several of them in a previous round's fix; every fix now carries a regression test verified to fail against its own unfixed code. One pre-existing bug was found on the way and left alone: a 4-byte character straddling an inflate chunk boundary decodes to U+FFFD at every odd byte offset of
word/document.xml, reproduced onorigin/main.🤖 Generated with Claude Code