feat: P10 header, inflate strategy, parse/parseAsync entry points - #6
Merged
Conversation
Phase 1 PR 4/5. Adds readPart10Header (preamble/DICM detection, explicit-LE meta group via the tokenizer's stopAt, headerless datasets via the transferSyntax option — #48, missing-preamble tolerance), the three-path inflate strategy (injected inflater / node:zlib via process.getBuiltinModule / DecompressionStream('deflate-raw') in parseAsync) killing the pako global-sniffing crash class (cornerstonejs#270/cornerstonejs#125/cornerstonejs#109), and parse()/parseAsync() returning a ParseResult with meta + dataset + typed error + partial results (cornerstonejs#203). GE private DLX transfer syntax is recognized and reported as unsupported (cornerstonejs#107). Meta-group truncation is a hard error (a clamped Transfer Syntax UID could silently parse wrong). Ports parseDicom_test (BE/LE explicit, implicit, raw) and the p10 synthetic builder from dcmtk.js; adds a real-fixture gate over testImages/ (CT1_UNC variants vs DCMTK ground truth, deflated files on both inflate paths, encapsulated single/multi-frame, whole-corpus smoke). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSSLWZvjSRByQP3KsghQcJ
There was a problem hiding this comment.
Pull request overview
Adds Phase 1 parsing entry points and core Part-10/deflate handling, establishing a safer header reader and a multi-environment inflate strategy while expanding fixture-backed coverage.
Changes:
- Introduces
readPart10Header()(Part-10 preamble/DICM+ meta group parsing with headerless override support). - Adds deflated-transfer-syntax inflate strategy (
options.inflate→node:zlibviaprocess.getBuiltinModule→DecompressionStream) and newparse()/parseAsync()entry points returning typed partialParseResult. - Ports/extends tests with synthetic builders plus a real-fixture “gate” over
testImages/.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/helpers/p10.ts | Adds synthetic Part-10 builders (including deflated + SQ helpers) used across new tests. |
| tests/fixtures.test.ts | Adds real-corpus fixture gate and smoke coverage over testImages/. |
| src/part10.ts | Implements Part-10 header/meta parsing and readUiString() helper. |
| src/part10.test.ts | Adds unit tests for Part-10 header parsing and readUiString(). |
| src/parse.ts | Adds parse()/parseAsync() entry points and ParseResult structure. |
| src/parse.test.ts | Adds transfer-syntax, deflate, and option-behavior tests for new parse entry points. |
| src/inflate.ts | Implements the three-path raw-deflate inflate strategy for deflated TS. |
| src/index.ts | Exports new public APIs (header reader, inflate helpers, parse entry points, TS constants). |
| src/errors.ts | Extends ParseWarningCode with missing-preamble. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+73
to
+83
| export async function inflateRawAsync(deflated: Uint8Array, inflate?: InflateFn): Promise<Uint8Array> { | ||
| if (inflate !== undefined || hasSyncInflate()) { | ||
| return inflateRaw(deflated, inflate); | ||
| } | ||
| try { | ||
| const stream = new Blob([deflated as BlobPart]).stream().pipeThrough(new DecompressionStream('deflate-raw')); | ||
| return new Uint8Array(await new Response(stream).arrayBuffer()); | ||
| } catch (cause) { | ||
| throw new DicomError('malformed', 'deflated transfer syntax: inflate failed (corrupt deflate stream)', { cause }); | ||
| } | ||
| } |
Comment on lines
+50
to
+60
| export function readUiString(bytes: Uint8Array, dataOffset: number, length: number): string { | ||
| let end = dataOffset + length; | ||
| while (end > dataOffset && (bytes[end - 1] === 0x00 || bytes[end - 1] === 0x20)) { | ||
| end--; | ||
| } | ||
| let result = ''; | ||
| for (let i = dataOffset; i < end; i++) { | ||
| result += String.fromCharCode(bytes[i] as number); | ||
| } | ||
| return result; | ||
| } |
MichaelLeeHobbs
added a commit
that referenced
this pull request
Jul 23, 2026
…elimiters as terminators Adversarial-review findings #1-#7 — a cluster of silent data-loss/corruption bugs on malformed input (the attack surface flagged by SECURITY.md), all sharing one root cause: child constructs were bounded by the whole stream rather than their enclosing item/sequence, and the delimiter-terminator set was incomplete. - #1/#3/#7: delimitation items are now structural terminators at element boundaries in any item frame — FFFE,E00D ends and is consumed; FFFE,E0DD ends the item without consuming (it belongs to the sequence). Previously a stray/mis-set delimiter was read as an element 'xfffee00d'/'xfffee0dd' and the item swallowed every following root element to EOF. - #4: scanUnknown is bounded by frame.bound; an undefined-length non-sequence element can no longer eat its siblings to end-of-stream when its delimiter is missing. - #5: sequence items are bounded by their enclosing sequence, not the stream; an overlong item length is clamped (warning) instead of pulling in siblings, removing the asymmetry with readValue/pushSequence recovery. - #2: the encapsulated basic offset table is bounded by the value end, not the stream, so a defined-length overrun no longer reads the next element's bytes as offset entries (falls back to opaque via the speculative frame). - #6: defined-length encapsulated pixel data always resumes exactly at the value end, so trailing padding can't surface as phantom elements. Six regression tests added; full suite (625) green including the byte-identical corpus round-trip (no offset-model regressions). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSSLWZvjSRByQP3KsghQcJ
MichaelLeeHobbs
added a commit
that referenced
this pull request
Jul 23, 2026
…elimiters as terminators (#15) Adversarial-review findings #1-#7 — a cluster of silent data-loss/corruption bugs on malformed input (the attack surface flagged by SECURITY.md), all sharing one root cause: child constructs were bounded by the whole stream rather than their enclosing item/sequence, and the delimiter-terminator set was incomplete. - #1/#3/#7: delimitation items are now structural terminators at element boundaries in any item frame — FFFE,E00D ends and is consumed; FFFE,E0DD ends the item without consuming (it belongs to the sequence). Previously a stray/mis-set delimiter was read as an element 'xfffee00d'/'xfffee0dd' and the item swallowed every following root element to EOF. - #4: scanUnknown is bounded by frame.bound; an undefined-length non-sequence element can no longer eat its siblings to end-of-stream when its delimiter is missing. - #5: sequence items are bounded by their enclosing sequence, not the stream; an overlong item length is clamped (warning) instead of pulling in siblings, removing the asymmetry with readValue/pushSequence recovery. - #2: the encapsulated basic offset table is bounded by the value end, not the stream, so a defined-length overrun no longer reads the next element's bytes as offset entries (falls back to opaque via the speculative frame). - #6: defined-length encapsulated pixel data always resumes exactly at the value end, so trailing padding can't surface as phantom elements. Six regression tests added; full suite (625) green including the byte-identical corpus round-trip (no offset-model regressions). Claude-Session: https://claude.ai/code/session_01DSSLWZvjSRByQP3KsghQcJ Co-authored-by: Claude Fable 5 <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.
Phase 1 PR 4/5 (core tokenizer rewrite, PLAN.md §7).
What
src/part10.ts—readPart10Header: preamble +DICMdetection, meta group parsed explicit-LE via the tokenizer'sstopAt(stops before the first non-group-0002 tag), first-class headerless datasets viaoptions.transferSyntax(test(ci): add a DCMTK dcm2xml read differential as an independent oracle #48). Missing preamble withDICMat 0 is tolerated with a warning. Meta-group value truncation is a hard error — a clamped (0002,0010) could silently yield a valid-looking prefix UID and mis-parse the whole dataset.src/inflate.ts— the three-path inflate strategy (why dicomParser does not have pako as dependency cornerstonejs/dicomParser#270/Fatal error when parsing image cornerstonejs/dicomParser#125/Status: Error - TypeError: Cannot read property 'module' of undefined (file of size 4.046 MB ) cornerstonejs/dicomParser#109): injectedoptions.inflate→node:zlibviaprocess.getBuiltinModule(no static import; browser bundles never see it) →DecompressionStream('deflate-raw')(async path). Kills the pako global-sniffing crash class.src/parse.ts—parse()/parseAsync()returningParseResult:metaanddataSetkept separate (the Phase 4 compat façade re-merges), typederrorwith partial results (Fix parsing of truncated byte arrays cornerstonejs/dicomParser#203),stoppedAt, andbytes(the spliced header+inflated array for deflated files, so offsets always resolve). GE private DLX TS is recognized and reportedunsupported(Clarify 1.2.840.113619.5.2 [Implicit VR Big Endian DLX (G.E Private)] support cornerstonejs/dicomParser#107).Tests
parseDicom_test(explicit BE/LE, implicit, raw headerless).tests/helpers/p10.ts, an asset-to-port from docs/porting-notes.md), extended with undefined-length SQ + deflated builders.testImages/: CT1_UNC explicit-LE/BE/implicit against DCMTK ground truth (Rows/Columns 512, PixelData 524288), all three deflated files on both inflate paths (zlib + DecompressionStream via stubbedgetBuiltinModule), encapsulated single-/multi-frame BOT/no-BOT variants, and a whole-corpus smoke test (every file parses with no error).236 tests, 97.7% stmts / 94.8% branch.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DSSLWZvjSRByQP3KsghQcJ