feat(clip): route the clip command onto the declarative chain builder

[nh13]

What

Route fgumi clip onto the declarative chain builder — R3.x. This is the clip half of the command-cutover campaign (filter #892, correct #893, simplex #894, codec #895, duplex #896).

clip --threads N now runs through execute_chain → ChainSpec::single_stage(Stage::Clip, …) → build_for(spec)?.run(). The no---threads single-threaded engine (execute_single_threaded) is kept unchanged as the in-process parity oracle. The old threaded engine (execute_threads_mode) is removed in this PR: once --threads dispatches to the chain it has no call site, and it was never the parity oracle, so it and its now-orphan helpers (CollectedClipMetrics, ClipProcessedBatch and its MemoryEstimate impl + unit test) are deleted here rather than carried dead into a follow-up.

clip is a core, non-feature-gated command, so the dispatch is unconditional. clip writes no rejects (only --output and single-threaded-only --metrics), so there is no rejects-header-provenance decision. The dispatch sits after the reader-free pre-flight (output-collision check, input + reference existence, the clipping-option validation, and the --metrics/--threads fail-fast) but before the banner / timer / reader, so add_clip — which re-emits those and opens its own source — does not double-log or pre-consume stdin.

Two real defects the cutover exposed (fixed here)

• require_query_grouped was missing on the chain path. Both legacy clip paths enforce fgbio's Bams.requireQueryGrouped, but the dormant add_clip did not — so routing --threads onto the chain would silently mis-clip coordinate-sorted input the legacy path rejected. Added the guard in add_clip, gated on Stage::Clip being the source stage so a future fused group→clip chain (upstream stage orders the records) is not wrongly rejected. ChainSpec exposes only single_stage today, so the guard always fires. Pinned by the already-parameterized test_clip_rejects_coordinate_sorted_input / test_clip_rejects_headerless_input, whose --threads case now runs the chain.

• The chain clip step over-clipped reads with existing clipping. The dormant build_clip_process_step reimplemented per-template clipping and carried an explicit KNOWN DIVERGENCE — MUST be resolved in the clip wiring PR comment: it selected the primary pair by positional index (ignoring secondary/supplementary reads, and skipping templates with ≥3 records) and applied fixed clipping with "clip N more" (clip_*_end_of_alignment) instead of the canonical "ensure at least N including existing clipping" semantics — over-clipping any read already carrying soft/hard clips. This surfaced immediately: the existing unit test test_fixed_position_clip_counts_existing_clipping::case_2_multi_threaded expects 5S10M5S (existing 5′ clip counted, 5 new 3′ bases) but the chain produced 8S7M5S. Fixed by exposing ClipParams/ClipParams::from_clip/ClipParams::clip_template as pub(crate) and delegating the chain step to cap.params.clip_template(records, &clipper, None) — the exact code the oracle runs (its docstring already declares it "the single shared implementation used by both threading paths"). Parity is now by construction, not a parallel implementation. This let the now-dead update_mate_info_raw (itself carrying a "resolve in the clip wiring PR" note) be removed; clip_template repairs mate info via the canonical set_mate_info_raw. The --metrics detailed collection is never produced under --threads, so the chain passes None and drives its atomic overlap_clipped/extend_clipped counters off clip_template's returned per-template flags.

Tests

• test_clip_chain_matches_single_threaded (#[case] threads 1/2/4): chain vs. single-threaded oracle, full normalized-header + record parity via read_bam_output, with non-vacuous guards (all 16 records survive; at least one output CIGAR ≠ 8M, so clipping actually ran).
• test_clip_rejects_metrics_with_threads: pins the reader-free --metrics + --threads fast-fail (expect_err, message contains "cannot be used with --threads"), so a regression can't silently drop the user's requested metrics file.
• The pre-existing threaded tests now route through the chain and pass: coordinate / header-less rejection (the guard), primary-pair all-options, supplementary mate repair, secondary-only, fragment, past-mate, upgrade-supplementary, --check-crc handling, stdin-once, and test_clip_command_single_and_multi_threaded_outputs_match.
• The fixed test_fixed_position_clip_counts_existing_clipping::case_2_multi_threaded.

Verification

cargo ci-fmt && cargo ci-lint && RUSTDOCFLAGS="-D warnings" cargo ci-doc && cargo ci-test — all green (9772 tests). The three feature legs compile (--no-default-features, default, --all-features, all --all-targets). No new unsafe.

Risk: output changes for clip threaded execution, pinned by single-threaded parity tests and shared ClipParams::clip_template; unsafe changes: none; memory, queue, and backpressure policy changes: none.

Threaded clip execution now uses the declarative chain builder. The single-threaded path remains the parity oracle. Reader-free validation runs before dispatch and rejects invalid query grouping, output collisions, clipping options, and --metrics with --threads.

The chain path reuses ClipParams::clip_template. This preserves existing clipping behavior, prevents over-clipping, and applies canonical mate repair. The obsolete threaded engine and helper code were removed.

Integration tests cover thread-count parity, normalized headers, clipping behavior, validation failures, and existing threaded scenarios.

[coderabbitai]

Note

Reviews paused

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: bcaba7ff-4688-4e7d-b6af-9ae4f9edf06b

📥 Commits

Reviewing files that changed from the base of the PR and between 8f131a1 and 3124a47.

📒 Files selected for processing (4)

• src/lib/commands/clip.rs
• src/lib/pipeline/chains/builder.rs
• src/lib/pipeline/chains/commands/clip.rs
• tests/integration/test_clip_command.rs

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

---

Walkthrough

Changes

Clip chain migration

Layer / File(s)	Summary
Shared clip configuration and preflight src/lib/commands/clip.rs	ClipParams and its construction and template methods are crate-visible. Validation now runs before single-threaded or threaded dispatch.
Chain-based threaded clipping src/lib/commands/clip.rs, src/lib/pipeline/chains/...	Threaded execution uses execute_chain, query-grouped input validation, shared ClipParams, and canonical template clipping. The former threaded pipeline was removed.
Cross-mode integration coverage tests/integration/test_clip_command.rs, src/lib/commands/clip.rs	Tests cover metrics validation and one-, two-, and four-thread parity with single-threaded output, headers, and clipping behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 3124a

The clip command is routed through the declarative chain while preserving the single-threaded parity path, with targeted fixes and broad verification reported as passing; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant Clip
  participant ChainBuilder
  participant ClipProcessCaptures
  participant ClipParams
  Clip->>Clip: Run preflight validation
  Clip->>ChainBuilder: execute_chain
  ChainBuilder->>ChainBuilder: add_clip
  ChainBuilder->>ClipProcessCaptures: build_clip_process_step
  ClipProcessCaptures->>ClipParams: clip_template
  ClipParams-->>ClipProcessCaptures: Clipped records and metrics

Loading

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title follows the required Conventional Commit format. It uses the valid type feat, the affected command scope clip, a lowercase imperative description, and no trailing period. It accurately d…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title follows the required Conventional Commit format. It uses the valid type feat, the affected command scope clip, a lowercase imperative description, and no trailing period. It accurately describes the main change.

---

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

[nh13]

@coderabbitai pause

[coderabbitai]

✅ Action performed

Reviews paused.

[codecov]

Codecov Report

❌ Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.05%. Comparing base (8f131a1) to head (3124a47).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lib/pipeline/chains/builder.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #897      +/-   ##
==========================================
+ Coverage   92.95%   93.05%   +0.10%     
==========================================
  Files         299      299              
  Lines      150352   150178     -174     
==========================================
- Hits       139760   139752       -8     
+ Misses      10592    10426     -166     

☔ View full report in Codecov by Harness.
📢 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.

[nh13]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

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.