Repository navigation
docs+ci: fix broken intra-doc links and gate rustdoc in CI - #574
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughRustdoc-only changes correct references and clarify raw BAM documentation. CI adds a locked workspace documentation job with ChangesRustdoc validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #574 +/- ##
==========================================
+ Coverage 92.96% 93.03% +0.06%
==========================================
Files 167 167
Lines 103266 103266
==========================================
+ Hits 96000 96071 +71
+ Misses 7266 7195 -71 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…non-code The `//!` module-doc walkthroughs in base_builder/caller/codec_caller/duplex_caller were fenced ```rust,ignore` — rendered as Rust but never compiled — and had rotted: they imported from the old monolith path `fgumi_lib::consensus::...` (the crate is now `fgumi_consensus`) and referenced the since-renamed `vanilla_consensus_caller` module (now `vanilla_caller`). So they showed readers import paths that don't exist, with nothing to catch it. These are genuine teaching sketches — undefined context vars (`reads`, `options`, `output`, ...), elided bodies, trait-shape skeletons — so they can't be compiled without gutting their clarity. Rather than leave them masquerading as verified Rust: - correct the crate paths (`fgumi_consensus::...`) and the `vanilla_caller` rename, including the prose "See Also" cross-references, so what's shown is accurate; and - change the fences to ```text`, honestly declaring them as illustrative rather than as Rust the doctest/rustdoc gates would be expected to check. Verified: introduces zero new rustdoc warnings vs main (the crate's remaining broken intra-doc links are fixed by the sibling PR #574).
|
@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: 2
🤖 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 165-166: Update the actions/checkout step in the docs job to set
persist-credentials to false, preventing Cargo build scripts and proc macros
from accessing the checkout token through Git configuration.
In `@crates/fgumi-raw-bam/src/noodles_compat.rs`:
- Around line 145-148: Update the documentation for the RawRecord encoding
method to qualify the scratch-buffer allocation claim: state that reusing
self.scratch avoids allocation when its capacity is sufficient, while encoding
larger records may allocate; retain the statement that the returned RawRecord
owns its bytes.
🪄 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: 8f4a5ebf-e9f2-4b50-aa8c-35ac39244fb5
📒 Files selected for processing (12)
.cargo/config.toml.github/workflows/check.ymlcrates/fgumi-consensus/src/codec_caller.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/indexed_reader.rscrates/fgumi-raw-bam/src/noodles_compat.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/src/tags.rscrates/fgumi-sort/src/external.rssrc/lib/commands/simulate/common.rssrc/lib/per_thread_accumulator.rssrc/lib/reference.rs
…non-code The `//!` module-doc walkthroughs in base_builder/caller/codec_caller/duplex_caller were fenced ```rust,ignore` — rendered as Rust but never compiled — and had rotted: they imported from the old monolith path `fgumi_lib::consensus::...` (the crate is now `fgumi_consensus`) and referenced the since-renamed `vanilla_consensus_caller` module (now `vanilla_caller`). So they showed readers import paths that don't exist, with nothing to catch it. These are genuine teaching sketches — undefined context vars (`reads`, `options`, `output`, ...), elided bodies, trait-shape skeletons — so they can't be compiled without gutting their clarity. Rather than leave them masquerading as verified Rust: - correct the crate paths (`fgumi_consensus::...`) and the `vanilla_caller` rename, including the prose "See Also" cross-references, so what's shown is accurate; and - change the fences to ```text`, honestly declaring them as illustrative rather than as Rust the doctest/rustdoc gates would be expected to check. Verified: introduces zero new rustdoc warnings vs main (the crate's remaining broken intra-doc links are fixed by the sibling PR #574).
…non-code The `//!` module-doc walkthroughs in base_builder/caller/codec_caller/duplex_caller were fenced ```rust,ignore` — rendered as Rust but never compiled — and had rotted: they imported from the old monolith path `fgumi_lib::consensus::...` (the crate is now `fgumi_consensus`) and referenced the since-renamed `vanilla_consensus_caller` module (now `vanilla_caller`). So they showed readers import paths that don't exist, with nothing to catch it. These are genuine teaching sketches — undefined context vars (`reads`, `options`, `output`, ...), elided bodies, trait-shape skeletons — so they can't be compiled without gutting their clarity. Rather than leave them masquerading as verified Rust: - correct the crate paths (`fgumi_consensus::...`) and the `vanilla_caller` rename, including the prose "See Also" cross-references, so what's shown is accurate; and - change the fences to ```text`, honestly declaring them as illustrative rather than as Rust the doctest/rustdoc gates would be expected to check. Verified: introduces zero new rustdoc warnings vs main (the crate's remaining broken intra-doc links are fixed by the sibling PR #574).
`cargo doc` with RUSTDOCFLAGS="-D warnings" failed on ~15 unresolved or
private-item intra-doc links that had accumulated because nothing in CI builds
the docs (nextest does not, and there was no docs job). Repoint or demote each:
- fgumi-raw-bam:
- `[`RawRecord`]` in noodles_compat -> `[`RawRecord`](crate::RawRecord)` (not
in that module's scope; the module uses `crate::RawRecord`).
- `[`encode`]` / `[`encode_into`]` -> qualified with `RecordBufEncoder::`.
- `[`query`]` / `[`read_header`]` -> `Self::` (same impl).
- `[`MemoryEstimate`]` -> code span (the trait lives in a downstream crate).
- `[`append_int_tag`]` (pub(crate)) and `[`BAM_CIGAR_TYPE`]` (private const)
-> code spans; they are not part of the public API rustdoc documents.
- fgumi-sort: a second `[`SpillCodec::Zstd`]` occurrence in a doc comment that
lacked the reference-link definition the first one has -> inline path.
- fgumi-consensus: `[`rejected_reads`]` / `[`take_rejected_reads`]` -> `Self::`.
- fgumi (main): the `simulate` model links -> `crate::simulate::...`; the
`[`fetch`]` links in reference.rs -> `Self::fetch`; and the private
`SLOT_COUNTER` / `THREAD_SLOT` links -> code spans.
`RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --features
compare,simulate,profile-adjacency` now builds clean.
…non-code (#579) The `//!` module-doc walkthroughs in base_builder/caller/codec_caller/duplex_caller were fenced ```rust,ignore` — rendered as Rust but never compiled — and had rotted: they imported from the old monolith path `fgumi_lib::consensus::...` (the crate is now `fgumi_consensus`) and referenced the since-renamed `vanilla_consensus_caller` module (now `vanilla_caller`). So they showed readers import paths that don't exist, with nothing to catch it. These are genuine teaching sketches — undefined context vars (`reads`, `options`, `output`, ...), elided bodies, trait-shape skeletons — so they can't be compiled without gutting their clarity. Rather than leave them masquerading as verified Rust: - correct the crate paths (`fgumi_consensus::...`) and the `vanilla_caller` rename, including the prose "See Also" cross-references, so what's shown is accurate; and - change the fences to ```text`, honestly declaring them as illustrative rather than as Rust the doctest/rustdoc gates would be expected to check. Verified: introduces zero new rustdoc warnings vs main (the crate's remaining broken intra-doc links are fixed by the sibling PR #574).
|
@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)
crates/fgumi-consensus/src/codec_caller.rs (1)
2903-2990: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMake the HDD fixture produce a real overlap disagreement.
duplex_disagreement_fixture()uses matching FR sequences, whilebuild_duplex_consensus_from_paddedonly incrementsduplex_disagreementsfor differing bases when both strands have data; lowercase-nsingle-strand tails do not increment it. Both new tests therefore reachpanic!("...should produce an error")instead of the typed HDD path. Inject a deterministic base mismatch inside the overlap (or adjust production accounting to the intended fgbio rule) and assert the fixture has a nonzero disagreement count.🤖 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 `@crates/fgumi-consensus/src/codec_caller.rs` around lines 2903 - 2990, Update duplex_disagreement_fixture so its FR sequences contain a deterministic mismatching base within the overlapping region, ensuring build_duplex_consensus_from_padded increments duplex_disagreements; verify the fixture produces a nonzero disagreement count before the tests exercise the HDD error path.
🤖 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-209: Add job-level permissions with contents: read to both the
docs job at .github/workflows/check.yml lines 197-209 and the MSRV lockstep job
at .github/workflows/check.yml lines 172-178. Keep the existing
persist-credentials setting and job steps unchanged.
---
Outside diff comments:
In `@crates/fgumi-consensus/src/codec_caller.rs`:
- Around line 2903-2990: Update duplex_disagreement_fixture so its FR sequences
contain a deterministic mismatching base within the overlapping region, ensuring
build_duplex_consensus_from_padded increments duplex_disagreements; verify the
fixture produces a nonzero disagreement count before the tests exercise the HDD
error path.
🪄 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: 812e2435-17fa-4102-8252-8bbf43f2c6f4
📒 Files selected for processing (13)
.cargo/config.toml.github/workflows/check.ymlcrates/fgumi-consensus/src/codec_caller.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/indexed_reader.rscrates/fgumi-raw-bam/src/noodles_compat.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/src/tags.rscrates/fgumi-sort/src/external.rssrc/lib/commands/simulate/common.rssrc/lib/fastq_parse.rssrc/lib/per_thread_accumulator.rssrc/lib/reference.rs
Nothing in CI builds the documentation: the test/coverage jobs use nextest (which does not run doctests), and there was no docs job. That let unresolved intra-doc links accumulate silently (fixed in the preceding commit). Add a `ci-doc` alias (`cargo doc --no-deps --workspace` with the standard feature set) and a `docs` job that runs it with RUSTDOCFLAGS="-D warnings", so any rustdoc warning -- broken intra-doc link, link to a private item, etc. -- now fails the build at PR time.
|
Addressed CodeRabbit feedback from the latest review: Token permissions ( Outside-diff finding on |
This branch was previously deployed
1 participant
Problem
RUSTDOCFLAGS="-D warnings" cargo docfails on ~15 unresolved or private-item intra-doc links spread across four crates. They accumulated silently because nothing in CI builds the docs — the test/coverage jobs runcargo nextest(which doesn't run doctests), and there was no docs job. Same root cause as the doctest gap fixed in #573, one layer up (doc links rather than doctest code).Changes
1. Fix the broken intra-doc links (
docs:commit) — repoint or demote each:[RawRecord]→[RawRecord](crate::RawRecord);encode/encode_into→RecordBufEncoder::;query/read_header→Self::;MemoryEstimate(downstream trait),append_int_tag(pub(crate)),BAM_CIGAR_TYPE(private const) → code spans.[SpillCodec::Zstd]occurrence in a doc comment lacking the reference-link definition → inline path.rejected_reads/take_rejected_reads→Self::.simulatemodel links →crate::simulate::…;[fetch]→Self::fetch; privateSLOT_COUNTER/THREAD_SLOT→ code spans.2. Gate rustdoc in CI (
ci:commit) — add aci-docalias and adocsjob that runscargo doc --no-deps --workspacewithRUSTDOCFLAGS="-D warnings", so any rustdoc warning (broken link, private-item link) fails the build going forward.Verification
RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --features compare,simulate,profile-adjacency→ clean.cargo ci-fmt,cargo ci-lint,cargo ci-tag-literals→ clean.Note
This started as a broader CI-hardening pass. The MSRV half (correcting the declared
rust-version) is intentionally not here: the declared1.87.0is false (depsnoodles/wideforce ≥1.89), but bumping it to the real floor unlocks ~85 MSRV-gatedcollapsible_if(let-chain) clippy warnings across the workspace that-D warningswould turn into errors. That needs its own decision (adopt let-chains vs.allowthe lint) and PR, so it's decoupled to keep this one focused and green.Summary by CodeRabbit
Documentation
Quality Improvements