From 8e3d5c397ec9e6bd995551851bde1232e73020c7 Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 14:37:18 -0700 Subject: [PATCH 01/15] refactor(group): expose resolve_strategy_and_edits on GroupOptions --- src/lib/commands/group.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/lib/commands/group.rs b/src/lib/commands/group.rs index 068cd62ef..008f67e69 100644 --- a/src/lib/commands/group.rs +++ b/src/lib/commands/group.rs @@ -738,6 +738,19 @@ impl Default for GroupOptions { } } +impl GroupOptions { + /// Resolve the effective grouping strategy/edits: `--no-umi` forces + /// Identity/0; Identity forces edits 0; otherwise the requested pair. + #[must_use] + pub fn resolve_strategy_and_edits(&self) -> (Strategy, u32) { + if self.no_umi { + return (Strategy::Identity, 0); + } + let edits = if matches!(self.strategy, Strategy::Identity) { 0 } else { self.edits }; + (self.strategy, edits) + } +} + impl GroupReadsByUmi { /// The minimum mapping quality to apply, defaulting when the flag is absent. #[must_use] @@ -1486,6 +1499,24 @@ mod tests { assert_eq!(cmd.resolve_strategy_and_edits(), (expected_strategy, expected_edits)); } + /// [`GroupOptions::resolve_strategy_and_edits`] applies the same `--no-umi` + /// and identity-implies-zero-edits rules as [`GroupReadsByUmi::resolve_strategy_and_edits`], + /// but reads them from the already-projected `GroupOptions` fields, which + /// `runall`'s future stage config needs without going through the CLI struct. + #[rstest] + #[case::adjacency(false, Strategy::Adjacency, 2, (Strategy::Adjacency, 2))] + #[case::identity_forces_zero_edits(false, Strategy::Identity, 2, (Strategy::Identity, 0))] + #[case::no_umi_forces_identity(true, Strategy::Adjacency, 5, (Strategy::Identity, 0))] + fn group_options_resolve_strategy_and_edits( + #[case] no_umi: bool, + #[case] strategy: Strategy, + #[case] edits: u32, + #[case] expected: (Strategy, u32), + ) { + let o = GroupOptions { strategy, edits, no_umi, ..GroupOptions::default() }; + assert_eq!(o.resolve_strategy_and_edits(), expected); + } + /// `--min-map-q` is optional on the command line but not optional for /// grouping, so the projection applies the same default `execute` does. #[rstest] From 02180c42ac638e8ee78d7c3b095a0cfdedcbf9ed Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 14:42:56 -0700 Subject: [PATCH 02/15] feat(zipper): derive clap::Args + multi_options on ZipperOptions --- src/lib/commands/zipper.rs | 122 ++++++++++++++++++++++++++++++++++--- 1 file changed, 115 insertions(+), 7 deletions(-) diff --git a/src/lib/commands/zipper.rs b/src/lib/commands/zipper.rs index 371f79bf1..9d5560e6c 100644 --- a/src/lib/commands/zipper.rs +++ b/src/lib/commands/zipper.rs @@ -246,33 +246,86 @@ pub struct Zipper { /// rather than having the builder reach into [`Zipper`] — is what lets a chain /// be constructed with no CLI struct behind it at all. /// -/// This deliberately does **not** derive `clap::Args`. Flattening it into -/// [`Zipper`] would move the fields off that struct and rewrite every -/// `self.` reference in this module and its tests, which buys the chain -/// builder nothing — it only ever reads the values. That refactor belongs with -/// the fused command that actually needs prefixed `--zipper::` flags, so -/// the CLI surface here is untouched. -#[derive(Debug, Clone)] +/// Derives `clap::Args` and carries `#[fgumi_cli_macros::multi_options]` so a +/// future fused `runall` command can re-expose each field as a prefixed +/// `--zipper::`, via the generated `MultiZipperOptions` companion, +/// without hand-maintaining a parallel option set. This struct itself is not +/// flattened into [`Zipper`] or anywhere else by this change — the standalone +/// command still fills [`Zipper`]'s own fields and projects them through +/// [`Zipper::to_zipper_options`]; that path is untouched. +#[fgumi_cli_macros::multi_options("zipper", "Zipper Options")] +#[derive(Debug, Clone, clap::Args)] pub struct ZipperOptions { /// Tags to remove from mapped reads before copying unmapped tags. + #[arg(long, value_delimiter = ',')] pub tags_to_remove: Vec, /// Tags to reverse for reads mapped to the negative strand. + #[arg(long, value_delimiter = ',')] pub tags_to_reverse: Vec, /// Tags to reverse complement for reads mapped to the negative strand. + #[arg(long, value_delimiter = ',')] pub tags_to_revcomp: Vec, /// Buffer size for the template channel. + #[arg(short = 'b', long, default_value = "50000")] pub buffer: usize, /// Accepted for backward compatibility; has no effect. Carried so the /// projection stays total — see [`Zipper::bwa_chunk_size`]. + #[arg(short = 'K', long = "bwa-chunk-size", default_value = "150000000", hide = true)] pub bwa_chunk_size: u64, /// Drop unmapped-BAM reads absent from the aligned BAM. + #[arg( + long = "exclude-missing-reads", + value_name = "true|false", + default_value = "false", + num_args = 0..=1, + default_missing_value = "true", + action = clap::ArgAction::Set, + value_parser = clap::builder::BoolishValueParser::new(), + hide_possible_values = true + )] pub exclude_missing_reads: bool, /// Skip adding `tc` tags to secondary/supplementary reads. + #[arg( + long = "skip-tc-tags", + alias = "skip-pa-tags", + value_name = "true|false", + default_value = "false", + num_args = 0..=1, + default_missing_value = "true", + action = clap::ArgAction::Set, + value_parser = clap::builder::BoolishValueParser::new(), + hide_possible_values = true + )] pub skip_tc_tags: bool, /// Restore unconverted bases in EM-seq consensus reads. + #[arg( + long = "restore-unconverted-bases", + value_name = "true|false", + default_value = "false", + num_args = 0..=1, + default_missing_value = "true", + action = clap::ArgAction::Set, + value_parser = clap::builder::BoolishValueParser::new(), + hide_possible_values = true + )] pub restore_unconverted_bases: bool, } +impl Default for ZipperOptions { + fn default() -> Self { + Self { + tags_to_remove: Vec::new(), + tags_to_reverse: Vec::new(), + tags_to_revcomp: Vec::new(), + buffer: 50_000, + bwa_chunk_size: 150_000_000, + exclude_missing_reads: false, + skip_tc_tags: false, + restore_unconverted_bases: false, + } + } +} + impl Zipper { /// Project the parsed CLI flags into [`ZipperOptions`]. #[must_use] @@ -4950,4 +5003,59 @@ mod tests { assert!(raw_tag_absent(&raw, *SamTag::MD), "MD should be removed when bases were changed"); Ok(()) } + + // ───────────────────────────────────────────────────────────────────── + // ZipperOptions / MultiZipperOptions parity (multi_options) + // ───────────────────────────────────────────────────────────────────── + + #[derive(clap::Parser, Debug)] + struct PrefixedZipper { + #[command(flatten)] + opts: MultiZipperOptions, + } + + /// The re-exposed `MultiZipperOptions` defaults must equal the standalone + /// `zipper` command's defaults, projected through `to_zipper_options`. This + /// is the strong oracle: it fails on a dropped default, a misclassified + /// field, or a value that only happens to match by coincidence. + #[test] + fn multi_zipper_options_defaults_match_command() { + // NB: zipper's reference flag is --reference (short -r), NOT --ref. + let base = Zipper::try_parse_from([ + "zipper", + "-i", + "m.bam", + "--unmapped", + "u.bam", + "--reference", + "r.fa", + "-o", + "o.bam", + ]) + .expect("parses") + .to_zipper_options(); + let multi = + PrefixedZipper::try_parse_from(["x"]).expect("parses").opts.validate().expect("valid"); + assert_eq!(multi.buffer, base.buffer); + assert_eq!(multi.bwa_chunk_size, base.bwa_chunk_size); + assert_eq!(multi.exclude_missing_reads, base.exclude_missing_reads); + assert_eq!(multi.skip_tc_tags, base.skip_tc_tags); + assert_eq!(multi.restore_unconverted_bases, base.restore_unconverted_bases); + assert!( + multi.tags_to_remove.is_empty() + && multi.tags_to_reverse.is_empty() + && multi.tags_to_revcomp.is_empty() + ); + } + + /// A prefixed flag must round-trip through `MultiZipperOptions::validate`. + #[test] + fn multi_zipper_options_round_trips_a_supplied_flag() { + let multi = PrefixedZipper::try_parse_from(["x", "--zipper::buffer", "99"]) + .expect("parses") + .opts + .validate() + .expect("valid"); + assert_eq!(multi.buffer, 99); + } } From aa0d2b36291cdaa6ff0287440af1ce09e71fd308 Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 14:49:35 -0700 Subject: [PATCH 03/15] feat(sort): derive clap::Args + multi_options on SortOptions Copy each 1:1 CLI field's #[arg] verbatim from Sort onto SortOptions, skip the 2 chain-engine knobs (block_batch, file_granularity) that have no CLI flag, and add an impl Default matching Sort::to_sort_options's projection. Annotate with #[multi_options("sort", "Sort Options")] so a future fused runall command can re-expose these as prefixed --sort:: options via the generated MultiSortOptions companion. SortOptions is not parsed by clap anywhere today (it's built only via Sort::to_sort_options), so this is purely additive: the standalone sort command's CLI, to_sort_options, and every consumer are unchanged. --- src/lib/commands/sort.rs | 235 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 221 insertions(+), 14 deletions(-) diff --git a/src/lib/commands/sort.rs b/src/lib/commands/sort.rs index 9d84a3f14..733a1288d 100644 --- a/src/lib/commands/sort.rs +++ b/src/lib/commands/sort.rs @@ -457,41 +457,187 @@ pub struct Sort { /// does not yet expose as CLI flags; until the sort command is rewired onto the /// chain they take the engine defaults (`block_batch = 4`, the original /// `MAX_BATCH_PER_CALL`; `file_granularity = false`, block-parallel). -#[derive(Debug, Clone)] +#[fgumi_cli_macros::multi_options("sort", "Sort Options")] +#[derive(Debug, Clone, clap::Args)] #[allow(clippy::struct_excessive_bools)] pub struct SortOptions { - /// Requested output sort order. + /// Sort order. + /// + /// Queryname sort supports sub-sort specifiers: + /// `queryname` Lexicographic byte ordering (default, fast) + /// `queryname::lexicographic` Explicit lexicographic ordering (alias: `queryname::lex`) + /// `queryname::lexicographical` Alias; written as `queryname:lexicographical` in `@HD` SS + /// `queryname::natural` Natural numeric ordering (samtools-compatible) + #[arg(long = "order", default_value = "template-coordinate", value_parser = SortOrderArg::parse)] pub order: SortOrderArg, - /// Expert override for the provisioned template-coordinate key lanes. + + /// Which optional lanes to keep in the template-coordinate sort key. + /// + /// Smaller keys use less memory and spill less. Only meaningful for + /// `--order template-coordinate`; ignored for other orders. + /// + /// (omitted) Auto-detect from the first record + verify (default). + /// full Keep all lanes (CB + library/MI). Largest key. + /// none Drop all optional lanes (smallest, bulk pre-group). + /// cb,library,mi Comma/space list; keep the named lanes. + /// + /// A record carrying a value in a dropped lane aborts the sort with a message + /// naming the field and the token to re-include it. + #[arg(long = "key-types", value_parser = parse_key_types)] pub key_types: Option, - /// In-memory sort budget before spilling. + + /// Maximum memory for in-memory sorting. + /// + /// Default is "768M" per thread (matching samtools behavior). Pass "auto" + /// to detect system memory and subtract --memory-reserve, leaving room + /// for the OS and co-running processes (e.g. an aligner). Explicit values + /// like "512M", "1G", "4GiB" are per-thread when --memory-per-thread is + /// enabled (default). + /// + /// When the limit is reached, sorted chunks spill to temporary files. + #[arg(short = 'm', long = "max-memory", default_value = "768M", value_parser = parse_memory)] pub max_memory: MemoryLimit, - /// Memory reserved for other processes under `--max-memory=auto`. + + /// Memory to reserve for other processes when --max-memory=auto. + /// + /// "auto" (default) reserves min(10 GiB, 50% of system memory). Explicit + /// values like "10G", "8GiB" set a fixed reservation. Set higher when + /// running alongside a memory-intensive aligner (e.g. `bwa mem` with a + /// human genome index uses ~8 GiB). + /// + /// Ignored when --max-memory is set to an explicit value. + #[arg(long = "memory-reserve", default_value = "auto", value_parser = parse_memory_reserve)] pub memory_reserve: MemoryReserve, - /// Whether `max_memory` is per-thread (samtools behavior). + + /// Scale memory limit by thread count (samtools behavior). + /// + /// When enabled (default), --max-memory specifies memory per thread. Total + /// memory = `max_memory` × the larger of --threads and --sort-threads, since + /// the sort phase is what fills the in-memory buffer. Disable for fixed total + /// memory. + /// + /// This formula is for the in-memory sort buffer. --max-memory also bounds + /// the inter-stage queue budget (see its docs), which scales by --threads + /// alone, so the two totals differ when --sort-threads > --threads. + #[arg(long = "memory-per-thread", value_name = "true|false", default_value = "true", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub memory_per_thread: bool, - /// Temp directories for spill chunks (free-space-aware round-robin). + + /// Temporary directory for intermediate files. Repeatable. + /// + /// Pass `-T ` one or more times to spread spill chunks across multiple + /// directories in free-space-aware round-robin order. Useful when one + /// filesystem is too small or slower than the aggregate of several. + /// + /// If no flags are given and the `FGUMI_TMP_DIRS` environment variable is + /// set, its value is parsed as a `PATH`-style list (colon-separated on + /// Unix, semicolon-separated on Windows) and used instead. + /// + /// If neither is provided, the system default temp directory is used. + /// For best performance, use fast SSDs. + #[arg(short = 'T', long = "tmp-dir", action = clap::ArgAction::Append)] pub tmp_dirs: Vec, - /// Worker threads for the accumulate/sort/spill phase (Phase 1). + + /// Number of threads for the sort phase (accumulate, sort, spill). + /// + /// Defaults to `--threads`. Lower this to cede cores to an upstream + /// producer while keeping the merge wide -- with `-@ 8 --sort-threads 4`, + /// ingest contends with the producer over only 4 threads, while the merge + /// still uses 8 because it cannot start until the input is exhausted, by + /// which point the producer has finished writing. + /// + /// The output is byte-identical, but this is not purely a scheduling knob: + /// with --memory-per-thread enabled (default) the budget scales by the larger + /// of --threads and --sort-threads, so raising this above --threads raises + /// total memory by the same factor. + #[arg(long = "sort-threads")] pub sort_threads: Option, - /// Worker threads for the k-way merge phase (Phase 2). + + /// Number of threads for the merge phase (k-way merge and output write). + /// + /// Defaults to `--threads`. This only changes scheduling; the output is + /// byte-identical. + #[arg(long = "merge-threads")] pub merge_threads: Option, - /// Compression level for temporary spill files (0-9). + + /// Compression level for temporary chunk files (0-9). + /// + /// Applies to the codec selected by `--temp-codec`: + /// * For `bgzf`, level 0 produces uncompressed (stored) BGZF blocks + /// (fastest, uses most disk space); 1..=9 are libdeflate levels. + /// * For `zstd`, only 1..=9 are valid; level 0 is rejected because zstd + /// has no equivalent "stored" mode and silently remapping it to 1 + /// would surprise users counting on uncompressed spill. + /// + /// Level 1 (default) provides fast compression with reasonable space savings. + /// Higher levels (up to 9) provide better compression but are slower. + #[arg(long = "temp-compression", default_value = "1", value_parser = clap::value_parser!(u32).range(0..=9))] pub temp_compression: u32, - /// Codec for temporary spill files. + + /// Codec used for temporary spill chunks: `zstd` (default) or `bgzf`. + /// + /// zstd is significantly faster than bgzf at comparable compression + /// ratios for BAM-record data; we default to zstd because spill files + /// are internal to the sort and never read by other tools. Pass `bgzf` + /// to fall back to the legacy on-disk format. + #[arg(long = "temp-codec", default_value = "zstd")] pub temp_codec: fgumi_sort::SpillCodec, - /// Spill-file consolidation limit (`--max-temp-files`). Resolved against the - /// host `RLIMIT_NOFILE` when `Auto`; without this the chain sorter fell back - /// to the engine's portable default and ignored the CLI value. + + /// Maximum number of temporary spill files kept before the oldest are + /// consolidated into a single run. + /// + /// Large inputs spill many sorted runs to disk. When the number of runs + /// reaches this limit, the oldest are merged together in a single pass so + /// the final k-way merge opens fewer files at once. That merge is the only + /// reason the limit exists: it opens every remaining run at once, so the + /// limit bounds how many file descriptors the sort needs. + /// + /// Consolidation rewrites data that is already sorted, so it is pure + /// overhead whenever the descriptor budget could have carried the runs. + /// Raising this avoids it on very large inputs (at the cost of more open + /// file descriptors during the final merge); lowering it keeps fewer files + /// open. Must be at least 2; to effectively disable consolidation, pass a + /// value larger than the number of runs you expect to spill. + /// + /// "auto" (default) sizes the limit to the process's soft open-file limit + /// (`ulimit -n`), less a reserve for the input, output and index handles, + /// and capped at a tested maximum. Explicit values like "64", "256" pin it; + /// a pinned value larger than the open-file budget is reported at startup. + /// Must be at least 2. + #[arg(long = "max-temp-files", default_value = "auto", value_parser = parse_max_temp_files)] pub max_temp_files: MaxTempFiles, + /// Records batched per parallel sort call (chain engine; not a CLI flag). + #[arg(skip = 4usize)] pub block_batch: usize, /// Spill at file rather than block granularity (chain engine; not a CLI flag). + #[arg(skip)] pub file_granularity: bool, /// Emit the sort's performance diagnostics (`--sort-stats`). + #[arg(long = "sort-stats", default_value_t = false, hide = true)] pub sort_stats: bool, } +impl Default for SortOptions { + fn default() -> Self { + Self { + order: SortOrderArg::TemplateCoordinate, + key_types: None, + max_memory: parse_memory("768M").expect("valid default"), + memory_reserve: parse_memory_reserve("auto").expect("valid default"), + memory_per_thread: true, + tmp_dirs: Vec::new(), + sort_threads: None, + merge_threads: None, + temp_compression: 1, + temp_codec: fgumi_sort::SpillCodec::Zstd, + max_temp_files: parse_max_temp_files("auto").expect("valid default"), + block_batch: 4, + file_granularity: false, + sort_stats: false, + } + } +} + impl Sort { /// Projects the parsed CLI flags into [`SortOptions`] for the chain builder. #[must_use] @@ -2288,4 +2434,65 @@ mod tests { let sort = Sort::try_parse_from(["sort", "-i", "in.bam", "-o", "out.bam"]).expect("parse"); assert!(sort.key_types.is_none()); } + + // ───────────────────────────────────────────────────────────────────── + // SortOptions / MultiSortOptions parity (multi_options) + // ───────────────────────────────────────────────────────────────────── + + #[derive(clap::Parser, Debug)] + struct PrefixedSort { + #[command(flatten)] + opts: MultiSortOptions, + } + + /// The re-exposed `MultiSortOptions` defaults must equal the standalone + /// `sort` command's defaults, projected through `to_sort_options`. This is + /// the strong oracle: it fails on a dropped default, a misclassified field, + /// or a value that only happens to match by coincidence — including the two + /// chain-engine skip fields (`block_batch`, `file_granularity`), which have + /// no CLI flag on `Sort` at all. + #[test] + fn multi_sort_options_defaults_match_command() { + let base = Sort::try_parse_from(["sort", "-i", "in.bam", "-o", "o.bam"]) + .expect("parses") + .to_sort_options(); + let multi = + PrefixedSort::try_parse_from(["x"]).expect("parses").opts.validate().expect("valid"); + + assert_eq!(multi.order, base.order); + assert_eq!(multi.order, SortOrderArg::TemplateCoordinate); + assert_eq!(multi.key_types, base.key_types); + assert_eq!(multi.max_memory, base.max_memory); + assert_eq!(multi.max_memory, parse_memory("768M").expect("valid")); + assert_eq!(multi.memory_reserve, base.memory_reserve); + assert_eq!(multi.memory_per_thread, base.memory_per_thread); + assert_eq!(multi.tmp_dirs, base.tmp_dirs); + assert_eq!(multi.sort_threads, base.sort_threads); + assert_eq!(multi.merge_threads, base.merge_threads); + assert_eq!(multi.temp_compression, base.temp_compression); + assert_eq!(multi.temp_codec, base.temp_codec); + assert_eq!(multi.max_temp_files, base.max_temp_files); + assert_eq!(multi.block_batch, base.block_batch); + assert_eq!(multi.block_batch, 4); + assert_eq!(multi.file_granularity, base.file_granularity); + assert!(!multi.file_granularity); + assert_eq!(multi.sort_stats, base.sort_stats); + assert!(!multi.sort_stats); + } + + /// A supplied prefixed flag must round-trip through + /// `MultiSortOptions::validate` — a value one, `--sort::max-memory`, and the + /// boolean `--sort::sort-stats` (whose base default is `false`, so an enabled + /// value is what proves the mapping is wired and not a parser-accepted no-op). + #[test] + fn multi_sort_options_round_trips_a_supplied_flag() { + let multi = + PrefixedSort::try_parse_from(["x", "--sort::max-memory", "1G", "--sort::sort-stats"]) + .expect("parses") + .opts + .validate() + .expect("valid"); + assert_eq!(multi.max_memory, parse_memory("1G").expect("valid")); + assert!(multi.sort_stats, "--sort::sort-stats must round-trip to sort_stats=true"); + } } From 16f50e6504c01b04487544725333457d132d4bfb Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 14:57:15 -0700 Subject: [PATCH 04/15] feat(correct): derive clap::Args + multi_options on CorrectOptions Adds clap::Args, an impl Default, and #[fgumi_cli_macros::multi_options] to CorrectOptions, generating MultiCorrectOptions for a future runall command. rejects_path is re-exposed directly as --correct::rejects rather than nesting RejectsOptions, which the macro rejects outright. min_distance_diff has no CLI default, so the macro lifts it to a staged-required field enforced by MultiCorrectOptions::validate(). CorrectUmis::to_correct_options and all consumers are unchanged; the standalone correct command's CLI behavior is unaffected. --- src/lib/commands/correct.rs | 150 ++++++++++++++++++++++++++++++++++-- 1 file changed, 144 insertions(+), 6 deletions(-) diff --git a/src/lib/commands/correct.rs b/src/lib/commands/correct.rs index f4ccfd28f..5e6f8675b 100644 --- a/src/lib/commands/correct.rs +++ b/src/lib/commands/correct.rs @@ -341,37 +341,89 @@ pub struct CorrectUmis { /// CorrectUmis-stage tuning, independent of how the values were supplied. /// -/// See [`crate::commands::zipper::ZipperOptions`] for why this is a plain -/// struct rather than a flattened `clap::Args`. Note that the rejects path is -/// held **flat** here even though [`CorrectUmis`] nests it behind a -/// `#[command(flatten)]` sub-struct: the chain builder wants one bag per stage, -/// not a re-run of the CLI's grouping. -#[derive(Debug, Clone)] +/// See [`crate::commands::zipper::ZipperOptions`] for why this derives +/// `clap::Args` and carries `#[fgumi_cli_macros::multi_options]` even though it +/// is never flattened into [`CorrectUmis`] or anywhere else by this change — +/// the standalone command still fills [`CorrectUmis`]'s own fields and projects +/// them through [`CorrectUmis::to_correct_options`]; that path is untouched. +/// Note that `rejects_path` is held **flat** here even though [`CorrectUmis`] +/// nests it behind a `#[command(flatten)]` `RejectsOptions` sub-struct: the +/// chain builder wants one bag per stage, not a re-run of the CLI's grouping, +/// and the macro rejects a field-level `#[command(flatten)]` outright — so it +/// is re-exposed directly as its own `--correct::rejects` flag here. +#[fgumi_cli_macros::multi_options("correct", "Correct Options")] +#[derive(Debug, Clone, clap::Args)] pub struct CorrectOptions { /// Optional metrics output. + #[arg(short = 'M', long)] pub metrics: Option, /// Which SAM tag is corrected: `RX`/`OX` for UMIs, `BC`/`ob` for barcodes. + #[arg(short = 't', long, value_enum, default_value_t = Target::Umi)] pub target: Target, /// Maximum mismatches when matching a UMI. + #[arg(long, default_value = "2")] pub max_mismatches: usize, /// Minimum distance to the runner-up UMI. + #[arg(short = 'd', long = "min-distance")] pub min_distance_diff: usize, /// Expected UMI sequences. + #[arg(short = 'u', long)] pub umis: Vec, /// Files holding expected UMI sequences. + #[arg(short = 'U', long)] pub umi_files: Vec, /// Skip storing the original UMI. + #[arg( + long = "dont-store-original", + alias = "dont-store-original-umis", + value_name = "true|false", + default_value = "false", + num_args = 0..=1, + default_missing_value = "true", + action = clap::ArgAction::Set, + value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true + )] pub dont_store_original_umis: bool, /// UMI match cache size. + #[arg(long, default_value = "100000")] pub cache_size: usize, /// Minimum corrected fraction before failing. + #[arg(long)] pub min_corrected: Option, /// Also match the reverse complement. + #[arg(long, value_name = "true|false", default_value = "false", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub revcomp: bool, /// Optional rejects output path. + #[arg(long = "rejects")] pub rejects_path: Option, } +/// Values equal `CorrectUmis::try_parse_from(["correct", "-i", "in.bam", "-o", +/// "out.bam", "-d", "2"]).to_correct_options()` — the minimal invocation that +/// satisfies every clap-required flag (`--min-distance` has no `default_value` +/// so clap demands it; `--umis` is a bare `Vec` and is not clap-required, +/// so it comes back empty). `min_distance_diff: 2` mirrors the module doc +/// example and the pipeline step's hand-written test fixture +/// (`pipeline::steps::correct::tests::make_default_opts`) rather than a +/// meaningless `0`. +impl Default for CorrectOptions { + fn default() -> Self { + Self { + metrics: None, + target: Target::Umi, + max_mismatches: 2, + min_distance_diff: 2, + umis: Vec::new(), + umi_files: Vec::new(), + dont_store_original_umis: false, + cache_size: 100_000, + min_corrected: None, + revcomp: false, + rejects_path: None, + } + } +} + impl CorrectUmis { /// Project the parsed CLI flags into [`CorrectOptions`]. #[must_use] @@ -4406,4 +4458,90 @@ mod tests { fn target_default_is_umi() { assert_eq!(Target::default(), Target::Umi); } + + // ───────────────────────────────────────────────────────────────────── + // CorrectOptions / MultiCorrectOptions parity (multi_options) + // ───────────────────────────────────────────────────────────────────── + + #[derive(clap::Parser, Debug)] + struct PrefixedCorrect { + #[command(flatten)] + opts: MultiCorrectOptions, + } + + /// The re-exposed `MultiCorrectOptions` defaults must equal the standalone + /// `correct` command's defaults, projected through `to_correct_options`. + /// This is the strong oracle: it fails on a dropped default, a + /// misclassified field, or a value that only happens to match by + /// coincidence. `--min-distance` has no `default_value` on the standalone + /// command, so both sides are anchored with the SAME non-default value + /// (`-d 1 -u AAA`) rather than compared against a default. + #[test] + fn multi_correct_options_defaults_match_command() { + let base = CorrectUmis::try_parse_from([ + "correct", "-i", "in.bam", "-o", "o.bam", "-d", "1", "-u", "AAA", + ]) + .expect("parses") + .to_correct_options(); + let multi = PrefixedCorrect::try_parse_from([ + "x", + "--correct::min-distance", + "1", + "--correct::umis", + "AAA", + ]) + .expect("parses") + .opts + .validate() + .expect("valid"); + + // Anchor fields: both sides were fed the same non-default value. + assert_eq!(multi.min_distance_diff, base.min_distance_diff); + assert_eq!(multi.umis, base.umis); + + // Every other projected field must sit at its default on both sides. + assert_eq!(multi.max_mismatches, base.max_mismatches); + assert_eq!(multi.cache_size, base.cache_size); + assert_eq!(multi.min_corrected, base.min_corrected); + assert_eq!(multi.revcomp, base.revcomp); + assert_eq!(multi.dont_store_original_umis, base.dont_store_original_umis); + assert_eq!(multi.metrics, base.metrics); + assert_eq!(multi.target, base.target); + assert_eq!(multi.umi_files, base.umi_files); + assert_eq!(multi.rejects_path, base.rejects_path); + } + + /// A prefixed flag must round-trip through `MultiCorrectOptions::validate`. + #[test] + fn multi_correct_options_round_trips_a_supplied_flag() { + let multi = PrefixedCorrect::try_parse_from([ + "x", + "--correct::min-distance", + "1", + "--correct::umis", + "AAA", + "--correct::max-mismatches", + "3", + ]) + .expect("parses") + .opts + .validate() + .expect("valid"); + assert_eq!(multi.max_mismatches, 3); + } + + /// `--correct::min-distance` has no default on the standalone command, so + /// the macro must lift it to a staged-required field: omitting it must + /// parse fine (staged validation, not clap, owns the requirement) but fail + /// `validate()`. + #[test] + fn multi_correct_options_min_distance_is_staged_required() { + let parsed = PrefixedCorrect::try_parse_from(["x"]).expect("parse is staged, not clap"); + let err = parsed.opts.validate().expect_err("min-distance must be required"); + let msg = format!("{err:#}"); + assert!( + msg.contains("--correct::min-distance is required"), + "error should name --correct::min-distance: {msg}" + ); + } } From c79a179657cd012c14d2b478af0d0a140fd03b0d Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 16:16:08 -0700 Subject: [PATCH 05/15] feat(filter): derive clap::Args + multi_options on FilterOptions Adds clap::Args, an impl Default, and #[fgumi_cli_macros::multi_options] to FilterOptions, generating MultiFilterOptions for a future runall command. rejects/stats are re-exposed directly as --filter::rejects and --filter::stats rather than nesting sub-structs, matching how Filter itself exposes them. methylation_mode is #[arg(skip)] since --methylation-mode is a cross-stage top-level runall flag, not a --filter:: flag; it falls back to MethylationMode::Disabled via FilterOptions::default(). min_reads has no CLI default, so the macro lifts it to a staged-required field enforced by MultiFilterOptions::validate(). Filter::to_filter_options and all consumers are unchanged; the standalone filter command's CLI behavior is unaffected. --- src/lib/commands/filter.rs | 161 +++++++++++++++++++++++++++++++++++-- 1 file changed, 156 insertions(+), 5 deletions(-) diff --git a/src/lib/commands/filter.rs b/src/lib/commands/filter.rs index 654ce4162..4b6ea89e8 100644 --- a/src/lib/commands/filter.rs +++ b/src/lib/commands/filter.rs @@ -224,47 +224,110 @@ pub struct Filter { /// Filter-stage tuning, independent of how the values were supplied. /// -/// See [`crate::commands::zipper::ZipperOptions`] for why this is a plain -/// struct rather than a flattened `clap::Args`: the chain builder only reads -/// these values, so moving the fields off [`Filter`] would rewrite this module -/// and its tests for no gain here. -#[derive(Debug, Clone)] +/// Derives `clap::Args` and carries `#[fgumi_cli_macros::multi_options]` so a +/// future fused `runall` command can re-expose each field as a prefixed +/// `--filter::`, via the generated `MultiFilterOptions` companion, +/// without hand-maintaining a parallel option set. This struct itself is not +/// flattened into [`Filter`] or anywhere else by this change — the standalone +/// command still fills [`Filter`]'s own fields and projects them through +/// [`Filter::to_filter_options`]; that path is untouched. `rejects` and +/// `stats` are held **flat** here, matching how [`Filter`] itself exposes +/// them (bare `--rejects`/`--stats` flags, not a nested sub-struct). +/// +/// `methylation_mode` is `#[arg(skip)]`: on the standalone command it is +/// resolved from `Option` via +/// [`crate::commands::common::resolve_methylation_mode`], and +/// `--methylation-mode` itself is a cross-stage top-level `runall` flag +/// (PR B), not a `--filter::` flag. [`fgumi_consensus::MethylationMode`] +/// implements `Default` (`Disabled`), which is what the skipped field falls +/// back to. +#[fgumi_cli_macros::multi_options("filter", "Filter Options")] +#[derive(Debug, Clone, clap::Args)] #[allow(clippy::struct_excessive_bools)] pub struct FilterOptions { /// Reference FASTA, required by methylation-aware filters. + #[arg(short = 'r', long = "ref")] pub reference: Option, /// Minimum reads supporting a consensus, per depth tier. + #[arg(short = 'M', long = "min-reads", value_delimiter = ',', required = true)] pub min_reads: Vec, /// Maximum per-read error rate, per depth tier. + #[arg( + short = 'E', + long = "max-read-error-rate", + value_delimiter = ',', + default_value = "0.025" + )] pub max_read_error_rate: Vec, /// Maximum per-base error rate, per depth tier. + #[arg(short = 'e', long = "max-base-error-rate", value_delimiter = ',', default_value = "0.1")] pub max_base_error_rate: Vec, /// Minimum consensus base quality. + #[arg(short = 'N', long = "min-base-quality")] pub min_base_quality: Option, /// Minimum mean base quality across the read. + #[arg(short = 'q', long = "min-mean-base-quality")] pub min_mean_base_quality: Option, /// Maximum fraction of no-called bases. + #[arg(short = 'n', long = "max-no-call-fraction", default_value = "0.2")] pub max_no_call_fraction: f64, /// Reverse per-base tags on negative-strand reads. + #[arg(short = 'R', long = "reverse-per-base-tags", value_name = "true|false", default_value = "false", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub reverse_per_base_tags: bool, /// Filter whole templates rather than individual reads. + #[arg(long = "filter-by-template", value_name = "true|false", default_value = "true", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub filter_by_template: bool, /// Optional path for rejected records. + #[arg(long = "rejects")] pub rejects: Option, /// Optional path for filter statistics. + #[arg(long = "stats")] pub stats: Option, /// Require both single-strand consensuses to agree. + #[arg(short = 's', long = "require-single-strand-agreement", value_name = "true|false", default_value = "false", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub require_single_strand_agreement: bool, /// Minimum methylation depth, per tier. + #[arg(long = "min-methylation-depth", value_delimiter = ',')] pub min_methylation_depth: Vec, /// Require both strands to agree on methylation. + #[arg(long = "require-strand-methylation-agreement", value_name = "true|false", default_value = "false", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub require_strand_methylation_agreement: bool, /// Minimum bisulfite conversion fraction. + #[arg(long = "min-conversion-fraction")] pub min_conversion_fraction: Option, /// Resolved methylation calling mode (`Disabled` when the flag is unset). + #[arg(skip)] pub methylation_mode: fgumi_consensus::MethylationMode, } +/// Values equal `Filter::try_parse_from(["filter", "-i", "in.bam", "-o", +/// "out.bam", "-M", "1"]).to_filter_options()` — the minimal invocation that +/// satisfies every clap-required flag (`--min-reads` has no `default_value` +/// so clap demands it once staged as required here; `-M 1` mirrors the +/// smallest valid depth tier). +impl Default for FilterOptions { + fn default() -> Self { + Self { + reference: None, + min_reads: vec![1], + max_read_error_rate: vec![0.025], + max_base_error_rate: vec![0.1], + min_base_quality: None, + min_mean_base_quality: None, + max_no_call_fraction: 0.2, + reverse_per_base_tags: false, + filter_by_template: true, + rejects: None, + stats: None, + require_single_strand_agreement: false, + min_methylation_depth: Vec::new(), + require_strand_methylation_agreement: false, + min_conversion_fraction: None, + methylation_mode: fgumi_consensus::MethylationMode::Disabled, + } + } +} + impl Filter { /// Project the parsed CLI flags into [`FilterOptions`]. #[must_use] @@ -1454,6 +1517,94 @@ mod tests { assert_eq!(opts.methylation_mode, fgumi_consensus::MethylationMode::Disabled); } + // ───────────────────────────────────────────────────────────────────── + // FilterOptions / MultiFilterOptions parity (multi_options) + // ───────────────────────────────────────────────────────────────────── + + #[derive(clap::Parser, Debug)] + struct PrefixedFilter { + #[command(flatten)] + opts: MultiFilterOptions, + } + + /// The re-exposed `MultiFilterOptions` defaults must equal the standalone + /// `filter` command's defaults, projected through `to_filter_options`. This + /// is the strong oracle: it fails on a dropped default, a misclassified + /// field, or a value that only happens to match by coincidence. + /// `--min-reads` has no `default_value` on the standalone command, so both + /// sides are anchored with the same value (`-M 1` / `--filter::min-reads + /// 1`) rather than compared against a default — the standalone side is + /// NOT staged-required and would otherwise come back empty (see + /// `to_filter_options_carries_defaults` above), while the multi side IS + /// staged-required and would fail `validate()` if omitted. + #[test] + fn multi_filter_options_defaults_match_command() { + let base = Filter::try_parse_from(["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1"]) + .expect("parses") + .to_filter_options(); + let multi = PrefixedFilter::try_parse_from(["x", "--filter::min-reads", "1"]) + .expect("parses") + .opts + .validate() + .expect("valid"); + + // Anchor field: both sides were fed the same non-default value. + assert_eq!(multi.min_reads, vec![1]); + assert_eq!(multi.min_reads, base.min_reads); + + // Every other projected field must sit at its default on both sides. + assert_eq!(multi.reference, base.reference); + assert_eq!(multi.max_read_error_rate, base.max_read_error_rate); + assert_eq!(multi.max_base_error_rate, base.max_base_error_rate); + assert_eq!(multi.min_base_quality, base.min_base_quality); + assert_eq!(multi.min_mean_base_quality, base.min_mean_base_quality); + assert_eq!(multi.max_no_call_fraction, base.max_no_call_fraction); + assert_eq!(multi.reverse_per_base_tags, base.reverse_per_base_tags); + assert_eq!(multi.filter_by_template, base.filter_by_template); + assert_eq!(multi.rejects, base.rejects); + assert_eq!(multi.stats, base.stats); + assert_eq!(multi.require_single_strand_agreement, base.require_single_strand_agreement); + assert_eq!(multi.min_methylation_depth, base.min_methylation_depth); + assert_eq!( + multi.require_strand_methylation_agreement, + base.require_strand_methylation_agreement + ); + assert_eq!(multi.min_conversion_fraction, base.min_conversion_fraction); + assert_eq!(multi.methylation_mode, base.methylation_mode); + } + + /// A prefixed flag must round-trip through `MultiFilterOptions::validate`. + #[test] + fn multi_filter_options_round_trips_a_supplied_flag() { + let multi = PrefixedFilter::try_parse_from([ + "x", + "--filter::min-reads", + "1", + "--filter::max-base-error-rate", + "0.3", + ]) + .expect("parses") + .opts + .validate() + .expect("valid"); + assert_eq!(multi.max_base_error_rate, vec![0.3]); + } + + /// `--filter::min-reads` has no default on the standalone command, so the + /// macro must lift it to a staged-required field: omitting it must parse + /// fine (staged validation, not clap, owns the requirement) but fail + /// `validate()`. + #[test] + fn multi_filter_options_min_reads_is_staged_required() { + let parsed = PrefixedFilter::try_parse_from(["x"]).expect("parse is staged, not clap"); + let err = parsed.opts.validate().expect_err("min-reads must be required"); + let msg = format!("{err:#}"); + assert!( + msg.contains("--filter::min-reads is required"), + "error should name --filter::min-reads: {msg}" + ); + } + use crate::sam::SamTag; use fgumi_raw_bam::{RawRecord, SamBuilder as RawSamBuilder, aux_data_slice, flags}; use noodles::sam::alignment::record_buf::RecordBuf; From b7e5f658e3613a4109324a7c58d49c3aea616022 Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 16:26:31 -0700 Subject: [PATCH 06/15] feat(duplex): derive clap::Args + multi_options on DuplexOptions Adds per-field #[arg] attributes (copied verbatim from ConsensusCallingOptions / OverlappingConsensusOptions) and #[fgumi_cli_macros::multi_options("duplex", "Duplex Options")] to DuplexOptions, generating a MultiDuplexOptions companion for a future fused runall command. tie_rule, allow_unmapped, io, rejects_opts, stats_opts, read_group, methylation_mode, and reference are #[arg(skip)] (the latter two are cross-stage runall flags landing in a later PR; the rest are hidden or data-carrier fields). DuplexOptions is not flattened anywhere by this change; Duplex, to_duplex_options, and DuplexOptions::consensus()/overlapping() are untouched. --- src/lib/commands/duplex.rs | 128 +++++++++++++++++++++++++++++++++++-- 1 file changed, 123 insertions(+), 5 deletions(-) diff --git a/src/lib/commands/duplex.rs b/src/lib/commands/duplex.rs index 40cc4a2c8..deb508547 100644 --- a/src/lib/commands/duplex.rs +++ b/src/lib/commands/duplex.rs @@ -244,49 +244,101 @@ pub struct Duplex { /// Duplex-stage tuning, independent of how the values were supplied. /// -/// See [`crate::commands::zipper::ZipperOptions`] for why this is a plain -/// struct rather than a flattened `clap::Args`. Note that the consensus-calling +/// Derives `clap::Args` and carries `#[fgumi_cli_macros::multi_options]` so a +/// future fused `runall` command can re-expose each field as a prefixed +/// `--duplex::`, via the generated `MultiDuplexOptions` companion, +/// without hand-maintaining a parallel option set. This struct itself is not +/// flattened into [`Duplex`] or anywhere else by this change — the standalone +/// command still fills [`Duplex`]'s own fields (including its nested +/// `consensus` / `overlapping` sub-structs) and projects them through +/// [`Duplex::to_duplex_options`]; that path is untouched. The consensus-calling /// knobs are held **flat** here even though [`Duplex`] nests them behind -/// `#[command(flatten)]` sub-structs: the chain builder wants one bag per stage, -/// not a re-run of the CLI's grouping. -#[derive(Debug, Clone)] +/// `#[command(flatten)]` sub-structs: the chain builder wants one bag per +/// stage, not a re-run of the CLI's grouping. Each `#[arg]` below is copied +/// verbatim from the corresponding field on [`ConsensusCallingOptions`] / +/// [`OverlappingConsensusOptions`]. +/// +/// `tie_rule` is `#[arg(skip)]`: on the standalone command it is resolved from +/// `TieRuleArg` (a hidden, cross-tool equivalency-testing knob), and +/// `--duplex::tie-rule` is not re-exposed by `runall`. [`fgumi_consensus::TieRule`] +/// implements `Default` (`FgbioCompat`), matching `TieRuleArg::FgbioCompat`'s +/// resolution. +/// +/// `allow_unmapped` is `#[arg(skip = AllowUnmappedOptions { enabled: false })]`: +/// [`AllowUnmappedOptions`] does not implement `Default`, so the explicit +/// expression form is required; `enabled: false` matches the standalone +/// command's default. +/// +/// `io` / `rejects_opts` / `stats_opts` / `read_group` are `#[arg(skip)]` +/// data carriers baked in by `runall`, not `--duplex::` flags — each carrier +/// type implements `Default`. +/// +/// `methylation_mode` is `#[arg(skip)]`: on the standalone command it is +/// resolved from `Option` via +/// [`crate::commands::common::resolve_methylation_mode`], and +/// `--methylation-mode` itself is a cross-stage top-level `runall` flag +/// (PR B), not a `--duplex::` flag. [`fgumi_consensus::MethylationMode`] +/// implements `Default` (`Disabled`), which is what the skipped field falls +/// back to. +/// +/// `reference` is `#[arg(skip)]`: `--ref` is a cross-stage top-level `runall` +/// flag (PR B), not a `--duplex::` flag. +#[fgumi_cli_macros::multi_options("duplex", "Duplex Options")] +#[derive(Debug, Clone, clap::Args)] pub struct DuplexOptions { /// Pre-UMI error rate (phred). + #[arg(short = '1', long = "error-rate-pre-umi", default_value = "45")] pub error_rate_pre_umi: u8, /// Post-UMI error rate (phred). + #[arg(short = '2', long = "error-rate-post-umi", default_value = "40")] pub error_rate_post_umi: u8, /// Minimum input base quality. + #[arg(short = 'm', long = "min-input-base-quality", default_value = "10")] pub min_input_base_quality: u8, /// Emit per-base consensus tags. + #[arg(short = 'B', long = "output-per-base-tags", value_name = "true|false", default_value = "true", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub output_per_base_tags: bool, /// Trim consensus reads. + #[arg(long = "trim", value_name = "true|false", default_value = "false", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub trim: bool, /// Minimum consensus base quality. + #[arg(long = "min-consensus-base-quality", default_value = "2")] pub min_consensus_base_quality: u8, /// How to resolve a near-tie between the two most likely consensus bases. + #[arg(skip)] pub tie_rule: fgumi_consensus::TieRule, /// Call overlapping bases jointly. + #[arg(long = "consensus-call-overlapping-bases", value_name = "true|false", default_value = "true", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub consensus_call_overlapping_bases: bool, /// Minimum reads per consensus, per tier. + #[arg(short = 'M', long = "min-reads", value_delimiter = ',', default_value = "1")] pub min_reads: Vec, /// Cap on reads per strand. + #[arg(long = "max-reads-per-strand")] pub max_reads_per_strand: Option, /// Let fully-unmapped primary templates through the pre-group filter. /// /// Carried as the whole flattened sub-struct, like `io` / `rejects_opts` / /// `read_group`, rather than as a bare `bool`. + #[arg(skip = AllowUnmappedOptions { enabled: false })] pub allow_unmapped: AllowUnmappedOptions, /// Input/output paths and reader mode. + #[arg(skip)] pub io: BamIoOptions, /// Optional rejects output. + #[arg(skip)] pub rejects_opts: RejectsOptions, /// Optional stats output. + #[arg(skip)] pub stats_opts: StatsOptions, /// Read-group identity for emitted reads. + #[arg(skip)] pub read_group: ReadGroupOptions, /// Resolved methylation calling mode (`Disabled` when the flag is unset). + #[arg(skip)] pub methylation_mode: fgumi_consensus::MethylationMode, /// Reference FASTA for methylation-aware modes. + #[arg(skip)] pub reference: Option, } @@ -2446,4 +2498,70 @@ mod tests { Ok(()) } + + // ───────────────────────────────────────────────────────────────────── + // DuplexOptions / MultiDuplexOptions parity (multi_options) + // ───────────────────────────────────────────────────────────────────── + + /// `MultiDuplexOptions` derives `clap::Args`, not `Parser` — flatten it into + /// a local wrapper to drive it through `try_parse_from`. + #[derive(clap::Parser, Debug)] + struct PrefixedDuplex { + #[command(flatten)] + opts: MultiDuplexOptions, + } + + /// The re-exposed `MultiDuplexOptions` defaults must equal the standalone + /// `duplex` command's defaults, projected through `to_duplex_options`. This + /// is the strong oracle: it fails on a dropped default, a misclassified + /// field, or a value that only happens to match by coincidence. + /// + /// `io` / `rejects_opts` / `stats_opts` / `read_group` are `#[arg(skip)]` + /// data carriers on `MultiDuplexOptions`, so their values come from each + /// carrier's own `Default` rather than from `base` — `base.io.input` / + /// `base.io.output` are real paths (`-i`/`-o` are required), while + /// `multi.io.input` / `multi.io.output` are the empty `PathBuf` default. + /// Every other carrier sub-field is unset on both sides and does compare + /// equal. + #[test] + fn multi_duplex_options_defaults_match_command() { + let base = Duplex::try_parse_from(["duplex", "-i", "in.bam", "-o", "o.bam"]) + .expect("parses") + .to_duplex_options(); + let multi = + PrefixedDuplex::try_parse_from(["x"]).expect("parses").opts.validate().expect("valid"); + + assert_eq!(multi.error_rate_pre_umi, base.error_rate_pre_umi); + assert_eq!(multi.error_rate_post_umi, base.error_rate_post_umi); + assert_eq!(multi.min_input_base_quality, base.min_input_base_quality); + assert_eq!(multi.output_per_base_tags, base.output_per_base_tags); + assert_eq!(multi.trim, base.trim); + assert_eq!(multi.min_consensus_base_quality, base.min_consensus_base_quality); + assert_eq!(multi.tie_rule, base.tie_rule); + assert_eq!(multi.consensus_call_overlapping_bases, base.consensus_call_overlapping_bases); + assert_eq!(multi.min_reads, base.min_reads); + assert_eq!(multi.max_reads_per_strand, base.max_reads_per_strand); + assert_eq!(multi.methylation_mode, base.methylation_mode); + assert_eq!(multi.reference, base.reference); + + assert_eq!(multi.allow_unmapped.enabled, base.allow_unmapped.enabled); + assert_eq!(multi.io.async_reader, base.io.async_reader); + assert_eq!(multi.io.check_crc, base.io.check_crc); + assert_eq!(multi.io.no_check_crc, base.io.no_check_crc); + assert_eq!(multi.rejects_opts.rejects, base.rejects_opts.rejects); + assert_eq!(multi.stats_opts.stats, base.stats_opts.stats); + assert_eq!(multi.read_group.read_group_id, base.read_group.read_group_id); + assert_eq!(multi.read_group.read_name_prefix, base.read_group.read_name_prefix); + } + + /// A prefixed flag must round-trip through `MultiDuplexOptions::validate`. + #[test] + fn multi_duplex_options_round_trips_min_reads() { + let multi = PrefixedDuplex::try_parse_from(["x", "--duplex::min-reads", "2,3"]) + .expect("parses") + .opts + .validate() + .expect("valid"); + assert_eq!(multi.min_reads, vec![2, 3]); + } } From a25d60c9e5e225e70eb42c6f9ebe499ea3bc3b46 Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 16:33:22 -0700 Subject: [PATCH 07/15] feat(simplex): derive clap::Args + multi_options on SimplexOptions Adds per-field #[arg] attributes (copied verbatim from ConsensusCallingOptions / OverlappingConsensusOptions) and #[fgumi_cli_macros::multi_options("simplex", "Simplex Options")] to SimplexOptions, generating a MultiSimplexOptions companion for a future fused runall command. tie_rule, allow_unmapped, io, rejects_opts, stats_opts, read_group, methylation_mode, and reference are #[arg(skip)] (the latter two are cross-stage runall flags landing in a later PR; the rest are hidden or data-carrier fields). min_reads has no default_value on the standalone command, so the macro lifts it to a staged-required field on MultiSimplexOptions. SimplexOptions previously had no Default impl; this adds one (min_reads defaults to 1, matching the smallest valid family size, since it is never read back through the staged-required validate() path). SimplexOptions is not flattened anywhere by this change; Simplex, to_simplex_options, and SimplexOptions::consensus()/overlapping()/ validate_read_bounds() are untouched. --- src/lib/commands/simplex.rs | 203 +++++++++++++++++++++++++++++++++++- 1 file changed, 198 insertions(+), 5 deletions(-) diff --git a/src/lib/commands/simplex.rs b/src/lib/commands/simplex.rs index f3b37df44..fdf6537da 100644 --- a/src/lib/commands/simplex.rs +++ b/src/lib/commands/simplex.rs @@ -255,52 +255,142 @@ pub struct Simplex { /// Simplex-stage tuning, independent of how the values were supplied. /// -/// See [`crate::commands::zipper::ZipperOptions`] for why this is a plain -/// struct rather than a flattened `clap::Args`. Note that the consensus-calling +/// Derives `clap::Args` and carries `#[fgumi_cli_macros::multi_options]` so a +/// future fused `runall` command can re-expose each field as a prefixed +/// `--simplex::`, via the generated `MultiSimplexOptions` companion, +/// without hand-maintaining a parallel option set. This struct itself is not +/// flattened into [`Simplex`] or anywhere else by this change — the standalone +/// command still fills [`Simplex`]'s own fields (including its nested +/// `consensus` / `overlapping` sub-structs) and projects them through +/// [`Simplex::to_simplex_options`]; that path is untouched. The consensus-calling /// knobs are held **flat** here even though [`Simplex`] nests them behind -/// `#[command(flatten)]` sub-structs: the chain builder wants one bag per stage, -/// not a re-run of the CLI's grouping. -#[derive(Debug, Clone)] +/// `#[command(flatten)]` sub-structs: the chain builder wants one bag per +/// stage, not a re-run of the CLI's grouping. Each `#[arg]` below is copied +/// verbatim from the corresponding field on [`ConsensusCallingOptions`] / +/// [`OverlappingConsensusOptions`]. +/// +/// `min_reads` has no `default_value` on the standalone command (fgbio's +/// `CallMolecularConsensusReads` requires it explicitly), so the macro lifts +/// it to a staged-required field: `MultiSimplexOptions::validate()` demands +/// `--simplex::min-reads`. +/// +/// `tie_rule` is `#[arg(skip)]`: on the standalone command it is resolved from +/// `TieRuleArg` (a hidden, cross-tool equivalency-testing knob), and +/// `--simplex::tie-rule` is not re-exposed by `runall`. [`fgumi_consensus::TieRule`] +/// implements `Default` (`FgbioCompat`), matching `TieRuleArg::FgbioCompat`'s +/// resolution. +/// +/// `allow_unmapped` is `#[arg(skip = AllowUnmappedOptions { enabled: false })]`: +/// [`AllowUnmappedOptions`] does not implement `Default`, so the explicit +/// expression form is required; `enabled: false` matches the standalone +/// command's default. +/// +/// `io` / `rejects_opts` / `stats_opts` / `read_group` are `#[arg(skip)]` +/// data carriers baked in by `runall`, not `--simplex::` flags — each carrier +/// type implements `Default`. +/// +/// `methylation_mode` is `#[arg(skip)]`: on the standalone command it is +/// resolved from `Option` via +/// [`crate::commands::common::resolve_methylation_mode`], and +/// `--methylation-mode` itself is a cross-stage top-level `runall` flag +/// (PR B), not a `--simplex::` flag. [`fgumi_consensus::MethylationMode`] +/// implements `Default` (`Disabled`), which is what the skipped field falls +/// back to. +/// +/// `reference` is `#[arg(skip)]`: `--ref` is a cross-stage top-level `runall` +/// flag (PR B), not a `--simplex::` flag. +#[fgumi_cli_macros::multi_options("simplex", "Simplex Options")] +#[derive(Debug, Clone, clap::Args)] pub struct SimplexOptions { /// Pre-UMI error rate (phred). + #[arg(short = '1', long = "error-rate-pre-umi", default_value = "45")] pub error_rate_pre_umi: u8, /// Post-UMI error rate (phred). + #[arg(short = '2', long = "error-rate-post-umi", default_value = "40")] pub error_rate_post_umi: u8, /// Minimum input base quality. + #[arg(short = 'm', long = "min-input-base-quality", default_value = "10")] pub min_input_base_quality: u8, /// Emit per-base consensus tags. + #[arg(short = 'B', long = "output-per-base-tags", value_name = "true|false", default_value = "true", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub output_per_base_tags: bool, /// Trim consensus reads. + #[arg(long = "trim", value_name = "true|false", default_value = "false", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub trim: bool, /// Minimum consensus base quality. + #[arg(long = "min-consensus-base-quality", default_value = "2")] pub min_consensus_base_quality: u8, /// How to resolve a near-tie between the two most likely consensus bases. + #[arg(skip)] pub tie_rule: fgumi_consensus::TieRule, /// Call overlapping bases jointly. + #[arg(long = "consensus-call-overlapping-bases", value_name = "true|false", default_value = "true", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub consensus_call_overlapping_bases: bool, /// Minimum reads per consensus. + #[arg(short = 'M', long = "min-reads")] pub min_reads: usize, /// Cap on reads per consensus. + #[arg(long = "max-reads")] pub max_reads: Option, /// Let fully-unmapped primary templates through the pre-group filter. /// /// Carried as the whole flattened sub-struct, like `io` / `rejects_opts` / /// `read_group`, rather than as a bare `bool`. + #[arg(skip = AllowUnmappedOptions { enabled: false })] pub allow_unmapped: AllowUnmappedOptions, /// Input/output paths and reader mode. + #[arg(skip)] pub io: BamIoOptions, /// Optional rejects output. + #[arg(skip)] pub rejects_opts: RejectsOptions, /// Optional stats output. + #[arg(skip)] pub stats_opts: StatsOptions, /// Read-group identity for emitted reads. + #[arg(skip)] pub read_group: ReadGroupOptions, /// Resolved methylation calling mode (`Disabled` when the flag is unset). + #[arg(skip)] pub methylation_mode: fgumi_consensus::MethylationMode, /// Reference FASTA for methylation-aware modes. + #[arg(skip)] pub reference: Option, } +/// Test/`runall`-construction default: consensus/overlapping knobs from the +/// shared option defaults, methylation disabled. `min_reads` has no +/// `default_value` on the standalone command (it is staged-required on the +/// `runall` side), so this picks `1` — the smallest valid family size — +/// purely for a usable `Default::default()`; it is never read back through +/// `MultiSimplexOptions::validate()`, which demands `--simplex::min-reads` +/// explicitly. +impl Default for SimplexOptions { + fn default() -> Self { + let consensus = ConsensusCallingOptions::default(); + let overlapping = OverlappingConsensusOptions::default(); + Self { + error_rate_pre_umi: consensus.error_rate_pre_umi, + error_rate_post_umi: consensus.error_rate_post_umi, + min_input_base_quality: consensus.min_input_base_quality, + output_per_base_tags: consensus.output_per_base_tags, + trim: consensus.trim, + min_consensus_base_quality: consensus.min_consensus_base_quality, + tie_rule: consensus.tie_rule.into(), + consensus_call_overlapping_bases: overlapping.consensus_call_overlapping_bases, + min_reads: 1, + max_reads: None, + allow_unmapped: AllowUnmappedOptions { enabled: false }, + io: BamIoOptions::default(), + rejects_opts: RejectsOptions::default(), + stats_opts: StatsOptions::default(), + read_group: ReadGroupOptions::default(), + methylation_mode: fgumi_consensus::MethylationMode::default(), + reference: None, + } + } +} + impl Simplex { /// Project the parsed CLI flags into [`SimplexOptions`]. #[must_use] @@ -2238,4 +2328,107 @@ mod tests { Ok(()) } + + // ───────────────────────────────────────────────────────────────────── + // SimplexOptions / MultiSimplexOptions parity (multi_options) + // ───────────────────────────────────────────────────────────────────── + + /// `MultiSimplexOptions` derives `clap::Args`, not `Parser` — flatten it + /// into a local wrapper to drive it through `try_parse_from`. + #[derive(clap::Parser, Debug)] + struct PrefixedSimplex { + #[command(flatten)] + opts: MultiSimplexOptions, + } + + /// The re-exposed `MultiSimplexOptions` defaults must equal the standalone + /// `simplex` command's defaults, projected through `to_simplex_options`. + /// This is the strong oracle: it fails on a dropped default, a + /// misclassified field, or a value that only happens to match by + /// coincidence. + /// + /// `--min-reads` has no `default_value` on the standalone command, so + /// both sides are anchored with the same value (`-M 3` / + /// `--simplex::min-reads 3`) rather than compared against a default — the + /// standalone side is NOT staged-required (clap itself demands it), while + /// the multi side IS staged-required and would fail `validate()` if + /// omitted. + /// + /// `io` / `rejects_opts` / `stats_opts` / `read_group` are `#[arg(skip)]` + /// data carriers on `MultiSimplexOptions`, so their values come from each + /// carrier's own `Default` rather than from `base` — `base.io.input` / + /// `base.io.output` are real paths (`-i`/`-o` are required), while + /// `multi.io.input` / `multi.io.output` are the empty `PathBuf` default. + /// Every other carrier sub-field is unset on both sides and does compare + /// equal. + #[test] + fn multi_simplex_options_defaults_match_command() { + let base = + Simplex::try_parse_from(["simplex", "-i", "in.bam", "-o", "o.bam", "--min-reads", "3"]) + .expect("parses") + .to_simplex_options(); + let multi = PrefixedSimplex::try_parse_from(["x", "--simplex::min-reads", "3"]) + .expect("parses") + .opts + .validate() + .expect("valid"); + + // Anchor field: both sides were fed the same non-default value. + assert_eq!(multi.min_reads, 3); + assert_eq!(multi.min_reads, base.min_reads); + + // Every other projected field must sit at its default on both sides. + assert_eq!(multi.error_rate_pre_umi, base.error_rate_pre_umi); + assert_eq!(multi.error_rate_post_umi, base.error_rate_post_umi); + assert_eq!(multi.min_input_base_quality, base.min_input_base_quality); + assert_eq!(multi.output_per_base_tags, base.output_per_base_tags); + assert_eq!(multi.trim, base.trim); + assert_eq!(multi.min_consensus_base_quality, base.min_consensus_base_quality); + assert_eq!(multi.tie_rule, base.tie_rule); + assert_eq!(multi.consensus_call_overlapping_bases, base.consensus_call_overlapping_bases); + assert_eq!(multi.max_reads, base.max_reads); + assert_eq!(multi.methylation_mode, base.methylation_mode); + assert_eq!(multi.reference, base.reference); + + assert_eq!(multi.allow_unmapped.enabled, base.allow_unmapped.enabled); + assert_eq!(multi.io.async_reader, base.io.async_reader); + assert_eq!(multi.io.check_crc, base.io.check_crc); + assert_eq!(multi.io.no_check_crc, base.io.no_check_crc); + assert_eq!(multi.rejects_opts.rejects, base.rejects_opts.rejects); + assert_eq!(multi.stats_opts.stats, base.stats_opts.stats); + assert_eq!(multi.read_group.read_group_id, base.read_group.read_group_id); + assert_eq!(multi.read_group.read_name_prefix, base.read_group.read_name_prefix); + } + + /// A prefixed flag must round-trip through `MultiSimplexOptions::validate`. + #[test] + fn multi_simplex_options_round_trips_max_reads() { + let multi = PrefixedSimplex::try_parse_from([ + "x", + "--simplex::min-reads", + "1", + "--simplex::max-reads", + "50", + ]) + .expect("parses") + .opts + .validate() + .expect("valid"); + assert_eq!(multi.max_reads, Some(50)); + } + + /// `--simplex::min-reads` has no default on the standalone command, so the + /// macro must lift it to a staged-required field: omitting it must parse + /// fine (staged validation, not clap, owns the requirement) but fail + /// `validate()`. + #[test] + fn multi_simplex_options_min_reads_is_staged_required() { + let parsed = PrefixedSimplex::try_parse_from(["x"]).expect("parse is staged, not clap"); + let err = parsed.opts.validate().expect_err("min-reads must be required"); + let msg = format!("{err:#}"); + assert!( + msg.contains("--simplex::min-reads is required"), + "error should name --simplex::min-reads: {msg}" + ); + } } From d9cceff2d9cc75ea94e1be7999b1a70a1655da3b Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 16:43:05 -0700 Subject: [PATCH 08/15] feat(group): derive clap::Args + multi_options on GroupOptions Adds per-field #[arg] annotations matching GroupReadsByUmi's flags and the #[fgumi_cli_macros::multi_options("group", "Group Options")] macro to GroupOptions, so it can be re-exposed as a prefixed --group::* flag set for a future runall stage. strategy has no default, so the macro lifts it to a staged-required field enforced by validate() rather than clap. effective_strategy/effective_edits stay at their #[arg(skip)] placeholders in this change; resolving them via resolve_strategy_and_edits is deferred to a later change. to_group_options, resolve_strategy_and_edits (both copies), resolved_min_map_q, and the existing Default impl are untouched. --- src/lib/commands/group.rs | 151 +++++++++++++++++++++++++++++++++++++- 1 file changed, 150 insertions(+), 1 deletion(-) diff --git a/src/lib/commands/group.rs b/src/lib/commands/group.rs index 008f67e69..4035faafe 100644 --- a/src/lib/commands/group.rs +++ b/src/lib/commands/group.rs @@ -674,42 +674,64 @@ pub struct GroupReadsByUmi { /// `effective_strategy` / `effective_edits` carry the `--no-umi` and /// identity-implies-zero-edits rules. Both come from the same methods /// `execute` uses, so the command and the chain builder cannot drift apart. -#[derive(Debug, Clone)] +#[fgumi_cli_macros::multi_options("group", "Group Options")] +#[derive(Debug, Clone, clap::Args)] #[allow(clippy::struct_excessive_bools)] // mirrors the CLI flags 1:1; each bool is a distinct option pub struct GroupOptions { /// Minimum mapping quality for mapped reads, with the default applied. + #[arg(short = 'm', long = "min-map-q", default_value_t = 1)] pub min_map_q: u8, /// Include non-PF reads. + #[arg(short = 'n', long = "include-non-pf-reads", value_name = "true|false", default_value = "false", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub include_non_pf_reads: bool, /// Allow fully unmapped templates. + #[arg(long = "allow-unmapped", value_name = "true|false", default_value = "false", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub allow_unmapped: bool, /// The strategy as requested on the command line. + #[arg(short = 's', long = "strategy", value_enum)] pub strategy: Strategy, /// The edit distance as requested on the command line. + #[arg(short = 'e', long = "edits", default_value = "1")] pub edits: u32, /// Minimum UMI length to accept. + #[arg(short = 'l', long = "min-umi-length")] pub min_umi_length: Option, /// When to build the N-gram/BK-tree index instead of scanning linearly. /// /// This is [`fgumi_umi::IndexThreshold`] rather than a bare count: the /// flag also accepts `always` / `never`, which a number cannot express. + #[arg(long = "index-threshold", default_value = "100")] pub index_threshold: IndexThreshold, /// Skip UMI-based grouping entirely. + #[arg(long = "no-umi", value_name = "true|false", default_value = "false", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub no_umi: bool, /// Template-count floor for handing a position group to a parallel assigner. + #[arg(long = "parallel-group-min-templates", value_name = "N|auto")] pub parallel_group_min_templates: Option, /// The strategy actually used, after applying `--no-umi`. + /// + /// Placeholder in PR A: `Multi::validate()` leaves this at the skip + /// default rather than resolving it — resolution via + /// [`GroupOptions::resolve_strategy_and_edits`] is wired up by PR B. + #[arg(skip = Strategy::Identity)] pub effective_strategy: Strategy, /// The edit distance actually used, after applying `--no-umi` and the /// identity-implies-zero rule. + /// + /// Placeholder in PR A; see `effective_strategy`. + #[arg(skip)] pub effective_edits: u32, /// Optional family-size histogram output. + #[arg(short = 'f', long = "family-size-histogram")] pub family_size_histogram: Option, /// Optional grouping-metrics output. + #[arg(short = 'g', long = "grouping-metrics")] pub grouping_metrics: Option, /// Optional output prefix for the full set of metrics files. + #[arg(short = 'M', long = "metrics")] pub metrics_prefix: Option, /// Verify the input is in strict template-coordinate sort order (`--verify`). + #[arg(long = "verify", value_name = "true|false", default_value = "false", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub verify: bool, } @@ -1636,6 +1658,133 @@ mod tests { assert_eq!(opts.metrics_prefix, None); } + // ───────────────────────────────────────────────────────────────────── + // GroupOptions / MultiGroupOptions parity (multi_options) + // ───────────────────────────────────────────────────────────────────── + + #[derive(clap::Parser, Debug)] + struct PrefixedGroup { + #[command(flatten)] + opts: MultiGroupOptions, + } + + /// The re-exposed `MultiGroupOptions` defaults must equal the standalone + /// `group` command's defaults, projected through `to_group_options` — + /// except `effective_strategy`/`effective_edits`, which PR A leaves at + /// their `#[arg(skip)]` placeholders (`Multi::validate()` does not + /// call `resolve_strategy_and_edits`; that wiring lands in PR B). + #[test] + fn multi_group_options_defaults_match_command() { + let base = GroupReadsByUmi::try_parse_from([ + "group", + "-i", + "in.bam", + "-o", + "o.bam", + "-s", + "adjacency", + ]) + .expect("parses") + .to_group_options(); + let multi = PrefixedGroup::try_parse_from(["x", "--group::strategy", "adjacency"]) + .expect("parses") + .opts + .validate() + .expect("valid"); + + assert_eq!(multi.min_map_q, base.min_map_q); + assert_eq!(multi.include_non_pf_reads, base.include_non_pf_reads); + assert_eq!(multi.allow_unmapped, base.allow_unmapped); + assert_eq!(multi.strategy, base.strategy); + assert_eq!(multi.edits, base.edits); + assert_eq!(multi.min_umi_length, base.min_umi_length); + assert_eq!(multi.index_threshold, base.index_threshold); + assert_eq!(multi.no_umi, base.no_umi); + assert_eq!(multi.parallel_group_min_templates, base.parallel_group_min_templates); + assert_eq!(multi.family_size_histogram, base.family_size_histogram); + assert_eq!(multi.grouping_metrics, base.grouping_metrics); + assert_eq!(multi.metrics_prefix, base.metrics_prefix); + assert_eq!(multi.verify, base.verify); + + // Divergence: PR A leaves effective_* at the skip defaults. The + // standalone `to_group_options` resolves them to (Adjacency, 1) for + // this same input; resolved in PR B via resolve_strategy_and_edits. + assert_eq!((multi.effective_strategy, multi.effective_edits), (Strategy::Identity, 0)); + } + + /// `--group::strategy` has no default on the standalone command, so the + /// macro must lift it to a staged-required field: omitting it must parse + /// fine (staged validation, not clap, owns the requirement) but fail + /// `validate()`. + #[test] + fn multi_group_options_strategy_is_staged_required() { + let parsed = PrefixedGroup::try_parse_from(["x"]).expect("parse is staged, not clap"); + let err = parsed.opts.validate().expect_err("strategy must be required"); + let msg = format!("{err:#}"); + assert!( + msg.contains("--group::strategy is required"), + "error should name --group::strategy: {msg}" + ); + } + + /// Every prefixed flag must round-trip through `MultiGroupOptions::validate` + /// with its supplied value, not silently retain its base default. Default + /// parity (see `multi_group_options_defaults_match_command`) cannot catch a + /// parser-accepted flag that maps to nothing, because an omitted flag and a + /// dropped flag both leave the field at its default. Supplying a *non*-default + /// value for each flag — every boolean enabled, the renamed `--group::metrics` + /// (→ `metrics_prefix`), the scalar and path options — and asserting the + /// validated field is what proves each mapping is wired end to end. The two + /// `#[arg(skip)]` placeholders (`effective_strategy`/`effective_edits`) are not + /// parseable and stay at their PR-A defaults; that divergence is asserted by the + /// default-parity test. + #[test] + fn multi_group_options_round_trips_every_supplied_flag() { + let multi = PrefixedGroup::try_parse_from([ + "x", + "--group::min-map-q", + "7", + "--group::include-non-pf-reads=true", + "--group::allow-unmapped=true", + "--group::strategy", + "adjacency", + "--group::edits", + "3", + "--group::min-umi-length", + "5", + "--group::index-threshold", + "42", + "--group::no-umi=true", + "--group::parallel-group-min-templates", + "16", + "--group::family-size-histogram", + "fs.txt", + "--group::grouping-metrics", + "gm.txt", + "--group::metrics", + "mp", + "--group::verify=true", + ]) + .expect("parses") + .opts + .validate() + .expect("valid"); + + assert_eq!(multi.min_map_q, 7); + assert!(multi.include_non_pf_reads, "--group::include-non-pf-reads=true must round-trip"); + assert!(multi.allow_unmapped, "--group::allow-unmapped=true must round-trip"); + assert_eq!(multi.strategy, Strategy::Adjacency); + assert_eq!(multi.edits, 3); + assert_eq!(multi.min_umi_length, Some(5)); + assert_eq!(multi.index_threshold, IndexThreshold::MinUmis(42)); + assert!(multi.no_umi, "--group::no-umi=true must round-trip"); + assert_eq!(multi.parallel_group_min_templates, Some(ParallelMinTemplates::Fixed(16))); + assert_eq!(multi.family_size_histogram, Some(PathBuf::from("fs.txt"))); + assert_eq!(multi.grouping_metrics, Some(PathBuf::from("gm.txt"))); + assert_eq!(multi.metrics_prefix, Some(PathBuf::from("mp"))); + assert!(multi.verify, "--group::verify=true must round-trip to verify=true"); + } + use crate::assigner::{IdentityUmiAssigner, PairedUmiAssigner, Strategy}; use crate::metrics::TemplateFilterReason; use bstr::BString; From 3ea0d8d8c1ae151d23489ec25976c6997c55b6bf Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 16:49:21 -0700 Subject: [PATCH 09/15] feat(codec): derive clap::Args + multi_options on CodecOptions Adds per-field #[arg] attrs and #[fgumi_cli_macros::multi_options("codec", "Codec Options")] to CodecOptions, mirroring the pattern already landed for DuplexOptions/SimplexOptions. Codec has no overlapping/methylation/reference fields, so its field set is a strict subset of duplex/simplex's. Also adds the (previously missing) Default impl, matching the standalone codec command's defaults. to_codec_options() and CodecOptions::validate() are untouched -- both keep reading the same flat fields. --- src/lib/commands/codec.rs | 166 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 161 insertions(+), 5 deletions(-) diff --git a/src/lib/commands/codec.rs b/src/lib/commands/codec.rs index d9329f5e1..72568e4d3 100644 --- a/src/lib/commands/codec.rs +++ b/src/lib/commands/codec.rs @@ -315,61 +315,148 @@ pub struct Codec { /// Codec-stage tuning, independent of how the values were supplied. /// -/// See [`crate::commands::zipper::ZipperOptions`] for why this is a plain -/// struct rather than a flattened `clap::Args`. Note that the consensus-calling +/// Derives `clap::Args` and carries `#[fgumi_cli_macros::multi_options]` so a +/// future fused `runall` command can re-expose each field as a prefixed +/// `--codec::`, via the generated `MultiCodecOptions` companion, +/// without hand-maintaining a parallel option set. This struct itself is not +/// flattened into [`Codec`] or anywhere else by this change — the standalone +/// command still fills [`Codec`]'s own fields (including its nested +/// `consensus` sub-struct) and projects them through +/// [`Codec::to_codec_options`]; that path is untouched. The consensus-calling /// knobs are held **flat** here even though [`Codec`] nests them behind -/// `#[command(flatten)]` sub-structs: the chain builder wants one bag per stage, -/// not a re-run of the CLI's grouping. -#[derive(Debug, Clone)] +/// `#[command(flatten)]` sub-structs: the chain builder wants one bag per +/// stage, not a re-run of the CLI's grouping. Each `#[arg]` below is copied +/// verbatim from the corresponding field on [`ConsensusCallingOptions`] (the +/// 6 consensus scalars) or [`Codec`] itself (the codec-specific fields). +/// Unlike [`crate::commands::duplex::DuplexOptions`] / +/// [`crate::commands::simplex::SimplexOptions`], CODEC has no +/// `OverlappingConsensusOptions`, no `consensus_call_overlapping_bases`, no +/// methylation mode, and no reference fields. +/// +/// `tie_rule` is `#[arg(skip)]`: on the standalone command it is resolved from +/// `TieRuleArg` (a hidden, cross-tool equivalency-testing knob), and +/// `--codec::tie-rule` is not re-exposed by `runall`. [`fgumi_consensus::TieRule`] +/// implements `Default` (`FgbioCompat`), matching `TieRuleArg::FgbioCompat`'s +/// resolution. +/// +/// `min_reads` has a `default_value` on the standalone command (unlike +/// simplex/duplex's `min-reads`), so the macro does NOT lift it to a +/// staged-required field: `MultiCodecOptions::validate()` accepts an omitted +/// `--codec::min-reads`. +/// +/// `allow_unmapped` is `#[arg(skip = AllowUnmappedOptions { enabled: false })]`: +/// [`AllowUnmappedOptions`] does not implement `Default`, so the explicit +/// expression form is required; `enabled: false` matches the standalone +/// command's default. +/// +/// `io` / `rejects_opts` / `stats_opts` / `read_group` are `#[arg(skip)]` +/// data carriers baked in by `runall`, not `--codec::` flags — each carrier +/// type implements `Default`. +#[fgumi_cli_macros::multi_options("codec", "Codec Options")] +#[derive(Debug, Clone, clap::Args)] pub struct CodecOptions { /// Pre-UMI error rate (phred). + #[arg(short = '1', long = "error-rate-pre-umi", default_value = "45")] pub error_rate_pre_umi: u8, /// Post-UMI error rate (phred). + #[arg(short = '2', long = "error-rate-post-umi", default_value = "40")] pub error_rate_post_umi: u8, /// Minimum input base quality. + #[arg(short = 'm', long = "min-input-base-quality", default_value = "10")] pub min_input_base_quality: u8, /// Emit per-base consensus tags. + #[arg(short = 'B', long = "output-per-base-tags", value_name = "true|false", default_value = "true", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub output_per_base_tags: bool, /// Trim consensus reads. + #[arg(long = "trim", value_name = "true|false", default_value = "false", num_args = 0..=1, default_missing_value = "true", action = clap::ArgAction::Set, value_parser = clap::builder::BoolishValueParser::new(), hide_possible_values = true)] pub trim: bool, /// Minimum consensus base quality. + #[arg(long = "min-consensus-base-quality", default_value = "2")] pub min_consensus_base_quality: u8, /// How to resolve a near-tie between the two most likely consensus bases. + #[arg(skip)] pub tie_rule: fgumi_consensus::TieRule, /// Minimum reads per consensus. + #[arg(short = 'M', long = "min-reads", default_value = "1")] pub min_reads: usize, /// Cap on reads per consensus. + #[arg(long = "max-reads")] pub max_reads: Option, /// Minimum duplex overlap length. + #[arg(short = 'd', long = "min-duplex-length", default_value = "1")] pub min_duplex_length: usize, /// Reproduce fgbio's legacy (pre-fgumi#761) overlap window for dovetailed /// FR pairs. Off by default; see [`Codec::legacy_overlap_window`]. + #[arg(long = "legacy-overlap-window")] pub legacy_overlap_window: bool, /// Quality cap for single-strand positions. + #[arg(long = "single-strand-qual")] pub single_strand_qual: Option, /// Quality cap for outer bases. + #[arg(short = 'Q', long = "outer-bases-qual")] pub outer_bases_qual: Option, /// How many bases count as outer. + #[arg(short = 'O', long = "outer-bases-length", default_value = "5")] pub outer_bases_length: usize, /// Maximum duplex disagreement rate. + #[arg(short = 'x', long = "max-duplex-disagreement-rate", default_value = "1.0")] pub max_duplex_disagreement_rate: f64, /// Maximum duplex disagreements. + #[arg(short = 'X', long = "max-duplex-disagreements")] pub max_duplex_disagreements: Option, /// Let fully-unmapped primary templates through the pre-group filter. /// /// Carried as the whole flattened sub-struct, like `io` / `rejects_opts` / /// `read_group`, rather than as a bare `bool`. + #[arg(skip = AllowUnmappedOptions { enabled: false })] pub allow_unmapped: AllowUnmappedOptions, /// Input/output paths and reader mode. + #[arg(skip)] pub io: BamIoOptions, /// Optional rejects output. + #[arg(skip)] pub rejects_opts: RejectsOptions, /// Optional stats output. + #[arg(skip)] pub stats_opts: StatsOptions, /// Read-group identity for emitted reads. + #[arg(skip)] pub read_group: ReadGroupOptions, } +/// Test/`runall`-construction default: consensus knobs from the shared +/// option defaults, codec-specific fields matching the standalone command's +/// `default_value`s (`min_reads: 1`, `min_duplex_length: 1`, +/// `outer_bases_length: 5`, `max_duplex_disagreement_rate: 1.0`). +impl Default for CodecOptions { + fn default() -> Self { + let consensus = ConsensusCallingOptions::default(); + Self { + error_rate_pre_umi: consensus.error_rate_pre_umi, + error_rate_post_umi: consensus.error_rate_post_umi, + min_input_base_quality: consensus.min_input_base_quality, + output_per_base_tags: consensus.output_per_base_tags, + trim: consensus.trim, + min_consensus_base_quality: consensus.min_consensus_base_quality, + tie_rule: consensus.tie_rule.into(), + min_reads: 1, + max_reads: None, + min_duplex_length: 1, + legacy_overlap_window: false, + single_strand_qual: None, + outer_bases_qual: None, + outer_bases_length: 5, + max_duplex_disagreement_rate: 1.0, + max_duplex_disagreements: None, + allow_unmapped: AllowUnmappedOptions { enabled: false }, + io: BamIoOptions::default(), + rejects_opts: RejectsOptions::default(), + stats_opts: StatsOptions::default(), + read_group: ReadGroupOptions::default(), + } + } +} + impl Codec { /// Project the parsed CLI flags into [`CodecOptions`]. #[must_use] @@ -2164,4 +2251,73 @@ mod tests { "underlying source must be preserved; got: {chain}" ); } + + // ───────────────────────────────────────────────────────────────────── + // CodecOptions / MultiCodecOptions parity (multi_options) + // ───────────────────────────────────────────────────────────────────── + + /// `MultiCodecOptions` derives `clap::Args`, not `Parser` — flatten it + /// into a local wrapper to drive it through `try_parse_from`. + #[derive(clap::Parser, Debug)] + struct PrefixedCodec { + #[command(flatten)] + opts: MultiCodecOptions, + } + + /// The re-exposed `MultiCodecOptions` defaults must equal the standalone + /// `codec` command's defaults, projected through `to_codec_options`. + /// Unlike simplex, `min_reads` has a `default_value` on the standalone + /// command, so `MultiCodecOptions` is NOT staged-required and validates + /// with no flags at all. + #[test] + fn multi_codec_options_defaults_match_command() { + let base = Codec::try_parse_from(["codec", "-i", "in.bam", "-o", "o.bam"]) + .expect("parses") + .to_codec_options(); + let multi = + PrefixedCodec::try_parse_from(["x"]).expect("parses").opts.validate().expect("valid"); + + assert_eq!(multi.error_rate_pre_umi, base.error_rate_pre_umi); + assert_eq!(multi.error_rate_post_umi, base.error_rate_post_umi); + assert_eq!(multi.min_input_base_quality, base.min_input_base_quality); + assert_eq!(multi.output_per_base_tags, base.output_per_base_tags); + assert_eq!(multi.trim, base.trim); + assert_eq!(multi.min_consensus_base_quality, base.min_consensus_base_quality); + assert_eq!(multi.tie_rule, base.tie_rule); + assert_eq!(multi.min_reads, base.min_reads); + assert_eq!(multi.min_reads, 1); + assert_eq!(multi.max_reads, base.max_reads); + assert_eq!(multi.min_duplex_length, base.min_duplex_length); + assert_eq!(multi.min_duplex_length, 1); + assert_eq!(multi.legacy_overlap_window, base.legacy_overlap_window); + assert_eq!(multi.single_strand_qual, base.single_strand_qual); + assert_eq!(multi.outer_bases_qual, base.outer_bases_qual); + assert_eq!(multi.outer_bases_length, base.outer_bases_length); + assert_eq!(multi.outer_bases_length, 5); + assert!( + (multi.max_duplex_disagreement_rate - base.max_duplex_disagreement_rate).abs() < 1e-12 + ); + assert!((multi.max_duplex_disagreement_rate - 1.0).abs() < 1e-12); + assert_eq!(multi.max_duplex_disagreements, base.max_duplex_disagreements); + + assert_eq!(multi.allow_unmapped.enabled, base.allow_unmapped.enabled); + assert_eq!(multi.io.async_reader, base.io.async_reader); + assert_eq!(multi.io.check_crc, base.io.check_crc); + assert_eq!(multi.io.no_check_crc, base.io.no_check_crc); + assert_eq!(multi.rejects_opts.rejects, base.rejects_opts.rejects); + assert_eq!(multi.stats_opts.stats, base.stats_opts.stats); + assert_eq!(multi.read_group.read_group_id, base.read_group.read_group_id); + assert_eq!(multi.read_group.read_name_prefix, base.read_group.read_name_prefix); + } + + /// A prefixed flag must round-trip through `MultiCodecOptions::validate`. + #[test] + fn multi_codec_options_round_trips_outer_bases_length() { + let multi = PrefixedCodec::try_parse_from(["x", "--codec::outer-bases-length", "7"]) + .expect("parses") + .opts + .validate() + .expect("valid"); + assert_eq!(multi.outer_bases_length, 7); + } } From 9bfc00fdf6d5837fde07eb007bb6a70b02b8a002 Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 17:01:29 -0700 Subject: [PATCH 10/15] feat(extract): add ExtractRunallOptions clap::Args variant for runall Add a runall-only clap::Args variant of extract's options. The standalone Extract CLI struct owns --output and the flattened engine sub-structs (threading/compression/scheduler/queue-memory), so it cannot be flattened into a fused runall command directly. ExtractRunallOptions mirrors the current Extract CLI-struct field shapes minus those, carries the multi_options macro so runall can re-expose each field as --extract::, and provides a verbatim-logic to_extract_options projection plus a validate() that re-enforces the three cross-field conflicts the macro requires be dropped from #[arg]. --- src/lib/commands/extract.rs | 398 ++++++++++++++++++++++++++++++++++++ 1 file changed, 398 insertions(+) diff --git a/src/lib/commands/extract.rs b/src/lib/commands/extract.rs index 3c44c1089..1ac2acb56 100644 --- a/src/lib/commands/extract.rs +++ b/src/lib/commands/extract.rs @@ -1648,6 +1648,265 @@ pub struct ExtractOptions { pub no_check_crc: bool, } +/// Extract-stage options for the fused `runall` pipeline — a `runall`-only +/// `clap::Args` variant of [`Extract`]'s options. +/// +/// The standalone [`Extract`] CLI struct owns `--output` and the flattened +/// engine sub-structs (`threading`/`compression`/`scheduler_opts`/ +/// `queue_memory`), which `runall` supplies itself, so [`Extract`] cannot be +/// flattened into a fused command directly. This struct mirrors the current +/// [`Extract`] CLI-struct field shapes minus those, and carries +/// `#[fgumi_cli_macros::multi_options]` so `runall` can re-expose each field as a +/// prefixed `--extract::` via the generated `MultiExtractRunallOptions` +/// companion, without hand-maintaining a parallel option set. +/// +/// The `inputs`/`read_structures`/`interleaved` source-construction fields are +/// not projected by [`Self::to_extract_options`] — they build the FASTQ source +/// for PR B and are exposed `pub` for that consumer. Every other field maps into +/// [`ExtractOptions`] exactly as [`Extract::to_extract_options`] does; in +/// particular `platform` (default `"illumina"`) becomes `Some(..)` and +/// `quality_encoding` is the [`QualityEncoding::Standard`] placeholder the chain +/// FASTQ source overrides at runtime. `--read-structures` is `required` here +/// (unlike the standalone's `+T` default): a fused `runall` always demands it. +/// +/// The three cross-field `conflicts_with` attrs on [`Extract`] +/// (`--store-umi-quals` vs `--extract-umis-from-read-names`, and the +/// `--check-crc`/`--no-check-crc` pair) are dropped because the `multi_options` +/// macro rejects `conflicts_with`; they are re-enforced in [`Self::validate`]. +#[fgumi_cli_macros::multi_options("extract", "Extract Options")] +#[derive(Debug, Clone, clap::Args)] +#[allow(clippy::struct_excessive_bools)] +pub struct ExtractRunallOptions { + // ── Source-construction fields (not projected by to_extract_options) ── + /// Input FASTQ files corresponding to each sequencing read (e.g. R1, I1, etc.). + /// Accepts one or more space-separated values; required for this runall variant. + #[arg(long = "inputs", required = true, num_args = 1..)] + pub inputs: Vec, + + /// Read structures, one for each of the FASTQs (optional if 1-2 template-only FASTQs). + /// Accepts one or more space-separated values; required for this runall variant. + #[arg(long = "read-structures", required = true, num_args = 1..)] + pub read_structures: Vec, + + /// Treat a single input as interleaved paired-end FASTQ (`R1, R2, R1, R2, …`), + /// de-interleaving it into the two reads. Requires exactly one `--input` (a + /// file or `-` for stdin) and describes both reads with two `--read-structures` + /// (defaults to `+T +T`). This lets a streaming trimmer or converter pipe + /// interleaved pairs straight into extract without staging two FASTQ files. + #[arg(long = "interleaved", default_value_t = false)] + pub interleaved: bool, + + // ── Header / behavior fields (projected into ExtractOptions) ── + /// The name of the sequenced sample + #[arg(long, required = true)] + pub sample: String, + + /// The name/ID of the sequenced library + #[arg(long, required = true)] + pub library: String, + + /// Sequencing Platform + #[arg(long, default_value = "illumina")] + pub platform: String, + + /// Library or Sample barcode sequence + #[arg(long)] + pub barcode: Option, + + /// Read group ID to use in the file header + #[arg(long = "read-group-id", default_value = "A")] + pub read_group_id: String, + + /// Platform unit (e.g. 'flowcell-barcode.lane.sample-barcode') + #[arg(long = "platform-unit")] + pub platform_unit: Option, + + /// Platform model to insert into the group header (ex. miseq, hiseq2500, hiseqX) + #[arg(long = "platform-model")] + pub platform_model: Option, + + /// The sequencing center from which the data originated + #[arg(long = "sequencing-center")] + pub sequencing_center: Option, + + /// Predicted median insert size, to insert into the read group header + #[arg(long = "predicted-insert-size")] + pub predicted_insert_size: Option, + + /// Description of the read group + #[arg(long)] + pub description: Option, + + /// Comment(s) to include in the output file's header + #[arg(long, num_args = 0..)] + pub comment: Vec, + + /// Date the run was produced, to insert into the read group header + #[arg(long = "run-date")] + pub run_date: Option, + + /// Store UMI base quality scores in the QX SAM tag + #[arg(long = "store-umi-quals")] + pub store_umi_quals: bool, + + /// Store cell barcode base quality scores in the CY SAM tag + #[arg(long = "store-cell-quals")] + pub store_cell_quals: bool, + + /// Store the sample barcode qualities in the QT Tag + #[arg(long = "store-sample-barcode-qualities")] + pub store_sample_barcode_qualities: bool, + + /// Extract UMI(s) from read names and prepend to UMIs from reads + #[arg(long = "extract-umis-from-read-names")] + #[allow(clippy::struct_field_names)] + pub extract_umis_from_read_names: bool, + + /// Annotate read names with UMIs (appends "+UMIs" to read names) + #[arg(long = "annotate-read-names")] + pub annotate_read_names: bool, + + /// Single tag to store all concatenated UMIs (in addition to per-segment tags) + #[arg(long = "single-tag")] + pub single_tag: Option, + + /// Tag containing adapter clipping position to adjust (e.g. 'XT' from `MarkIlluminaAdapters`) + #[arg(long = "clipping-attribute")] + pub clipping_attribute: Option, + + /// Wrap FASTQ inputs in a userspace async prefetch reader. Dedicates one + /// OS thread per input stream to issue reads ahead of decompression/parsing. + /// Hidden experimental flag. + #[arg(long = "async-reader", default_value_t = false, hide = true)] + pub async_reader: bool, + + /// Verify each BGZF block's CRC32 checksum while decoding the input. + /// + /// Applies to BGZF-compressed FASTQ input (bgzip'd); plain gzip is not + /// block-structured and has no per-block CRC to skip. The policy is honored + /// at any thread count, though BGZF decode runs single-threaded when skipping + /// (verifying BGZF input can decode in parallel). Without either flag, + /// verification defaults on for file input and off for trusted stdin (a + /// freshly-piped stream is trusted; a file may have been archived or + /// transferred since it was written, where a flipped bit is what CRC32 exists + /// to catch). Pass `--check-crc` to force it on. Mutually exclusive with + /// `--no-check-crc`. + #[arg(long = "check-crc", default_value_t = false)] + pub check_crc: bool, + + /// Skip CRC32 verification while decoding BGZF FASTQ input. + /// + /// Trades the CRC32 integrity check for faster BGZF decode (which then runs + /// single-threaded). See `--check-crc` for the default policy this overrides. + /// Mutually exclusive with `--check-crc`. + #[arg(long = "no-check-crc", default_value_t = false)] + pub no_check_crc: bool, +} + +/// Hand-written to match what parsing the minimal required flags (`--extract::sample`, +/// `--extract::library`, `--extract::inputs`, `--extract::read-structures`) through +/// `MultiExtractRunallOptions::try_parse_from` and `validate()` would produce, per the +/// branch-wide invariant "Default == the minimal-parse projection". `#[derive(Default)]` +/// cannot be used here because it would give `platform`/`read_group_id` empty-string +/// defaults instead of the CLI defaults (`"illumina"` / `"A"`). `sample`/`library`/ +/// `inputs`/`read_structures` have no natural default (they are staged-required, lifted +/// by `MultiExtractRunallOptions::validate`, never `Extract`'s own `Default`) and are set +/// to placeholders that are never consumed, mirroring how the other structs' `Default` +/// impls handle required fields. +impl Default for ExtractRunallOptions { + fn default() -> Self { + Self { + inputs: Vec::new(), + read_structures: Vec::new(), + interleaved: false, + sample: String::new(), + library: String::new(), + platform: "illumina".to_string(), + barcode: None, + read_group_id: "A".to_string(), + platform_unit: None, + platform_model: None, + sequencing_center: None, + predicted_insert_size: None, + description: None, + comment: Vec::new(), + run_date: None, + store_umi_quals: false, + store_cell_quals: false, + store_sample_barcode_qualities: false, + extract_umis_from_read_names: false, + annotate_read_names: false, + single_tag: None, + clipping_attribute: None, + async_reader: false, + check_crc: false, + no_check_crc: false, + } + } +} + +impl ExtractRunallOptions { + /// Project into the per-stage [`ExtractOptions`] the chain builder consumes. + /// + /// Verbatim-logic copy of [`Extract::to_extract_options`]: `quality_encoding` + /// is the [`QualityEncoding::Standard`] placeholder (the chain FASTQ source + /// overrides it while opening its readers), `platform` becomes `Some(..)`, and + /// `clipping_attribute` is intentionally dropped — it does not apply to FASTQ + /// input. The `inputs`/`read_structures`/`interleaved` source fields are not + /// part of this projection (PR B builds the FASTQ source from them). + #[must_use] + pub fn to_extract_options(&self) -> ExtractOptions { + ExtractOptions { + sample: self.sample.clone(), + library: self.library.clone(), + platform: Some(self.platform.clone()), + platform_unit: self.platform_unit.clone(), + read_group_id: self.read_group_id.clone(), + comments: self.comment.clone(), + barcode: self.barcode.clone(), + platform_model: self.platform_model.clone(), + sequencing_center: self.sequencing_center.clone(), + predicted_insert_size: self.predicted_insert_size, + description: self.description.clone(), + run_date: self.run_date.clone(), + quality_encoding: QualityEncoding::Standard, + store_umi_quals: self.store_umi_quals, + store_cell_quals: self.store_cell_quals, + single_tag: self.single_tag, + annotate_read_names: self.annotate_read_names, + extract_umis_from_read_names: self.extract_umis_from_read_names, + store_sample_barcode_qualities: self.store_sample_barcode_qualities, + async_reader: self.async_reader, + check_crc: self.check_crc, + no_check_crc: self.no_check_crc, + } + } + + /// Re-enforce the cross-field conflicts the `multi_options` macro required us + /// to drop from the `#[arg]` attributes (mirrors [`Extract::validate`]). + /// + /// This is separate from the macro-generated `MultiExtractRunallOptions:: + /// validate()`, which only lifts the staged-required flags; `runall`/PR B + /// calls this after building the [`ExtractRunallOptions`]. + /// + /// # Errors + /// + /// Returns an error if `--store-umi-quals` is combined with + /// `--extract-umis-from-read-names`, or if `--check-crc` and `--no-check-crc` + /// are both set. + pub fn validate(&self) -> Result<()> { + ensure!( + !self.extract_umis_from_read_names || !self.store_umi_quals, + "Cannot store UMI qualities (--store-umi-quals) when also extracting UMIs from read names (--extract-umis-from-read-names)." + ); + ensure!( + !(self.check_crc && self.no_check_crc), + "--check-crc and --no-check-crc are mutually exclusive." + ); + Ok(()) + } +} + /// Build raw BAM `RawRecord`s from a [`FastqSet`]. /// /// This is the core extract logic: applies read structures (via the segments @@ -5574,4 +5833,143 @@ mod tests { only_from_name.extract_umis_from_read_names = true; only_from_name.validate().expect("extract-umis-from-read-names alone must validate"); } + + // ───────────────────────────────────────────────────────────────────── + // ExtractRunallOptions / MultiExtractRunallOptions parity (multi_options) + // ───────────────────────────────────────────────────────────────────── + + /// `MultiExtractRunallOptions` derives `clap::Args` (not `Parser`), so parse + /// it through a local `#[command(flatten)]` wrapper. + #[derive(clap::Parser, Debug)] + struct PrefixedExtract { + #[command(flatten)] + opts: MultiExtractRunallOptions, + } + + /// The minimal required flags to parse a `MultiExtractRunallOptions`. + fn minimal_runall_args() -> Vec<&'static str> { + vec![ + "x", + "--extract::sample", + "s1", + "--extract::library", + "lib1", + "--extract::inputs", + "r1.fq", + "--extract::read-structures", + "+T", + ] + } + + /// Parsing the minimal required flags projects to the expected + /// [`ExtractOptions`] (all defaults), and the source fields round-trip. + #[test] + fn multi_extract_runall_options_defaults_match_projection() { + let opts = PrefixedExtract::try_parse_from(minimal_runall_args()) + .expect("parses") + .opts + .validate() + .expect("valid"); + + let projected = opts.to_extract_options(); + assert_eq!(projected.sample, "s1"); + assert_eq!(projected.library, "lib1"); + assert_eq!(projected.platform, Some("illumina".to_string())); + assert_eq!(projected.read_group_id, "A"); + assert_eq!(projected.quality_encoding, QualityEncoding::Standard); + assert!(projected.platform_unit.is_none()); + assert!(projected.barcode.is_none()); + assert!(projected.platform_model.is_none()); + assert!(projected.sequencing_center.is_none()); + assert!(projected.predicted_insert_size.is_none()); + assert!(projected.description.is_none()); + assert!(projected.run_date.is_none()); + assert!(projected.single_tag.is_none()); + assert!(projected.comments.is_empty()); + assert!(!projected.store_umi_quals); + assert!(!projected.store_cell_quals); + assert!(!projected.store_sample_barcode_qualities); + assert!(!projected.annotate_read_names); + assert!(!projected.extract_umis_from_read_names); + assert!(!projected.async_reader); + assert!(!projected.check_crc); + assert!(!projected.no_check_crc); + + assert_eq!(opts.inputs, vec![PathBuf::from("r1.fq")]); + assert_eq!(opts.read_structures.len(), 1); + } + + /// Each staged-required flag omitted makes `validate()` fail. + #[rstest] + #[case::sample("--extract::sample")] + #[case::library("--extract::library")] + #[case::inputs("--extract::inputs")] + #[case::read_structures("--extract::read-structures")] + fn multi_extract_runall_options_missing_required_is_err(#[case] flag_to_drop: &str) { + // Drop the flag and its following value from the minimal arg list. + let full = minimal_runall_args(); + let mut args: Vec<&str> = Vec::with_capacity(full.len()); + let mut idx = 0; + while idx < full.len() { + if full[idx] == flag_to_drop { + idx += 2; // skip the flag and its value + } else { + args.push(full[idx]); + idx += 1; + } + } + let parsed = PrefixedExtract::try_parse_from(args).expect("parses").opts.validate(); + assert!(parsed.is_err(), "omitting {flag_to_drop} must fail validate()"); + } + + /// `--store-umi-quals` with `--extract-umis-from-read-names` is rejected by + /// the struct's own `validate()` (the dropped `conflicts_with`). + #[test] + fn extract_runall_options_store_umi_quals_conflicts_with_extract_from_names() { + let mut args = minimal_runall_args(); + args.push("--extract::store-umi-quals"); + args.push("--extract::extract-umis-from-read-names"); + let opts = PrefixedExtract::try_parse_from(args) + .expect("parses") + .opts + .validate() + .expect("staged-required lifting passes"); + assert!(opts.validate().is_err()); + } + + /// `--check-crc` with `--no-check-crc` is rejected by `validate()`. + #[test] + fn extract_runall_options_check_crc_conflicts_with_no_check_crc() { + let mut args = minimal_runall_args(); + args.push("--extract::check-crc"); + args.push("--extract::no-check-crc"); + let opts = PrefixedExtract::try_parse_from(args) + .expect("parses") + .opts + .validate() + .expect("staged-required lifting passes"); + assert!(opts.validate().is_err()); + } + + /// `ExtractRunallOptions::default()` must match the CLI defaults + /// (`"illumina"` / `"A"`), not the derived-`Default` empty strings — guards + /// the branch-wide invariant "Default == the minimal-parse projection". + #[test] + fn multi_extract_runall_options_default_matches_cli_defaults() { + let defaults = ExtractRunallOptions::default(); + assert_eq!(defaults.platform, "illumina"); + assert_eq!(defaults.read_group_id, "A"); + } + + /// A supplied `--extract::barcode` round-trips into the projection. + #[test] + fn multi_extract_runall_options_round_trips_barcode() { + let mut args = minimal_runall_args(); + args.push("--extract::barcode"); + args.push("ACGT"); + let opts = + PrefixedExtract::try_parse_from(args).expect("parses").opts.validate().expect("valid"); + assert_eq!(opts.barcode, Some("ACGT".to_string())); + assert_eq!(opts.to_extract_options().barcode, Some("ACGT".to_string())); + } } From 9c9c56aaa35bfe65342010bf3405293ad127abef Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 17:11:49 -0700 Subject: [PATCH 11/15] docs(commands): correct stale option-struct doc comments after clap::Args conversion --- src/lib/commands/extract.rs | 5 +++-- src/lib/commands/group.rs | 6 ++++-- src/lib/pipeline/steps/correct/tests.rs | 9 +++++---- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/lib/commands/extract.rs b/src/lib/commands/extract.rs index 1ac2acb56..4e2c0c42d 100644 --- a/src/lib/commands/extract.rs +++ b/src/lib/commands/extract.rs @@ -1691,7 +1691,8 @@ pub struct ExtractRunallOptions { /// Treat a single input as interleaved paired-end FASTQ (`R1, R2, R1, R2, …`), /// de-interleaving it into the two reads. Requires exactly one `--input` (a /// file or `-` for stdin) and describes both reads with two `--read-structures` - /// (defaults to `+T +T`). This lets a streaming trimmer or converter pipe + /// (required for this runall variant — unlike the standalone `Extract`, there + /// is no `+T +T` default). This lets a streaming trimmer or converter pipe /// interleaved pairs straight into extract without staging two FASTQ files. #[arg(long = "interleaved", default_value_t = false)] pub interleaved: bool, @@ -1883,7 +1884,7 @@ impl ExtractRunallOptions { } /// Re-enforce the cross-field conflicts the `multi_options` macro required us - /// to drop from the `#[arg]` attributes (mirrors [`Extract::validate`]). + /// to drop from the `#[arg]` attributes (mirrors `Extract::validate`). /// /// This is separate from the macro-generated `MultiExtractRunallOptions:: /// validate()`, which only lifts the staged-required flags; `runall`/PR B diff --git a/src/lib/commands/group.rs b/src/lib/commands/group.rs index 4035faafe..a8e69203e 100644 --- a/src/lib/commands/group.rs +++ b/src/lib/commands/group.rs @@ -665,8 +665,10 @@ pub struct GroupReadsByUmi { /// Group-stage tuning, independent of how the values were supplied. /// -/// See [`crate::commands::zipper::ZipperOptions`] for why this is a plain -/// struct rather than a flattened `clap::Args`. +/// Derives `clap::Args` and carries `#[fgumi_cli_macros::multi_options]` so a +/// future `runall` command can re-expose each field as a prefixed +/// `--group::`, via the generated `MultiGroupOptions` companion, without +/// hand-maintaining a parallel option set. /// /// Two fields hold *resolved* values rather than raw flags, because grouping /// cannot be configured from the raw ones alone: `min_map_q` applies the diff --git a/src/lib/pipeline/steps/correct/tests.rs b/src/lib/pipeline/steps/correct/tests.rs index c5b4d3334..84faff738 100644 --- a/src/lib/pipeline/steps/correct/tests.rs +++ b/src/lib/pipeline/steps/correct/tests.rs @@ -17,10 +17,11 @@ use rstest::rstest; /// `CorrectOptions` mirroring the CLI's documented defaults. /// -/// Spelled out rather than derived: a `Default` impl on `CorrectOptions` would -/// hand back `max_mismatches: 0`, `min_distance_diff: 0` and `cache_size: 0`, -/// none of which match the flags' `default_value`s, so it would be a trap for -/// any non-test caller that reached for it. +/// `CorrectOptions` now has an `impl Default` matching those same +/// `default_value`s (`max_mismatches: 2`, `min_distance_diff: 2`, +/// `cache_size: 100_000`); this fixture spells the values out explicitly +/// rather than delegating to it so the test data stays self-contained and +/// doesn't silently drift if the `Default` impl ever changes. fn make_default_opts() -> CorrectOptions { CorrectOptions { metrics: None, From b2911fb0a18e8baa654c45452618e5aa1e924a6d Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 18:03:37 -0700 Subject: [PATCH 12/15] refactor(group): share strategy/edits resolution via one helper GroupOptions::resolve_strategy_and_edits and GroupReadsByUmi::resolve_strategy_and_edits carried identical logic for applying the --no-umi and identity-implies-zero-edits rules. Extract a single private free function and have both methods delegate to it, so the standalone command and the chain builder cannot drift apart by editing one copy and not the other. --- src/lib/commands/group.rs | 47 ++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/src/lib/commands/group.rs b/src/lib/commands/group.rs index a8e69203e..b698d3cce 100644 --- a/src/lib/commands/group.rs +++ b/src/lib/commands/group.rs @@ -762,16 +762,34 @@ impl Default for GroupOptions { } } +/// Resolve the effective grouping strategy/edits: `--no-umi` forces +/// `Strategy::Identity`/0; `Strategy::Identity` forces `edits` 0; otherwise the +/// requested pair. +/// +/// Shared by [`GroupOptions::resolve_strategy_and_edits`] and +/// [`GroupReadsByUmi::resolve_strategy_and_edits`] so the standalone command, +/// the chain builder, and `execute` cannot disagree about what was +/// configured — both methods delegate here rather than each re-implementing +/// the same two rules. +fn resolve_strategy_and_edits(no_umi: bool, strategy: Strategy, edits: u32) -> (Strategy, u32) { + if no_umi { + return (Strategy::Identity, 0); + } + let edits = if matches!(strategy, Strategy::Identity) { 0 } else { edits }; + (strategy, edits) +} + impl GroupOptions { /// Resolve the effective grouping strategy/edits: `--no-umi` forces - /// Identity/0; Identity forces edits 0; otherwise the requested pair. + /// `Strategy::Identity`/0; `Strategy::Identity` forces `edits` 0; otherwise + /// the requested pair. + /// + /// Delegates to the shared free function [`resolve_strategy_and_edits`] so + /// this and [`GroupReadsByUmi::resolve_strategy_and_edits`] cannot + /// disagree. #[must_use] pub fn resolve_strategy_and_edits(&self) -> (Strategy, u32) { - if self.no_umi { - return (Strategy::Identity, 0); - } - let edits = if matches!(self.strategy, Strategy::Identity) { 0 } else { self.edits }; - (self.strategy, edits) + resolve_strategy_and_edits(self.no_umi, self.strategy, self.edits) } } @@ -785,17 +803,16 @@ impl GroupReadsByUmi { /// Resolve the strategy and edit distance grouping will actually use. /// /// `--no-umi` forces identity grouping, and identity grouping requires an - /// edit distance of zero; both rules live here so `execute` and the chain - /// builder cannot disagree about what was configured. The caller is - /// responsible for rejecting `--no-umi` with `--strategy paired` and for - /// logging the override — this method only computes. + /// edit distance of zero; both rules live in the shared free function + /// [`resolve_strategy_and_edits`], which this and + /// [`GroupOptions::resolve_strategy_and_edits`] both delegate to, so + /// `execute` and the chain builder cannot disagree about what was + /// configured. The caller is responsible for rejecting `--no-umi` with + /// `--strategy paired` and for logging the override — this method only + /// computes. #[must_use] pub fn resolve_strategy_and_edits(&self) -> (Strategy, u32) { - if self.no_umi { - return (Strategy::Identity, 0); - } - let edits = if matches!(self.strategy, Strategy::Identity) { 0 } else { self.edits }; - (self.strategy, edits) + resolve_strategy_and_edits(self.no_umi, self.strategy, self.edits) } /// Project the parsed CLI flags into [`GroupOptions`]. From 3c87095ebcab40f2d76e6d18e7e7301e636e664a Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 18:04:35 -0700 Subject: [PATCH 13/15] docs(commands): scope Sort/Codec/ExtractRunall option docs to actual enforcement SortOptions::temp_compression and SortOptions::max_temp_files stated zstd-level-0 rejection and the fd-budget-overrun warning as unconditional properties, but both checks live only in Sort::execute_sort, not on SortOptions/MultiSortOptions itself; reword both field docs to attribute the check to the standalone command. CodecOptions' doc comment claimed duplex's --min-reads has no default_value, contradicting duplex.rs (it does, default_value = "1"); correct the comparison to name simplex, whose --min-reads is the one that's actually required. ExtractRunallOptions::validate doc now states explicitly that it re-enforces only the two macro-dropped conflicts_with checks (store-umi-quals/extract-umis-from-read-names and check-crc/no-check-crc), leaving the remaining Extract::validate checks (template-count, single_tag collision, read-structure non-emptiness) to the future runall command once it builds the FASTQ source. --- crates/fgumi-sort/src/sync_spill_writer.rs | 2 +- src/lib/commands/codec.rs | 9 +- src/lib/commands/extract.rs | 22 ++++- src/lib/commands/sort.rs | 101 +++++++++++++++++---- 4 files changed, 111 insertions(+), 23 deletions(-) diff --git a/crates/fgumi-sort/src/sync_spill_writer.rs b/crates/fgumi-sort/src/sync_spill_writer.rs index 2669dbde2..edc02003f 100644 --- a/crates/fgumi-sort/src/sync_spill_writer.rs +++ b/crates/fgumi-sort/src/sync_spill_writer.rs @@ -138,7 +138,7 @@ impl SyncSpillWriter { /// Create a writer for `path` using `codec` at `compression` level. /// /// For zstd, `compression` is the zstd level (must be ≥ 1 — level 0 is - /// rejected up front by `SortOptions::validate`, since zstd has no + /// rejected up front by `SortOptions::validate_spill_settings`, since zstd has no /// uncompressed mode). For bgzf, `compression == 0` writes *framed* stored /// (uncompressed) BGZF blocks and `> 0` writes deflate-compressed BGZF blocks /// at that level — in both cases valid, reader-consumable BGZF. diff --git a/src/lib/commands/codec.rs b/src/lib/commands/codec.rs index 72568e4d3..47e691b92 100644 --- a/src/lib/commands/codec.rs +++ b/src/lib/commands/codec.rs @@ -339,10 +339,11 @@ pub struct Codec { /// implements `Default` (`FgbioCompat`), matching `TieRuleArg::FgbioCompat`'s /// resolution. /// -/// `min_reads` has a `default_value` on the standalone command (unlike -/// simplex/duplex's `min-reads`), so the macro does NOT lift it to a -/// staged-required field: `MultiCodecOptions::validate()` accepts an omitted -/// `--codec::min-reads`. +/// `min_reads` has a `default_value` on the standalone command (like duplex's +/// `min-reads`, which also carries `default_value = "1"` — unlike simplex's +/// `min-reads`, which has none and is clap-required), so the macro does NOT +/// lift it to a staged-required field: `MultiCodecOptions::validate()` +/// accepts an omitted `--codec::min-reads`. /// /// `allow_unmapped` is `#[arg(skip = AllowUnmappedOptions { enabled: false })]`: /// [`AllowUnmappedOptions`] does not implement `Default`, so the explicit diff --git a/src/lib/commands/extract.rs b/src/lib/commands/extract.rs index 4e2c0c42d..929c40548 100644 --- a/src/lib/commands/extract.rs +++ b/src/lib/commands/extract.rs @@ -1883,8 +1883,26 @@ impl ExtractRunallOptions { } } - /// Re-enforce the cross-field conflicts the `multi_options` macro required us - /// to drop from the `#[arg]` attributes (mirrors `Extract::validate`). + /// Re-enforce ONLY the two cross-field conflicts the `multi_options` macro + /// required us to drop from the `#[arg]` attributes: on the standalone + /// [`Extract`], `--store-umi-quals`/`--extract-umis-from-read-names` and + /// `--check-crc`/`--no-check-crc` are each a clap-level `conflicts_with` + /// pair (not part of `Extract::validate`'s own body), and the macro + /// rejects `conflicts_with` on a `multi_options` struct, so those two + /// checks are re-implemented here by hand. + /// + /// This method does NOT re-implement the rest of [`Extract::validate`] — + /// the template-count-1-to-2 check, the `--single-tag` reserved-tag + /// collision check, the read-structure-non-empty check, or the + /// input/read-structure count and stdin/file-existence checks. Those all + /// depend on the fully-resolved read structures and input list, which are + /// only known once the FASTQ source is built; applying them here would + /// duplicate logic that must live with that construction. The future + /// `runall` command (PR B) is responsible for running the remaining + /// `Extract::validate` checks (template-count 1–2, the `single_tag` + /// reserved-tag collision, and read-structure non-emptiness) when it + /// builds the FASTQ source from [`Self::inputs`] / [`Self::read_structures`] + /// / [`Self::interleaved`]; nothing in this PR calls those checks. /// /// This is separate from the macro-generated `MultiExtractRunallOptions:: /// validate()`, which only lifts the staged-required flags; `runall`/PR B diff --git a/src/lib/commands/sort.rs b/src/lib/commands/sort.rs index 733a1288d..4e36af3e1 100644 --- a/src/lib/commands/sort.rs +++ b/src/lib/commands/sort.rs @@ -564,9 +564,17 @@ pub struct SortOptions { /// Applies to the codec selected by `--temp-codec`: /// * For `bgzf`, level 0 produces uncompressed (stored) BGZF blocks /// (fastest, uses most disk space); 1..=9 are libdeflate levels. - /// * For `zstd`, only 1..=9 are valid; level 0 is rejected because zstd - /// has no equivalent "stored" mode and silently remapping it to 1 - /// would surprise users counting on uncompressed spill. + /// * For `zstd`, only 1..=9 are valid; the standalone `sort` command + /// rejects level 0 with `--temp-codec zstd` at start-up + /// (`Sort::execute_sort`), since zstd has no equivalent "stored" mode + /// and silently remapping it to 1 would surprise users counting on + /// uncompressed spill. Clap only enforces the per-field range (0..=9) + /// here; the cross-field zstd/level-0 check lives in + /// [`SortOptions::validate_spill_settings`] (shared with + /// `Sort::execute_sort`). A chain consumer that builds a sort stage from + /// a `SortOptions`/`MultiSortOptions` must call + /// `validate_spill_settings()` before stage construction to reject the + /// combination up front rather than failing on the first spill. /// /// Level 1 (default) provides fast compression with reasonable space savings. /// Higher levels (up to 9) provide better compression but are slower. @@ -595,14 +603,18 @@ pub struct SortOptions { /// overhead whenever the descriptor budget could have carried the runs. /// Raising this avoids it on very large inputs (at the cost of more open /// file descriptors during the final merge); lowering it keeps fewer files - /// open. Must be at least 2; to effectively disable consolidation, pass a - /// value larger than the number of runs you expect to spill. + /// open. Must be at least 2 (enforced by the shared clap value parser); to + /// effectively disable consolidation, pass a value larger than the number + /// of runs you expect to spill. /// /// "auto" (default) sizes the limit to the process's soft open-file limit /// (`ulimit -n`), less a reserve for the input, output and index handles, - /// and capped at a tested maximum. Explicit values like "64", "256" pin it; - /// a pinned value larger than the open-file budget is reported at startup. - /// Must be at least 2. + /// and capped at a tested maximum. Explicit values like "64", "256" pin it. + /// The standalone `sort` command reports at start-up (`Sort::execute_sort`) + /// when a pinned value exceeds the open-file budget; this field itself + /// carries no such check, so nothing warns on a `SortOptions`/ + /// `MultiSortOptions` value that overruns the budget until (and unless) a + /// future consumer applies it. #[arg(long = "max-temp-files", default_value = "auto", value_parser = parse_max_temp_files)] pub max_temp_files: MaxTempFiles, @@ -638,6 +650,46 @@ impl Default for SortOptions { } } +/// Rejects the one invalid temp-codec/compression pairing: zstd with level 0. +/// +/// zstd has no level-0 "stored" (uncompressed) mode, so silently remapping to 1 +/// would surprise users who pass `--temp-compression 0` to disable temp +/// compression (which works for BGZF). This is the single cross-field check +/// shared by the standalone `sort` command ([`Sort::execute_sort`]) and by +/// [`SortOptions::validate_spill_settings`] so a chain consumer (e.g. a fused `runall`) can +/// reject the combination before constructing the spill stages, rather than +/// failing later during lazy compressor creation on the first spill. +fn reject_zstd_uncompressed( + temp_compression: u32, + temp_codec: fgumi_sort::SpillCodec, +) -> Result<()> { + if temp_compression == 0 && matches!(temp_codec, fgumi_sort::SpillCodec::Zstd) { + bail!( + "--temp-compression 0 is only supported with --temp-codec bgzf; \ + zstd does not have an uncompressed mode. Pass --temp-codec bgzf \ + to keep level-0 spill, or pick a zstd level >= 1." + ); + } + Ok(()) +} + +impl SortOptions { + /// Validates the cross-field spill settings that clap's per-field parsers + /// cannot express. + /// + /// Named distinctly from the macro-generated `MultiSortOptions::validate` + /// (which only checks required-ness while converting a `MultiSortOptions` + /// into a `SortOptions`, and does *not* run this check): a chain consumer + /// holding a `SortOptions` must call this explicitly so the zstd/level-0 + /// combination (zstd has no uncompressed mode) is rejected up front rather + /// than during the first spill. A chain consumer that builds a sort stage + /// from a `SortOptions`/`MultiSortOptions` (e.g. a fused `runall`) should + /// call this before stage construction. + pub fn validate_spill_settings(&self) -> Result<()> { + reject_zstd_uncompressed(self.temp_compression, self.temp_codec) + } +} + impl Sort { /// Projects the parsed CLI flags into [`SortOptions`] for the chain builder. #[must_use] @@ -1000,14 +1052,8 @@ impl Sort { // zstd has no level-0 "stored" mode; silently remapping to 1 would // surprise users who pass --temp-compression 0 to disable temp // compression (which works for BGZF). Reject the combination - // explicitly. - if self.temp_compression == 0 && matches!(self.temp_codec, fgumi_sort::SpillCodec::Zstd) { - bail!( - "--temp-compression 0 is only supported with --temp-codec bgzf; \ - zstd does not have an uncompressed mode. Pass --temp-codec bgzf \ - to keep level-0 spill, or pick a zstd level >= 1." - ); - } + // explicitly, via the same shared check `SortOptions::validate_spill_settings` uses. + reject_zstd_uncompressed(self.temp_compression, self.temp_codec)?; // The "Sorting BAM ..." start line and its completion line are both owned // by the chain's `SortSummaryFinalizeHook` timer (`add_sort` constructs an @@ -2309,6 +2355,29 @@ mod tests { ); } + #[rstest] + #[case::zstd_level_zero_rejected(0, fgumi_sort::SpillCodec::Zstd, false)] + #[case::zstd_level_one_ok(1, fgumi_sort::SpillCodec::Zstd, true)] + #[case::zstd_level_nine_ok(9, fgumi_sort::SpillCodec::Zstd, true)] + #[case::bgzf_level_zero_ok(0, fgumi_sort::SpillCodec::Bgzf, true)] + #[case::bgzf_level_one_ok(1, fgumi_sort::SpillCodec::Bgzf, true)] + fn test_sort_options_validate_temp_codec_compression( + #[case] temp_compression: u32, + #[case] temp_codec: fgumi_sort::SpillCodec, + #[case] expect_ok: bool, + ) { + let opts = SortOptions { temp_compression, temp_codec, ..SortOptions::default() }; + let result = opts.validate_spill_settings(); + assert_eq!(result.is_ok(), expect_ok, "unexpected result: {result:?}"); + if !expect_ok { + let msg = result.unwrap_err().to_string(); + assert!( + msg.contains("--temp-compression 0 is only supported with --temp-codec bgzf"), + "unexpected error: {msg}" + ); + } + } + #[test] fn test_verify_coordinate_fails_on_unsorted() -> Result<()> { use fgumi_sort::RawBamRecordReader; From 95feb6b09ae6b997a13c81d64372d333eb162850 Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 18:04:48 -0700 Subject: [PATCH 14/15] test(commands): guard each stage-option struct's Default against its CLI defaults Each of the 9 pipeline stage option structs (SortOptions, CorrectOptions, FilterOptions, DuplexOptions, SimplexOptions, CodecOptions, ZipperOptions, GroupOptions, ExtractRunallOptions) carries a hand-written impl Default required by the multi_options macro (its generated Multi code references X::default() for bare #[arg(skip)] fields). The existing parity tests only compare Multi::validate() against the standalone command's projection, so a drift between impl Default and a #[arg(default_value...)] literal was never caught. Add one guard test per struct that parses the standalone command with only its required flags and asserts every default-bearing field equals X::default()'s value, so such a drift now fails CI. Required-no-default fields (min_reads for filter/simplex, strategy for group, min_distance for correct) are supplied but not asserted, since they have no CLI default to compare against. Carrier skip fields (io, rejects_opts, stats_opts, read_group, allow_unmapped) are excluded as tautological by construction and PartialEq-less; group's effective_strategy/effective_edits are checked directly against the deliberate PR-A skip default (Identity/0) rather than against the parsed projection. --- src/lib/commands/codec.rs | 29 +++++++++++++++++++++++++++++ src/lib/commands/correct.rs | 19 +++++++++++++++++++ src/lib/commands/duplex.rs | 25 +++++++++++++++++++++++++ src/lib/commands/extract.rs | 26 ++++++++++++++++++++++++-- src/lib/commands/filter.rs | 25 +++++++++++++++++++++++++ src/lib/commands/group.rs | 37 +++++++++++++++++++++++++++++++++++++ src/lib/commands/simplex.rs | 26 ++++++++++++++++++++++++++ src/lib/commands/sort.rs | 26 ++++++++++++++++++++++++++ src/lib/commands/zipper.rs | 27 +++++++++++++++++++++++++++ 9 files changed, 238 insertions(+), 2 deletions(-) diff --git a/src/lib/commands/codec.rs b/src/lib/commands/codec.rs index 47e691b92..cea5ecf88 100644 --- a/src/lib/commands/codec.rs +++ b/src/lib/commands/codec.rs @@ -2321,4 +2321,33 @@ mod tests { .expect("valid"); assert_eq!(multi.outer_bases_length, 7); } + + /// Guards the hand-written `impl Default for CodecOptions` against + /// drifting from the standalone `codec` command's + /// `#[arg(default_value...)]` literals. + #[test] + fn codec_options_default_matches_cli_defaults() { + let parsed = Codec::try_parse_from(["codec", "-i", "in.bam", "-o", "o.bam"]) + .expect("parses") + .to_codec_options(); + let d = CodecOptions::default(); + assert_eq!(d.error_rate_pre_umi, parsed.error_rate_pre_umi); + assert_eq!(d.error_rate_post_umi, parsed.error_rate_post_umi); + assert_eq!(d.min_input_base_quality, parsed.min_input_base_quality); + assert_eq!(d.output_per_base_tags, parsed.output_per_base_tags); + assert_eq!(d.trim, parsed.trim); + assert_eq!(d.min_consensus_base_quality, parsed.min_consensus_base_quality); + assert_eq!(d.min_reads, parsed.min_reads); + assert_eq!(d.min_duplex_length, parsed.min_duplex_length); + assert_eq!(d.outer_bases_length, parsed.outer_bases_length); + assert!( + (d.max_duplex_disagreement_rate - parsed.max_duplex_disagreement_rate).abs() < 1e-12 + ); + // Chain-engine skip field: no `--codec::tie-rule` CLI flag exists to + // parse, so compare directly against the resolved default. + assert_eq!(d.tie_rule, fgumi_consensus::TieRule::default()); + // Skip field: `AllowUnmappedOptions` has no `PartialEq`, so compare its + // `enabled` flag directly against the `#[arg(skip = ...)]` literal. + assert!(!d.allow_unmapped.enabled); + } } diff --git a/src/lib/commands/correct.rs b/src/lib/commands/correct.rs index 5e6f8675b..475933cf2 100644 --- a/src/lib/commands/correct.rs +++ b/src/lib/commands/correct.rs @@ -4544,4 +4544,23 @@ mod tests { "error should name --correct::min-distance: {msg}" ); } + + /// Guards the hand-written `impl Default for CorrectOptions` against + /// drifting from the standalone `correct` command's + /// `#[arg(default_value...)]` literals. `--min-distance` has no CLI + /// default (it is required), so it is supplied but not asserted here. + #[test] + fn correct_options_default_matches_cli_defaults() { + let parsed = CorrectUmis::try_parse_from([ + "correct", "-i", "in.bam", "-o", "o.bam", "-d", "1", "-u", "AAA", + ]) + .expect("parses") + .to_correct_options(); + let d = CorrectOptions::default(); + assert_eq!(d.target, parsed.target); + assert_eq!(d.max_mismatches, parsed.max_mismatches); + assert_eq!(d.dont_store_original_umis, parsed.dont_store_original_umis); + assert_eq!(d.cache_size, parsed.cache_size); + assert_eq!(d.revcomp, parsed.revcomp); + } } diff --git a/src/lib/commands/duplex.rs b/src/lib/commands/duplex.rs index deb508547..ca5935de7 100644 --- a/src/lib/commands/duplex.rs +++ b/src/lib/commands/duplex.rs @@ -2564,4 +2564,29 @@ mod tests { .expect("valid"); assert_eq!(multi.min_reads, vec![2, 3]); } + + /// Guards the hand-written `impl Default for DuplexOptions` against + /// drifting from the standalone `duplex` command's + /// `#[arg(default_value...)]` literals. + #[test] + fn duplex_options_default_matches_cli_defaults() { + let parsed = Duplex::try_parse_from(["duplex", "-i", "in.bam", "-o", "o.bam"]) + .expect("parses") + .to_duplex_options(); + let d = DuplexOptions::default(); + assert_eq!(d.error_rate_pre_umi, parsed.error_rate_pre_umi); + assert_eq!(d.error_rate_post_umi, parsed.error_rate_post_umi); + assert_eq!(d.min_input_base_quality, parsed.min_input_base_quality); + assert_eq!(d.output_per_base_tags, parsed.output_per_base_tags); + assert_eq!(d.trim, parsed.trim); + assert_eq!(d.min_consensus_base_quality, parsed.min_consensus_base_quality); + assert_eq!(d.consensus_call_overlapping_bases, parsed.consensus_call_overlapping_bases); + assert_eq!(d.min_reads, parsed.min_reads); + // Chain-engine skip field: no `--duplex::tie-rule` CLI flag exists to + // parse, so compare directly against the resolved default. + assert_eq!(d.tie_rule, fgumi_consensus::TieRule::default()); + // Skip field: `AllowUnmappedOptions` has no `PartialEq`, so compare its + // `enabled` flag directly against the `#[arg(skip = ...)]` literal. + assert!(!d.allow_unmapped.enabled); + } } diff --git a/src/lib/commands/extract.rs b/src/lib/commands/extract.rs index 929c40548..4cc7264b8 100644 --- a/src/lib/commands/extract.rs +++ b/src/lib/commands/extract.rs @@ -5971,13 +5971,35 @@ mod tests { } /// `ExtractRunallOptions::default()` must match the CLI defaults - /// (`"illumina"` / `"A"`), not the derived-`Default` empty strings — guards - /// the branch-wide invariant "Default == the minimal-parse projection". + /// (`"illumina"` / `"A"`, plus every other default-bearing field), not the + /// derived-`Default` empty strings — guards the branch-wide invariant + /// "Default == the minimal-parse projection". `sample`/`library`/`inputs`/ + /// `read_structures` are required (staged/clap), so they have no CLI + /// default and are not asserted here. #[test] fn multi_extract_runall_options_default_matches_cli_defaults() { let defaults = ExtractRunallOptions::default(); assert_eq!(defaults.platform, "illumina"); assert_eq!(defaults.read_group_id, "A"); + assert!(!defaults.interleaved); + assert_eq!(defaults.barcode, None); + assert_eq!(defaults.platform_unit, None); + assert_eq!(defaults.platform_model, None); + assert_eq!(defaults.sequencing_center, None); + assert_eq!(defaults.predicted_insert_size, None); + assert_eq!(defaults.description, None); + assert!(defaults.comment.is_empty()); + assert_eq!(defaults.run_date, None); + assert!(!defaults.store_umi_quals); + assert!(!defaults.store_cell_quals); + assert!(!defaults.store_sample_barcode_qualities); + assert!(!defaults.extract_umis_from_read_names); + assert!(!defaults.annotate_read_names); + assert_eq!(defaults.single_tag, None); + assert_eq!(defaults.clipping_attribute, None); + assert!(!defaults.async_reader); + assert!(!defaults.check_crc); + assert!(!defaults.no_check_crc); } /// A supplied `--extract::barcode` round-trips into the projection. diff --git a/src/lib/commands/filter.rs b/src/lib/commands/filter.rs index 4b6ea89e8..ba3bef0fc 100644 --- a/src/lib/commands/filter.rs +++ b/src/lib/commands/filter.rs @@ -1605,6 +1605,31 @@ mod tests { ); } + /// Guards the hand-written `impl Default for FilterOptions` against + /// drifting from the standalone `filter` command's + /// `#[arg(default_value...)]` literals. `--min-reads` has no CLI default + /// (it is required), so it is supplied but not asserted here. + #[test] + fn filter_options_default_matches_cli_defaults() { + let parsed = Filter::try_parse_from(["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1"]) + .expect("parses") + .to_filter_options(); + let d = FilterOptions::default(); + assert_eq!(d.max_read_error_rate, parsed.max_read_error_rate); + assert_eq!(d.max_base_error_rate, parsed.max_base_error_rate); + assert_eq!(d.max_no_call_fraction, parsed.max_no_call_fraction); + assert_eq!(d.reverse_per_base_tags, parsed.reverse_per_base_tags); + assert_eq!(d.filter_by_template, parsed.filter_by_template); + assert_eq!(d.require_single_strand_agreement, parsed.require_single_strand_agreement); + assert_eq!( + d.require_strand_methylation_agreement, + parsed.require_strand_methylation_agreement + ); + // Chain-engine skip field: resolved from `Option` + // on the standalone command, not from a CLI default literal. + assert_eq!(d.methylation_mode, fgumi_consensus::MethylationMode::Disabled); + } + use crate::sam::SamTag; use fgumi_raw_bam::{RawRecord, SamBuilder as RawSamBuilder, aux_data_slice, flags}; use noodles::sam::alignment::record_buf::RecordBuf; diff --git a/src/lib/commands/group.rs b/src/lib/commands/group.rs index b698d3cce..53bd3ad17 100644 --- a/src/lib/commands/group.rs +++ b/src/lib/commands/group.rs @@ -1804,6 +1804,43 @@ mod tests { assert!(multi.verify, "--group::verify=true must round-trip to verify=true"); } + /// Guards the hand-written `impl Default for GroupOptions` against + /// drifting from the standalone `group` command's + /// `#[arg(default_value...)]`/`default_value_t` literals. `--strategy` has + /// no CLI default (it is required), so it is supplied but not asserted + /// here. `effective_strategy`/`effective_edits` are the deliberate PR-A + /// skip-default (`Strategy::Identity`/0), asserted directly rather than against + /// `parsed` (which resolves them via `to_group_options`). + #[test] + fn group_options_default_matches_cli_defaults() { + let parsed = GroupReadsByUmi::try_parse_from([ + "group", + "-i", + "in.bam", + "-o", + "o.bam", + "-s", + "adjacency", + ]) + .expect("parses") + .to_group_options(); + let d = GroupOptions::default(); + assert_eq!(d.min_map_q, parsed.min_map_q); + assert_eq!(d.include_non_pf_reads, parsed.include_non_pf_reads); + assert_eq!(d.allow_unmapped, parsed.allow_unmapped); + assert_eq!(d.edits, parsed.edits); + assert_eq!(d.min_umi_length, parsed.min_umi_length); + assert_eq!(d.index_threshold, parsed.index_threshold); + assert_eq!(d.no_umi, parsed.no_umi); + assert_eq!(d.parallel_group_min_templates, parsed.parallel_group_min_templates); + assert_eq!(d.family_size_histogram, parsed.family_size_histogram); + assert_eq!(d.grouping_metrics, parsed.grouping_metrics); + assert_eq!(d.metrics_prefix, parsed.metrics_prefix); + assert_eq!(d.verify, parsed.verify); + assert_eq!(d.effective_strategy, Strategy::Identity); + assert_eq!(d.effective_edits, 0); + } + use crate::assigner::{IdentityUmiAssigner, PairedUmiAssigner, Strategy}; use crate::metrics::TemplateFilterReason; use bstr::BString; diff --git a/src/lib/commands/simplex.rs b/src/lib/commands/simplex.rs index fdf6537da..ac896f877 100644 --- a/src/lib/commands/simplex.rs +++ b/src/lib/commands/simplex.rs @@ -2431,4 +2431,30 @@ mod tests { "error should name --simplex::min-reads: {msg}" ); } + + /// Guards the hand-written `impl Default for SimplexOptions` against + /// drifting from the standalone `simplex` command's + /// `#[arg(default_value...)]` literals. `--min-reads` has no CLI default + /// (it is required), so it is supplied but not asserted here. + #[test] + fn simplex_options_default_matches_cli_defaults() { + let parsed = + Simplex::try_parse_from(["simplex", "-i", "in.bam", "-o", "o.bam", "--min-reads", "3"]) + .expect("parses") + .to_simplex_options(); + let d = SimplexOptions::default(); + assert_eq!(d.error_rate_pre_umi, parsed.error_rate_pre_umi); + assert_eq!(d.error_rate_post_umi, parsed.error_rate_post_umi); + assert_eq!(d.min_input_base_quality, parsed.min_input_base_quality); + assert_eq!(d.output_per_base_tags, parsed.output_per_base_tags); + assert_eq!(d.trim, parsed.trim); + assert_eq!(d.min_consensus_base_quality, parsed.min_consensus_base_quality); + assert_eq!(d.consensus_call_overlapping_bases, parsed.consensus_call_overlapping_bases); + // Chain-engine skip field: no `--simplex::tie-rule` CLI flag exists to + // parse, so compare directly against the resolved default. + assert_eq!(d.tie_rule, fgumi_consensus::TieRule::default()); + // Skip field: `AllowUnmappedOptions` has no `PartialEq`, so compare its + // `enabled` flag directly against the `#[arg(skip = ...)]` literal. + assert!(!d.allow_unmapped.enabled); + } } diff --git a/src/lib/commands/sort.rs b/src/lib/commands/sort.rs index 4e36af3e1..f348daac4 100644 --- a/src/lib/commands/sort.rs +++ b/src/lib/commands/sort.rs @@ -2564,4 +2564,30 @@ mod tests { assert_eq!(multi.max_memory, parse_memory("1G").expect("valid")); assert!(multi.sort_stats, "--sort::sort-stats must round-trip to sort_stats=true"); } + + /// Guards the hand-written `impl Default for SortOptions` against drifting + /// from the standalone `sort` command's `#[arg(default_value...)]` literals. + /// Asserts every default-bearing field (plus the chain-engine skip fields, + /// whose defaults are checked directly against the struct) matches the + /// standalone command's own default, parsed with only its required flags. + #[test] + fn sort_options_default_matches_cli_defaults() { + let parsed = Sort::try_parse_from(["sort", "-i", "in.bam", "-o", "o.bam"]) + .expect("parses") + .to_sort_options(); + let d = SortOptions::default(); + assert_eq!(d.order, parsed.order); + assert_eq!(d.max_memory, parsed.max_memory); + assert_eq!(d.memory_reserve, parsed.memory_reserve); + assert_eq!(d.memory_per_thread, parsed.memory_per_thread); + assert_eq!(d.temp_compression, parsed.temp_compression); + assert_eq!(d.temp_codec, parsed.temp_codec); + assert_eq!(d.max_temp_files, parsed.max_temp_files); + assert_eq!(d.sort_stats, parsed.sort_stats); + assert!(!d.sort_stats); + // Chain-engine skip fields: no CLI flag on `Sort` exists to parse, so + // compare directly against the struct's documented defaults. + assert_eq!(d.block_batch, 4); + assert!(!d.file_granularity); + } } diff --git a/src/lib/commands/zipper.rs b/src/lib/commands/zipper.rs index 9d5560e6c..fba548161 100644 --- a/src/lib/commands/zipper.rs +++ b/src/lib/commands/zipper.rs @@ -5058,4 +5058,31 @@ mod tests { .expect("valid"); assert_eq!(multi.buffer, 99); } + + /// Guards the hand-written `impl Default for ZipperOptions` against + /// drifting from the standalone `zipper` command's + /// `#[arg(default_value...)]` literals. + #[test] + fn zipper_options_default_matches_cli_defaults() { + // NB: zipper's reference flag is --reference (short -r), NOT --ref. + let parsed = Zipper::try_parse_from([ + "zipper", + "-i", + "m.bam", + "--unmapped", + "u.bam", + "--reference", + "r.fa", + "-o", + "o.bam", + ]) + .expect("parses") + .to_zipper_options(); + let d = ZipperOptions::default(); + assert_eq!(d.buffer, parsed.buffer); + assert_eq!(d.bwa_chunk_size, parsed.bwa_chunk_size); + assert_eq!(d.exclude_missing_reads, parsed.exclude_missing_reads); + assert_eq!(d.skip_tc_tags, parsed.skip_tc_tags); + assert_eq!(d.restore_unconverted_bases, parsed.restore_unconverted_bases); + } } From 75a0921dc669b5a9ea26b2601a4bede49a3c576c Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Thu, 3 Sep 2026 20:02:46 -0700 Subject: [PATCH 15/15] fix(docs): resolve broken private-intra-doc links in extract/group --- src/lib/commands/extract.rs | 2 +- src/lib/commands/group.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib/commands/extract.rs b/src/lib/commands/extract.rs index 4cc7264b8..dde746578 100644 --- a/src/lib/commands/extract.rs +++ b/src/lib/commands/extract.rs @@ -1891,7 +1891,7 @@ impl ExtractRunallOptions { /// rejects `conflicts_with` on a `multi_options` struct, so those two /// checks are re-implemented here by hand. /// - /// This method does NOT re-implement the rest of [`Extract::validate`] — + /// This method does NOT re-implement the rest of `Extract::validate` — /// the template-count-1-to-2 check, the `--single-tag` reserved-tag /// collision check, the read-structure-non-empty check, or the /// input/read-structure count and stdin/file-existence checks. Those all diff --git a/src/lib/commands/group.rs b/src/lib/commands/group.rs index 53bd3ad17..966381ae3 100644 --- a/src/lib/commands/group.rs +++ b/src/lib/commands/group.rs @@ -784,7 +784,7 @@ impl GroupOptions { /// `Strategy::Identity`/0; `Strategy::Identity` forces `edits` 0; otherwise /// the requested pair. /// - /// Delegates to the shared free function [`resolve_strategy_and_edits`] so + /// Delegates to the shared free function `resolve_strategy_and_edits` so /// this and [`GroupReadsByUmi::resolve_strategy_and_edits`] cannot /// disagree. #[must_use] @@ -804,7 +804,7 @@ impl GroupReadsByUmi { /// /// `--no-umi` forces identity grouping, and identity grouping requires an /// edit distance of zero; both rules live in the shared free function - /// [`resolve_strategy_and_edits`], which this and + /// `resolve_strategy_and_edits`, which this and /// [`GroupOptions::resolve_strategy_and_edits`] both delegate to, so /// `execute` and the chain builder cannot disagree about what was /// configured. The caller is responsible for rejecting `--no-umi` with