refactor(features)!: collapse consensus features, repair reduced-feature CI, and structural fixes (1/6) - #458
Merged
Merged
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR consolidates consensus feature gating under ChangesConsensus feature consolidation
Finalize hook and BAI sidecar flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat-runall #458 +/- ##
==============================================
Coverage ? 93.96%
==============================================
Files ? 109
Lines ? 48466
Branches ? 0
==============================================
Hits ? 45540
Misses ? 2926
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/integration/test_bgzf_eof.rs (1)
638-670: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the rejected family identity, not just EOF.
These setups rely on
--min-reads 2rejecting a single paired-end template, but the test only proves both BAMs were closed and the rejects header was copied. A regression that miscounts the pair as two reads, or routes the wrong family to the wrong file, still passes. Read both BAMs back and assert therejectfamily is present inrejects.bamand absent from the kept output.Based on learnings,
--min-readshere should be validated at the template level, and as per path instructions, tests under**/tests/**/*.rsshould assert the stated contract instead of a weaker end-to-end side effect.Also applies to: 723-763
🤖 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_bgzf_eof.rs` around lines 638 - 670, The test in test_simplex_rejects_has_bgzf_eof only checks EOF and header copying, so it can miss misrouted or miscounted families. Update this and the related reject-path test to read both BAM outputs back and assert the reject family from create_rejected_family/write_grouped_bam is present in rejects_bam and absent from the kept output, using the Simplex execute path and the min-reads contract as the source of truth.Sources: Path instructions, Learnings
src/lib/commands/filter.rs (1)
481-488: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winChange the rebuild hint to
consensus.This fallback still tells users to rebuild with
--features simplex, but the root crate no longer exposes that feature. In a--no-default-featuresbuild, following this message now leads straight into another build failure instead of the intended recovery path.Suggested fix
- "reference-dependent methylation filters require building fgumi with the `simplex` feature" + "reference-dependent methylation filters require building fgumi with the `consensus` feature"🤖 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/filter.rs` around lines 481 - 488, The rebuild hint in the `filter.rs` fallback is stale and still points users to `simplex`, which the root crate no longer exposes. Update the `bail!` message in the `ref_base_map` / `require_strand_methylation_agreement` path to tell users to rebuild with the `consensus` feature instead, so the guidance matches the current feature set.src/lib/commands/common.rs (1)
775-788: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmit the deprecation warning on
fgumi sorttoo.Line 779 only helps builder paths that call
warn_unwired_pipeline_flags(), butChainBuilder::add_sortnever does.fgumi sort --scheduler ...is therefore accepted and silently ignored instead of degrading with the promised warning. Move this warning closer to CLI parsing, or invoke the helper from both sort branches as well.🤖 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/common.rs` around lines 775 - 788, The deprecation warning for --scheduler is only emitted inside warn_unwired_pipeline_flags(), so fgumi sort can still accept and ignore it silently through ChainBuilder::add_sort. Update the sort command path to call the same warning helper, or move the warning into the CLI parsing layer so both builder and sort branches share it; use warn_unwired_pipeline_flags and ChainBuilder::add_sort as the key places to wire this in.
🤖 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 @.github/workflows/check.yml:
- Around line 197-198: The new Checkout code step currently leaves the GitHub
token persisted in git config, which exposes credentials to PR code executed by
cargo check. Update the actions/checkout usage in this workflow step to set
persist-credentials to false so untrusted build scripts and proc macros cannot
access GITHUB_TOKEN.
In `@crates/xtask/src/generate_tools.rs`:
- Around line 72-75: `collect_commands()` is missing the `runall` command, so
consensus builds can expose it in `main.rs` without actually
discovering/registering it. Update the command import/register list in
`generate_tools.rs` to include `runall` alongside the other consensus commands,
and add an explicit assertion that `runall` is present in the collected command
names instead of relying only on the weaker `names.len()` check. Use the
existing `collect_commands()` flow and the consensus-gated command set as the
place to verify this.
In `@src/lib/pipeline/chains/builder.rs`:
- Around line 2459-2484: The reduced-build rebuild hint is inconsistent: the
consensus stubs in builder.rs already point to `--features consensus`, but the
remaining fallback in filter.rs still tells users to rebuild with `--features
simplex`. Update the bailout message in the filter command’s reduced-build path
to match the new consensus feature name so all reduced-build paths give the same
guidance.
In `@src/lib/pipeline/chains/commands/sort.rs`:
- Around line 43-45: The chain finalize list is incorrectly re-exporting
IndexBamFinalizeHook, which lets built pipelines drain and publish a .bai even
after a failed run. Update the sort chain wiring in the re-export around
build_sort_step/log_sort_start so ChainBuilder::add_sort only includes the
summary finalize hook, and move index writing to a success-gated post-run path
like the standalone sort flow after run_result?; keep IndexBamFinalizeHook out
of the always-drain finalize vec.
In `@src/lib/pipeline/chains/validate.rs`:
- Around line 176-181: The `validate_stage_opts_present()` match arm for
`Stage::Duplex`, `Stage::Codec`, and `Stage::Simplex` under `#[cfg(not(feature =
"consensus"))]` should not return `false`, because that causes `build_for()` to
surface a generic missing-options error instead of the intended feature-disabled
failure. Update this branch to explicitly bail with an error stating that the
`consensus` feature is required, keeping the behavior consistent with the
non-consensus stubs and the `build_for()` validation flow.
In `@tests/integration/main.rs`:
- Around line 34-40: The current gating in the integration test module is too
broad because test_runall_parity still includes non-consensus parity coverage
for always-supported runall paths. Update test_runall_parity so only the
consensus-specific cases remain under the consensus cfg, and keep the
sort/group/zipper/correct parity oracles available outside that gate, either by
moving them into an always-on module or applying inner #[cfg(feature =
"consensus")] only to the consensus-only tests. Use the test_runall_parity and
test_simplex_command module boundaries to separate reduced-feature coverage from
consensus-only coverage.
---
Outside diff comments:
In `@src/lib/commands/common.rs`:
- Around line 775-788: The deprecation warning for --scheduler is only emitted
inside warn_unwired_pipeline_flags(), so fgumi sort can still accept and ignore
it silently through ChainBuilder::add_sort. Update the sort command path to call
the same warning helper, or move the warning into the CLI parsing layer so both
builder and sort branches share it; use warn_unwired_pipeline_flags and
ChainBuilder::add_sort as the key places to wire this in.
In `@src/lib/commands/filter.rs`:
- Around line 481-488: The rebuild hint in the `filter.rs` fallback is stale and
still points users to `simplex`, which the root crate no longer exposes. Update
the `bail!` message in the `ref_base_map` /
`require_strand_methylation_agreement` path to tell users to rebuild with the
`consensus` feature instead, so the guidance matches the current feature set.
In `@tests/integration/test_bgzf_eof.rs`:
- Around line 638-670: The test in test_simplex_rejects_has_bgzf_eof only checks
EOF and header copying, so it can miss misrouted or miscounted families. Update
this and the related reject-path test to read both BAM outputs back and assert
the reject family from create_rejected_family/write_grouped_bam is present in
rejects_bam and absent from the kept output, using the Simplex execute path and
the min-reads contract as the source of truth.
🪄 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: 2d952c40-37e9-43c0-b36f-abc5b4b65ced
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (30)
.github/workflows/check.ymlCLAUDE.mdCargo.tomlcrates/fgumi-bam-io/src/lib.rscrates/fgumi-bam-io/src/writer.rscrates/fgumi-pipeline-core/Cargo.tomlcrates/fgumi-pipeline-core/src/finalize.rscrates/fgumi-pipeline-core/src/lib.rscrates/fgumi-sort-cli/src/chains.rscrates/fgumi-sort-cli/src/sort.rscrates/xtask/Cargo.tomlcrates/xtask/src/generate_tools.rsdocs/DEVELOPING.mdsrc/lib/commands/common.rssrc/lib/commands/consensus_runner.rssrc/lib/commands/filter.rssrc/lib/commands/mod.rssrc/lib/consensus/mod.rssrc/lib/mod.rssrc/lib/pipeline/chains/builder.rssrc/lib/pipeline/chains/commands/mod.rssrc/lib/pipeline/chains/commands/sort.rssrc/lib/pipeline/chains/finalize.rssrc/lib/pipeline/chains/options_bag.rssrc/lib/pipeline/chains/validate.rssrc/lib/reference.rssrc/main.rstests/integration/main.rstests/integration/test_bgzf_eof.rstests/integration/test_streaming_input.rs
💤 Files with no reviewable changes (1)
- docs/DEVELOPING.md
… flag and repair reduced-feature CI Collapse the umbrella crate's `simplex`/`duplex`/`codec` features into a single default-on `consensus` feature. The shipped binary always bundles all three consensus callers, so the per-codec split of the umbrella crate bought nothing while letting the always-compiled chain/runall layer reference a half-present consensus surface: every reduced-feature build of `fgumi` failed to compile (X1-001, escalating S5d-001), and the `--no-default-features` cleanliness promised by #314 had regressed. The granular `simplex`/`duplex`/`codec` features are preserved one level down in `fgumi-consensus` for embedders that want a reduced library build. - Co-gate the always-compiled consensus references behind `consensus`: the chain command modules (codec/duplex/simplex), the option-bag slots, the builder `add_simplex`/`add_duplex`/`add_codec` methods and their helpers (`finish_consensus_tail`, `wire_consensus_rejects_branch`, `templates_to_mi_step`, `FuseState`), and the validator option-presence arms. The builder keeps `bail!` stubs for the three consensus stages so `add_stage` stays exhaustive when consensus is off. - Collapse the feature-gated-command hint to one `consensus` hint and add the missing `runall` entry (S5d-009); drop the dead `stress-tests` feature (X3-004). - Repair the reduced-feature CI so the regression cannot recur: give xtask its own default-on `consensus` feature mapping to `fgumi/consensus` (gating the consensus command doc-gen imports) plus `default-features = false`, so the workspace `--no-default-features` check stops masking the breakage via feature-unification (X1-002); and add a dedicated `fgumi-feature-check` job covering the reduced, `--features consensus`, and default combos (X1-003). The repaired CI fails at the pre-collapse tip and passes after, closing the masking (X1-002) and coverage gap (X1-003). Docs updated (CLAUDE.md Features); CHANGELOG #314 wording deferred to the docs pass. BREAKING CHANGE: the `simplex`, `duplex`, and `codec` cargo features of the `fgumi` crate are removed; build with `consensus` (default-on) instead.
The pluggable scheduler-strategy mechanism was removed in the issue #330 typed-step dispatch migration, so feat-runall dropped the (already hidden) `--scheduler` flag from the shared `SchedulerOptions`. Scripts still passing `--scheduler <strategy>` then hard-failed with a clap "unexpected argument" error. Re-add it as a hidden, deprecated no-op that accepts any value and logs a deprecation warning on the multi-threaded dispatch path, so legacy invocations degrade gracefully — mirroring the `--queue-memory*` alias precedent. The value is ignored; the typed-step model has no scheduler strategy to select. (AUDIT-002)
…e sort hooks
The `FinalizeHook` trait lived only in the umbrella `fgumi` crate, so
`fgumi-sort-cli` — which builds its `fgumi sort` pipeline directly on
`fgumi-pipeline-core` without the umbrella's `ChainBuilder` — could not
implement it. As a result `SortFinalizeHook` and `IndexBamFinalizeHook` were
defined twice with byte-identical bodies: an inherent-method copy in sort-cli
and a `impl FinalizeHook` copy in the umbrella (X1-005). The duplicated
`IndexBamFinalizeHook` body also carried the `with_extension("bam.bai")` path
footgun in both copies (S3-006 / S5b2-008), forcing any fix to be applied
twice.
Hoist the bare trait into `fgumi-pipeline-core` (its natural home — it owns
`Pipeline`), re-export it from the umbrella's `chains::finalize` so the
canonical `crate::pipeline::chains::FinalizeHook` path and all ~13 in-crate
impls are unchanged. sort-cli's two hooks now `impl FinalizeHook` directly and
the umbrella re-exports them, deleting both umbrella struct twins and the
duplicated BAI round-trip test. `BuiltPipeline`, `drain_finalize`, and the
built-in stats/timing hooks stay umbrella-side (they reference umbrella-only
types).
sort-cli's standalone `Sort::execute` updates its two hook call sites to invoke
the trait (`Box::new(hook).finalize()`), preserving the existing semantics that
the summary hook drains even on the error path while the pipeline-run error
takes precedence.
This is the structural prerequisite for Phase 4, which moves the single
remaining `IndexBamFinalizeHook` body into `fgumi-bam-io::write_bai_sidecar` and
fixes the path footgun once.
nh13
force-pushed
the
nh/runall-fix-1-foundation
branch
from
June 25, 2026 01:17
5b4d0ad to
5900b0d
Compare
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 `@crates/fgumi-sort-cli/src/chains.rs`:
- Around line 74-76: Update the docs in the `chains.rs` comment near the BAM
index-writing flow to match the current `write_bai_sidecar` behavior. Replace
the stale `<output>.bam.bai`/`fgumi_bam_io::write_bai_index` wording with
language that reflects appending `.bai` to the full output path, so
`build_chain` and the sidecar contract are described correctly for non-`.bam`
outputs.
🪄 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: 97ab60da-6f43-4c3a-8494-f757777ee74d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (32)
.github/workflows/check.ymlCLAUDE.mdCargo.tomlcrates/fgumi-bam-io/src/lib.rscrates/fgumi-bam-io/src/writer.rscrates/fgumi-pipeline-core/Cargo.tomlcrates/fgumi-pipeline-core/src/finalize.rscrates/fgumi-pipeline-core/src/lib.rscrates/fgumi-sort-cli/src/chains.rscrates/fgumi-sort-cli/src/sort.rscrates/xtask/Cargo.tomlcrates/xtask/src/generate_tools.rsdocs/DEVELOPING.mdsrc/lib/commands/common.rssrc/lib/commands/consensus_runner.rssrc/lib/commands/filter.rssrc/lib/commands/mod.rssrc/lib/consensus/mod.rssrc/lib/mod.rssrc/lib/pipeline/chains/builder.rssrc/lib/pipeline/chains/commands/mod.rssrc/lib/pipeline/chains/commands/sort.rssrc/lib/pipeline/chains/finalize.rssrc/lib/pipeline/chains/options_bag.rssrc/lib/pipeline/chains/validate.rssrc/lib/reference.rssrc/main.rstests/integration/helpers/assertions.rstests/integration/main.rstests/integration/test_bgzf_eof.rstests/integration/test_runall_parity.rstests/integration/test_streaming_input.rs
💤 Files with no reviewable changes (1)
- docs/DEVELOPING.md
`IndexBamFinalizeHook` derived its sidecar path with
`output_path.with_extension("bam.bai")`, which *replaces* whatever follows the
last `.` rather than appending. It produced the correct `foo.bam` →
`foo.bam.bai` only because the input was assumed to always end in `.bam`; any
other output path was silently mis-named (`foo` → `foo.bam.bai`,
`foo.sorted` → `foo.bam.bai` instead of `foo.bai` / `foo.sorted.bai`)
— S3-006 / S5b2-008.
Move the index-and-write body into `fgumi-bam-io::write_bai_sidecar`, with the
path derived by a new `bai_sidecar_path` that appends `.bai` to the full path
(samtools convention), correct for any path. The single `IndexBamFinalizeHook`
(de-duplicated in the preceding trait-hoist commit) collapses to one call plus
its log lines, so the fix lives in exactly one place (X1-004).
`bai_sidecar_path` is a pure function unit-tested over `.bam`, extensionless,
and non-`.bam`-extension paths; the sort-cli hook smoke test continues to
round-trip a real BAM through `write_bai_sidecar`. No behavior change on the
common `*.bam` path.
nh13
force-pushed
the
nh/runall-fix-1-foundation
branch
from
June 25, 2026 01:36
5900b0d to
12ef53a
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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: 1 / 6 · base:
feat-runallFoundation layer for the feat-runall fix series. Structural changes everything else builds on.
refactor(features)!— collapse the umbrellasimplex/duplex/codecfeatures into a single default-onconsensusflag (the granular features still live one level down infgumi-consensusfor reduced-library builds).--no-default-featuresnow cleanly drops all consensus code and still compiles; a newfgumi-feature-checkCI job exercises the reduced-feature build so chain/runall code can never silently reference a gated module again.fix(cli)— accept the removed--schedulerflag as a hidden, deprecated no-op (warns and ignores) so existing invocations don't hard-fail.refactor(pipeline-core)— hoist theFinalizeHooktrait intofgumi-pipeline-coreso the sort hooks are single-sourced.fix(bam-io)— fix BAI sidecar path naming for non-.bam/extensionless outputs (append.bai, samtools convention).Summary by CodeRabbit
<bam>.bainaming), used automatically by the sort/index finalize flow.--scheduleris accepted as a no-op.