Stop mass transfer partitions failing on an empty StudyTime

[samuelvkwong]

Summary

• Follow-up to Make mass transfer discovery resilient to non-conformant StudyDate/StudyTime #400. Mass transfer jobs 3 and 4 still had failed daily partitions (tasks 12553, 17994, 18018) with an empty task message. All three tracebacks end in _study_datetime → ResultDataset.StudyTime → convert_to_python_time at assert time_valuerep is not None: pydicom's TM("") returns None, so a study with an empty StudyTime raised a bare AssertionError. The handlers from Make mass transfer discovery resilient to non-conformant StudyDate/StudyTime #400 only catch ValueError, so that one study failed the whole partition, again on every retry.
• StudyDate and StudyTime are Type 2, so an empty value is valid PACS input, not an internal invariant (which is what the assertion guideline in CLAUDE.md is about). convert_to_python_date / convert_to_python_time now raise ValueError for empty values, as pydicom already does for malformed ones.
• The existing handling from Make mass transfer discovery resilient to non-conformant StudyDate/StudyTime #400 then applies: an empty StudyTime falls back to midnight and the study is transferred; an empty StudyDate skips only that study with a WARNING.
• Other readers of ResultDataset.StudyDate / StudyTime (selective transfer, batch query/transfer, DICOM explorer) are unchanged: they still fail on such a study, now with a ValueError naming the empty value instead of a bare AssertionError.

Test plan

• New test_convert_to_python_{date,time}_rejects_empty_value failed with AssertionError before the fix and pass after
• test_discover_series_salvages_malformed_study_time from Make mass transfer discovery resilient to non-conformant StudyDate/StudyTime #400 is now parametrized as test_discover_series_salvages_unparseable_study_time[non-conformant|empty]; the empty case reproduced the production traceback before the fix and now falls back to midnight
• Full suite without acceptance tests: 903 passed, 3 xfailed (existing dicom_web markers); acceptance tests not run locally
• ruff check ., pyright, djlint . --lint clean

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Empty DICOM date and time values now produce clear validation errors instead of assertion failures.
  • Mass transfer discovery now recovers study time as midnight when the time value is empty or malformed.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: e2f9cdd0-6457-4d24-b365-b7b21d8320d9

📥 Commits

Reviewing files that changed from the base of the PR and between 1e0f516 and 5b6f3d5.

📒 Files selected for processing (3)

• adit/core/tests/utils/test_dicom_utils.py
• adit/core/utils/dicom_utils.py
• adit/mass_transfer/tests/test_processor.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The date and time conversion functions now raise ValueError for empty values. Tests cover these errors and study-time discovery for empty and non-conformant values.

Changes

DICOM empty value handling

Layer / File(s)	Summary
Conversion errors and study-time discovery adit/core/utils/dicom_utils.py, adit/core/tests/utils/test_dicom_utils.py, adit/mass_transfer/tests/test_processor.py	The date and time converters raise ValueError when the converted value is None. Tests check empty-string inputs and verify study-time discovery for empty and non-conformant values.

Priority: ⚪ Not assessed

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: numericaladvantage

Merge Risk: ⚪ Minimal · up to 5b6f3

Empty dates are skipped with a warning, and empty times retain the midnight fallback. No merge-blocking risk was identified in the reviewed change.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 5b6f3

Empty study times can now be handled without failing a mass-transfer partition. The study still passes through the existing filters and transfer controls. The change affects operational behavior, but the review found no new security boundary or control bypass.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — An empty time in a returned DICOM study can now permit that study to proceed through discovery with midnight as derived metadata. It does not itself select a new source, destination, patient, or series, although it changes which previously failing study can reach transfer.

Trust Boundaries and Controls

• observed — The mass-transfer route retains configured DICOM nodes, required filters, and conditional pseudonymization. Its include and exclude checks remain outside the date/time conversion utility.

Resilience and Maintainability Implications

• observed — Retry cleanup targets the current partition’s output and volumes. Transfer continues to use persisted study and series identity, with per-volume failure handling; the midnight fallback does not supply transfer identity.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing mass-transfer partitions from failing when StudyTime is empty.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}