fix/docs/test: additional correctness fixes, doc sweep, and expanded coverage (6/6) - #463
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
WalkthroughThe PR updates CI, docs, runtime contracts, sort/index validation, comparator and validation behavior, and integration coverage to reflect typed-step pipeline semantics and stricter runtime/test assertions. ChangesTyped-step pipeline and command surface
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat-runall #463 +/- ##
==============================================
Coverage ? 94.25%
==============================================
Files ? 110
Lines ? 50089
Branches ? 0
==============================================
Hits ? 47211
Misses ? 2878
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nh13
force-pushed
the
nh/runall-fix-5-runall-consensus
branch
from
June 25, 2026 04:22
c82d616 to
ad680ab
Compare
nh13
force-pushed
the
nh/runall-fix-6-review-docs-tests
branch
from
June 25, 2026 04:26
495f749 to
8a2456d
Compare
nh13
force-pushed
the
nh/runall-fix-5-runall-consensus
branch
from
June 25, 2026 05:32
ad680ab to
71a68ca
Compare
nh13
force-pushed
the
nh/runall-fix-6-review-docs-tests
branch
from
June 25, 2026 05:32
8a2456d to
347cac7
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 25-31: The changelog entry for fgumi runall incorrectly suggests
it can start from extract, but runall’s documented stage range begins after
extraction. Update the wording in the runall bullet to describe it as the fused
post-extraction pipeline, and if mentioning end-to-end usage, reference the
composed fgumi extract | fgumi runall --start-from correct flow instead of
implying extract is part of runall.
In `@docs/src/guide/best-practices.md`:
- Around line 71-76: Clarify the `fgumi runall` tip so it does not read like an
end-to-end raw FASTQ pipeline: update the wording around `runall` in the
best-practices guide to state that it is post-extract only and starts after the
separate `extract` step, covering `correct`, `align`, `zipper`, `sort`, `group`,
and `consensus`. If helpful, mention the composed workflow explicitly or show
that `extract` must be run first, and keep the existing
`--start-from`/`--stop-after` guidance tied to `runall`.
In `@docs/src/index.md`:
- Around line 74-76: Update the `runall` entry in the command summary so it no
longer describes the pipeline as full FASTQ→consensus; revise the wording in the
table near `runall` to reflect that it starts post-extraction, such as
BAM→consensus, and keep the surrounding help text unchanged.
In `@tests/integration/test_runall_parity.rs`:
- Around line 902-939: `deterministic_simplex_fixture` is not actually
generating reads across multiple template-coordinate positions, so the
determinism test is weaker than intended. Update the fixture used by
`runall_simplex_record_stream_is_deterministic` to place each family at a
distinct position, using a positioned helper like `create_paired_umi_family_at`
or an equivalent single-end positioned builder instead of `create_umi_family`.
Keep the existing `Sort`/`run_standalone` flow, but ensure the generated BAM
spans many coordinates so `ByItemOrdinal` and worker-order nondeterminism are
exercised.
In `@tests/integration/test_sort_correctness.rs`:
- Around line 598-629: The test coordinate_sort_consistent_across_threads
currently only validates each thread count independently, so it does not enforce
the stated cross-thread consistency contract. Update this test to sort the same
input with threads set to 1, 2, and 4, then compare the resulting
coordinate-ordered record stream (or equivalent key sequence) across runs in
addition to keeping the existing sortedness and preservation checks. Use the
existing helpers fgumi_sort_in_process, verify_sorted,
assert_coordinate_ordered, and assert_records_preserved to keep the test focused
on comparing outputs rather than just self-validating each case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a1914dc-e102-42b6-84a9-47db367af837
📒 Files selected for processing (77)
.github/workflows/check.yml.gitignoreCHANGELOG.mdCLAUDE.mdREADME.mdbenches/core_functions.rscrates/fgumi-bgzf/src/writer.rscrates/fgumi-consensus/src/base_builder.rscrates/fgumi-consensus/src/codec_caller.rscrates/fgumi-consensus/src/duplex_caller.rscrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/held.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/reorder.rscrates/fgumi-pipeline-core/src/runtime/driver.rscrates/fgumi-pipeline-core/src/runtime/fused.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/stats.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/signal.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/source/read_bam.rscrates/fgumi-raw-bam/src/sort.rscrates/fgumi-raw-bam/src/tags.rscrates/fgumi-sort-cli/Cargo.tomlcrates/fgumi-sort-cli/src/sort.rscrates/fgumi-sort/src/worker_pool.rscrates/fgumi-umi/src/assigner.rsdocs/design/deterministic-mi-numbering.mddocs/simulate-cli.mddocs/src/guide/best-practices.mddocs/src/guide/performance-tuning.mddocs/src/index.mdsrc/lib/aligner.rssrc/lib/commands/codec.rssrc/lib/commands/common.rssrc/lib/commands/correct.rssrc/lib/commands/downsample.rssrc/lib/commands/filter.rssrc/lib/commands/runall.rssrc/lib/commands/simulate/mod.rssrc/lib/commands/zipper.rssrc/lib/fastq_parse.rssrc/lib/mi_group.rssrc/lib/pipeline/chains/build.rssrc/lib/pipeline/chains/builder.rssrc/lib/pipeline/chains/validate.rssrc/lib/pipeline/steps/align_and_merge.rssrc/lib/pipeline/steps/coalesce.rssrc/lib/pipeline/steps/correct/mod.rssrc/lib/pipeline/steps/correct/tests.rssrc/lib/pipeline/steps/extract.rssrc/lib/pipeline/steps/process.rssrc/lib/pipeline/steps/roundtrip.rssrc/lib/pipeline/steps/serialize_processed.rssrc/lib/pipeline/steps/source/pair_fastq.rssrc/lib/pipeline/steps/source/read_fastq.rssrc/lib/umi/parallel_assigner.rssrc/lib/validation.rstests/integration/helpers/assertions.rstests/integration/helpers/bam_generator.rstests/integration/helpers/cli_runner.rstests/integration/helpers/parity.rstests/integration/main.rstests/integration/test_chain_build.rstests/integration/test_codec_command.rstests/integration/test_dedup_command.rstests/integration/test_runall_parity.rstests/integration/test_simulate_aligner.rstests/integration/test_sort_correctness.rstests/integration/test_sort_write_index.rstests/integration/test_streaming_input.rs
💤 Files with no reviewable changes (2)
- tests/integration/helpers/assertions.rs
- crates/fgumi-sort-cli/Cargo.toml
nh13
force-pushed
the
nh/runall-fix-5-runall-consensus
branch
from
June 25, 2026 14:37
71a68ca to
a92836c
Compare
nh13
force-pushed
the
nh/runall-fix-6-review-docs-tests
branch
from
June 25, 2026 14:37
347cac7 to
e32c782
Compare
nh13
force-pushed
the
nh/runall-fix-5-runall-consensus
branch
from
June 25, 2026 16:20
a92836c to
46df46f
Compare
nh13
force-pushed
the
nh/runall-fix-6-review-docs-tests
branch
from
June 25, 2026 16:20
e32c782 to
4a150de
Compare
nh13
force-pushed
the
nh/runall-fix-5-runall-consensus
branch
from
June 25, 2026 17:03
46df46f to
6cf8aba
Compare
nh13
force-pushed
the
nh/runall-fix-6-review-docs-tests
branch
from
June 25, 2026 17:03
4a150de to
e7f4e25
Compare
nh13
force-pushed
the
nh/runall-fix-5-runall-consensus
branch
from
June 25, 2026 17:57
6cf8aba to
1f4afc6
Compare
nh13
force-pushed
the
nh/runall-fix-6-review-docs-tests
branch
from
June 25, 2026 17:57
e7f4e25 to
e80bf2c
Compare
nh13
force-pushed
the
nh/runall-fix-5-runall-consensus
branch
from
June 25, 2026 18:45
1f4afc6 to
2d1a84e
Compare
nh13
force-pushed
the
nh/runall-fix-6-review-docs-tests
branch
from
June 25, 2026 18:45
e80bf2c to
dcfc9a8
Compare
Close runall parity/E2E gaps (stdin, FASTQ fusion, reject-discard, floors); cover correct batch routing, header/desync invariants, methylation guards, and coalesce flush; honest ChainTailKind variant, asserted MiKey invariant, testable downsample bound; bench iter_batched.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
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 (1)
src/lib/pipeline/chains/builder.rs (1)
1504-1508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale inline comments still claim
add_alignre-wiresGroupByQueryname.The rustdoc block above (1452-1456) was fixed per a prior review to state that
add_alignskipsGroupByQuerynameon an incomingBamTemplateBatch. These two inline comments insideadd_correctstill say the opposite ("add_aligncan wireGroupByQueryname → AlignAndMergeStepnext"), which contradicts both the fixed doc and the actualadd_alignlogic (line 1839:if chain_tail_kind == BamTemplateBatch { tail } else { append GroupByQueryname }).🐛 Proposed fix
- // `Intermediate` is used when correct feeds into align-and-merge - // (the `--start-from correct --stop-after ≥ zipper` fused chain). - // In that case we skip `SerializeBamRecords` and leave the tail at - // the correct step's `BamTemplateBatch` output (branch 0) so - // `add_align` can wire `GroupByQueryname → AlignAndMergeStep` next. + // `Intermediate` is used when correct feeds into align-and-merge + // (the `--start-from correct --stop-after ≥ zipper` fused chain). + // In that case we skip `SerializeBamRecords` and leave the tail at + // the correct step's `BamTemplateBatch` output (branch 0); `add_align` + // skips `GroupByQueryname` on an incoming `BamTemplateBatch` and wires + // `AlignAndMergeStep` directly.- } else { - // Intermediate: leave the tail as BamTemplateBatch so the next - // stage (add_align → GroupByQueryname → AlignAndMergeStep) can - // consume it directly. + } else { + // Intermediate: leave the tail as BamTemplateBatch so the next + // stage (add_align → AlignAndMergeStep, skipping GroupByQueryname) + // can consume it directly.Also applies to: 1633-1635
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/pipeline/chains/builder.rs` around lines 1504 - 1508, Update the stale inline comments in add_correct so they match the current add_align behavior: when the chain tail is a BamTemplateBatch, add_align skips GroupByQueryname instead of re-wiring it. Keep the wording consistent with the corrected rustdoc above and the chain-tail logic in add_align, and revise the comments near the Intermediate/SerializeBamRecords explanation as well as the duplicate later comment block.
♻️ Duplicate comments (1)
tests/integration/test_sort_correctness.rs (1)
444-453: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCompare a per-record projection here, not just names.
Swapping R1/R2 inside an equal-name run still passes this oracle. Compare
record_key(r)(or the fullRecordBuf) against samtools so the natural-sort test pins exact output identity, not just qname grouping.Suggested fix
fn assert_name_order_matches(fgumi_out: &Path, samtools_out: &Path) { - let fgumi_names: Vec<Vec<u8>> = - read_records(fgumi_out).iter().map(|r| record_key(r).name).collect(); - let samtools_names: Vec<Vec<u8>> = - read_records(samtools_out).iter().map(|r| record_key(r).name).collect(); - assert_eq!(fgumi_names.len(), samtools_names.len(), "fgumi / samtools record count differs"); - assert_eq!( - fgumi_names, samtools_names, - "fgumi read-name order disagrees with samtools (independent natural oracle)" - ); + let fgumi_keys: Vec<RecordKey> = read_records(fgumi_out).iter().map(record_key).collect(); + let samtools_keys: Vec<RecordKey> = read_records(samtools_out).iter().map(record_key).collect(); + assert_eq!(fgumi_keys.len(), samtools_keys.len(), "fgumi / samtools record count differs"); + assert_eq!( + fgumi_keys, samtools_keys, + "fgumi queryname-natural order disagrees with samtools on per-record identity" + ); }As per path instructions, "New correctness-critical behavior needs an INDEPENDENT oracle" and "Flag assertions weaker than the stated contract."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_sort_correctness.rs` around lines 444 - 453, The oracle in assert_name_order_matches only compares record_key(r).name, so it can miss per-record differences like swapped mates within the same name group. Update the comparison to use a per-record projection that includes the full identity you want to validate, such as record_key(r) itself or the full RecordBuf, for both fgumi_out and samtools_out, and keep the count check plus the direct equality assertion so the natural-sort test verifies exact output identity rather than only qname order.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/commands/runall.rs`:
- Around line 563-567: The `sort` and `group` help text in `runall.rs` is too
absolute and implies fixed downstream stages even when `--stop-after` changes
the chain. Update the doc comments around the `sort`/`group` bullets to describe
only the input contract for each start point, and make the mention of following
stages conditional on `--stop-after` so the text aligns with the valid
`sort→sort` and `group→group` flows handled by the `runall` command logic.
In `@tests/integration/test_dedup_command.rs`:
- Around line 211-225: The test in test_dedup_command uses
alignment_start().unwrap().unwrap().get() and asserts 1-based coordinates, but
the inline projection comment still says 0-based positions 99/199. Update the
comment near the surviving/projection check to match the actual oracle values,
or adjust the projection/assertion to use a consistent coordinate system,
keeping the is_first_segment and is_reverse_complemented checks aligned with the
expected pair.
---
Outside diff comments:
In `@src/lib/pipeline/chains/builder.rs`:
- Around line 1504-1508: Update the stale inline comments in add_correct so they
match the current add_align behavior: when the chain tail is a BamTemplateBatch,
add_align skips GroupByQueryname instead of re-wiring it. Keep the wording
consistent with the corrected rustdoc above and the chain-tail logic in
add_align, and revise the comments near the Intermediate/SerializeBamRecords
explanation as well as the duplicate later comment block.
---
Duplicate comments:
In `@tests/integration/test_sort_correctness.rs`:
- Around line 444-453: The oracle in assert_name_order_matches only compares
record_key(r).name, so it can miss per-record differences like swapped mates
within the same name group. Update the comparison to use a per-record projection
that includes the full identity you want to validate, such as record_key(r)
itself or the full RecordBuf, for both fgumi_out and samtools_out, and keep the
count check plus the direct equality assertion so the natural-sort test verifies
exact output identity rather than only qname order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bfbd8558-50df-42f9-b012-dd3b48808cf6
📒 Files selected for processing (75)
.github/workflows/check.yml.gitignoreCHANGELOG.mdCLAUDE.mdbenches/core_functions.rscrates/fgumi-bgzf/src/writer.rscrates/fgumi-consensus/src/base_builder.rscrates/fgumi-consensus/src/codec_caller.rscrates/fgumi-consensus/src/duplex_caller.rscrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/held.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/reorder.rscrates/fgumi-pipeline-core/src/runtime/driver.rscrates/fgumi-pipeline-core/src/runtime/fused.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/stats.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/signal.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/source/read_bam.rscrates/fgumi-raw-bam/src/sort.rscrates/fgumi-raw-bam/src/tags.rscrates/fgumi-sort-cli/Cargo.tomlcrates/fgumi-sort-cli/src/sort.rscrates/fgumi-sort/src/worker_pool.rscrates/fgumi-umi/src/assigner.rsdocs/design/deterministic-mi-numbering.mddocs/simulate-cli.mddocs/src/guide/best-practices.mddocs/src/guide/performance-tuning.mdsrc/lib/aligner.rssrc/lib/commands/codec.rssrc/lib/commands/common.rssrc/lib/commands/correct.rssrc/lib/commands/downsample.rssrc/lib/commands/filter.rssrc/lib/commands/runall.rssrc/lib/commands/simulate/mod.rssrc/lib/commands/zipper.rssrc/lib/fastq_parse.rssrc/lib/mi_group.rssrc/lib/pipeline/chains/build.rssrc/lib/pipeline/chains/builder.rssrc/lib/pipeline/chains/validate.rssrc/lib/pipeline/steps/align_and_merge.rssrc/lib/pipeline/steps/coalesce.rssrc/lib/pipeline/steps/correct/mod.rssrc/lib/pipeline/steps/correct/tests.rssrc/lib/pipeline/steps/extract.rssrc/lib/pipeline/steps/process.rssrc/lib/pipeline/steps/roundtrip.rssrc/lib/pipeline/steps/serialize_processed.rssrc/lib/pipeline/steps/source/pair_fastq.rssrc/lib/pipeline/steps/source/read_fastq.rssrc/lib/umi/parallel_assigner.rssrc/lib/validation.rstests/integration/helpers/assertions.rstests/integration/helpers/bam_generator.rstests/integration/helpers/cli_runner.rstests/integration/helpers/parity.rstests/integration/main.rstests/integration/test_chain_build.rstests/integration/test_codec_command.rstests/integration/test_dedup_command.rstests/integration/test_runall_parity.rstests/integration/test_simulate_aligner.rstests/integration/test_sort_correctness.rstests/integration/test_sort_write_index.rstests/integration/test_streaming_input.rs
💤 Files with no reviewable changes (2)
- crates/fgumi-sort-cli/Cargo.toml
- tests/integration/helpers/assertions.rs
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/umi/parallel_assigner.rs`:
- Around line 1525-1561: The new invalid-UMI adjacency test only compares
AdjacencyUmiAssigner and ParallelAdjacencyAssigner against each other, so it can
miss a shared regression from fgbio. Update the adjacency invalid-UMI test
around adjacency_invalid_umi_maps_to_none_in_both_paths to include a
programmatic fgbio oracle for the mixed and all-invalid inputs, or reuse an
existing adjacency baseline that already covers these cases. Use the existing
assign() calls and MoleculeId assertions to verify both implementations still
match the expected fgbio behavior, especially that invalid UMIs map to
MoleculeId::None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c428f68e-4797-4d49-b9d2-2dace5a5df35
📒 Files selected for processing (14)
benches/core_functions.rscrates/fgumi-consensus/src/base_builder.rscrates/fgumi-consensus/src/codec_caller.rscrates/fgumi-pipeline-core/src/runtime/driver.rssrc/lib/aligner.rssrc/lib/commands/downsample.rssrc/lib/commands/runall.rssrc/lib/pipeline/chains/builder.rssrc/lib/pipeline/steps/coalesce.rssrc/lib/umi/parallel_assigner.rstests/integration/test_dedup_command.rstests/integration/test_runall_parity.rstests/integration/test_sort_correctness.rstests/integration/test_streaming_input.rs
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/pipeline/steps/coalesce.rs (1)
250-259: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFixture can never exercise the "threshold + one block" overshoot the test claims to pin.
COALESCE_INPUT_BLOCK_BYTES(1000) evenly dividesCOALESCE_THRESHOLD_BYTES(8000). Step 3's threshold check fires only whenpending.len() >= threshold_bytes, and since every increment is exactly 1000,pendingcan only ever land exactly on 8000 — never on 8001–8999. So every emitted block is exactlyCOALESCE_THRESHOLD_BYTES, and thesz <= per_block_ceilingcheck at line 437 never actually exercises the tighter "at most one input block past threshold" bound the comment at lines 376-381 describes; only a much grosser regression (a fullMAX_BATCHES_PER_LOCKbatch) would be caught here.Use a block size that does not evenly divide the threshold (e.g. keep the block count/threshold ratio non-integral) so at least one flush genuinely overshoots
COALESCE_THRESHOLD_BYTESby a partial block.🐛 Proposed fix — make the last accumulation cycle overshoot by a partial block
-const COALESCE_INPUT_BLOCK_BYTES: usize = 1000; -const COALESCE_THRESHOLD_BYTES: usize = 8 * COALESCE_INPUT_BLOCK_BYTES; -const COALESCE_INPUT_BLOCKS: usize = 64 * 8; +const COALESCE_INPUT_BLOCK_BYTES: usize = 1000; +// Not a multiple of the block size: this forces `pending` to cross the +// threshold mid-block on most flushes, so `sz` genuinely exceeds +// `COALESCE_THRESHOLD_BYTES` and the `per_block_ceiling` bound is exercised +// for real, not just trivially satisfied by an exact multiple. +const COALESCE_THRESHOLD_BYTES: usize = 8 * COALESCE_INPUT_BLOCK_BYTES + 1; +const COALESCE_INPUT_BLOCKS: usize = 64 * 8;Also applies to: 364-451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/pipeline/steps/coalesce.rs` around lines 250 - 259, The coalescing fixture in coalesce.rs never hits the intended “threshold + one block” overshoot because COALESCE_INPUT_BLOCK_BYTES divides COALESCE_THRESHOLD_BYTES exactly, so the threshold check in the coalescing path only ever flushes at an exact boundary. Adjust the test setup around COALESCE_INPUT_BLOCK_BYTES, COALESCE_THRESHOLD_BYTES, and COALESCE_INPUT_BLOCKS so the input block size does not evenly divide the threshold, ensuring at least one flush in the coalescer overshoots by a partial block and the per-block ceiling assertion actually verifies the intended bound.
♻️ Duplicate comments (1)
src/lib/commands/runall.rs (1)
429-439: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale
ref_is_aligner_onlyhelp reference.Line 438 names an internal exemption that no longer exists; reword this as “the derived-chain align/zipper exemption in
execute”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/commands/runall.rs` around lines 429 - 439, The help text for the reference FASTA argument still mentions the stale internal `ref_is_aligner_only` exemption. Update the documentation in `runall.rs` so it refers instead to the derived-chain align/zipper exemption in `execute`, while keeping the rest of the conditions described by the `validate_align_and_merge` and `Stage::Align` logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/test_dedup_command.rs`:
- Around line 211-225: The surviving-pair assertion in the dedup integration
test is too weak because it only checks “not first” for the second mate, so a
malformed record could still pass. Update the projection and/or final assertion
in the test around surviving records to require flags.is_last_segment() for the
R2 mate (or explicitly assert one first and one last) alongside the existing
position and reverse-complement checks, using the rec.flags() projection in
test_dedup_command.rs.
---
Outside diff comments:
In `@src/lib/pipeline/steps/coalesce.rs`:
- Around line 250-259: The coalescing fixture in coalesce.rs never hits the
intended “threshold + one block” overshoot because COALESCE_INPUT_BLOCK_BYTES
divides COALESCE_THRESHOLD_BYTES exactly, so the threshold check in the
coalescing path only ever flushes at an exact boundary. Adjust the test setup
around COALESCE_INPUT_BLOCK_BYTES, COALESCE_THRESHOLD_BYTES, and
COALESCE_INPUT_BLOCKS so the input block size does not evenly divide the
threshold, ensuring at least one flush in the coalescer overshoots by a partial
block and the per-block ceiling assertion actually verifies the intended bound.
---
Duplicate comments:
In `@src/lib/commands/runall.rs`:
- Around line 429-439: The help text for the reference FASTA argument still
mentions the stale internal `ref_is_aligner_only` exemption. Update the
documentation in `runall.rs` so it refers instead to the derived-chain
align/zipper exemption in `execute`, while keeping the rest of the conditions
described by the `validate_align_and_merge` and `Stage::Align` logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f7192c1-80dc-44c9-ace7-df5a975eb76e
📒 Files selected for processing (14)
benches/core_functions.rscrates/fgumi-consensus/src/base_builder.rscrates/fgumi-consensus/src/codec_caller.rscrates/fgumi-pipeline-core/src/runtime/driver.rssrc/lib/aligner.rssrc/lib/commands/downsample.rssrc/lib/commands/runall.rssrc/lib/pipeline/chains/builder.rssrc/lib/pipeline/steps/coalesce.rssrc/lib/umi/parallel_assigner.rstests/integration/test_dedup_command.rstests/integration/test_runall_parity.rstests/integration/test_sort_correctness.rstests/integration/test_streaming_input.rs
Reject non-finite downsample fraction; fix parallel-assigner invalid-UMI parity (invalid reads -> None in both paths); log aligner try_wait errors; mark the terminal extract chain tail as SerializedBytes; strengthen oracles for dedup, coalesce, sort, runall, streaming, and the driver round-robin path.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack: 6 / 6 · base:
nh/runall-fix-5-runall-consensusCross-cutting polish on top of the stack: additional correctness fixes surfaced during self-review, a documentation sweep, and expanded test coverage.
MoleculeId::Nonefor invalid UMIs in both the sequential and parallel adjacency assigners; bgzf buffer-pool cap; log alignertry_waiterrors; mark the terminal extract chain tail asSerializedBytes.Summary by CodeRabbit
New Features
runallstage routing documentation and validation for--ref/--methylation-mode, including clearer consensus-start behavior and hazard warnings.Bug Fixes
Tests
runall.Documentation
runall/simulation help text.