Skip to content

fix(msl): reject >32,767 fragments/entry on write instead of silent overflow#1079

Merged
trishorts merged 3 commits into
smith-chem-wisc:masterfrom
trishorts:fix/msl-fragmentcount-validation
Jun 29, 2026
Merged

fix(msl): reject >32,767 fragments/entry on write instead of silent overflow#1079
trishorts merged 3 commits into
smith-chem-wisc:masterfrom
trishorts:fix/msl-fragmentcount-validation

Conversation

@trishorts

Copy link
Copy Markdown
Contributor

Problem

MslPrecursorRecord.FragmentCount is a signed 16-bit field, but MslWriter narrows the fragment count with an unchecked (short) cast. An entry with more than 32,767 fragment ions silently wraps to a negative count on write, producing a file that throws OverflowException (negative array length) only later, on read — i.e. silent corruption at write time, surfaced far from the cause.

This is reachable in practice: with internal-ion annotation (a supported .msl feature), the fragment count grows roughly quadratically with peptide length, so a proteoform of ~250+ residues annotated with internal ions can exceed 32,767 fragments.

Fix

Validate on write. MslWriter now throws a clear, actionable ArgumentException that names the offending entry and the limit, on both the streaming and in-memory write paths, instead of emitting a corrupt file. No on-disk format change.

Verification

  • 32,767 fragments/entry → writes and round-trips OK (unchanged).
  • 32,768 fragments/entry → now throws on write:
    Entry '…' has 32768 fragment ions, which exceeds the .msl per-entry limit of 32767 (FragmentCount is a 16-bit field in MslPrecursorRecord). …
    (Previously: silent wrap on write → OverflowException on read.)

Scope

This is the safe, non-breaking guard against silent data corruption. Lifting the limit entirely (widening FragmentCount to int32) is a versioned format change and is intentionally out of scope here.

🤖 Generated with Claude Code

…verflow

MslPrecursorRecord.FragmentCount is int16, so an entry with more than
32,767 fragment ions silently wrapped to a negative count on write and
threw OverflowException only later on read. Validate on write and throw
a clear ArgumentException naming the entry and the limit.

Surfaced while verifying the .msl manuscript's "unbounded fragments per
entry" claim; reachable for ~250+ residue proteoforms annotated with
internal ions. A true fix (widen FragmentCount to int32) is a versioned
format change; this is the safe non-breaking guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TM8gpAxcjWYschz3yEjixZ
@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.34%. Comparing base (a730eaa) to head (ac8100a).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1079      +/-   ##
==========================================
+ Coverage   82.33%   82.34%   +0.01%     
==========================================
  Files         382      382              
  Lines       50509    50518       +9     
  Branches     6102     6103       +1     
==========================================
+ Hits        41586    41599      +13     
+ Misses       7734     7729       -5     
- Partials     1189     1190       +1     
Files with missing lines Coverage Δ
mzLib/Readers/SpectralLibrary/MslWriter.cs 96.01% <100.00%> (+0.59%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Add four NUnit tests for the FragmentCount int16 overflow guard: the
in-memory (Write) and streaming (WriteStreaming) paths each reject a
32,768-fragment entry with an ArgumentException naming the entry and the
32,767 limit, and the 32,767 boundary still writes with no wrap (locks
the comparison as '>' rather than '>='). Previously the throw branch was
unhit, so whole-file coverage masked ~29% patch coverage on the new
guard; this lifts it to 100%.

Ref: smith-chem-wisc#1079
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@trishorts trishorts merged commit 125c995 into smith-chem-wisc:master Jun 29, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants