Repository navigation
feat: @dcmjs-org/dicomdir assembly — DICOMDIR reading and writing - #575
Draft
awatson1978 wants to merge 146 commits into
Draft
awatson1978 wants to merge 146 commits into
awatson1978 wants to merge 146 commits into
Conversation
Supersedes the 2026-08-07 plan. Describes the package split, the 1.0-beta release branch, per-package staging branches, the test-first sequence for core, routing of the 33 review findings, and lockstep publishing with semantic-release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two trivial assertions on the committed sample-sr.dcm fixture. The purpose is to exercise the CI checks on the 1.0-beta branch and prove they run and report on pull requests. See RELEASE_PLAN.md section 6, step 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e 24 fs.readFileSync can return a small file inside a larger shared memory pool. Taking .buffer on that result hands the parser the whole pool, with the file sitting at a nonzero offset, and the DICM marker check then fails. Node 24 pools more aggressively, which is why anonymizer.test.js and data.test.js fail there today (7 tests) while Node 22 passes by luck of alignment. This adds readFileAsArrayBuffer() to testUtils and routes all 22 file-read sites through it. Typed-array .buffer uses are untouched. Same class of problem as issue #311. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ware helper The first pass converted single-line fs.readFileSync(...).buffer sites. CI on Node 24 then failed in a second population: reads assigned to a variable first, with .buffer taken on a later line. This converts those sites in data.test.js, data-options.test.js, lossless-read-write.test.js and normalizers.test.js, and drops the imports that became unused. Sites that already handle byteOffset correctly are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ensureTestDataDir checked for the directory and then created it. Two parallel jest workers can both pass the check, and the slower mkdir then fails with EEXIST, which failed test_multiframe_1 on one CI leg. mkdirSync with recursive:true succeeds whether or not the directory exists, so the check goes away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three process documents the release plan calls for. The style guide defines the audience (healthcare-literate, no assumed programming or imaging jargon) and bans machine-flavored prose. The PR template requires plain-language descriptions, runnable examples, test evidence, and fixture citations. DATACITATION.md records the origin and license of every committed DICOM test file, with the legacy files honestly marked as provenance-pending. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
semantic-release computes one version from conventional commits; stamp-workspace-versions.mjs writes it into every publishable workspace package.json, and pnpm -r publish releases them together under the npm beta tag. The publish workflow now also fires on 1.0-beta and release/0.5x, and passes NODE_AUTH_TOKEN for pnpm publish. Nothing publishes until the repository owner adds 1.0-beta to the publish environment allowlist. See RELEASE_PLAN.md sections 9 and 12. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adapts the rules from Cursor's unslop skill: the fuller AI-vocabulary list, filler phrases, fancy synonyms for is, vague attributions, synonym cycling, false ranges, punctuation and formatting rules (em dashes, colons, bold, sentence-case headings, straight quotes), abstract metaphor nouns, and a plain-speech section that includes the rule against over-compressed arrow-speak. Also states the boundary this guide draws that unslop does not: metaphors that teach a concept to a non-technical reader stay, metaphors that decorate go. The guide's own prose now follows its own em-dash rule. Source: https://github.com/cursor/plugins/blob/main/pstack/skills/unslop/SKILL.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adopts wayfarer3130's review comments: the engines split from the data types (core = building blocks only; legacy = today's readers and writers; parser = the new streaming reader; generator = the new writers). The vendored dicom-parser tokenizer is dropped from the 1.0 stream entirely, and about 300 tests retire with it. @dcmjs/media is added as a bundle over dicomdir, pdfs, and video, which all remain individually installable. The wrapper publishes as @dcmjs/dcmjs during the beta so the real dcmjs name stays untouched until the team is ready. FHIR stays in the plan with its landing order marked as an open question. Section 7 is rewritten to the simpler tests-travel-with-code approach discussed since the first draft. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s sequencing Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e git mv) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hrough fixturePath() Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
refactor: move committed test DICOM fixtures into packages/fixtures
…regression bank Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test: add the issue triage catalog and part10 walker for the regression bank
…avior Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gth behavior Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rdering Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d strict-write Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… value-handling behaviors Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test: pin round-trip integrity, UUID-derived UIDs, and UN padding behavior
test: pin encapsulated write, latin1 fallback, and CS multi-value length behavior
test: pin private tag handling, undefined value writes, and element ordering
refactor: split the eager and async engines into packages/legacy
Adds packages/parser/package.json and README.md, following the same scaffold precedent as @dcmjs/core and @dcmjs/legacy (private, version 0.0.0 until the publish switch flips). The package depends on @dcmjs/core via workspace:* plus pako (the streaming reader inflates deflated transfer syntaxes); the lockfile picks up the new project. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ader seam Preparation for moving the streaming reader into packages/parser, where it must carry no static edge to @dcmjs/legacy: - singleVRs (the canonical element-shaping list, the one with LT) moves from DicomMessage to core's constants/dicom.js; DicomMessage imports and re-exports it, so its historic surface is unchanged - DicomMessage.lookupTag's body moves to DicomMetaDictionary.lookupTag (it was a one-line read of DicomMetaDictionary.dictionary, which is core's); the DicomMessage static delegates to it - ValueRepresentation gains getDicomMessageClass(), the read side of the late-binding slot it already carries, so the streaming reader's narrow eager-delegation fallbacks can resolve the eager reader at call time instead of importing it - legacy's DicomMessage module wires the ValueRepresentation / Tag / DicomDict seams at load, not only via the dcmjs wrapper index, so direct module consumers keep exactly the reach they had when the streaming modules imported DicomMessage statically (the wrapper's identical calls stay; the setters are idempotent) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…re git mv) The contract (EventStreamListener, the vocabulary, CONTRACT_VERSION, mergeFragmentsPerBotWindow), the collecting and naturalizing listeners, the shared emit helpers, the async-iterator adapter, the four event generators (fromPart10, fromPart10Stream, fromDicomWebJson, fromDataSet), and the element-decode core move under packages/parser/src/ with their relative structure preserved. Zero edits; git records every file as an R100 rename. The wrapper-level glue (src/eventStream/api.js and index.js) and the writers stay put for the follow-up rewiring and generator-package commits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Hand-written rewiring for the pure-move commit before it: - the moved modules import their building blocks from @dcmjs/core by name instead of relative paths; sibling imports inside the package stay relative and needed no edits - the two deliberate eager-reader delegations — decodeCore's decodeWithEagerReadTag (DicomMessage._readTag over rare element shapes) and fromPart10Stream's bare-dataset fallback (DicomMessage.readFile) — now resolve the eager reader at call time through core's late-binding slot (requireEagerReader over ValueRepresentation.getDicomMessageClass), with a clear error when no eager reader is loaded, so this package carries no static edge to @dcmjs/legacy - a shim at each old path under src/ re-exports from the new packages/parser location; the fromPart10, fromPart10Stream, and decodeCore shims also side-effect import ../DicomMessage.js so direct consumers of the old paths keep exactly the reach the static import gave them - packages/parser/src/index.js exports the package surface (the contract, the listeners, the sources, and the emit helpers); the eventStream barrel and the DicomEventStream facade stay in root src/ as wrapper-level glue, unchanged - core's index additionally exports lookupPrivateTag, the packed private dictionary's one by-name function, for the naturalized listener Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
refactor: split the event-stream reading side into packages/parser
Adds packages/generator/package.json and README.md, following the same scaffold precedent as the core, legacy, and parser packages (private, version 0.0.0 until the publish switch flips). The package depends on @dcmjs/parser (the contract and the collector), @dcmjs/core (the write primitives), and — temporarily, for Part10Writer's delegation to the canonical DicomDict.write encoder only — @dcmjs/legacy; the README spells that edge out. The lockfile picks up the new project and links. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Preparation for moving the writer sinks into packages/generator, so the streaming writer needs no edge to @dcmjs/legacy: - DicomMessage.write, writeTagObject, and _getTagWriteValues move verbatim into core's core/writeCore.js (as writeDataSet, writeTagObject, getTagWriteValues) — everything they touch is core's: Tag.write drives the element encoding through ValueRepresentation, deepEqual backs the _rawValue fidelity check. The DicomMessage statics delegate, so every existing caller (DicomDict, the tests, external users) is unaffected - DicomMessage.isEncapsulated's body moves to isEncapsulatedSyntax in core's constants/dicom.js, next to the syntax table it reads, and DicomMessage._normalizeSyntax now delegates to core's existing normalizeSyntax instead of duplicating it; both statics remain as delegating surfaces - Tag.write resolves those two helpers directly instead of through its late-binding DicomMessage slot, which now has no readers inside Tag (the setter stays for API compatibility) - ValueRepresentation's sequence-item writer keeps calling the late-bound DicomMessage.write slot rather than importing writeCore: Tag.js wires ValueRepresentation.setTagClass at module-evaluation time, so a static ValueRepresentation -> writeCore -> Tag edge would recreate exactly the cycle the slots exist to avoid Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(pure git mv) Pure file moves, zero edits: Part10Writer, StreamingPart10Writer, and DicomWebJsonWriter record as R100 renames into packages/generator/src/eventStream/. The rewiring commit in this PR repoints their imports at @dcmjs/parser, @dcmjs/core, and (for Part10Writer's DicomDict delegation only) @dcmjs/legacy, and adds shims at the old paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Hand-written rewiring for the pure-move commit before it: - the writers import the contract from @dcmjs/parser (EventStreamListener, CollectorListener) and the write machinery from @dcmjs/core (WriteBufferStream, the syntax constants, writeDataSet, writeTagObject); StreamingPart10Writer and DicomWebJsonWriter carry no other edges - Part10Writer keeps its by-design delegation to the canonical DicomDict.write encoder, now as an explicit — and TEMPORARY — @dcmjs/legacy import, flagged in the package README and the PR - a shim at each old path under src/eventStream/ re-exports from the new packages/generator location, so every existing import in src/, test/, and the barrel keeps working unchanged - packages/generator/src/index.js exports the three sinks; the eventStream barrel and the DicomEventStream facade stay in root src/ as wrapper-level glue, unchanged Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
refactor: split the event-stream writer sinks into packages/generator
The @dcmjs npm organization is not ours; per review discussion the packages publish under @dcmjs-org (matching the GitHub organization), and the team can reconsider the shorter scope if its owner responds. Every package name in the plan now reads @dcmjs-org/. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
refactor: publish the workspace packages under the @dcmjs-org scope
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@dcmjs-org/core, @dcmjs-org/legacy, @dcmjs-org/parser, and @dcmjs-org/generator drop their scaffold-era private flags so the lockstep publish can ship them. @dcmjs-org/fixtures stays private per RELEASE_PLAN section 11, and the root package stays private until the wrapper PR. Publishing remains gated behind the publish GitHub environment — merging this changes nothing on npm. BREAKING CHANGE: dcmjs 1.0 replaces the single monolithic package with the @dcmjs-org scoped package set (core, legacy, parser, generator, schemas). The legacy package preserves today's reader and writer surfaces; the event-stream engines are new. See RELEASE_PLAN.md and docs/BREAKING_CHANGES.md for the behavior changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat: mark the engine packages publishable
package.json modeled on @dcmjs-org/core's (version 0.0.0, publishable, MIT, main ./src/index.js, repository directory), depending on @dcmjs-org/core at runtime and on @dcmjs-org/legacy only as a devDependency for test readback. Plus a short README with a usage sample. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
src/media/dicomdir.js, src/media/directoryOffsets.js, and
src/media/index.js from the v2.0-development branch land as
packages/dicomdir/src/{dicomdir,directoryOffsets,index}.js, and
test/dicomdir.test.js as packages/dicomdir/test/dicomdir.test.js —
byte-for-byte v2 content. Imports still point at the v2 tree; the next
commit rewires them onto @dcmjs-org/core and replaces the DicomDict
dependency.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The two legacy imports the v2 code carried are gone, keeping the hard
rule that this package never depends on @dcmjs-org/legacy:
- directoryOffsets.js measured element sizes through DicomMessage.write
and DicomMessage.writeTagObject; it now calls core's writeDataSet and
writeTagObject directly — the exact functions those statics are
one-line delegates to, so measured sizes are unchanged.
- dicomdir.js built a DicomDict and called dicomDict.write(); the Part
10 envelope (preamble + DICM, meta group behind its group length,
then the body) is now inlined as writePart10 on the same core
primitives, minus the deflate branch a DICOMDIR can never take.
Verified byte-identical against legacy DicomDict.write on the same
denaturalized { meta, dict }.
The test readback oracle rewires from the root dcmjs facade to
DicomMessage from @dcmjs-org/legacy (devDependency) plus
DicomMetaDictionary and validationLog from @dcmjs-org/core, and the
local EXPLICIT_LITTLE_ENDIAN constant gives way to core's export.
Full suite: 860 passed, 12 skipped, 872 total.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat: port the DICOMDIR builder into packages/dicomdir
Writing a DICOMDIR from @dcmjs-org/dicomdir alone threw: core's SequenceOfItems.writeBytes serialized each item through the late-bound DicomMessage, which only @dcmjs-org/legacy (or the dcmjs wrapper) registers — so the package's hard no-legacy rule held in its imports but not at runtime. The committed suite never saw it because its readback oracle imports legacy, wiring the seam as a side effect. core/writeCore.js now registers writeDataSet into ValueRepresentation at load (a direct import between the two files is a module-level CJS cycle that breaks Tag's registration, hence the seam), and the sequence writer calls that instead of DicomMessage.write — the exact function DicomMessage.write delegates to, so bytes are unchanged either way. The new packages/dicomdir/test/standalone.test.js imports nothing from legacy (jest's per-file module registry guarantees the seam stays unwired) and proves a full DICOMDIR write: DICM magic, four undefined-length item headers for PATIENT/STUDY/SERIES/IMAGE, deterministic bytes. Full suite: 861 passed, 12 skipped, 873 total. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fix: let core's sequence writer run with no eager engine loaded
This was referenced Oct 2, 2026
…ented GHSA-vfj7-8cjw-p6xm was published 2026-10-02 with a claimed fix range of >=3.0.4, but no such braces release exists on the registry, so there is nothing to override to. Every path is dev tooling; braces never reaches the published packages. Same entry the fhir staging branch already carries (#589) — remove it the moment a patched braces ships. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 7261970)
Per review on #542: the guide existed to hold the staged-landing PR descriptions to one standard, and every description is now written. Removed rather than kept stale; git history preserves it if the team wants it back later. The PR template keeps the one-line spirit of it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 2a5935e)
This branch has not been deployed
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.
This is the assembly PR for the dicomdir package: it brings
release/1.0-beta-dicomdirinto1.0-beta, carrying wave PR #573 and its follow-up #580 on top of the core branch. Like the other assemblies (#542, #543) it's open as a draft on purpose — please don't merge it yet; the landing order in RELEASE_PLAN.md section 6 means the assemblies go in deliberately, not as soon as they're green.What this is in plain terms. A DICOMDIR is the index file at the root of a patient CD, DVD, or USB stick — the Media Storage Directory of PS3.10 / PS3.3 Annex F. It's what lets a workstation show the patients, studies, and series on the disc without opening every file.
@dcmjs-org/dicomdirbuilds and writes one: you handwriteDicomDira flat list of file-set entries (one per file on the media, with its path, SOP identity, and patient/study/series attributes), and it builds the PATIENT → STUDY → SERIES → leaf directory record hierarchy, computes the real byte offsets that chain the records together, and serializes the complete Part 10 file in a single pass. The interesting engineering fact is in how those offsets are computed: every DICOMDIR offset attribute is VR UL — a fixed four bytes — so the values never affect the layout, and one measurement pass with placeholder zeros yields the exact offsets of the final file. No patch-and-rewrite, no buffer scanning.The package depends on
@dcmjs-org/coreand nothing else at runtime — and that claim is now enforced, not just implied by the imports. The v2 code this was ported from leaned on what is now@dcmjs-org/legacyin two places (size measurement throughDicomMessage.write, and serialization throughDicomDict.write); both were rebuilt on core'swriteDataSet/writeTagObjectprimitives — the exact functions those legacy statics delegate to — and I verified the inlined Part 10 envelope byte-identical against legacyDicomDict.writeon the same denaturalized{ meta, dict }before relying on the argument (details in #573). Then, while verifying this PR's worked example in a clean process, I found core's sequence writer still reached for the late-boundDicomMessagethat only legacy registers — so the package worked only when legacy happened to be loaded. #580 fixes that at the root (core'swriteCore.jsregisterswriteDataSetinto the sequence writer's seam at load) and adds a standalone regression test that imports nothing from legacy and checks the written bytes directly.A worked example (run on this branch, in a process with no legacy engine loaded — that's exactly what
packages/dicomdir/test/standalone.test.jspins):And reading one back — a DICOMDIR is an ordinary Part 10 file, so any engine reader handles it; here's the streaming path (run on this branch, output verbatim):
To be plain about scope: the package exports the builder and writer; there is no dedicated DICOMDIR read API in it, because reading needs nothing special — the records come back as the
DirectoryRecordSequenceshown above through either engine. If a convenience walker (offset-chasing the lower/next record chains for you) turns out to be wanted, it would be a small additive follow-up here.buildDicomDirDatasetis also exported for inspecting the record tree before serialization.What's in it. Two PRs. #573 is the port: a scaffold commit (package.json + README), a verbatim v2 commit (three source files and the 16-test suite, byte-for-byte from
v2.0-development'ssrc/media/), and the hand-written rewiring onto@dcmjs-org/core. #580 is the standalone fix described above — a small core seam (setWriteDataSet, mirroring the existingDicomMessageseam) plus the no-legacy regression test. Hand-written totals are declared in each PR (~120 and ~100 lines); the ported lines are exempt per the budget convention.Test evidence. The 17 tests in
packages/dicomdir/test/are fully synthetic — no fixtures. They prove the property that matters by checking the written bytes themselves: every nonzero offset value must land exactly on anFFFE,E000item tag, the next/lower chains plus the first-root offset must address every record exactly once, the write must be deterministic, and the whole thing must work with no eager engine wired. Full suite on the branch tip: 861 passed / 12 skipped / 873 total (the 844-test core baseline plus these 17), on Node 22 and 24. CI from the merged PRs: #573 tests + build, #580 tests + build, lint/format.GitHub issues. Honestly: none. I searched the tracker for DICOMDIR and found nothing but the 1.0 RFC (#517) this whole effort serves — dcmjs never had a DICOMDIR builder, so there was no backlog to fix. This is new capability, ported from the v2 rewrite.
Breaking changes. None. The package is purely additive, and the one touch to existing code — the sequence-writer seam in #580 — routes item serialization to the very function
DicomMessage.writealready delegates to, so bytes are identical whichever package loads first; the full regression suite agrees.The trunk overlap, so the diff size isn't alarming.
release/1.0-beta-dicomdirwas cut from the tip ofrelease/1.0-beta-core(e5f46e4), so until the core assembly #542 merges, this PR's diff shows the whole engine stack it sits on. Once #542 is in, this shrinks to justpackages/dicomdir/plus the lockfile and the seam fix. That's the same absorb-the-trunk pattern as the earlier assemblies, and it's the other reason this stays a draft.🤖 Generated with Claude Code