chore: pre-release polish — stdout support, CLI UX, tests, docs, dead code removal

[nh13]

Summary

Stacked on #109. Pre-release polish addressing items from the pre-release review checklist.

• Uniform stdout support — Generalize BAM writers from File to Box<dyn Write + Send>, add is_stdout_path(), replace pipeline EOF reopen pattern with EofWriter drop wrapper, use shared helpers in zipper
• Fix lowercase tags — Use cd/ce for per-base depth and error arrays (were incorrectly using uppercase cD/cE)
• Add --verbose flag — Global -v/--verbose flag for debug-level logging
• Integration tests — Add integration tests for clip, correct, dedup, duplex, filter, and simplex commands
• Documentation — Complete metrics.md with filter/simplex/duplex/codec/dedup output formats; document feature flags, verbose logging, and advanced pipeline options in DEVELOPING.md and performance-tuning.md
• Dead code removal (~430 lines) — Delete unused second impl GroupReadsByUmi block, PositionGroupResult, CollectedMetrics::merge, create_corrector_with_paths, PARALLEL_STEPS, unused TemplateInfo fields, stale #[allow(dead_code)] annotations, and scoped_threadpool dependency
• Minor code quality — unimplemented!() → unreachable!() in sort key, vendored module doc wording

Test plan

• All 1778 tests pass (cargo ci-test)
• cargo ci-fmt clean
• cargo ci-lint clean

[codecov]

Codecov Report

❌ Patch coverage is 94.38202% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.27%. Comparing base (b10e946) to head (876c2c4).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lib/unified_pipeline/bam.rs	90.90%	6 Missing ⚠️
src/commands/group.rs	96.55%	2 Missing ⚠️
src/lib/bam_io.rs	97.95%	1 Missing ⚠️
src/lib/sort/inline_buffer.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
+ Coverage   82.38%   83.27%   +0.88%     
==========================================
  Files         127      127              
  Lines       51277    51062     -215     
==========================================
+ Hits        42245    42522     +277     
+ Misses       9032     8540     -492     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR removes the scoped_threadpool dependency; renames consensus per-base tags from uppercase to lowercase (cD/cE → cd/ce) and updates related guards/messages; converts several GroupReadsByUmi methods into free-standing helpers and adds build_grouping_metrics; refactors BAM writers to use Box<dyn Write + Send> with stdout helpers and adds an EofWriter to append BGZF EOF on successful completion; adds run_bam_pipeline_with_grouper and a verbose CLI flag; removes some unused constants/attributes; expands documentation (developer, metrics, performance) and adds seven new integration test modules (clip, correct, dedup, duplex, filter, simplex, streaming).

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title accurately summarizes the primary changes: stdout support, CLI improvements, tests, docs, and dead code removal.
Description check	✅ Passed	Description clearly relates to the changeset, detailing stdout support, tag fixes, verbose flag, tests, docs, and dead code removal with specific examples.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch nhomer/pre-release-polish

---

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

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[nh13]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[nh13]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

src/commands/group.rs (2)

1091-1111: ⚠️ Potential issue | 🟡 Minor

Error from assign_umi_groups_impl is silently swallowed in pipeline mode.

Pipeline path (here) discards the error and returns an empty group with no log. Single-threaded path (line 1625) logs warn!. This inconsistency means UMI assignment failures in multi-threaded mode vanish without a trace.

At minimum, add a log::warn! before returning the empty result, matching the single-threaded path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/group.rs` around lines 1091 - 1111, The pipeline path currently
swallows the error from assign_umi_groups_impl (caught as _e) and returns an
empty ProcessedPositionGroup; change this to capture the error (e) and emit a
log::warn! with the error and contextual info (e.g., raw_tag, assigner or
assign_tag_bytes) before returning, mirroring the single-threaded path's
warning; update the catch in the assign_umi_groups_impl call block so it uses
the captured error variable in the warn! message and then returns the empty
ProcessedPositionGroup as before.

---

3525-3527: ⚠️ Potential issue | 🟡 Minor

Tautological assertion — always passes.

result.is_ok() || result.is_err() is true for every Result. This test asserts nothing useful. Either assert a specific outcome or remove it.

-    // The command should either succeed with filtered reads or fail gracefully
-    assert!(result.is_ok() || result.is_err());
+    // A UMI of just "-" is invalid for paired strategy; expect an error or empty output
+    if let Ok(()) = result {
+        let output_records = read_bam_records(&paths.output)?;
+        assert_eq!(output_records.len(), 0, "Should filter out invalid paired UMIs");
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/group.rs` around lines 3525 - 3527, The test contains a
tautological assertion using result.is_ok() || result.is_err() which always
passes; replace it with a meaningful assertion for the intended behavior of
cmd.execute("test") (for example assert that it succeeds with
assert!(result.is_ok()) or assert that it fails with assert!(result.is_err())),
or remove the assertion entirely if no deterministic outcome is expected; locate
the call to cmd.execute in this test and update the assertion accordingly
(reference symbols: cmd.execute and result).

src/commands/zipper.rs (1)

870-878: ⚠️ Potential issue | 🟠 Major

Single-threaded path needs explicit BGZF finalization and missing safety attribute.

The writer is created but never explicitly finalized—it just drops out of scope after process_singlethreaded. This silently swallows any finalization errors (broken pipe to stdout, disk full, etc.). The multi-threaded path (line 732) properly calls writer.into_inner().finish()?;, so the single-threaded path should do the same for consistency.

Additionally, this file is missing #![deny(unsafe_code)] at the top, which is required for all src/**/*.rs files per guidelines.

Suggested changes

Add after the process_singlethreaded call:

            self.process_singlethreaded(
                unmapped_iter,
                mapped_iter,
                &output_header,
                &mut writer,
                &tag_info,
-            )?
+            )?;
+
+            writer.into_inner().finish()?;

And add at the file header:

 //! `zipper`: Merge unmapped and mapped BAM files with metadata transfer
+#![deny(unsafe_code)]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/zipper.rs` around lines 870 - 878, The single-threaded branch
creates a noodles::bam::io::Writer (variable writer) but never finalizes it, so
any IO/finalization errors are lost; after calling process_singlethreaded(...)
ensure you call writer.into_inner().finish()? and propagate the error (same
pattern as the multi-threaded path uses writer.into_inner().finish()?); also add
the crate-level directive #![deny(unsafe_code)] at the top of this file to
satisfy the repository guideline.

🧹 Nitpick comments (5)

crates/fgumi-sam/src/builder.rs (2)

2206-2213: Test doesn't verify the new lowercase tag names.

test_consensus_tags_per_base only checks tags.len() == 2. Consider asserting the actual tag bytes are [b'c', b'd'] and [b'c', b'e'] so a regression to uppercase would be caught.

Suggested addition

     let tags = ConsensusTagsBuilder::new()
         .per_base_depths(&[10, 20, 30])
         .per_base_errors(&[1, 2, 3])
         .build();
 
     assert_eq!(tags.len(), 2);
+    assert_eq!(tags[0].0, Tag::from([b'c', b'd']));
+    assert_eq!(tags[1].0, Tag::from([b'c', b'e']));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/fgumi-sam/src/builder.rs` around lines 2206 - 2213, The test
test_consensus_tags_per_base currently only checks tags.len(); update it to also
assert the actual lowercase tag names to prevent regressions: after building
with
ConsensusTagsBuilder::new().per_base_depths(&[10,20,30]).per_base_errors(&[1,2,3]).build(),
add assertions that the two produced tags have byte names equal to [b'c', b'd']
and [b'c', b'e'] (i.e., verify the tag name/bytes fields on the produced tag
objects rather than only checking length).

---

1707-1721: Doc omits the new cd/ce per-base tag names.

Line 1713 says "Per-base arrays for detailed metrics" without naming the tags. Consider:

-/// - Per-base arrays for detailed metrics
+/// - `cd`: Per-base depth array
+/// - `ce`: Per-base error array

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/fgumi-sam/src/builder.rs` around lines 1707 - 1721, The doc comment
for the consensus SAM tag builder omits the new per-base tag names; update the
comment in the Builder for consensus-specific SAM tags (the doc block around the
consensus tag list in builder.rs) to explicitly list the per-base tags `cd`
(per-base depth/coverage array) and `ce` (per-base error rate array) alongside
the existing tags (`cD`, `cM`, `cE`, etc.), and briefly annotate their meanings
(e.g., `cd`: per-base coverage depth array, `ce`: per-base error rate array) so
readers can find `cd`/`ce` when scanning the builder docs.

src/commands/group.rs (2)

1502-1546: Metrics calculation logic is duplicated between pipeline and single-threaded paths.

Lines 1323–1367 and 1502–1546 are nearly identical (median, min, max family size computation + summary fields). Consider extracting a shared helper, e.g. fn build_grouping_metrics(filter_metrics: &FilterMetrics, family_sizes: &AHashMap<usize, u64>) -> UmiGroupingMetrics.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/group.rs` around lines 1502 - 1546, Duplicate metrics-building
logic (median/min/max family size and summary fields) appears in two places;
extract it into a shared helper like fn build_grouping_metrics(filter_metrics:
&FilterMetrics, family_sizes: &AHashMap<usize,u64>) -> UmiGroupingMetrics and
replace both blocks with calls to that helper; the helper should initialize
UmiGroupingMetrics, copy totals from total_filter_metrics (or FilterMetrics),
compute total_families/unique_molecule_ids from family_size_counter,
avg_reads_per_molecule, median_reads_per_molecule by sorting family_size_counter
(as Vec<(usize,u64)>) and scanning cumulative counts, and set
min_reads_per_molecule/max_reads_per_molecule—update callers to pass
total_filter_metrics and family_size_counter and remove the duplicated code in
group.rs and the pipeline path.

---

1-2: Stale scaffolding comments.

These look like leftover build-plan notes. They should be removed.

-// This file will contain the complete group.rs with all integration tests implemented
-// We'll build it section by section
 //! Groups reads by UMI to identify reads from the same original molecule.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/group.rs` around lines 1 - 2, Remove the leftover scaffolding
comments at the top of src/commands/group.rs (the two lines starting with "//
This file will contain..." and "// We'll build it section by section"); simply
delete those comment lines so the file no longer contains stale build-plan notes
and keep only relevant code and documentation in group.rs (no other changes
needed).

docs/metrics.md (1)

44-49: Add language specifiers to fenced code blocks.

Markdownlint flags these example blocks (lines 44, 115, 179, 218, 237) for missing language specifiers. Use ```text or ```tsv to silence the warnings.

Also applies to: 115-126, 179-182, 218-221, 237-244

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/metrics.md` around lines 44 - 49, The fenced code blocks showing metric
examples (e.g., the block with "total_reads 10000 / passed_reads 8542 /
failed_reads 1458 / pass_rate 0.8542") are missing a language specifier; update
each such fenced block in docs/metrics.md (all occurrences that include those
metric lines) to start with a language tag like ```text or ```tsv to satisfy
markdownlint (apply the same change to the other blocks noted in the comment).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/lib/unified_pipeline/bam.rs`:
- Around line 3749-3755: The Drop impl for EofWriter unconditionally appends
BGZF_EOF (via BGZF_EOF in Drop for EofWriter), which can mask failed pipeline
output; change the design so EOF is only written on successful completion by
adding an explicit finish()/disarm() method (e.g., EofWriter::finish(&mut self)
-> io::Result<()> that writes BGZF_EOF, flushes, and takes/self.inner = None)
and modify Drop to avoid writing BGZF_EOF (only perform a best-effort flush or
nothing) or alternatively add a shared Arc<AtomicBool> success flag that
finish() sets to true and Drop checks before writing EOF; update uses of
EofWriter to call finish() on success instead of relying on Drop.

In `@tests/integration/test_duplex_command.rs`:
- Around line 150-158: Update the tag check in the duplex consensus test to use
the new lowercase per-base depth tag: replace the cd_tag value currently set to
[b'c', b'D'] with the lowercase [b'c', b'd'] (or alternately implement a check
that accepts either [b'c', b'D'] or [b'c', b'd'] for backward compatibility) in
the loop that iterates over reader.records() and asserts the tag presence (the
variable named cd_tag and the assert!("Duplex consensus should have cD tag")
should be updated accordingly).

In `@tests/integration/test_simplex_command.rs`:
- Around line 68-76: The test is expecting the old uppercase tag; update the tag
bytes to the new lowercase form by changing the cd_tag definition from [b'c',
b'D'] to [b'c', b'd'] (and similarly adjust any other tag checks that used
uppercase, e.g., CE to [b'c', b'e'] if present) so the assertion
assert!(record.data().get(&cd_tag).is_some(), ...) matches the current output.

---

Outside diff comments:
In `@src/commands/group.rs`:
- Around line 1091-1111: The pipeline path currently swallows the error from
assign_umi_groups_impl (caught as _e) and returns an empty
ProcessedPositionGroup; change this to capture the error (e) and emit a
log::warn! with the error and contextual info (e.g., raw_tag, assigner or
assign_tag_bytes) before returning, mirroring the single-threaded path's
warning; update the catch in the assign_umi_groups_impl call block so it uses
the captured error variable in the warn! message and then returns the empty
ProcessedPositionGroup as before.
- Around line 3525-3527: The test contains a tautological assertion using
result.is_ok() || result.is_err() which always passes; replace it with a
meaningful assertion for the intended behavior of cmd.execute("test") (for
example assert that it succeeds with assert!(result.is_ok()) or assert that it
fails with assert!(result.is_err())), or remove the assertion entirely if no
deterministic outcome is expected; locate the call to cmd.execute in this test
and update the assertion accordingly (reference symbols: cmd.execute and
result).

In `@src/commands/zipper.rs`:
- Around line 870-878: The single-threaded branch creates a
noodles::bam::io::Writer (variable writer) but never finalizes it, so any
IO/finalization errors are lost; after calling process_singlethreaded(...)
ensure you call writer.into_inner().finish()? and propagate the error (same
pattern as the multi-threaded path uses writer.into_inner().finish()?); also add
the crate-level directive #![deny(unsafe_code)] at the top of this file to
satisfy the repository guideline.

---

Nitpick comments:
In `@crates/fgumi-sam/src/builder.rs`:
- Around line 2206-2213: The test test_consensus_tags_per_base currently only
checks tags.len(); update it to also assert the actual lowercase tag names to
prevent regressions: after building with
ConsensusTagsBuilder::new().per_base_depths(&[10,20,30]).per_base_errors(&[1,2,3]).build(),
add assertions that the two produced tags have byte names equal to [b'c', b'd']
and [b'c', b'e'] (i.e., verify the tag name/bytes fields on the produced tag
objects rather than only checking length).
- Around line 1707-1721: The doc comment for the consensus SAM tag builder omits
the new per-base tag names; update the comment in the Builder for
consensus-specific SAM tags (the doc block around the consensus tag list in
builder.rs) to explicitly list the per-base tags `cd` (per-base depth/coverage
array) and `ce` (per-base error rate array) alongside the existing tags (`cD`,
`cM`, `cE`, etc.), and briefly annotate their meanings (e.g., `cd`: per-base
coverage depth array, `ce`: per-base error rate array) so readers can find
`cd`/`ce` when scanning the builder docs.

In `@docs/metrics.md`:
- Around line 44-49: The fenced code blocks showing metric examples (e.g., the
block with "total_reads 10000 / passed_reads 8542 / failed_reads 1458 /
pass_rate 0.8542") are missing a language specifier; update each such fenced
block in docs/metrics.md (all occurrences that include those metric lines) to
start with a language tag like ```text or ```tsv to satisfy markdownlint (apply
the same change to the other blocks noted in the comment).

In `@src/commands/group.rs`:
- Around line 1502-1546: Duplicate metrics-building logic (median/min/max family
size and summary fields) appears in two places; extract it into a shared helper
like fn build_grouping_metrics(filter_metrics: &FilterMetrics, family_sizes:
&AHashMap<usize,u64>) -> UmiGroupingMetrics and replace both blocks with calls
to that helper; the helper should initialize UmiGroupingMetrics, copy totals
from total_filter_metrics (or FilterMetrics), compute
total_families/unique_molecule_ids from family_size_counter,
avg_reads_per_molecule, median_reads_per_molecule by sorting family_size_counter
(as Vec<(usize,u64)>) and scanning cumulative counts, and set
min_reads_per_molecule/max_reads_per_molecule—update callers to pass
total_filter_metrics and family_size_counter and remove the duplicated code in
group.rs and the pipeline path.
- Around line 1-2: Remove the leftover scaffolding comments at the top of
src/commands/group.rs (the two lines starting with "// This file will
contain..." and "// We'll build it section by section"); simply delete those
comment lines so the file no longer contains stale build-plan notes and keep
only relevant code and documentation in group.rs (no other changes needed).

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)

src/lib/unified_pipeline/bam.rs (1)

4164-4168: ⚠️ Potential issue | 🟡 Minor

Primary output doesn't use open_pipeline_output — no stdout support here.

All other run_bam_pipeline_* variants call open_pipeline_output(output_path), but this one uses File::create directly. If stdout support is intended to be uniform (per PR objective: "uniform stdout support"), this is inconsistent.

Proposed fix

-    // Create primary output BAM and write the output header
-    let output_file = File::create(output_path)
-        .map_err(|e| io::Error::new(e.kind(), format!("Failed to create output: {e}")))?;
-
-    let mut header_writer = bam::io::Writer::new(output_file);
+    // Create primary output BAM and write the output header
+    let output_writer = open_pipeline_output(output_path)?;
+
+    let mut header_writer = bam::io::Writer::new(output_writer);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/unified_pipeline/bam.rs` around lines 4164 - 4168, The primary output
creation currently uses File::create(output_path) which bypasses stdout
handling; replace this with a call to open_pipeline_output(output_path) and use
its returned writer (same variable name output_file) so stdout support matches
the other run_bam_pipeline_* variants; ensure you propagate the same error
mapping as before and that the subsequent write of the output header uses the
new output_file writer returned by open_pipeline_output.

src/commands/group.rs (2)

2482-2483: ⚠️ Potential issue | 🟡 Minor

Test ignores execute result — doesn't assert success or failure.

let _result = cmd.execute("test");

This only checks "no panic." If the intent is that a "-" UMI should error gracefully, assert _result.is_err(). If it should succeed, assert _result.is_ok(). As-is, a regression that changes the outcome won't be caught.

Proposed fix

-        // Should complete without panic — either succeeds or returns a structured error
-        let _result = cmd.execute("test");
+        // Should complete without panic — assert it returns a structured error or succeeds
+        let result = cmd.execute("test");
+        // Uncomment one of the following based on expected behavior:
+        // assert!(result.is_ok(), "Expected success but got: {result:?}");
+        // assert!(result.is_err(), "Expected error for UMI '-' but got Ok");
+        let _ = result; // TODO: pick the correct assertion

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/group.rs` around lines 2482 - 2483, The test currently captures
the return of cmd.execute("test") into let _result = ... but never asserts its
outcome, so change the test to assert the expected Result of Command::execute:
replace the unused _result with a binding (e.g., let result =
cmd.execute("test")) and then call either assert!(result.is_ok()) if a "-" UMI
should succeed or assert!(result.is_err()) if it should produce an error;
reference the execute method on the cmd instance to locate and update the
assertion accordingly.

---

1140-1161: ⚠️ Potential issue | 🟡 Minor

UMI assignment errors are now silently swallowed (warn + empty group).

Previously this would have propagated the error upward. Now a warn! is logged and processing continues with an empty group, silently dropping all templates in the position group. This could mask data-integrity issues (e.g., malformed UMI tags across an entire region) with no signal beyond a log line that may go unnoticed.

Consider at minimum incrementing a counter (e.g., in FilterMetrics) so the final summary reports how many groups were dropped due to assignment failure.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/group.rs` around lines 1140 - 1161, The current catch of errors
from assign_umi_groups_impl silently discards templates and returns an empty
ProcessedPositionGroup after only logging a warn; change this so the failure is
recorded in FilterMetrics and not silently swallowed: inside the Err(e) branch
(where assign_umi_groups_impl, templates, assigner, raw_tag, assign_tag_bytes,
filter_config.min_umi_length are handled) increment or set an appropriate
counter/flag on filter_metrics (e.g., add a dropped_assignment_groups or
increment assignment_errors) before returning, and keep the existing
memory-debug tracking; alternatively, if business logic requires propagation,
return Err(e) instead of returning an empty ProcessedPositionGroup so the caller
sees the failure (choose one approach and update nodes that consume
ProcessedPositionGroup to account for the metric or propagated error).

docs/metrics.md (2)

423-440: ⚠️ Potential issue | 🟡 Minor

group family_sizes.txt docs show 2 columns but the code writes 4.

TagFamilySizeMetric (in group.rs) has family_size, count, fraction, and fraction_gt_or_eq_family_size. The docs here only list family_size and count. Should match the dedup family_sizes.txt table (lines 229-234) which correctly shows all 4 columns.

Proposed fix

 | Column | Type | Description |
 |--------|------|-------------|
 | `family_size` | INT | Number of reads in the family |
 | `count` | INT | Number of families with this size |
+| `fraction` | FLOAT | Fraction of all families with this size |
+| `fraction_gt_or_eq_family_size` | FLOAT | Cumulative fraction of families with size >= this value |

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/metrics.md` around lines 423 - 440, The docs for family_sizes.txt only
list two columns but the implementation (TagFamilySizeMetric in group.rs) writes
four columns: family_size, count, fraction, and fraction_gt_or_eq_family_size;
update the family_sizes.txt section to document all four columns to match the
dedup family_sizes.txt table and the TagFamilySizeMetric output, listing each
column name, type and description (including that fraction is proportion of
families and fraction_gt_or_eq_family_size is cumulative proportion >= that
family size) so the docs align with the code.

---

446-462: ⚠️ Potential issue | 🔴 Critical

grouping_metrics.txt documentation completely mismatches the actual output schema.

The documented columns (total_templates, assigned_templates, filtered_templates, unique_positions, mean_family_size, singletons, singleton_fraction) don't match the UmiGroupingMetrics struct fields actually serialized to the TSV file.

The actual fields written are: total_records, accepted_records, discarded_non_pf, discarded_poor_alignment, discarded_ns_in_umi, discarded_umi_too_short, unique_molecule_ids, total_families, avg_reads_per_molecule, median_reads_per_molecule, min_reads_per_molecule, max_reads_per_molecule.

Additionally, the documented fields singletons and singleton_fraction don't exist in the struct. Users parsing the TSV will find completely different column names than documented.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/metrics.md` around lines 446 - 462, The docs for grouping_metrics.txt
are out of sync with the serialized struct UmiGroupingMetrics: update the table
in docs/metrics.md (the grouping_metrics.txt section) to exactly match the
actual serialized field names and types (total_records, accepted_records,
discarded_non_pf, discarded_poor_alignment, discarded_ns_in_umi,
discarded_umi_too_short, unique_molecule_ids, total_families,
avg_reads_per_molecule, median_reads_per_molecule, min_reads_per_molecule,
max_reads_per_molecule) and remove references to nonexistent fields (singletons,
singleton_fraction); ensure descriptions and types match the UmiGroupingMetrics
struct and that column order matches the TSV writer.

🧹 Nitpick comments (10)

tests/integration/test_duplex_command.rs (1)

164-203: Consider extracting the shared molecule-setup into a helper.

Both tests duplicate the exact same molecule creation loop. Minor nit for a test file, but it'd reduce noise.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_duplex_command.rs` around lines 164 - 203, Extract the
repeated molecule-creation logic into a small helper function (e.g., fn
build_test_molecule() -> Vec<(ReadType1, ReadType2)> or the concrete pair type
used) that runs the two for-loops calling create_duplex_read_pair and returns
the Vec of pairs; then replace the duplicated loop code in
test_duplex_command_with_stats (and the other test that duplicates it) with a
call to this helper and pass its result to create_duplex_bam (e.g.,
create_duplex_bam(&input_bam, vec![build_test_molecule()]). Keep the same
parameters (IDs, flags, sequence, qualities) and the existing
create_duplex_read_pair / create_duplex_bam calls so types and behavior are
unchanged.

tests/integration/test_simplex_command.rs (1)

19-19: Prefer &Path over &PathBuf.

Idiomatic Rust: accept &Path (or impl AsRef<Path>) rather than &PathBuf. &PathBuf auto-derefs, so it works, but it's unnecessarily specific.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_simplex_command.rs` at line 19, The function signature
create_grouped_bam currently takes path: &PathBuf which is unnecessarily
specific; change it to accept a more idiomatic type such as path: &Path or path:
impl AsRef<Path> (e.g., &Path for minimal change) and update all call sites
accordingly (where create_grouped_bam is invoked) to pass the same Path/PathBuf
references—the compiler will auto-deref PathBuf to &Path so callers need no
change unless they match the old signature explicitly. Ensure imports (use
std::path::Path) remain or are added and update the function body to use &Path
methods if any were relying on PathBuf-specific APIs.

tests/integration/test_filter_command.rs (3)

21-37: Returned fai_path is never used.

The clip command's equivalent helper returns only PathBuf. Returning a tuple here just to discard the second element in every call site is unnecessary noise. Consider matching the clip pattern.

Suggested fix

-fn create_test_reference(dir: &std::path::Path) -> (PathBuf, PathBuf) {
+fn create_test_reference(dir: &std::path::Path) -> PathBuf {
     ...
-    (ref_path, fai_path)
+    ref_path
 }

Then at each call site:

-    let (ref_path, _fai) = create_test_reference(temp_dir.path());
+    let ref_path = create_test_reference(temp_dir.path());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_filter_command.rs` around lines 21 - 37, The helper
create_test_reference returns a tuple (ref_path, fai_path) but callers only use
the first PathBuf; change create_test_reference to return a single PathBuf (the
ref_path) and stop constructing/returning fai_path, update its signature and all
call sites to expect a PathBuf instead of (PathBuf, PathBuf), and remove any
unused fai_path variable/creation inside create_test_reference while still
writing the FAI file if needed (or omit creating it if unused).

---

176-219: Stats test only checks file existence.

Consider reading a line or checking non-empty content so the test catches regressions where the file is created but empty.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_filter_command.rs` around lines 176 - 219, The test
test_filter_command_with_stats currently only asserts stats_path.exists();
update it to also open and read the stats file (using stats_path and
std::fs::read_to_string or a reader) and assert the content is non-empty or
contains at least one expected line/value so an empty-but-present file fails the
test; keep the existing Command::new(...) invocation and status.success() check,
then add an assertion like reading stats_path into a String and
assert!(!content.trim().is_empty()) (or check for a specific header/token if
applicable).

---

40-40: Accept &Path instead of &PathBuf.

Idiomatic Rust — &PathBuf auto-derefs, but the signature should use &Path for flexibility.

Suggested fix

-fn create_consensus_bam(path: &PathBuf, records: Vec<RecordBuf>) {
+fn create_consensus_bam(path: &std::path::Path, records: Vec<RecordBuf>) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_filter_command.rs` at line 40, Change the function
signature for create_consensus_bam from taking &PathBuf to taking &Path to be
idiomatic and more flexible: update the function declaration
(create_consensus_bam) and all call sites to accept a &Path (or let callers pass
PathBuf.as_path()), and ensure any uses of methods that require PathBuf are
adjusted (e.g., cloning or converting to owned PathBuf only when necessary).

tests/integration/test_clip_command.rs (2)

20-35: Duplicate create_test_reference helper across test files.

This helper is nearly identical to the one in test_filter_command.rs (same ref sequence, same FAI). Consider extracting it into helpers::bam_generator or a shared test utility to reduce copy-paste.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_clip_command.rs` around lines 20 - 35, The
create_test_reference helper is duplicated across integration tests; extract it
into a shared test helper module (e.g., helpers::bam_generator) and replace the
duplicate definitions by importing and calling
helpers::bam_generator::create_test_reference in both test_clip_command.rs and
test_filter_command.rs; ensure the shared function preserves the same signature
(fn create_test_reference(dir: &std::path::Path) -> PathBuf) and behavior
(writes ref.fa and ref.fa.fai with the same sequence and FAI contents) and
update use/import statements in the test files to reference the new module.

---

38-38: Prefer &Path over &PathBuf.

&PathBuf auto-derefs, but idiomatic Rust and Clippy (clippy::ptr_arg) prefer &Path.

Suggested fix

-fn create_paired_bam(path: &PathBuf, pairs: Vec<(RecordBuf, RecordBuf)>) {
+fn create_paired_bam(path: &Path, pairs: Vec<(RecordBuf, RecordBuf)>) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_clip_command.rs` at line 38, The function
create_paired_bam currently takes a parameter of type &PathBuf which is
non-idiomatic; change its signature to accept &Path (fn create_paired_bam(path:
&Path, pairs: Vec<(RecordBuf, RecordBuf)>) ) and update any call sites to pass
the Path or let the existing PathBuf auto-deref (e.g., use my_path.as_path() or
pass &my_path directly). Ensure you import std::path::Path if needed and adjust
any pattern matches or method calls on `path` to work with &Path (they should,
since Path and PathBuf share methods).

src/commands/group.rs (3)

1619-1620: MI base incremented by templates.len(), not by unique MI count.

Both here and in the pipeline path (line 1243), next_mi_base advances by the number of templates, not the number of distinct molecule IDs. This creates sparse/non-contiguous MI values. Functionally correct, but if downstream tools assume dense MI numbering, this could surprise.

Noting for awareness — both paths are consistent, so likely intentional.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/group.rs` around lines 1619 - 1620, The code increments
next_mi_base by templates.len(), causing gaps when multiple templates share the
same molecule ID; change the increment to the count of unique molecule IDs
instead. Compute the distinct molecule ID count from templates (e.g., collect
unique template.molecule_id values into a set and take its length) and add that
count to next_mi_base rather than templates.len(); apply the same change in the
other path that currently uses templates.len() so both branches remain
consistent. Ensure you reference and update the same variable names
(next_mi_base and templates) in both locations.

---

384-427: split_templates_by_pair_orientation governs the branch — naming is non-obvious.

true → single-UMI path (uppercase), false → paired-UMI path (split on '-'). The method name suggests the opposite at first glance. A short inline comment above the if would save future readers a trip to the trait definition.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/group.rs` around lines 384 - 427, The branch controlled by
assigner.split_templates_by_pair_orientation() is counterintuitive (true selects
single-UMI/uppercase path, false selects paired-UMI/split-on '-')—add a concise
inline comment above the if in umi_for_read_impl explaining this mapping and why
(e.g., "true => treat templates as oriented per-pair => single-UMI uppercase;
false => paired UMI with '-' delimiter, prefix with read-specific tags"), and
mention the paired behavior requiring downcast to PairedUmiAssigner and the
expected '-' split so future readers won't need to inspect the trait.

---

781-830: Median calculation is off-by-one for even-count distributions.

median_target = total_families / 2 with integer division and the >= check means for an even number of families you pick the lower-median bucket, not the average of the two middle values. For metrics reporting this is typically acceptable, but worth a note if exact medians matter.

Also: when total_families == 1, median_target == 0, and the first iteration sets the median correctly since cumulative (>= 1) >= 0 is always true.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/group.rs` around lines 781 - 830, The median calculation in
build_grouping_metrics is biased for even counts because median_target =
total_families / 2 picks the lower-middle bucket; fix by computing both middle
indices (let lower_idx = (total_families - 1) / 2 and let upper_idx =
total_families / 2), iterate the sorted sizes to find the bucket sizes at those
two cumulative positions (size_lower and size_upper), and set
metrics.median_reads_per_molecule to the average of those two values (use a
float average if metrics.median_reads_per_molecule is floating or cast
appropriately), keeping the rest of the size/min/max logic the same.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/DEVELOPING.md`:
- Around line 112-123: The docs table is missing two Cargo feature entries: add
rows for `compare` (enables the compare subcommand for BAM and metrics developer
tools; usage e.g. `cargo build --features compare` or `cargo nextest run
--features compare`) and `simulate` (enables simulate commands for generating
synthetic test data; usage e.g. `cargo build --features simulate`), matching the
existing style and wording of the other feature rows (`memory-debug`,
`dhat-heap`, `profile-adjacency`, `stress-tests`) so the table fully documents
all development-only features.

In `@tests/integration/test_filter_command.rs`:
- Around line 118-174: The test test_filter_command_rejects_low_depth only
asserts the output files exist; update it to open both output_bam and
rejects_bam after running Command (same invocation in the diff) and assert the
record counts: the output BAM (produced by create_consensus_bam input) should
contain exactly 1 accepted record (the "good" read) and the rejects BAM should
contain exactly 1 rejected record (the "low_depth" read). Use the project’s BAM
reading utility (or the same reader used elsewhere in tests) to iterate/read
records and assert counts (or collected record names) to verify the low-depth
record was moved to rejects. Ensure these assertions are added after the
existing file-exists checks in test_filter_command_rejects_low_depth.

---

Outside diff comments:
In `@docs/metrics.md`:
- Around line 423-440: The docs for family_sizes.txt only list two columns but
the implementation (TagFamilySizeMetric in group.rs) writes four columns:
family_size, count, fraction, and fraction_gt_or_eq_family_size; update the
family_sizes.txt section to document all four columns to match the dedup
family_sizes.txt table and the TagFamilySizeMetric output, listing each column
name, type and description (including that fraction is proportion of families
and fraction_gt_or_eq_family_size is cumulative proportion >= that family size)
so the docs align with the code.
- Around line 446-462: The docs for grouping_metrics.txt are out of sync with
the serialized struct UmiGroupingMetrics: update the table in docs/metrics.md
(the grouping_metrics.txt section) to exactly match the actual serialized field
names and types (total_records, accepted_records, discarded_non_pf,
discarded_poor_alignment, discarded_ns_in_umi, discarded_umi_too_short,
unique_molecule_ids, total_families, avg_reads_per_molecule,
median_reads_per_molecule, min_reads_per_molecule, max_reads_per_molecule) and
remove references to nonexistent fields (singletons, singleton_fraction); ensure
descriptions and types match the UmiGroupingMetrics struct and that column order
matches the TSV writer.

In `@src/commands/group.rs`:
- Around line 2482-2483: The test currently captures the return of
cmd.execute("test") into let _result = ... but never asserts its outcome, so
change the test to assert the expected Result of Command::execute: replace the
unused _result with a binding (e.g., let result = cmd.execute("test")) and then
call either assert!(result.is_ok()) if a "-" UMI should succeed or
assert!(result.is_err()) if it should produce an error; reference the execute
method on the cmd instance to locate and update the assertion accordingly.
- Around line 1140-1161: The current catch of errors from assign_umi_groups_impl
silently discards templates and returns an empty ProcessedPositionGroup after
only logging a warn; change this so the failure is recorded in FilterMetrics and
not silently swallowed: inside the Err(e) branch (where assign_umi_groups_impl,
templates, assigner, raw_tag, assign_tag_bytes, filter_config.min_umi_length are
handled) increment or set an appropriate counter/flag on filter_metrics (e.g.,
add a dropped_assignment_groups or increment assignment_errors) before
returning, and keep the existing memory-debug tracking; alternatively, if
business logic requires propagation, return Err(e) instead of returning an empty
ProcessedPositionGroup so the caller sees the failure (choose one approach and
update nodes that consume ProcessedPositionGroup to account for the metric or
propagated error).

In `@src/lib/unified_pipeline/bam.rs`:
- Around line 4164-4168: The primary output creation currently uses
File::create(output_path) which bypasses stdout handling; replace this with a
call to open_pipeline_output(output_path) and use its returned writer (same
variable name output_file) so stdout support matches the other
run_bam_pipeline_* variants; ensure you propagate the same error mapping as
before and that the subsequent write of the output header uses the new
output_file writer returned by open_pipeline_output.

---

Duplicate comments:
In `@src/lib/unified_pipeline/bam.rs`:
- Around line 3725-3764: The implementation of EofWriter correctly gates writing
BGZF_EOF on the armed Arc<AtomicBool> and uses Acquire/Release ordering; no
change required—leave EofWriter::new, the armed flag, the Write impl
(write/flush), and the Drop impl that writes BGZF_EOF and flushes when armed
as-is.

In `@tests/integration/test_duplex_command.rs`:
- Around line 150-158: Update the test to expect the renamed lowercase tag:
replace the cd_tag definition and any checks that reference [b'c', b'D'] with
the lowercase bytes [b'c', b'd'] (e.g., in
tests/integration/test_duplex_command.rs update the cd_tag used in the loop
where record.data().get(&cd_tag) is asserted); ensure any analogous checks for
the complementary tag follow the same lowercase rename.

In `@tests/integration/test_simplex_command.rs`:
- Around line 75-76: The test uses the wrong byte tag case: change the cd_tag
definition from [b'c', b'D'] to lowercase [b'c', b'd'] so the assertion in
test_simplex_command.rs (the cd_tag variable and the
assert!(record.data().get(&cd_tag)...)) checks for the new lowercase consensus
tag; update any similar occurrences of cD/cE to cd/ce in this test to match the
PR rename.

---

Nitpick comments:
In `@src/commands/group.rs`:
- Around line 1619-1620: The code increments next_mi_base by templates.len(),
causing gaps when multiple templates share the same molecule ID; change the
increment to the count of unique molecule IDs instead. Compute the distinct
molecule ID count from templates (e.g., collect unique template.molecule_id
values into a set and take its length) and add that count to next_mi_base rather
than templates.len(); apply the same change in the other path that currently
uses templates.len() so both branches remain consistent. Ensure you reference
and update the same variable names (next_mi_base and templates) in both
locations.
- Around line 384-427: The branch controlled by
assigner.split_templates_by_pair_orientation() is counterintuitive (true selects
single-UMI/uppercase path, false selects paired-UMI/split-on '-')—add a concise
inline comment above the if in umi_for_read_impl explaining this mapping and why
(e.g., "true => treat templates as oriented per-pair => single-UMI uppercase;
false => paired UMI with '-' delimiter, prefix with read-specific tags"), and
mention the paired behavior requiring downcast to PairedUmiAssigner and the
expected '-' split so future readers won't need to inspect the trait.
- Around line 781-830: The median calculation in build_grouping_metrics is
biased for even counts because median_target = total_families / 2 picks the
lower-middle bucket; fix by computing both middle indices (let lower_idx =
(total_families - 1) / 2 and let upper_idx = total_families / 2), iterate the
sorted sizes to find the bucket sizes at those two cumulative positions
(size_lower and size_upper), and set metrics.median_reads_per_molecule to the
average of those two values (use a float average if
metrics.median_reads_per_molecule is floating or cast appropriately), keeping
the rest of the size/min/max logic the same.

In `@tests/integration/test_clip_command.rs`:
- Around line 20-35: The create_test_reference helper is duplicated across
integration tests; extract it into a shared test helper module (e.g.,
helpers::bam_generator) and replace the duplicate definitions by importing and
calling helpers::bam_generator::create_test_reference in both
test_clip_command.rs and test_filter_command.rs; ensure the shared function
preserves the same signature (fn create_test_reference(dir: &std::path::Path) ->
PathBuf) and behavior (writes ref.fa and ref.fa.fai with the same sequence and
FAI contents) and update use/import statements in the test files to reference
the new module.
- Line 38: The function create_paired_bam currently takes a parameter of type
&PathBuf which is non-idiomatic; change its signature to accept &Path (fn
create_paired_bam(path: &Path, pairs: Vec<(RecordBuf, RecordBuf)>) ) and update
any call sites to pass the Path or let the existing PathBuf auto-deref (e.g.,
use my_path.as_path() or pass &my_path directly). Ensure you import
std::path::Path if needed and adjust any pattern matches or method calls on
`path` to work with &Path (they should, since Path and PathBuf share methods).

In `@tests/integration/test_duplex_command.rs`:
- Around line 164-203: Extract the repeated molecule-creation logic into a small
helper function (e.g., fn build_test_molecule() -> Vec<(ReadType1, ReadType2)>
or the concrete pair type used) that runs the two for-loops calling
create_duplex_read_pair and returns the Vec of pairs; then replace the
duplicated loop code in test_duplex_command_with_stats (and the other test that
duplicates it) with a call to this helper and pass its result to
create_duplex_bam (e.g., create_duplex_bam(&input_bam,
vec![build_test_molecule()]). Keep the same parameters (IDs, flags, sequence,
qualities) and the existing create_duplex_read_pair / create_duplex_bam calls so
types and behavior are unchanged.

In `@tests/integration/test_filter_command.rs`:
- Around line 21-37: The helper create_test_reference returns a tuple (ref_path,
fai_path) but callers only use the first PathBuf; change create_test_reference
to return a single PathBuf (the ref_path) and stop constructing/returning
fai_path, update its signature and all call sites to expect a PathBuf instead of
(PathBuf, PathBuf), and remove any unused fai_path variable/creation inside
create_test_reference while still writing the FAI file if needed (or omit
creating it if unused).
- Around line 176-219: The test test_filter_command_with_stats currently only
asserts stats_path.exists(); update it to also open and read the stats file
(using stats_path and std::fs::read_to_string or a reader) and assert the
content is non-empty or contains at least one expected line/value so an
empty-but-present file fails the test; keep the existing Command::new(...)
invocation and status.success() check, then add an assertion like reading
stats_path into a String and assert!(!content.trim().is_empty()) (or check for a
specific header/token if applicable).
- Line 40: Change the function signature for create_consensus_bam from taking
&PathBuf to taking &Path to be idiomatic and more flexible: update the function
declaration (create_consensus_bam) and all call sites to accept a &Path (or let
callers pass PathBuf.as_path()), and ensure any uses of methods that require
PathBuf are adjusted (e.g., cloning or converting to owned PathBuf only when
necessary).

In `@tests/integration/test_simplex_command.rs`:
- Line 19: The function signature create_grouped_bam currently takes path:
&PathBuf which is unnecessarily specific; change it to accept a more idiomatic
type such as path: &Path or path: impl AsRef<Path> (e.g., &Path for minimal
change) and update all call sites accordingly (where create_grouped_bam is
invoked) to pass the same Path/PathBuf references—the compiler will auto-deref
PathBuf to &Path so callers need no change unless they match the old signature
explicitly. Ensure imports (use std::path::Path) remain or are added and update
the function body to use &Path methods if any were relying on PathBuf-specific
APIs.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/commands/group.rs (1)

1140-1161: ⚠️ Potential issue | 🟡 Minor

Silently dropping entire position groups on UMI assignment failure.

A systematic misconfiguration (wrong tag, corrupt data) would silently discard all reads with only warn! messages. Consider adding a counter and failing hard if the error rate exceeds a threshold, or at least emitting a summary count at the end.

src/lib/unified_pipeline/bam.rs (1)

3879-3891: ⚠️ Potential issue | 🟠 Major

EOF arm happens after the writer is already dropped.

run_bam_pipeline(...) drops the output during finalization, so setting eof_armed after it returns is too late — BGZF_EOF never gets appended on success. Move the arming into the pipeline finalization path (before flush_output drops the writer), or add an explicit finish() on EofWriter that is called on success before the output is taken/dropped. This affects all run_bam_pipeline_* wrappers using the same pattern.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/unified_pipeline/bam.rs` around lines 3879 - 3891, The EOF arm is set
after run_bam_pipeline(...) returns but the pipeline finalization already
dropped the output, so BGZF_EOF never gets written; modify the pipeline
finalization to arm EOF before the writer is dropped: either (a) move
eof_armed.store(true, Ordering::Release) into the pipeline’s finalization path
(e.g., inside the code that currently calls flush_output or right before
flush_output drops the writer), or (b) add and call an explicit finish() method
on EofWriter from the run_bam_pipeline finalization branch on success (ensure
finish() appends BGZF_EOF and sets eof_armed) so that all run_bam_pipeline_*
wrappers follow the same pattern and arm EOF before the output is taken/dropped.

🧹 Nitpick comments (2)

src/commands/group.rs (2)

2482-2483: Test doesn't assert on the result. Ignoring _result means a future regression (e.g., panic→error→silent success) won't be caught. Consider at least asserting _result.is_ok() or _result.is_err() to lock in the expected behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/group.rs` around lines 2482 - 2483, The test currently ignores
the `_result` returned after pushing `r2` into `records`, which can hide
regressions; update the test to assert the expected outcome (e.g.,
`assert!(_result.is_ok())` or `assert!(_result.is_err())` depending on the
intended behavior) immediately after the `records.push(r2);` call so the test
fails on unexpected changes—refer to the local symbols `records`, `r2`, and
`_result` to locate and modify the assertion.

---

1831-1852: _mapq2 parameter is unused. RecordPairBuilder::mapping_quality sets MAPQ for both reads uniformly. If per-read MAPQ control is intended, this is a latent bug in tests; if not, remove the parameter to avoid confusion.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/group.rs` around lines 1831 - 1852, The build_test_pair function
declares an unused _mapq2 parameter which is confusing or indicates a latent
per-read MAPQ intention; either remove the _mapq2 parameter from build_test_pair
signature and all its call sites, or modify the code to apply per-read MAPQ
(e.g., use a builder method that sets r2 mapping quality if RecordPairBuilder
supports separate per-read MAPQ). Update the function signature for
build_test_pair and any tests calling it (or call the appropriate
RecordPairBuilder method instead of mapping_quality if per-read control is
required) to keep parameter usage consistent with
RecordPairBuilder::mapping_quality behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/lib/unified_pipeline/bam.rs`:
- Around line 3879-3891: The EOF arm is set after run_bam_pipeline(...) returns
but the pipeline finalization already dropped the output, so BGZF_EOF never gets
written; modify the pipeline finalization to arm EOF before the writer is
dropped: either (a) move eof_armed.store(true, Ordering::Release) into the
pipeline’s finalization path (e.g., inside the code that currently calls
flush_output or right before flush_output drops the writer), or (b) add and call
an explicit finish() method on EofWriter from the run_bam_pipeline finalization
branch on success (ensure finish() appends BGZF_EOF and sets eof_armed) so that
all run_bam_pipeline_* wrappers follow the same pattern and arm EOF before the
output is taken/dropped.

---

Duplicate comments:
In `@tests/integration/test_duplex_command.rs`:
- Around line 172-179: The test is checking for an uppercase consensus tag cD
but outputs now use lowercase cd; update the tag check in the test by changing
the cd_tag used in the record.data().get(...) call from [b'c', b'D'] to the
lowercase bytes [b'c', b'd'] (the variables involved are reader, record, and
cd_tag in the test_duplex_command.rs test loop).

In `@tests/integration/test_simplex_command.rs`:
- Around line 68-76: The test currently checks for an uppercase consensus tag by
building cd_tag = [b'c', b'D'] and asserting
record.data().get(&cd_tag).is_some(); update the tag to the new lowercase form
(e.g., cd_tag = [b'c', b'd']) in tests/integration/test_simplex_command.rs so
the lookup in record.data() matches the output; keep the same assertion and
message but ensure the byte pair uses lowercase 'd'.

---

Nitpick comments:
In `@src/commands/group.rs`:
- Around line 2482-2483: The test currently ignores the `_result` returned after
pushing `r2` into `records`, which can hide regressions; update the test to
assert the expected outcome (e.g., `assert!(_result.is_ok())` or
`assert!(_result.is_err())` depending on the intended behavior) immediately
after the `records.push(r2);` call so the test fails on unexpected changes—refer
to the local symbols `records`, `r2`, and `_result` to locate and modify the
assertion.
- Around line 1831-1852: The build_test_pair function declares an unused _mapq2
parameter which is confusing or indicates a latent per-read MAPQ intention;
either remove the _mapq2 parameter from build_test_pair signature and all its
call sites, or modify the code to apply per-read MAPQ (e.g., use a builder
method that sets r2 mapping quality if RecordPairBuilder supports separate
per-read MAPQ). Update the function signature for build_test_pair and any tests
calling it (or call the appropriate RecordPairBuilder method instead of
mapping_quality if per-read control is required) to keep parameter usage
consistent with RecordPairBuilder::mapping_quality behavior.