fix(review): keep dotted output prefixes intact

src/lib/commands/codec.rs

@@ -250,6 +250,11 @@ pub struct Codec {
     /// the inline path does not emit `<prefix>.duplex_umi_counts.txt` (the
     /// tally `duplex-metrics` gates behind `--duplex-umi-counts`); run
     /// `duplex-metrics` separately if you need that file.
+    ///
+    /// Each suffix is appended to the whole prefix, dots included (`out.v1` →
+    /// `out.v1.<suffix>`). A trailing path separator is dropped, so `out/` writes
+    /// `out.<suffix>` beside `out`, not inside it. The prefix must name a file, not
+    /// `.`, `..` or `/`.
     #[arg(long = "metrics")]
     pub metrics: Option<std::path::PathBuf>,
 
@@ -548,7 +553,7 @@ impl Command for Codec {
         // guard also registers) so the two never drift. Kept in a local so
         // `outputs` can borrow it.
         let metrics_artifacts: Vec<std::path::PathBuf> = match &self.metrics {
-            Some(prefix) => crate::inline_metrics_collector::duplex_metrics_paths(prefix),
+            Some(prefix) => crate::inline_metrics_collector::duplex_metrics_paths(prefix)?,
             None => Vec::new(),
         };
         let mut outputs: Vec<(&Path, &str)> = vec![(self.io.output.as_path(), "--output")];

src/lib/commands/common.rs

@@ -803,6 +803,35 @@ pub(crate) fn paths_refer_to_same_file(a: &Path, b: &Path) -> bool {
     }
 }
 
+/// Build an output path by appending `.{suffix}` to the full prefix path.
+///
+/// Unlike [`Path::with_extension`], the prefix is kept intact even when its last component
+/// contains dots (`out.v1` → `out.v1.family_sizes.txt`). Trailing path separators (and a
+/// trailing `.` component) are dropped first, so `out/` → `out.family_sizes.txt` rather than
+/// a hidden `out/.family_sizes.txt`; this matches fgbio, whose `Path` normalization drops
+/// trailing separators. The rest of the prefix is kept verbatim.
+///
+/// # Errors
+///
+/// Returns an error if, after that trimming, the prefix does not end in a file name (`.`,
+/// `./`, `..`, `out/..`, `/` or an empty prefix): appending to it would write dot-named
+/// files such as `./.txt` or `/.txt` rather than files the prefix names.
+pub(crate) fn append_suffix(prefix: &Path, suffix: &str) -> anyhow::Result<PathBuf> {
+    // `Components::as_path` trims trailing separators and `.` components only.
+    let trimmed = prefix.components();
+    if !matches!(trimmed.clone().next_back(), Some(std::path::Component::Normal(_))) {
+        anyhow::bail!(
+            "output prefix '{}' does not name a file: it must end in a file name (e.g. `out` \
+             or `dir/out`), not `.`, `..` or a root",
+            prefix.display()
+        );
+    }
+    let mut s = trimmed.as_path().as_os_str().to_owned();
+    s.push(".");
+    s.push(suffix);
+    Ok(PathBuf::from(s))
+}
+
 /// Reject any write target that is the same file as one of the command's
 /// inputs, which would truncate the file being read.
 ///
@@ -3552,6 +3581,53 @@ mod tests {
 
     use rstest::rstest;
 
+    /// `append_suffix` appends to the full prefix and drops only trailing separators and `.`
+    /// components; the comparison is on the raw string so separator normalization cannot hide
+    /// a difference.
+    #[rstest]
+    #[case::plain("out", "txt", "out.txt")]
+    #[case::dotted_prefix("out.v1", "family_sizes.txt", "out.v1.family_sizes.txt")]
+    #[case::trailing_separator("out/", "family_sizes.txt", "out.family_sizes.txt")]
+    #[case::double_trailing_separator("out//", "txt", "out.txt")]
+    #[case::absolute_trailing_separator("/a/b/out.v1/", "txt", "/a/b/out.v1.txt")]
+    #[case::trailing_cur_dir("out/.", "txt", "out.txt")]
+    #[case::leading_cur_dir_kept("./out/", "txt", "./out.txt")]
+    #[case::interior_double_separator_kept("a//out", "txt", "a//out.txt")]
+    #[case::interior_cur_dir_kept("a/./out", "txt", "a/./out.txt")]
+    #[case::interior_parent_dir_kept("a/../out", "txt", "a/../out.txt")]
+    #[case::trailing_dot_in_name_kept("out.", "txt", "out..txt")]
+    fn append_suffix_appends_to_the_full_prefix(
+        #[case] prefix: &str,
+        #[case] suffix: &str,
+        #[case] expected: &str,
+    ) {
+        assert_eq!(append_suffix(Path::new(prefix), suffix).unwrap().as_os_str(), expected);
+    }
+
+    /// A prefix whose last component (after trailing separators and `.` are dropped) is not a
+    /// file name is rejected, rather than writing dot-named files such as `/.txt` or `out/...txt`.
+    #[rstest]
+    #[case::cur_dir(".")]
+    #[case::cur_dir_trailing_separator("./")]
+    #[case::cur_dir_twice("././")]
+    #[case::parent_dir("..")]
+    #[case::parent_dir_trailing_separator("../")]
+    #[case::trailing_parent_dir("out/..")]
+    #[case::trailing_parent_dir_then_cur_dir("out/../.")]
+    #[case::root("/")]
+    #[case::root_cur_dir("/.")]
+    #[case::empty("")]
+    fn append_suffix_rejects_a_prefix_that_names_no_file(#[case] prefix: &str) {
+        let err = append_suffix(Path::new(prefix), "txt").unwrap_err();
+        assert_eq!(
+            err.to_string(),
+            format!(
+                "output prefix '{prefix}' does not name a file: it must end in a file name \
+                 (e.g. `out` or `dir/out`), not `.`, `..` or a root"
+            )
+        );
+    }
+
     /// The `ConsensusCallingIterator` pre-group filter (fgbio parity,
     /// `ConsensusCallingIterator.scala:56-58`): the default keeps only mapped primary
     /// records. Secondary and supplementary alignments are always dropped, and an

src/lib/commands/duplex.rs

@@ -152,6 +152,11 @@ pub struct Duplex {
     /// `duplex-metrics` gates behind its own `--duplex-umi-counts` flag); it
     /// has no inline equivalent, so run `duplex-metrics` separately if you
     /// need that file.
+    ///
+    /// Each suffix is appended to the whole prefix, dots included (`out.v1` →
+    /// `out.v1.<suffix>`). A trailing path separator is dropped, so `out/` writes
+    /// `out.<suffix>` beside `out`, not inside it. The prefix must name a file, not
+    /// `.`, `..` or `/`.
     #[arg(long = "metrics")]
     pub metrics: Option<std::path::PathBuf>,
 
@@ -495,7 +500,7 @@ impl Command for Duplex {
         // guard also registers) so the two never drift. Kept in a local so
         // `outputs` can borrow it.
         let metrics_artifacts: Vec<std::path::PathBuf> = match &self.metrics {
-            Some(prefix) => crate::inline_metrics_collector::duplex_metrics_paths(prefix),
+            Some(prefix) => crate::inline_metrics_collector::duplex_metrics_paths(prefix)?,
             None => Vec::new(),
         };
         let mut outputs: Vec<(&Path, &str)> = vec![(self.io.output.as_path(), "--output")];

src/lib/commands/duplex_metrics.rs

@@ -9,13 +9,15 @@
 use crate::commands::common::{
     CompressionOptions, QueueMemoryOptions, SchedulerOptions, ThreadingOptions,
 };
+use crate::inline_metrics_collector::{DuplexMetricsPaths, write_duplex_metrics};
 use crate::logging::OperationTimer;
 use crate::metrics::duplex::DuplexMetricsCollector;
 use crate::simple_umi_consensus::SimpleUmiConsensusCaller;
 use crate::validation::validate_input_exists;
 use anyhow::Result;
 use clap::Parser;
 use log::info;
+use std::ffi::OsStr;
 use std::path::PathBuf;
 
 use super::command::Command;
@@ -113,6 +115,11 @@ pub struct DuplexMetrics {
     pub input: PathBuf,
 
     /// Output prefix for metrics files
+    ///
+    /// Each suffix is appended to the whole prefix, dots included (`out.v1` →
+    /// `out.v1.<suffix>`). A trailing path separator is dropped, so `out/` writes
+    /// `out.<suffix>` beside `out`, not inside it. The prefix must name a file, not
+    /// `.`, `..` or `/`.
     #[arg(short = 'o', long = "output")]
     pub output: PathBuf,
 
@@ -227,6 +234,11 @@ impl Command for DuplexMetrics {
             );
         }
 
+        // Derive every output name up front, so a prefix that names no file fails here rather
+        // than after the whole BAM has been read (on either path).
+        let paths = DuplexMetricsPaths::new(&self.output)?;
+        let pdf_path = crate::commands::common::append_suffix(&self.output, "duplex_qc.pdf")?;
+
         // With `--threads`, run the parallel typed-step chain (parallel decode +
         // MI-grouping + per-thread metric accumulation). It writes the SAME TSV
         // files byte-identically via the chain's finalize hook. The R/PDF step
@@ -321,61 +333,34 @@ impl Command for DuplexMetrics {
         let main_collector =
             collectors.pop().expect("collectors is non-empty (always includes 100% fraction)");
 
-        // Generate and write metrics
+        // Write the metrics with the same writer and file names the `--threads` chain path's
+        // finalize hook uses, so both paths write identical files.
         info!("Writing metrics...");
-
-        // Family size metrics. Use `write_metrics_auto` (not a bare `DelimFile::write_tsv`)
-        // so an empty distribution still produces a header-only, fgbio-readable file.
-        let family_size_metrics = main_collector.family_size_metrics();
-        let family_size_path = format!("{}.family_sizes.txt", self.output.display());
-        crate::metrics::writer::write_metrics_auto(&family_size_path, &family_size_metrics)?;
-        info!("Wrote family size metrics to {family_size_path}");
-
-        // Duplex family size metrics
-        let duplex_family_size_metrics = main_collector.duplex_family_size_metrics();
-        let duplex_family_size_path = format!("{}.duplex_family_sizes.txt", self.output.display());
-        crate::metrics::writer::write_metrics_auto(
-            &duplex_family_size_path,
-            &duplex_family_size_metrics,
-        )?;
-        info!("Wrote duplex family size metrics to {duplex_family_size_path}");
-
-        // UMI metrics
-        let umi_metrics = main_collector.umi_metrics();
-        let umi_path = format!("{}.umi_counts.txt", self.output.display());
-        crate::metrics::writer::write_metrics_auto(&umi_path, &umi_metrics)?;
-        info!("Wrote UMI metrics to {umi_path}");
-
-        // Duplex UMI metrics (if enabled)
+        write_duplex_metrics(&main_collector, &yield_metrics, self.duplex_umi_counts, &paths)?;
+        info!("Wrote family size metrics to {}", paths.family_sizes.display());
+        info!("Wrote duplex family size metrics to {}", paths.duplex_family_sizes.display());
+        info!("Wrote UMI metrics to {}", paths.umi_counts.display());
         if self.duplex_umi_counts {
-            let duplex_umi_metrics = main_collector.duplex_umi_metrics(&umi_metrics);
-            let duplex_umi_path = format!("{}.duplex_umi_counts.txt", self.output.display());
-            crate::metrics::writer::write_metrics_auto(&duplex_umi_path, &duplex_umi_metrics)?;
-            info!("Wrote duplex UMI metrics to {duplex_umi_path}");
+            info!("Wrote duplex UMI metrics to {}", paths.duplex_umi_counts.display());
         }
-
-        // Yield metrics
-        let yield_path = format!("{}.duplex_yield_metrics.txt", self.output.display());
-        crate::metrics::writer::write_metrics_auto(&yield_path, &yield_metrics)?;
-        info!("Wrote yield metrics to {yield_path}");
+        info!("Wrote yield metrics to {}", paths.yield_metrics.display());
 
         // Generate PDF plots using R script (optional)
-        let pdf_path = format!("{}.duplex_qc.pdf", self.output.display());
         if is_r_available() {
             let description = self.description.as_deref().unwrap_or("Sample");
             match execute_r_script(
                 R_SCRIPT,
                 &[
-                    &family_size_path,
-                    &duplex_family_size_path,
-                    &yield_path,
-                    &umi_path,
-                    &pdf_path,
-                    description,
+                    paths.family_sizes.as_os_str(),
+                    paths.duplex_family_sizes.as_os_str(),
+                    paths.yield_metrics.as_os_str(),
+                    paths.umi_counts.as_os_str(),
+                    pdf_path.as_os_str(),
+                    OsStr::new(description),
                 ],
                 "fgumi_CollectDuplexSeqMetrics.R",
             ) {
-                Ok(()) => info!("Generated PDF plots: {pdf_path}"),
+                Ok(()) => info!("Generated PDF plots: {}", pdf_path.display()),
                 Err(e) => {
                     log::warn!("Failed to generate PDF plots: {e}. Continuing without plots.");
                     log::warn!(
@@ -622,6 +607,56 @@ mod tests {
         Ok(())
     }
 
+    /// A prefix with a trailing path separator (`out/`) names the metrics files `out.*` beside
+    /// it, not hidden files inside an `out` directory, on both the serial and the `--threads`
+    /// chain path (the two must name their outputs identically).
+    #[rstest]
+    #[case::serial(None)]
+    #[case::chain(Some(2))]
+    fn trailing_separator_prefix_names_files_beside_it(
+        #[case] threads: Option<usize>,
+    ) -> Result<()> {
+        let (r1, r2) = build_test_pair("q1", 0, 100, 200, "AAA-GGG", "1/A", true, false);
+        let input = create_test_bam(vec![r1, r2])?;
+        let dir = TempDir::new()?;
+        let mut prefix = dir.path().join("out").into_os_string();
+        prefix.push("/");
+
+        let cmd = DuplexMetrics {
+            input: input.path().to_path_buf(),
+            output: std::path::PathBuf::from(prefix),
+            min_ab_reads: 1,
+            min_ba_reads: 1,
+            duplex_umi_counts: true,
+            intervals: None,
+            description: None,
+            threading: crate::commands::common::ThreadingOptions { threads },
+            scheduler_opts: crate::commands::common::SchedulerOptions::default(),
+            queue_memory: crate::commands::common::QueueMemoryOptions::default(),
+        };
+        cmd.execute("test")?;
+
+        // The serial path may also write `out.duplex_qc.pdf` when R is installed.
+        let mut produced: Vec<String> = std::fs::read_dir(dir.path())?
+            .map(|e| e.map(|e| e.file_name().to_string_lossy().into_owned()))
+            .collect::<std::io::Result<Vec<_>>>()?
+            .into_iter()
+            .filter(|name| std::path::Path::new(name).extension().is_none_or(|ext| ext != "pdf"))
+            .collect();
+        produced.sort();
+        assert_eq!(
+            produced,
+            vec![
+                "out.duplex_family_sizes.txt".to_string(),
+                "out.duplex_umi_counts.txt".to_string(),
+                "out.duplex_yield_metrics.txt".to_string(),
+                "out.family_sizes.txt".to_string(),
+                "out.umi_counts.txt".to_string(),
+            ]
+        );
+        Ok(())
+    }
+
     /// DXM3-06: `--min-ab-reads 0` is rejected (fgbio validates `minAbReads >= 1`);
     /// otherwise every tag family would be labeled a duplex.
     #[test]