fix(deposition): normalise raw-read file extensions to the validated set

[corneliusroemer-agent]

Make ENA submission work with what prior raw reads validation has checked and guarantees:
file ending is one of (".fastq.gz", ".fq.gz", ".fastq", ".fq") with arbitrary casing.

Fix double compression bug

download_fastq_files decided whether to compress with file_path.endswith((".gz", ".bz2")), which is case-sensitive. The upstream check in raw-reads-processing is not: _has_extension lower-cases before comparing, so READS.FASTQ.GZ is a perfectly legal submission today. It reached this code, failed the .gz test, and got gzipped a second time.

Make extensions lowercase so that webin is happy

ENA submission needs to lowercase extensions so that webin-cli accepts it (it compares suffixes with String.endsWith, so r.FASTQ.GZ is rejected outright with "Invalid FASTQ file suffix".

Misnamed extension should be rejected in raw reads validation not here

A genuinely gzipped file named reads.fastq still gets compressed twice. The right place to catch it is at submission: separate PR #7266

Worth deciding separately

If raw-reads-processing were narrowed to gzip-only the code would be simplified a lot. We would also save on storage, raw reads on Loculus/PPX would be more uniform and faster to download for everyone (and make upload for submitter faster). The trade-off is that submitters would have to create gzipped raw reads - but that's probably not an issue (and ENA requires gzip (or bz2) anyways so it's a normal requirement.

🚀 Preview: Add preview label to enable

[claude]

Claude finished @corneliusroemer's task in 2m 24s —— View job

---

Review

The core change is right, and it fixes more than the PR description claims. The old "".join(pathlib.Path(name).suffixes) was broken for any dotted basename, not just casing: my.sample.reads.fastq.gz produced the extension .sample.reads.fastq.gz, so the file landed on disk as <fileId>.sample.reads.fastq.gz. Matching against a fixed accepted set removes that whole class of problem. Deriving the extension from name while the basename comes from fileId also keeps the existing path-traversal protection intact.

Checks I made:

• ACCEPTED_FASTQ_EXTENSIONS here matches the upstream set in raw-reads-processing/src/raw_reads_processing/file_format_validation.py:21 exactly, and the lower-case-then-endswith matching mirrors _has_extension there, so nothing that passes submission validation is rejected here.
• Tuple ordering isn't load-bearing ("r.fastq.gz".endswith(".fastq") is false), so the tuple-vs-set difference between the two files is harmless.
• Dropping .bz2 from the compression check is correct — no accepted extension is .bz2, so the old branch was unreachable for it.
• RuntimeError matches how create_raw_reads.py:115's caller already handles failures from this function.

Three inline comments, all minor: hoisting the extension check above the download loop, one test that doesn't assert what its name claims, and an unspecced mock.Mock() config.

Two things not worth an inline comment:

• The duplicated constant has a comment here pointing at file_format_validation.py, but not the reverse. A one-line comment there pointing back would make drift visible to whoever edits the upstream set — that's the direction the drift will come from.
• Worth confirming webin-cli accepts .fq.gz and not only .fastq.gz. If it's the latter, reads.fq would need normalising to .fastq.gz rather than .fq.gz. Low risk, but test_uncompressed_fq_is_gzipped bakes the assumption in, so it's cheap to verify once.

I could not run the test suite — micromamba run -n loculus-ena-submission python -m pytest isn't in this job's allowed tools, so the new DownloadFastqFilesTests are unverified locally.

• Read PR diff
• Read call_loculus.py in full context
• Cross-check upstream file_format_validation.py and the caller
• Review tests and integration-test mock
• Post review
  · branch raw-reads-ext-normalisation

[Copilot]

🟡 Changes recommended

The new unit-test mock context manager can inadvertently suppress exceptions, and one updated comment is misleading relative to the enforced extension contract.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes ENA raw-read submission edge cases by normalizing FASTQ filename extensions to the same validated, case-insensitive set used upstream, preventing double-gzip for uppercase .GZ and ensuring webin-cli accepts the resulting filenames.

Changes:

• Introduce canonical_fastq_extension() and a local ACCEPTED_FASTQ_EXTENSIONS contract to lower-case and validate raw-read FASTQ suffixes before download/compression.
• Update download_fastq_files() to decide gzip behavior based on the canonicalized extension (avoiding double compression for e.g. READS.FASTQ.GZ).
• Improve unit/integration tests so mocks mirror production naming and so download/compression behavior is actually exercised.

File summaries

File	Description
ena-submission/src/ena_deposition/call_loculus.py	Adds canonical FASTQ extension validation/normalization and updates gzip decision logic.
ena-submission/test/test_ena_submission.py	Adds unit tests covering download_fastq_files() naming and compression behaviors.
ena-submission/test/test_ena_submission_integration.py	Aligns the integration mock’s output filenames with production behavior using canonicalized extensions.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[anna-parker]

@claude review

[claude]

Claude finished @anna-parker's task in 1m 37s —— View job

---

Review

• Read current state of call_loculus.py and tests
• Check diff against origin/main
• Verify whether prior review comments were addressed
• Post review

All the previously raised review comments (mine, Copilot's, and the discussion with @anna-parker) have been addressed in the current state of the branch (commit 124f7f2):

• Validation hoisted above the download loop (call_loculus.py:282): extensions are now computed for all files up front via a list comprehension and zipped with files (strict=True), so an unaccepted extension on file N is now rejected before any of the preceding files are downloaded — matches what test_unaccepted_extension_raises_before_download claims.
• Test now proves "before download" (test_ena_submission.py:820-832): patches requests.get and asserts get.assert_not_called().
• Mock hardened (test_ena_submission.py:773): self.config = mock.Mock(spec=Config). Confirmed raw_reads_metadata_field and s3_request_timeout_seconds are real fields on Config (config.py:137, :170), so this will correctly fail if a test starts relying on an attribute that doesn't exist.
• Context manager no longer swallows exceptions (test_ena_submission.py:787): response.__exit__.return_value = False is set explicitly.
• .bz2 comment reworded (call_loculus.py:295-296) to distinguish what webin-cli accepts from what upstream validation actually allows through, so the mention of .bz2 no longer implies it's a supported input extension. .bz2 support was discussed and intentionally left out per @anna-parker's comment.

Cross-checked against upstream again: ACCEPTED_FASTQ_EXTENSIONS in call_loculus.py:224 still matches raw-reads-processing/src/raw_reads_processing/file_format_validation.py:21 exactly, and canonical_fastq_extension's lower-then-endswith matching still mirrors _has_extension there (file_format_validation.py:49-52). No remaining correctness issues found.

One very minor, non-blocking nit: the comment at call_loculus.py:295-296 is now accurate but reads as a single run-on sentence ("webin-cli rejects any FASTQ whose name does not end in .gz or .bz2, upstream only accepts .gz, so gzip..."). Splitting it into two sentences would make the two separate facts (what webin-cli accepts vs. what upstream lets through) easier to parse at a glance. Not worth blocking on.

I could not run the test suite locally — the loculus-ena-submission micromamba environment isn't set up in this sandbox — so DownloadFastqFilesTests remains unverified by me beyond static reading. Given the fixes align with every specific concern raised in earlier reviews, this looks ready to merge from a correctness standpoint.
· branch raw-reads-ext-normalisation