xls-codec: work toward a genuine 100% mutation score - #1263
Merged
Merged
Conversation
Mearman
force-pushed
the
feat/100-percent-mutation-xls-codec
branch
4 times, most recently
from
September 14, 2026 08:07
482999a to
7cd0ce5
Compare
Adds a dedicated container.test.ts exercising the compound-file container layer on its own: the legacy 'Book' stream rejection, the no-Workbook-stream rejection, the CompoundFileFormatError wrapping into BiffFormatError, SummaryInformation and MBD<hex>/Package embedding-storage stream selection (including a near-miss path that must not match), and isXlsFile's own true/false/catch-all cases. None of these had a direct test before, relying only on incidental coverage from content.test.ts's full round-trip fixtures.
…dation The createdIso/modifiedIso validation this package adds on top of archive-codec's own mapping had no direct test, so a malformed date in either field, and a malformed createdIso alongside a valid modifiedIso, are pinned here rather than only reachable through a full writeXlsContent round trip.
…CellRecord Each of the five independent conditions cellCarriesFormatting checks (background, alignment, verticalAlignment, font, and each of the four border sides) gets its own cell that carries only that one property, so a mutant turning any one check into a no-op or an && instead of || fails a test that isolates it -- the write-path round trips in write.test.ts only ever combine several of them at once.
writeEmbeddedObjectPackage/readEmbeddedObjectPackage round-trips through write.test.ts before now only ever exercised the accepting path. Adds direct coverage of the foreign-label, non-object, missing-objectKind, missing-document, and schema-validation-failure rejections, the placement fields' deliberate non-round-trip, and the source residue field, plus bytes that are not a Package stream at all.
Direct coverage for a module previously exercised only through write.test.ts's full round trips: no commented cells, one comment's position/text/author, an absent author staying absent rather than an empty-string placeholder, an empty-text comment writing no Continue record, multi-comment round trips, the row-then-column Note ordering independent of input order, the Note-records-first-then-Obj/TxO-pairs emission order, sequential object id assignment, and the 16-bit FtCmo.id ceiling.
…spatch edges Direct coverage of paths content.test.ts's full CFB round trips never reach: an unrecognised wEncryptionType, RC4 CryptoAPI's own EncryptionVersionInfo rejected by name, the no-password message naming the right scheme for each of RC4/XOR, an XOR password too long for obfuscation to represent folding into "incorrect password" rather than a raw RangeError, and -- for both RC4 and XOR -- the never-encrypted record-type bypass and the BoundSheet8 lbPlyPos-preserved special case, built directly against BiffRecord values rather than a full compound file.
…tests
The previous commit's fixtures used a nonexistent Color.rgbHex field
and an invalid ContentStrokeStyle value ("thin", not one of
solid/dashed/dotted/double), which typechecked as vitest's own loose
mock inference but failed tsc -p tsconfig.node.json outright.
…ently FTAB_NAMES/FTAB_FIXED_ARITY/FTAB_IFTAB_BY_NAME are built from one 372-entry literal table with no test of its own -- formula tests elsewhere only ever exercise a handful of these functions by name, so every other entry's own string and arity literals had no test able to notice a change. Transcribes the full published Ftab table ([MS-XLS] 2.5.198.17) as an independent reference array and checks every entry against it, rather than deriving the expectation from the module under test itself.
…t branches directly Adds a dedicated cfb.test.ts for the test-support compound-file writer, previously exercised only incidentally through container.test.ts/content.test.ts's higher-level round trips: the path-segment and entry-name validation errors, the mini-stream vs FAT-chained big-stream cutoff, multi-sector FAT/directory chains, and both major-version (3 and 4) header layouts.
…list, not recursion compileNode walked a formula's own AST by native recursion, one JavaScript call frame per operator -- a long but legitimate chain of many thousands of binary operators (a generated SUM(...)+SUM(...)+... expression, say) would overflow the stack at a tree depth far shallower than MAX_RGCE_LENGTH's own 65535-byte ceiling ever needs throwing for. Rewrites it as an iterative post-order walk over an explicit worklist held on the heap, so compiling degrades gracefully to that ceiling's own BiffWriteError instead of an uncontrolled RangeError. Also removes two conditions from numberNode that can never affect its result: NUMBER_RE never captures a sign or a non-digit character, so a token already matching the plain-digit form always parses to a non-negative whole number regardless of magnitude, making Number.isInteger(value) and value >= 0 restate a fact the regex already established rather than narrow it further. Finally, replaces the parser's own "token stream ran past its own end" throw -- unreachable, since tokenize() always appends one trailing eof token and advance() only ever fires once the current token is confirmed non-eof -- with a fallback to that same shared eof token, avoiding both a untested throw and (this package's non-null assertions are lint errors) a type assertion to state the invariant instead.
…mpiler directly Adds a dedicated ptg-writer.test.ts, previously exercised only incidentally through cell/data-validation/conditional-format formulas elsewhere in the package: every whitespace character, integer/decimal/exponent number literals either side of the PtgInt/PtgNum boundary, PtgNum's own little-endian float encoding, string literals including doubled-quote escaping and the unterminated-string error, all eight BIFF8 error literals and the two ways an error literal can be malformed, TRUE/FALSE, every comparison/arithmetic/unary/percent operator and their precedence against each other, cell and area references across every combination of $-absolute flags and both grid boundaries (row and column, upper and lower), fixed- and variable-arity function calls including omitted arguments and the 255-argument PtgFuncVar ceiling, and the writer's own error messages for a malformed formula at every stage from tokenizing through compiling.
…stive switches CP_BY_OPERATOR, SIMPLE_KIND_TO_ICF_TEMPLATE, and CTP_BY_TEXT_TYPE were each a ReadonlyMap covering every member of a closed string-literal union, with an "operator/rule type has no value" throw guarding a Map.get() miss that can never actually happen given the union those types already close over -- but a Map's own .get() always types its result as possibly undefined regardless of how completely its literal entries cover the key type, so the throw stayed live and untestable. Replaces each with a real exhaustive switch statement instead, which the compiler itself checks covers every union member, so the impossible branch and its message are gone rather than merely unreachable. cfvoTypeCodeOf, already a switch, drops its own equivalent default case the same way. Also removes two structurally unreachable "internal error" throws by threading validated data instead of re-deriving it: validateRuleGrid now returns a rule's ranges narrowed to a provably non-empty tuple rather than void, so its own caller can pass a rule's real first range down to writeCf12Record/textRuleFormula as a plain required parameter instead of that function re-indexing rule.ranges[0] and guarding against an absence validateRuleGrid, called moments earlier on the same rule, already rules out. assignPriorities now returns each CF12 rule zipped together with its own resolved ipriority, rather than a same-length array of bare numbers the caller re-correlated to its own rule list by array index -- removing the "fewer priorities than rules" throw that guarded against the two arrays ever silently drifting out of step, since there is no longer a second array to drift.
…bytes directly Adds the boolean-flag combinations the existing conditional-format round trips left only half-tested (each ternary needs both its own true and false input to distinguish it from a mutant that always takes one branch): a data bar's shown value, an icon set's hidden value with no reverse, a top10 rule selecting by count from the top, every aboveAverage/equalAverage combination, and a rule declaring stopIfTrue. Also adds a percentile- and formula-typed colour-scale stop, and the "threshold carries no value" refusal for a value-bearing CFVO type. Adds a new describe block calling writeSheetConditionalFormats directly to check two things no round trip through the reader can ever observe: that a style-less rule's own DXFN12 block writes cbDxf as a genuine 0 rather than a padded-but-still-empty block (the reader treats both identically, since it degrades on flag bits rather than data length), and that a colour scale's own fixed interpolation-position floats (0.0/1.0 for two stops, 0.0/0.5/1.0 for three) are the ones [MS-XLS] 2.5.33 itself pins per stop count rather than the other set -- values the reader skips over as unused padding and so never round-trips into anything observable.
…trictEqual
Every reader function in this package builds its result through a chain
of `...(x !== undefined ? { key: x } : {})` spreads, so an optional field
the source omits entirely and one a mutant forces to `{ key: undefined }`
produce objects that plain .toEqual cannot tell apart -- Jest/Vitest's own
loose equality treats a missing key and one explicitly set to undefined
as the same thing, so a whole family of "if (x !== undefined)" mutants
across content.ts, conditional-format-write.ts, and elsewhere survived
regardless of how many present/absent fixtures a round trip already
covered. Switches every .toEqual in this package's own test suite to
.toStrictEqual, which does distinguish the two, catching that entire
mutant family in one pass rather than needing a bespoke assertion per
optional field.
Fixes the two fixtures this actually caught as genuinely under-specified:
conditional-format-12.test.ts's raw CF12 reader tests for a plain top10
rule and for every operand-free icfTemplate now state style: undefined
explicitly, matching RawConditionalFormat12's own field (declared without
a `?`, so always present even when its value is absent) the same way
every other fixture in that file already does.
One fixture is intentionally left on .toEqual, not upgraded: reading a
hand-built SummaryInformation stream that states only title/author/
createdIso surfaces every other LayoutMetadata field as an explicit
undefined rather than omitted, because archive-codec's own shared
summaryInformationToLayoutMetadata (also used by doc-codec and ppt-codec)
states every field unconditionally -- a genuine, if minor, contract
inconsistency belonging to that shared package rather than this one.
…s directly Adds the writeSheetDataValidations error paths the existing round trips never exercised: an unrecognised type/operator string (via a deliberately schema-violating fixture, the same as this file's own existing wrong- arity/missing-formula tests), a comparison rule carrying a second formula its own operator doesn't take two of, an empty ranges array, and each of the four grid-boundary checks (row/column, both edges) individually. Also calls writeSheetDataValidations directly for the one case a round trip through the reader cannot observe: an empty dataValidations array must write no Dval/Dv records at all, not a Dval stating a zero rule count -- content.ts's own mapDataValidations().length > 0 check already omits the field for either shape on the way back in, so only inspecting the writer's own output distinguishes them.
…er ignores Adds a dedicated globals-writer.test.ts calling buildWorkbookGlobals directly: the fifteen built-in STYLE records (globals.ts's own reader never looks for RECORD_STYLE at all, so no round trip can tell whether they were written), the SST record's presence gated correctly on whether the workbook actually carries shared strings, and an ExternSheet stating exactly one XTI per sheet rather than one too many.
…alue gaps Adds the grid-boundary opposite cases checkedCellPosition's own OR-chain needed (a row-only violation, and the exact last valid row/column succeeding rather than throwing), a sheet whose cells carry no row/column metadata staying with empty rows/columns arrays rather than one every mutation of the fDyZero/fUnsynced/hidden flags would also leave unchanged, and a merge spanning only rows or only columns rather than always both together. Adds formula-cached date/time/date-time and (checking the value, not just the formula text) error results, none of which the existing cached-result tests exercised. Adds a row-only and a column-only manual page break, and a custom page size's own Setup record fPortrait bit read directly (custom page-size dimensions never round-trip at all, so no test through readXlsContent could otherwise tell portrait from landscape here). Also adds a dedicated describe block calling buildWorksheetSubstream directly for the Dimensions record's own rwMic/rwMac/colMic/colMac bytes, which content.ts reads into RawSheet.usedRange but never maps into a ContentSheet field, so no round trip observes them either.
… pass
buildFormatPlan, buildPalettePlan, and buildFontPlan each repeated an
identical "if (!writesCellRecord(cell)) continue" guard inside their own
cell loop -- genuinely dead code today (an unwritten cell can never carry
a font/colour/format these scans would otherwise register, since
writesCellRecord being false already implies cellCarriesFormatting is
false too), but still real, load-bearing protection against a future
change making one of these scans disagree with sheet-writer.ts's own
record-emission predicate about which cells matter. Filtering once via
sheet.cells.filter(writesCellRecord) at each loop's own head keeps that
protection -- every pass still filters through the identical shared
predicate -- while removing the standalone if/continue three separate
mutation opportunities were hiding behind despite it being unreachable by
construction.
Also adds the one case that genuinely was an observable gap: two cells
bordered identically on different single sides, distinguishing the
decoration-signature string's own side-prefix characters ("l"/"r"/"t"/
"b") from each other -- previously only ever exercised by a single cell
carrying two different sides, which a swapped or blanked prefix could not
have been told apart from.
…ndaries Every existing applyTint test used pure red or exact grey, both of which happen to compute an exact 0.5 lightness and a g === b tie -- so no test ever selected rgbToHsl's own l > 0.5 saturation branch, its max === g or max === b hue branches, or the g < b tie-break's true side, and hueToRgb's own wraparound and midpoint branches went similarly unexercised by any colour whose computed hue actually landed there. Adds four colours chosen to land in those specific branches, each checked against an independent reference implementation of the same documented W3C HSL algorithm (a copy Stryker's mutations to the source file can never touch, so a mutated formula and this reference disagree exactly where the mutation changed something) rather than by hand-derived expected values. Also adds exact boundary tests for resolveIcvColor's own palette-range check: icv 63 (the last valid palette index) and 64 (one past it).
Adds direct write-path tests for validateUserName's 255-character cch ceiling, scopeOf's out-of-range scopeSheetIndex refusal, and compileRefersTo's MAX_ROW_INDEX/MAX_COLUMN_INDEX grid boundary, each exercising both the accepted edge and the refused one-past-it case.
No test asserted the class's own `.name` override or that its message survives construction, so a mutant clearing `this.name` to an empty string went undetected.
A freshly allocated ArrayBuffer is already zero-filled, and endianness has no observable effect on a word of all-zero bytes, so the explicit setUint32(4, 0, ...) call restated a fact the buffer already held rather than a real one about the format.
The refusal only asserted the error's class, not that its message actually names the record's own type and length -- a mutant clearing the template string to an empty one went undetected.
The existing round-trip and never-narrower properties both still hold for a coldx computed by adding the digit-width allowance instead of subtracting it, so neither killed a mutant flipping that sign. A direct exact-value assertion does.
…ance boundary Neither the landscape check's requirement that BOTH dimensions match, nor the tolerance comparison's own boundary, had a test that would fail if the conjunction were loosened to a disjunction or the tolerance's own <= narrowed to a strict <.
The identical `if (!(err instanceof BiffFormatError)) throw err` guard was duplicated at every one of dozens of per-record recovery boundaries across workbook/ and biff/, each one separately exposed to the same handful of mutations (the condition inverted, the guard's own block emptied) with no test anywhere actually proving a genuine bug still propagates rather than being silently absorbed alongside a malformed record. Centralising the classification into one function, tested directly against both a real BiffFormatError and a genuine TypeError, removes that duplicated surface everywhere it is adopted.
writeSheetConditionalFormats' own empty-rules early return produced exactly the output the general path already produces for zero rules (validateRuleCount(0) never throws, and an empty forEach plus assignPriorities([]) both no-op), so the special case stated nothing the general path didn't already state on its own. writeCfFilter recomputed a top10 rule's fTop/fPercent flags byte a second time, independently from the identical computation writeCf12Record already makes for CFExFilterParams -- this package's own reader (conditional-format-12.ts) reads only the latter, so the two computations could silently diverge with no way to notice. Both now share one flags value, computed once and passed through. textRuleFormula is exported so its four formula shapes (one per text-predicate rule kind) are directly testable: the shape a containsText/notContainsText/beginsWith/endsWith rule's formula takes is otherwise invisible to a round trip, since this package's own reader determines a text rule's kind from CFExTextTemplateParams' ctp field rather than from the formula's own shape.
…signment Adds direct textRuleFormula assertions for each of the four text-predicate formula shapes, isolated top10/stopIfTrue boolean branches (bottom/percent selected independently, each of colorScale/dataBar/iconSet refused on its own rather than only dataBar), exact-boundary range acceptance at BIFF8's own four grid edges (alongside the existing one-past-the-edge refusal), a refusal at exactly 32768 conditional-format rules built from cheap placeholder objects so it exercises the real write path without the cost of constructing that many genuine rules, and raw-record nID sequencing checks for both the base CondFmt and CF12 record families, since nID's own correctness is invisible to every round-trip test (this package's reader never emits or resolves a CFEx cross-reference on a self-written file, and explicitly skips CondFmt12's own copy of the field).
…port testable seams readSheetDrawing's own empty-drawingChunks early return produced exactly the output the general path already produces: concatBytes([]) is an empty Uint8Array, and readSheetShapes' own readEscherRecords loop condition (offset < bytes.length) never runs against a zero-length stream, so shapes stays [] and the pairing loop below never executes either way. The worksheet-record loop's own index<worksheetRecords.length bound and the pair loop's own Math.min(shapes.length, objEntries.length) count were both redundant with the undefined-guards already inside each loop body -- an index one past either array's real length reads back undefined and gets skipped exactly like the existing "no shape"/"no obj"/note-type cases already are, so the exact bound (min, max, or anything in between) is unobservable. The worksheet loop is rewritten over `.entries()`; the pair loop now runs until both arrays are simultaneously exhausted, with no separate count to compute or bound to compare against. SheetGridGeometry, resolveAnchorPlacement, embeddedObjectFromObjRecord, imageFromShape, chartFromShape, and drawingObjectFromShape are exported so each can be exercised directly with a minimal DrawingShape/geometry fixture, rather than only through a full Escher-byte round trip that makes several of their own internal branches (a real declared column width, an isolated zero-width vs zero-height anchor, a chart substream's own offset-bound edges) prohibitively fiddly to hit precisely.
…iring edges Adds direct SheetGridGeometry tests proving a column/row's own declared width/height is used (not the default) and that xPt/yPt sum only the entries strictly before the one asked for; isolated zero-width vs zero-height anchor tests for imageFromShape, embeddedObjectFromObjRecord, and drawingObjectFromShape (the same widthPt<=0||heightPt<=0 guard appears in all three); an exact full-document assertion for drawingObjectFromShape's own generated page/shape; readSheetDrawing tests isolating obj.ot===PICTURE from shape.shapeType===PICTURE_FRAME (each sufficient on its own) and covering a sheet whose shape and Obj-record counts genuinely disagree; and chartFromShape tests isolating each of its three substream-matching conditions (chart-typed, after the Obj record, before the next one) plus an exact assertion of the generated single-sheet chart document among several near-miss candidate substreams.
…austed side The loop's own break condition required BOTH arrays to run out before stopping, waiting through however many extra no-op iterations the longer array still had left -- but since neither array has genuine holes, the index where one first goes missing is also the index where every later index goes missing too, so no real pair can exist past that point regardless of the other array's own remaining length. Breaking the moment EITHER side is exhausted reaches the identical result without the pointless trailing iterations, and folds the note-type skip into its own single, separate condition instead of sharing one compound guard with the exhaustion check.
…ctFromObjRecord's guard tests The zero-size anchor tests previously paired a resolvable storage id with arbitrary, non-Package bytes -- readEmbeddedObjectPackage's own foreign-payload degrade already returns undefined for content that isn't a real OLE Package carrying this codec's own JSON payload, so the tests could not tell a genuine size-guard rejection apart from the downstream parse simply failing on garbage input regardless of size. Building the fixture through writeEmbeddedObjectPackage itself gives the tests real, parseable bytes, so a defined result is the only possible outcome once the size guard no longer runs.
…d index resolveBlip's own repeat-image path looked a stored blip back up by indexing into the blips array with a previously-minted index, guarded by an "internal error" throw for an index the array itself somehow didn't recognise -- a case that can never actually arise, since the index handed back for any base64 already in blipIndexByBase64 is exactly blips.length was at the moment that entry was pushed, and blips only ever grows. Keying the map directly by the blip object (alongside its index) removes the array indexing this dedup path never needed in the first place, so there is no possible-but-provably- unreachable branch left to guard against.
…byte layout Adds a dedicated drawing-writer.test.ts exercising the module's own internal functions directly, exported specifically for this: exact locateX/locateY boundary behaviour at each grid edge and at a plain mid-column/mid-row point (proportional, not the offset scaled by the width/height a second time); placementOfImage's grid-boundary refusal isolated per edge, at each edge's exact boundary; placementOfEmbedded's offset arithmetic; byte-exact FtCmo/FtCf/FtPioGrbit/FtPictFmla field checks; bytesFromBase64's own padding-character handling (previously untested by any fixture in this package's suite, every one of which happened to use base64 needing no padding at all); and buildDrawingWritePlan checks that read the actual written Escher bytes directly -- a Blip Store entry's own cRef reference count, an OLE-embedded shape's fOleShape bit versus a plain picture's, an Obj record's own id continuing past a sheet's comment count, an Embedding Storage path's uppercase hex spelling, and the FDGG block's own spidMax/cspSaved/cdgSaved across two sheets -- none of which this package's own reader ever reads back, so a round trip alone cannot prove any of them correct.
…ilently skipping it bytesFromBase64 skipped any character outside the base64 alphabet with no distinction between genuine trailing padding and a stray invalid character elsewhere in the string -- and skipping padding specifically was already provably unobservable regardless of whether the skip ran, since the output array is pre-sized to exactly the real decoded byte count, so anything a mishandled padding character could have written would only ever land at or past that length, where a typed array write is a silent no-op. Padding is now sliced off up front instead of skipped inside the loop, and any character that still isn't part of the alphabet after that is a genuine malformed payload, refused by name rather than quietly dropped.
…oss-sheet spids Adds direct writePictureObjRecord/writeEmbeddedObjRecord byte checks for FtPioGrbit's own fAutoPict bit, a jpeg-accepted counterpart to the existing gif-refused test, and a base64-alphabet-refusal test matching the source's new behaviour. Replaces the single-item storage-id, object-id, and drawing-id checks with multi-item ones: 11 embedded objects to prove storage ids increment sequentially and reach into the hex alphabet's own uppercase letters past single digits, three cells (two commented, one not) with two images to prove the comment-count filter and the object-id sequence both count correctly, and two sheets each carrying a real image to prove their own FDG drawingId and shape spids are distinct and increasing rather than colliding or resetting.
padding is only ever 0, 1, or 2, and slicing zero characters off the end of a string returns the identical string -- so the "no padding" branch of the padding>0 ternary was just base64.slice(0, base64.length) restated as a separate case, not a genuinely different one. Slicing unconditionally removes the redundant condition entirely.
…tInRange's redundant edge cases readChartSeries' own record-type switch ended in a bare default: break, the last case in the switch and no different in effect from having no default arm at all. pointInRange special-cased a single-row range (walk across columns) and a single-column range (walk down rows) ahead of the general row-major formula -- but with width always the range's own real column count and index always staying within the range's own point count for a well-formed chart, Math.floor(index / width) can never reach a second row when the range is one row tall, and index % width is always 0 when the range is one column wide, so the general formula already reduces to exactly what each special case computed by hand. Both are removed, leaving the one formula that handles every range shape a real chart can carry.
…ge resolution Adds direct coverage for readChartSeries' own dispatch and cache logic that the existing four tests never reached: a SeriesText cache only applying when it follows a name-role AI, not values/categories; a range-reference AI name (id=0, rt=range) resolved from the referenced cell; literal and auto-generated (rt=0/1) AI records whose own token bytes happen to share a range opcode's byte shape, proving rt itself gates the range interpretation rather than the bytes alone; an AI id naming neither values nor categories (bubble size); an SIIndex naming neither cache role; BoolErr's own error-text and TRUE/FALSE cache branches; a Blank record's own no-op cache contribution; single-row, single-column, and genuine rectangular range walks; empty-formula guards and the PtgParen display wrapper for both literal and range tokens; PtgInt/PtgNum literals; each of PtgRef3d's and PtgArea3d's own three class-variant opcodes; a reversed row/column pair normalised through Math.min/max; and isOwnSheetRef's own four ways to answer "not the owning sheet" (a different sheet with no cache to mask it, a genuinely multi-sheet 3D reference spanning the owning sheet, an unresolvable ixti, and a genuinely external workbook reference).
…return A cached record type addCacheEntry doesn't recognise (chiefly a Blank) left text at its own initial undefined either way -- the switch's default case returned early to state that explicitly, but the very next line already returns early for exactly that condition, so the default case said nothing the following check didn't already say on its own.
Adds a genuine unmatched-cell own-sheet range test (proving the displayText fallback path itself, not just the no-range case), a PtgRef3d cross-sheet test mirroring the existing PtgArea3d one, an unrecognised range-token opcode falling through to undefined, a reversed row pair normalised through Math.min/max (the row counterpart to the existing reversed-column test), and two cached points under the same role proving the cache's own lazy-init guards keep earlier entries rather than starting the role over on every call.
pointInRange's row-major walk only ever reads a range's startRow and its width (derived from startColumn/endColumn) -- a well-formed chart's own point count stays within the range's real extent, so nothing here ever needs to know where the range's last row actually is. endRow was computed at both of readRangeToken's own construction sites and never read anywhere after.
…correcting bounds Every counted loop and Map/array lookup this test-support compound- file writer made defensively (a name length TypeScript can prove non-empty but the type system can't, a sibling/child lookup a single upstream tree walk always populates before it's ever read, an off-by-one loop bound whose extra iteration writes a byte the target buffer was already zero at, or a subarray/typed-array set that clamps to the buffer's own real end) turned out to be provably unreachable or genuinely unobservable, confirmed by applying each mutation directly and running the real reader round-trip suite against it rather than reasoning about it in the abstract. Restructured accordingly: checkedName drops its own unreachable empty-name check (every name reaching it already comes from a validated path segment or the root's own fixed "Root Entry"); the path-splitting and tree-linking Map/array lookups route through two small, directly-tested requiredLeaf/requiredRecord/requiredSectorStart helpers instead of silent `?? fallback` values; the small- and big-stream sector-allocation passes pair each record with its own derived value in one array (miniEntries/bigEntries) rather than two same-length arrays walked by a shared index; counted loops that build or copy a fixed number of sectors iterate `Array(n).keys()` instead of a hand-written `i < n` comparison; and the two truly redundant special cases (an empty mini stream's own zero-byte copy, and Math.ceil(0 / x) already being 0 without a separate zero check) are removed outright.
…, and raw byte layout Adds a DEL-byte (0x7f)/one-past (0x80) boundary pair for checkedName's own ASCII check; a same-named stream-then-storage test proving the two are never confused for one another; a two-streams-under-one- storage test proving a second child reuses the storage its sibling already created rather than duplicating it; direct unit tests for requiredLeaf/requiredRecord/requiredSectorStart, including the internal-error branch each throws for an input no real caller ever produces; and raw DataView-parsed checks for fields no round trip through archive-codec's own reader ever proves correct on its own -- a stream entry's own colour flag byte, the root entry's own literal name bytes, which sectors the DIFAT names as FATSECT versus FREESECT, and the directory-sector-count header field's own version-3-zero versus version-4-real-count distinction.
…me storage edges Adds a storage-name isolation test (an existing differently-named storage must never be mistaken for the one a new path segment names), an exact-at-cutoff (4096-byte) stream classified as big rather than small, an exact total-byte-length assertion for a workbook holding only a small stream (catching a stream wrongly counted as both small and big, which the round trip alone tolerates as wasted, unreferenced space), and three streams together large enough to force a second mini-FAT sector -- the one shape in this suite where the earlier sector-copy loop's own start/offset arithmetic actually diverges from its neighbours' identical values at index zero.
Byte 0x48 (the DIFAT's own sector count) is genuinely, always zero -- this writer never allocates a DIFAT sector of its own (the DIFAT always fits the header's own 109-entry array), and view is backed by a freshly-allocated, zero-initialised file buffer, so writing 0 there again would restate what is already true rather than change anything.
…proves Adds a directory-entry-count test proving a storage genuinely gets reused across every stream nested under it (four streams sharing one storage segment push the directory past a sector boundary a correctly-deduplicated tree never reaches, catching a lookup that never finds what it already created), and raw header-field checks for the minor version and mini-FAT sector count -- both real, spec- mandated values archive-codec's own reader never reads back, so a round trip alone cannot prove the writer stated them correctly.
A full, non-incremental Stryker run across the whole package came back at a genuine 100.00% mutation score (0 survived, 0 no coverage), confirming every mutation opportunity is either killed by a real isolating test or removed by restructuring away the equivalent-mutant AST node. The break threshold now reflects that.
Mearman
marked this pull request as ready for review
September 15, 2026 10:31
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
enabled auto-merge (rebase)
September 15, 2026 12:24
Contributor
|
🎉 This PR is included in version 4.15.7 🎉 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.
Adds direct unit tests for xls-codec modules that previously had no test file of their own and relied entirely on incidental coverage from content.test.ts/write.test.ts's full round trips, and restructures genuinely equivalent-mutant boundaries out of existence rather than chasing them with tests.
Final state: a genuine 100.00% mutation score, confirmed by a fresh, non-incremental, full package-wide Stryker run: 4546 killed, 75 timeout (counted as killed), 0 survived, 0 no-coverage.
breakThresholdinstryker.config.tsis now set to 100. No// Stryker disablecomment exists anywhere in this package (grep -rn "Stryker disable" packages/xls-codec/srcreturns nothing), and none was ever added — every mutant is closed either by a genuine isolating test or by restructuring the code so the mutation opportunity no longer exists as an AST node.Files brought to 100% this session, on top of everything closed earlier in the branch's history (
biff/strings.ts,biff/string-writer.ts,test-support/biff.ts,biff/xf-colors.ts,drawing/blips.ts,drawing/escher-writer.ts,test-support/cfb.ts,biff/ptg-writer.ts,workbook/conditional-format-write.ts,workbook/data-validation-write.ts,workbook/globals-writer.ts,workbook/sheet-writer.ts/write.ts,biff/write-errors.ts,biff/rk.ts,biff/record-writer.ts,units.ts,biff/print-setup.ts,biff/records.ts,drawing/md4.ts,workbook/conditional-format-ex.ts,container.ts,biff/substreams.ts,workbook/comment-writer.ts,workbook/comments.ts,workbook/encryption.ts,workbook/embedded-object.ts):workbook/chart.ts,workbook/sheet.ts,workbook/drawing-writer.ts,workbook/globals.ts,workbook/drawing.ts,workbook/defined-names.ts,write.ts.Representative techniques from the closing pass:
workbook/chart.ts: removed a dead trailingdefault: break;, a redundantdefault: return;already covered by the next line'sundefinedcheck, and both special-case branches ofpointInRange(the general row-major formula already reduces to each of them exactly); deletedOwnSheetRange.endRow, a field computed at both its construction sites but never read anywhere in the file.workbook/drawing.ts/workbook/drawing-writer.ts: exported several previously-private helpers so their boundaries are directly testable; replaced aMath.min-bounded shape/Obj pairing loop with one that breaks the moment either array is exhausted (the earliest-correct, unambiguous termination point, since neither array has genuine holes); restructuredbytesFromBase64to slice trailing padding off up front and throw on a genuinely invalid character instead of silently skipping it.test-support/cfb.ts: replaced every countedfor (let i=0; i<N; i++)loop withfor (const i of Array(N).keys()), removing the comparison from the AST entirely; paired parallel arrays (smallStreamRecords/miniChunks,bigStreamRecords/sector counts) into single arrays of tuples to eliminate index-correlation-based "always defined" fallback checks; addedrequiredLeaf/requiredRecord/requiredSectorStartnarrowing helpers, each directly unit-tested against its own contrived impossible input; removed a zero-initialized-buffer no-op write (put32(view, 0x48, 0)) and an unreachable name-length guard.TypedArray.prototype.set/.subarrayclamping behaviour) — from real, testable gaps.CI note on the mutation-testing workflow's own shard failures: this run's
.github/workflows/mutation.ymlfell back to testing the entire workspace (23 packages, all 8 shards) rather than just the packages this PR touches, because the plan job'scompute-mutation-shards.tsfailed to resolve a localmainref against the PR's shallow-but-full checkout (Failed to resolve base ref 'main' ... unknown revision, then "assuming all files have changed") — a pre-existing quirk of that script's ref resolution, not anything in this PR's diff. Within that full-workspace run, xls-codec's own shard reported the exact same genuine 100.00 this PR claims above. The shard failures that resulted are all in packages this PR never touches:document-schema.jsat 99.92 (pre-existing, below its own 100 threshold),document-outline.jsat 99.83 (same), and apdf-codecinitial-test-run timeout in an unrelated encryption test. Shards 0 and 3 (thedocuments.jsengine package anddocument-cli/epub-codec/byte-codec) were cancelled by the job's 180-minute timeout, a direct consequence of the same full-workspace fallback forcing a cold run of every package instead of only the ones this PR's diff affects. None of this is caused by, or reproducible from, any change in this PR's diff (packages/xls-codec/**andpackages/xls-codec/stryker.config.tsonly).