fix(tokenizer): keep parse() never-throwing and bound nested encapsulated scans (§3) - #26
Merged
Merged
Conversation
…ated scans (§3) Two correctness fixes from the MedFusion field review §3: - recoverToFallback runs inside run()'s catch; the addElement that materializes the opaque fallback value could itself trip maxElements and throw limit-exceeded out of the catch, breaking the "parse() never throws" contract (repro'd with maxElements=1, reachable at the 1M default). The fallback add is now guarded like the salvage path, so the cap surfaces as a partial result on the next read. - Undefined-length encapsulated pixel data was scanned to stream.length rather than the enclosing item/dataset bound (the review-#4 fix covered scanUnknown but not the encapsulated scanner). Nested in a sequence item with a missing FFFE,E0DD, the fragment scan read a following root sibling's bytes as fragments. scanEncapsulatedPixelData now takes an explicit frameBound (kept separate from the defined-length `end` so resume semantics are unchanged); scanFragments reports missingDelimiter and the caller emits the warning. Both have byte-level regression tests that throw / swallow the sibling without the fix. Full suite 703 passed; lint/typecheck/format clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fdQkZCohJHGNBjEMbk8ga
There was a problem hiding this comment.
Pull request overview
This PR tightens tokenizer error-containment and byte-bounding behavior to preserve the parse() never-throws contract and prevent nested undefined-length encapsulated Pixel Data scans from consuming bytes beyond their enclosing item.
Changes:
- Guarded the speculative sequence fallback path so
maxElementsno longer throws out ofrun()’s catch, preserving partial results. - Added an explicit
frameBoundtoscanEncapsulatedPixelDataand propagated it from the tokenizer so nested encapsulated scans can’t run tostream.lengthwhen delimiters are missing. - Added two §3 regression tests and documented the fixes in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tokenizer.ts | Guards fallback materialization and passes an enclosing bound into encapsulated scanning for undefined-length Pixel Data. |
| src/encapsulated.ts | Accepts an enclosing frameBound, scans fragments to an explicit bound, and reports missing-delimiter warnings via the caller-visible warnings sink. |
| src/tokenizer.test.ts | Adds regressions for fallback/maxElements never-throwing and nested undefined-length encapsulated Pixel Data bounding. |
| CHANGELOG.md | Records the two correctness fixes as user-facing changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
209
to
+227
| @@ -217,6 +224,7 @@ class Tokenizer { | |||
| endOffset: frame.header.dataOffset + frame.header.lengthField, | |||
| hadUndefinedLength: false, | |||
| }); | |||
| this.salvaging = prevSalvaging; | |||
MichaelLeeHobbs
added a commit
that referenced
this pull request
Jul 24, 2026
Substantive: - tokenizer (#26 follow-up): the salvaging guard around the fallback addElement could let structureCount exceed maxElements without ever surfacing limit-exceeded when the fallback was the last element read (EOF / pre-stopAt) — contradicting the resource-bound contract. Removed the guard; run()'s recovery call site now catches a limit-exceeded from recoverToFallback and surfaces it as a partial result, consistent with how every other cap trip is reported. Repro'd: merged code returned ok/no-error with the cap exceeded; fixed reports limit-exceeded with the pre-limit elements retained. - pixelData (#28 follow-up): nativePixelDataView assumed a little-endian host. Now byte-swaps when dataset order differs from the detected host order, so it is correct on a big-endian host too. - errors (#30 follow-up): isDicomError also requires `instanceof Error` (reliable across the dual ESM/CJS build, both extend the one global Error), so a bare branded plain object is rejected. Test updated to a real cross-build Error. Minor: - parse: removed a leftover duplicate JSDoc above NATIVE_TRANSFER_SYNTAXES (#28). - dataSet: floatStrings/intStrings docs say "absent or empty"; added a NaN-contract test for empty/non-numeric components (#32). - valueParsers: isValidUid doc references DicomDataSet.string() concretely (#32). - PLAN: disambiguated field-review W7/W13 from the adversarial-review W-series (#32). - tokenizer.test: D1 title notes the input is conformant (#25). Copilot's `unique symbol` concern on errors.ts is a false positive — it typechecks clean on ts7 + typescript@6 and the annotation is required for the computed-key brand field. Full suite 717 passed; lint/typecheck/format/attw clean. Claude-Session: https://claude.ai/code/session_015fdQkZCohJHGNBjEMbk8ga Co-authored-by: Claude Opus 4.8 <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.
Summary
Two correctness fixes from the MedFusion field review §3 (both byte-level verified against the reverted code):
parse() could throw despite the never-throws contract
recoverToFallbackruns insiderun()'s catch. TheaddElementthat materializes the opaque fallback value could itself tripmaxElementsand throwlimit-exceededout of the catch — escapingparse(). Repro'd withmaxElements=1; reachable at the 1M default. The fallback add is now guarded like the salvage path, so the cap surfaces as a partial result with alimit-exceedederror on the next read instead.Nested undefined-length encapsulated pixel data was bounded by the whole stream
The review-#4 fix bounded
scanUnknownbyframe.boundbut not the encapsulated scanner. An undefined-length(7FE0,0010)nested in a sequence item with a missingFFFE,E0DDscanned tostream.length, reading a following root sibling's bytes as fragments.scanEncapsulatedPixelDatanow takes an explicitframeBound(kept separate from the defined-lengthend, so #6 resume semantics are unchanged);scanFragmentsreportsmissingDelimiterand the caller emits the warning.Verification
src/tokenizer.test.ts— without the fix, the first throwslimit-exceeded, the second loses the(0010,0010)sibling.🤖 Generated with Claude Code
https://claude.ai/code/session_015fdQkZCohJHGNBjEMbk8ga