fix(cli): refuse --fastqc where no report is written, wire --fastqc_args (#421) - #432
Merged
Conversation
…rgs (#421) `--fastqc` was accepted and silently ignored on five dispatch arms: the run exited 0, wrote its normal output, produced no QC report and never mentioned FastQC. A wrapper passing the flag unconditionally got no QC and no warning. Five arms reach no `fastqc::run` call site -- `--hardtrim5`, `--hardtrim3`, `--clock`, `--implicon` (src/specialty.rs has no FastQC at all), and paired FASTQ output from a single interleaved uBAM (`run_paired_ubam_single_file`). They are now refused in `Cli::validate`, matching the house style for unsupported combinations. Arm 5's condition needs the output format, not just the input shape: `--paired --output-format ubam <one.bam>` routes above it into `run_ubam_output_paired_single_file`, which DOES run FastQC, and CI asserts that it produces a report. Refusing on `paired && input.len() == 1` alone would have removed a working feature. `--clump_only` is excluded so main.rs's own "requires two FASTQ input files" diagnosis still wins, which names the actual remedy. The guard block is the last fallible check in `validate`. Sited any earlier it masks messages a user can act on: a malformed `--hardtrim5` value, an odd input count, a duplicated mate, a malformed `--adapter2` spec. A malformed value should report before an unsupported combination -- the argument main.rs:344-351 already makes. Dual plan review found the same defect from the other side, so it is fixed here rather than deferred. The gate was not uniform: the five main.rs sites tested `cli.fastqc || cli.fastqc_args.is_some()`, but the four `--clump_only` drivers took a `fastqc: bool` fed with bare `cli.fastqc`, so `--fastqc_args` alone was a silent no-op on a path that is otherwise capable. `Cli::fastqc_requested()` now carries the predicate once and is used at all ten sites, making `--fastqc_args`' documented "Implies --fastqc" true everywhere. The drivers' parameter is renamed `fastqc_requested`: a parameter named `fastqc` is what invited `cli.fastqc` at the call site, and the rename is what stops this recurring. #422's tripwire loses its `fastqc_capable` scoping, which existed as a workaround for this defect. The surviving rule -- a FastQC-flagged run must write artefacts -- is now unconditional across every case, and two new cases pin the refusals and the sixth arm. Reverting the pass-throughs makes the latter fail with "a FastQC flag is set but no FastQC artefact was written", so the mechanism that missed the original defect now covers it. Tests: 663 -> 684. Docs corrected in four places, including a blanket "works on both output paths" promise and a report count that omitted `--passthrough`'s carrier.
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.
--fastqcwas accepted and silently ignored on five dispatch arms — the run exited 0, wrote its normal output, produced no QC report and never mentioned FastQC, so a wrapper passing the flag unconditionally got no QC and no warning. Those five (--hardtrim5,--hardtrim3,--clock,--implicon, and paired FASTQ output from a single interleaved uBAM) reach nofastqc::runcall site at all, and are now refused inCli::validaterather than ignored. Dual plan review found the same defect from the other side, so it is fixed here too: the four--clump_onlydrivers took afastqc: boolfed with barecli.fastqc, which made--fastqc_argsalone a silent no-op on a path that is otherwise capable.Cli::fastqc_requested()now carries that predicate once and is used at all ten sites, so--fastqc_args' documented "Implies --fastqc" is finally true everywhere. 663 → 684 tests.Addresses #421.
AI-assisted detail
Arm 5 needs the output format, not just the input shape.
--paired --output-format ubam <one.bam>routes above the interleaved-FASTQ arm intorun_ubam_output_paired_single_file, which does run FastQC —ci.yml:1173andtests/integration_ubam_out.rs:572both assert it produces a report. A condition ofpaired && input.len() == 1alone would have refused a working feature and turned CI red.--clump_onlyis excluded somain.rs:599's "requires two FASTQ input files" still wins, which names the actual remedy.Guard placement is load-bearing and took two attempts. The block is the last fallible check in
validate. Sited earlier it masks messages the user can act on — a malformed--hardtrim5value, an odd input count, a duplicated mate, a malformed--adapter2spec. The first implementation put it after the input-existence loop, which still masked the--adapter2parse error; that is pinned now bymalformed_adapter2_precedes_the_fastqc_refusal. The principle is the onemain.rs:344-351already argues: a malformed value reports before an unsupported combination.The gate was never uniform, which is what hid the sixth arm. Five
main.rssites testedcli.fastqc || cli.fastqc_args.is_some(); five others passed barecli.fastqcinto the--clump_onlydrivers. The original plan asserted the disjunction held at all twelve call sites on the strength of grepping forfastqc::run— grep finds calls, not guards. The drivers' parameter is renamedfastqc_requested, because a parameter namedfastqcis what invitedcli.fastqcat the call site.#422's tripwire loses its FastQC allowance scoping, which existed as a workaround for this defect (its own comment said so). The surviving rule — a FastQC-flagged run must write artefacts — is now unconditional across all 55 cases instead of being scoped away from five arms. Two new cases: one looping over every refused arm asserting non-zero exit, the message, and that nothing was written; and
clump_only_se_fastqc_args_only, which puts the sixth arm under that rule. Reverting the five pass-throughs makes the latter fail with "a FastQC flag is set but no FastQC artefact was written" — the mechanism that missed the original defect now covers it.Verification. Both code reviewers hunted specifically for over-rejection (the only way this change can do harm) and neither found an invocation that works on the base commit and now fails; one swept 16 invocations through a purpose-built binary. Nine mutations were designed against the guards and all nine go red. A coverage audit re-measured every claimed number, including reproducing both pre-fix defects on a binary it built from the unpatched base itself.
Docs corrected in four places, including a blanket "works on both FASTQ and uBAM output paths" promise, a report count that omitted
--passthrough's carrier report, and a--helpremedy that offered--output-format ubamfor four arms where it does nothing.