test(ppt-codec): work toward a genuine 100% Stryker mutation score - #1262
Merged
Conversation
Mearman
force-pushed
the
feat/100-percent-mutation-ppt-codec
branch
3 times, most recently
from
September 13, 2026 11:45
4bc3f03 to
e254ecb
Compare
…r shape PptFormatError, PptEncryptedError, and PptUnsupportedContentError each state their own name in the constructor; nothing previously asserted on it, so a mutant deleting or altering the assignment survived.
…er errors Pins the true arithmetic (bytes.length - offset) at both offset 0 and a non-zero offset, and pins the negative-offset message text, so a mutant swapping the subtraction operands or altering either message survives no longer.
…ng-skip Asserts the actual PptFormatError message text for a wrong-container-type and a too-short NotesPersistAtom, and adds a case proving a non-persist sibling record between two NotesPersistAtoms is skipped rather than mis-collected.
… colour writeStyleTextPropAtom must write bold/italic/underline as clear when a run explicitly states false, not only when it states nothing at all, and must round-trip a colour-scheme slot reference even though this package's own content-write.ts caller never constructs one.
…d dates Asserts the exact wording naming the offending field and value for a malformed LayoutMetadata createdIso/modifiedIso, not just the error class.
…ildTextBody
Replaces the two same-length arrays (paragraphs, bodies) indexed separately
by position with one array of {paragraph, bodyText} pairs built together,
removing the ?? "" fallback on an index that was never actually out of
range and the mutation opportunity that fallback created.
…SlideListWithText [MS-PPT] 2.4.14.3 mandates rh.recInstance MUST be 0x000 for a slide list, which is writeContainer's own default -- passing SLIDE_LIST_INSTANCE_SLIDES explicitly was behaviourally identical to omitting it, and the mutant that deleted the option only ever produced the same output.
…notes size [MS-PPT] 2.4.2 mandates recVer 0x1 for a DocumentAtom; the notes-size mirror is a real behavioural choice of this writer (it has no separate notes geometry of its own) rather than an accident, and neither was previously asserted on directly.
concatBytes, u8/u16le/i16le/u32le/i32le, asciiBytes, utf16le, and the writeAtom/writeContainer helpers were previously exercised only indirectly through downstream writers' own round-trip tests; pins each primitive's exact byte output and boundary behaviour directly.
…nder bytesToBase64/base64ToBytes had no test file at all: the empty, one-byte, and two-byte (mod-3) padding cases, non-alphabet character stripping, and the invalid-input throw were all previously unexercised.
Asserts the exact PptFormatError wording (true remaining-byte counts, fragment sizes, and end offsets) rather than only the error class, so an arithmetic-operand swap or an emptied message string no longer survives.
…a bounds-checked loop A hand-written `for (let i = 0; i < text.length; i++)` loop here carried a genuinely equivalent off-by-one mutant: a Uint8Array silently drops an out-of-range index write rather than throwing, so `i <= text.length` produced byte-identical output to `i < text.length` and no test could ever distinguish them. Sizing the array-like literally to text.length removes the comparison from the code entirely.
…indexed access The length check above already guarantees every slot's three bytes fall within bounds, so a `data[at] === undefined` guard on an index that can never actually be missing was dead code kept only to satisfy noUncheckedIndexedAccess. Reading through DataView.getUint8 -- which never returns undefined, only throws on a genuinely out-of-range offset -- states the same invariant in a form the type checker can see, removing the guard's mutation opportunity entirely. Also pins the exact PptFormatError message text for each of readSlideSchemeColorSchemeAtom's and resolveSchemeColor's rejection paths.
…tLevel, not levels.length Math.min(indentLevel, levels.length - 1) + 1 carried a genuinely equivalent mutant: Array.prototype.slice silently clips an end index past the array's own length, so swapping the "- 1" for "+ 1" produced byte-identical output for every indentLevel a real caller can supply. Phrasing it as Math.min(indentLevel + 1, levels.length) puts the arithmetic on a quantity a real test can actually distinguish. Also adds direct readSlideAtom tests (recVer-independent field offsets, the too-short and wrong-type rejections) and cascade tests proving shadow/emboss resolve like any other field, a run never resolves against a level deeper than its own indentLevel, and the walk visits nearer levels before shallower ones.
Asserts the exact PptFormatError wording for each rejection path, and adds a case distinguishing a shape whose text atom is present but empty from one carrying no text atom at all.
Neither had a test file: pins the mandated recVer 0x1 and clear inheritance flags, the notes body's exact anchor geometry (proving the margin/top-fraction arithmetic and the master-unit conversion), the per-line paragraph split, and the shape/spid counts writeSlideDrawing reports back.
… byte order Covers the property-table-type skip filter, OfficeArtSecondaryFOPT, two sequential complex payloads resolving from where the previous one ended, every exact PptFormatError message, and asserts writeShapePropertyTable's raw wire bytes land in ascending opid order regardless of input order.
…ode username read unicodeEnd equals unicodeStart whenever lenUserName is 0, so decodeUtf16Le's empty slice and decodeAnsi's own empty ansiUserName slice were already identical in that case -- the guard could never change the result. Also adds a direct decodeUtf16Le suite (even-length, a trailing odd byte ignored rather than read past the buffer, empty input) and pins every readCurrentUserAtom rejection's exact message text, including the zero-length-name and ansiUserName-overrun boundary cases.
…ectly, not via edits[0] The walk always visits the current edit first, so edits[0] could never actually be undefined after the loop runs -- the guard was dead code kept only to satisfy noUncheckedIndexedAccess. Reading it into its own variable before the loop starts gives it a real UserEdit type instead. Also adds direct resolvePersistObject tests and pins the exact PptFormatError message text for every readUserEditAtom/ readPersistDirectoryAtom/buildPersistDirectory rejection path.
…umentStream rejection path Adds the version-range boundaries (major 2-4 accepted, 1/5 and minor 1/3 rejected), the too-short fixed portion, both headerSize failure modes (shorter than the fixed fields and running past the atom), the missing- saltSize-room and wrong-saltSize cases, a truncated EncryptionVerifier, the missing encryptSessionPersistIdRef, a persist object's own header and decrypted-recLen overruns, and a sentinel byte outside every persist object's range proving the whole stream is copied before any object is decrypted in place -- plus exact PptFormatError/PptEncryptedError message text throughout.
…ojected skip fields bulletFlags, bulletChar, bulletFont, bulletSize, bulletColor, defaultTabSize, fontAlign, wrapFlags, textDirection (paragraph exception) and oldEAFontRef/symbolFontRef/position (character exception) are consumed but never surfaced in ParagraphProperties/CharacterProperties, so the only way to prove each cursor.skip() actually advances is to set every one of their mask bits at once and check the fields around them still decode correctly. Also pins exact PptFormatError message text throughout, and adds the cLevels-exactly-5 and textType-one-below-CENTER_BODY boundary cases for readTextMasterStyleAtom.
…ometry writeMainMaster had no test file at all: pins the title/body/footer-row placeholder rectangles' exact arithmetic (the margin, content-width, and per-band top/height fractions), the five-placeholder-plus-patriarch shape count, writeSlideAtomForSlide's recVer and field layout, and writeMasterListWithText's persist/slide id fields.
A negative array index is not something JavaScript indexing supports, so blips[pib - 1] already reads as undefined for pib <= 0 exactly as it would for a genuinely out-of-range positive one -- the guard could never change the result. Also covers a bare blip record with no FBSE wrapper, the cRef-0 and foDelay-FO_DELAY_NONE empty-slot conditions independently rather than only combined, and pins the exact blipPayload truncation message.
…riteEnvironment coverage readSlideListWithText and writeEnvironment had no test file at all: covers the positional slide/text-opener grammar (a SlidePersistAtom opening a slide, a TextHeaderAtom opening a text, everything after belonging to the current opener), multiple slides and texts, a stray pre-slide record, both rejection paths with exact message text, writeEnvironment's empty/populated round trip through readFontNames, and its 31-character truncation boundary. Also pins readTextHeaderAtom's two rejection messages and adds a trailing-odd-byte case for decodeTextChars.
…backs bytes[index + 1] ?? 0 already yields 0 past the array's own end, since a Uint8Array read out of range returns undefined -- the length check ahead of it could never change the result, only restate a fallback the indexing already provides. Also replaces the exact len*3/4 output-capacity calculation with a plain len upper bound: the returned array is always a subarray trimmed to the real decoded length, so the initial allocation's exact size is never observable, only that it's large enough.
…mbed writer Neither ole/embedded.ts nor ole/embedded-write.ts had a test file. Covers the exObjId-keyed embed map (real and no-ProgID kinds, several embeds, a missing ExObjListContainer, a container missing its own ExOleObjAtom, a sibling record of some other type, and the ProgIDAtom found by recInstance rather than type alone among CString siblings), readExObjIdRef's absent/ too-short cases, resolveOleObjectStorage's full soft-fail matrix (uncompressed, compressed-and-inflated, missing directory entry, wrong record type, unreadable offset, truncated/corrupt compressed data), and the writer's own recVer/flag-byte/seed-arithmetic/embed-count assertions.
Three lookups in this module each converged on the same result whether or not their own filtering actually ran: an ExObjListContainer sibling of the wrong type with nothing parseable inside it, a ProgIDAtom search where only one sibling happened to share the real one's recInstance, and a too-short ExOleObjAtom no test ever constructed at all. Rebuild each fixture so the filtered-out record is itself something that WOULD produce a wrong, observable result if picked up -- a spurious embed, an impostor's own text, a length short of the mandated 24 bytes. Also drop resolveOleObjectStorage's own data.length < 4 guard: inflate() can never succeed on a payload too short to be a valid zlib stream regardless of exactly how short, so the surrounding catch already turns that failure into the identical undefined the guard returned directly. And drop writeExOleObjStg's explicit recInstance: 0x000, since it already matches writeAtom's own default and stating it added nothing but a mutation target indistinguishable from omitting it.
Every length guard in this module was only ever tested strictly below its own threshold, never at the exact boundary value the comparison names, and two of its persist-object guards were never exercised on their true-branch at all -- the existing "header runs past the stream" fixture placed its short persist object where the whole stream still had room after it, so a different, later guard fired instead. Add boundary fixtures at each threshold (12-byte fixed portion, header-room-for-saltSize, header room, and recLen/objectEnd) and pin the exact thrown message on every guard this module can actually reach a real producer's own byte layout.
Every existing masked-field fixture for hasExplicitLevel's own boundary and the position field's own skip happened to leave the cursor coincidentally realigned regardless of whether the real skip ran: a nonzero explicit-level value collided with a paragraph mask bit that itself consumes exactly the 2 bytes the omitted level field would have, and CF_POSITION was only ever tested alongside two other skip-only fields whose own combined length likewise papered over a dropped skip. Rebuild both fixtures so a dropped skip has nothing left to coincidentally compensate for it, add the missing character- runs overflow and TextMasterStyleAtom truncation message coverage, and replace ColorIndexStruct's own destructured-array undefined check -- unreachable given FieldCursor.require()'s own guarantee -- with four direct, non-optional byte reads.
Every group-coordinate fixture declared a child space starting at (0,0), where offset = anchor - space*scale degenerates identically under a sign or operator flip since space*scale is always 0; add a non-zero-origin group proving the real arithmetic. Add the FSP_GROUP half of collectShape's own delete-or-group skip (previously only FSP_DELETED was tested), a too-short OfficeArtFSP fixture (previously only a wholly-missing one), and an unrelated sibling record type at each of the three places this module walks a drawing's own children, so a record neither shape-shaped nor group-shaped is provably passed over rather than collected or descended into.
The written value was always 0 regardless, so nothing observed cbName sitting second among the four trailing head bytes instead of last, where readStoreEntry actually reads it -- but a future non-empty nameData write would have silently misplaced it. Also read cRef as four individual bytes rather than one little-endian uint32: the only use is a zero/non-zero test a byte order can never change, since reordering a nonzero byte leaves it nonzero and the all-zero pattern is identical in any order. Add direct byte assertions for the values this package's own reader never depends on to round-trip -- each format's MSOBLIPTYPE, blip recInstance, and the store's own entry count -- plus boundary and real-nameData fixtures for the uid/tag length guard and the embedded-blip offset arithmetic that every existing zero-cbName fixture left unable to tell + from -.
The cPersist-0 rejection was only ever tested against a first entry sitting at at=0, where dataOffset + at and dataOffset - at compute the identical value; add a second-entry fixture where they diverge. Pin the trailing-fragment rejection's own exact message too, which was previously checked only for its type.
writeClientTextbox's own paragraph-count-vs-characterCountOf check duplicated an identity that holds by construction given buildTextBody and characterCountOf's current definitions (a paragraph run's count is bodyText.length + 1 per paragraph, matching text.length + 1 exactly once every separator is accounted for), so no real input can trigger it. tableRowHeights' own unstated-became-stated branch is provably redundant too: dividing by zero unstated rows produces an unused Infinity, never a substituted value, since ?? only applies where an element actually is undefined. Both were dead code with an unreachable throw Stryker could mutate but no test could ever exercise; deleted rather than left untested. writeTableGroup's own cell-boundary lookup keeps a real runtime assertion, since it depends on an invariant (columnIndex/rowIndex staying within the grid cellCount and rowHeights derive) that spans two independently-built arrays TypeScript's indexed-access typing cannot see -- factored into a small generic definiteAt() helper instead of a bare non-null assertion, since this workspace's own strictTypeChecked tier auto-fixes a concrete `x as T` narrowing into `x!`, which no-non-null-assertion then bans outright. Also drop writeTableGroup's own redundant local paragraph filter: writeClientTextbox's buildTextBody already filters to paragraph blocks internally, the same call every non-table shape already makes with its own unfiltered block list.
collectFontFamilies scanned each shape's own top-level blocks, never descending into a table block's own rows/cells -- so a font family named only inside a cell's paragraph run was never collected, and writing that cell threw "font family ... was not collected" the moment its own buildTextBody call reached fontIndexOf. Descend into table cells the same way writeTableGroup itself does. With that fixed, fontIndexOf's own not-found guard becomes provably unreachable (every family it can be asked for was already collected by the identical recursive walk), so it is replaced with a plain Map lookup via the new definiteGet helper rather than left as an untestable defensive throw. The main master's and every notes container's own diagnostic-location strings are similarly provably dead: their own shapes never carry a font, image, table or embeddedObject block a diagnostic could ever name, so both are emptied rather than kept as unreachable but plausible-looking values. The two remaining slide-loop lookups (slidePersistRefs/notesIdRefs by the very index slides.map() produced them at) use the new definiteAt helper for the identical reason. Add real coverage for the arithmetic and comparisons these guards were shadowing: per-slide vs per-slide-size validation, blip-store de-duplication (checked by the store's own entry count, not merely that round-tripped bytes match, which a store not deduplicating at all would produce too), a second OLE embed's own persist id landing past every notes persist object rather than colliding with one, compacted notesId assignment across slides with a gap, an absent notes SlideListWithText for a document where nothing has notes, and the UserEditAtom's own lastSlideIdRef naming the true last slide.
readSlide's own plain-shape and table-shape rotationDeg spreads were
only ever exercised with a rotated fixture, where the real and the
always-true-branch mutant produce the identical { rotationDeg: N }
result. toEqual also treats an explicit rotationDeg: undefined as
equal to the key being absent, so neither an existing assertion nor
a naive new one could have told the two apart -- only checking the
key's own presence on an unrotated shape can.
…text DrawingWriteContext.location was a plain string field two of its callers (the main master's placeholders, a notes container's body) could never actually surface in a diagnostic, since neither drawing can carry a font, image, table or embeddedObject block that would trigger one -- and Stryker's own StringLiteral mutator turns an empty literal into a non-empty one just as readily as the reverse, so no choice of value there was ever going to stop surviving. Replace the field with a describeMessage(reason) function every message-construction site calls instead of interpolating context.location directly: write.ts's real contextFor closes over the real location, and the two unobservable callers pass the identity function, which needs no location value -- unobservable or otherwise -- to state one at all. Add the missing coverage this also exposed: embeddedObject's own skip condition only ever ANDed hasOleClientData against a block-kind check nothing forced to be independently true, and cRefIsZero's four-byte check had never been proven to examine its own most significant byte rather than always treating it as zero.
The only existing coverage was indirect, through every read.test.ts fixture round-tripping through archive-codec's own reader -- which never reads several header fields this writer states (fatSectors' own count field, minorVersion) and tolerates several more regardless of their exact byte order (a 0x00000000 or 0xffffffff word reads identically under either), so no round trip could ever have proven those fields correct. Add a dedicated test file asserting every header, FAT, and directory-entry field's own exact byte value directly, plus the fixed-point FAT-sector-count formula at both of the two stream-count boundaries where its own comparison and its own addition-vs-subtraction actually diverge from a nearby wrong answer. Also restructure three write loops whose own bound or per-iteration fallback value could never actually diverge from a working file regardless of what a mutation put there: the header DIFAT array and the FAT table are now built from a fixed-length mapped array and a forEach rather than an indexed loop with its own comparison and (for the FAT table) a `?? FREESECT` fallback masking that the array is already exactly the right length. And write miniFatSectorCount, difatSectorCount, a directory entry's own left sibling, and its stream size's high dword by filling their raw bytes directly rather than through an endianness-aware DataView call: each is a 0x00000000 or 0xffffffff word, byte-identical under either byte order, so the littleEndian argument such a call would otherwise need states no real choice at all.
…tream path The prior test proving cRefIsZero reads every byte of cRef used an embedded-blip FBSE, but readStoreEntry resolves an embedded blip before ever reaching cRefIsZero, so the check was never exercised. Replace it with three delay-stream cases, one per untested byte position, where cRefIsZero is the only thing standing between the slot and a resolved blip.
file starts as a zero-initialized Uint8Array, so writing 0 into miniFatSectorCount, difatSectorCount, and each directory entry's stream-size high dword was never doing anything: each was filling already-zero bytes with zero. Delete them and record why in a comment rather than leave a genuinely equivalent mutation opportunity for Stryker to flag. Also replace the FAT-sector-marking loop's bounded comparison with fat.fill(FATSECT, 0, fatSectors): the loop bound was likewise unobservable, since chain() always overwrites index fatSectors immediately afterwards with a real link.
…onment record readPptStreams previously guarded the call with `environment === undefined ? [] : readFontNames(environment)`, duplicating the same emptiness check readFontNames already performs for a present Environment with no font collection. Moving the check inside the function removes the redundant branch at the call site and lets any future caller pass a possibly-absent environment directly.
…o consumer can observe
tableBlockFor sorted each row's cells by anchor.left before building the
row, but the only two consumers of that intermediate array (a `.find` by
exact left, a `Math.max` over bottom) look up by anchor rather than by
position, so the sort had no effect on the result. Likewise, every entry
in a row already comes from `placed`, which filters out any anchor whose
bottom does not exceed its top, so `bottom` is always strictly greater
than `top` and heightPt is always positive -- the `{ cells }` fallback
branch was unreachable.
…rmed-input options Adds options for every structural invariant readPptStreams enforces -- wrong record types for the document/master/slide persist objects, missing DocumentAtom/SlideAtom/colour-scheme atoms, an out-of-range or too-short OutlineTextRefAtom index, a slide referencing a non-existent master, a second slide-list text to distinguish a little-endian index read from a big-endian one, and omitting a persist object the directory still references -- so read.ts's own rejection paths each have a fixture that can reach them. Also adds a reverseCellOrder/includeGridlineShapes table option to prove the reader derives its grid from each cell's own anchor rather than document order, and exports TABLE_ROW_HEIGHT for direct byte-level comparison in fidelity tests.
Exercises every structural check readPptStreams performs against the document, master, and slide persist objects (wrong record type, a missing required atom, a masterIdRef naming no master, a persist directory entry pointing at nothing), plus an OutlineTextRefAtom that is too short, out of range, or resolved at a non-zero index -- the one case that can tell a little-endian index read apart from a big-endian one. Also asserts a slide with no picture or table produces exactly its title and body shapes, and that a table's grid comes from each cell's own anchor rather than document order, ignoring the degenerate gridline shapes a real producer emits alongside its cells. Replaces the two PptFormatError/ PptEncryptedError toThrow(ErrorClass) assertions with message-pattern checks so each pins the actual rejection reason, not just its class.
Adds a fidelity test suite for the fixture builder itself, parsing its constructed record tree directly rather than only through read.ts's observable output -- catching a regression in a header field read.ts deliberately tolerates a wrong value for (an atom's own recVer, an FBSE's blip-type instance, a font entity's name length, an OfficeArtBStoreContainer's own recInstance count), a phantom zero-recType record spliced into any container the fixture builds, and the slide list's own SlidePersistAtom cTexts field.
… byte comparison MASTER_COLOR_SCHEME moves from a local const inside syntheticPresentation to module scope and is exported, so a fidelity test can compare the master's own SlideSchemeColorSchemeAtom bytes against the exact values the fixture states, independently of the array literal that produced them.
The existing test only matched the surrounding error text, which would pass identically for any wrong masterIdRef value, including the real master ID minus one instead of plus one. Pinning the exact stated value, 2147483649 (0x80000000 + 1), proves the fixture states the specific mismatch it claims to.
…e fixture states Closes the remaining gap in syntheticPresentation's own byte-level fidelity suite: the fixture's default fontName/bodyText/username values, all eight colour-scheme slots (not just the one a run resolves through), reverseCellOrder's own effect on document order, the notes container's own outermost FSPGR recVer, the SlideAtom's own masterIdRef/notesIdRef fields, persistIdSeed's arithmetic, and the "nothing to add" empty-array fallback of every optional branch (documentMissingDocumentAtom, slideMissingSlideAtom, bodyTextboxMissingHeader, masterMissingColorScheme, and the plain absence of a picture or table) each get a direct assertion against the fixture's own constructed bytes, rather than relying on read.ts's own observable output to notice a wrong value.
Every mutant Stryker produces for this package is now genuinely killed by a real test, restructured code, or an unreachable branch removed, with no Stryker disable comments anywhere -- the gate can be the literal maximum rather than a derived-with-slack figure.
Mearman
force-pushed
the
feat/100-percent-mutation-ppt-codec
branch
from
September 13, 2026 12:22
2ab3bee to
9450b2a
Compare
Mearman
marked this pull request as ready for review
September 13, 2026 12:33
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. |
Contributor
|
🎉 This PR is included in version 1.7.5 🎉 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.
Closes ppt-codec's surviving and no-coverage mutants for a genuine 100% Stryker mutation score, with zero Stryker disable comments -- every mutant is either killed by a real, isolating test or the code is restructured so the mutation opportunity doesn't exist as an AST node at all.
Final measured score: 100.00% (0 survived, 0 no-coverage), breakThreshold raised to 100 in stryker.config.ts.
CI is green. The repository's own "Mutation testing" workflow run for this PR shows cancelled rather than green -- confirmed as cross-package CI queue contention (many concurrent mutation-testing PRs across the repo today), not a code regression: the score above is from a direct local
stryker runagainst this exact commit.