fix(anvil): restrict scoped doctests to library packages - #177
Evgenii (Vaiz) wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved blocking issues were identified in the final review.
Pull request overview
This PR restricts scoped Anvil doctest runs to packages with library targets while preserving feature coverage and workspace behavior.
Changes:
- Add locked metadata filtering and validation.
- Gate internal
cargo-gamma-libdoctests. - Update recipes, tests, snapshots, documentation, and checksums.
File summaries
| File | Description |
|---|---|
justfiles/anvil/checks/doc-test.just |
Adds scoped doctest filtering. |
crates/cargo-gamma-lib/src/cfg/mod.rs |
Gates internal doctest examples. |
crates/cargo-gamma-lib/src/cfg/features.rs |
Gates the internal API example. |
crates/cargo-anvil/tests/snapshots/snapshots__local_only.snap |
Refreshes local output. |
crates/cargo-anvil/tests/snapshots/snapshots__github_backend.snap |
Refreshes GitHub output. |
crates/cargo-anvil/tests/snapshots/snapshots__ado_backend.snap |
Refreshes ADO output. |
crates/cargo-anvil/tests/recipe_contracts.rs |
Adds recipe behavior coverage. |
crates/cargo-anvil/templates/justfiles/anvil/checks/doc-test.just |
Updates the recipe source. |
crates/cargo-anvil/docs/design/checks.md |
Documents updated doctest behavior. |
.anvil.lock |
Updates generated checksums. |
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (97.7%) is below the target coverage (100.0%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #177 +/- ##
=======================================
+ Coverage 97.5% 97.7% +0.1%
=======================================
Files 300 295 -5
Lines 69314 65121 -4193
=======================================
- Hits 67647 63674 -3973
+ Misses 1667 1447 -220
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Sergey Galkin (sgalkin)
left a comment
There was a problem hiding this comment.
The scoped doctest filtering correctly implements the PR intent and is safe to approve on code merits. Required Linux checks still need the cross-platform diagnostic assertion fixed and rerun before merge.
There was a problem hiding this comment.
🔵 Needs a closer look
Restrict package lookup to workspace members before approval.
Review details
Suppressed comments (1)
crates/cargo-anvil/templates/justfiles/anvil/checks/doc-test.just:36
- This should iterate only over workspace members before building the lookup. The repository's metadata fixture explicitly models packages that appear in
packagesbut notworkspace_members(recipe_contracts.rs:215-228); with the current loop, a stale or malformed--package external-dep@0.1.0selection is accepted whenever that non-member has a library target and is passed to both doctest runs instead of being rejected as outside the affected workspace.
foreach ($package in $metadata.packages) {
- Files reviewed: 9/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The installer fallback introduces an additional behavior change and can fail when cached binaries already exist.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
crates/cargo-anvil/templates/github/setup-action.yml:126
- On a warm runner, this action restores
~/.cargo/bin/before this step (the cache path is configured earlier in the same action), so a failed release install can leave an older cachedcargo-binstallexecutable in place. The fallback invokescargo installwithout replacing existing binaries; this repository's installer documents that cached binaries can makecargo installfail withbinary already exists in destination(justfiles/anvil/tools.just:432-437). The intended release-failure fallback therefore can fail on the cache-hit path instead of recovering. Install with an overwrite mode or remove the stale executable before the source build.
cargo install --locked --version 1.21.0 cargo-binstall
- Files reviewed: 13/14 changed files
- Comments generated: 1
- Review effort level: Lite
Extract the doctest repairs from #174 without installer changes. Preserve locked metadata, exact package specs, both feature runs, and recipe diagnostics. Gate gamma-lib examples on internals. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The fake-Cargo contracts cover recipe diagnostics. Keep the real-Cargo case focused on the cross-platform exit behavior instead of pwsh's terminal-width-dependent rendering. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fall back to building the same pinned cargo-binstall version when the immutable release action cannot download its binary. Keep the degraded path visible and regenerate the GitHub setup action, snapshot, and Anvil lock. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
0ac9f80 to
0b5a48d
Compare
|
Addressed both suppressed Copilot findings in
|
There was a problem hiding this comment.
🔵 Needs a closer look
The GitHub setup behavior change needs to be removed or explicitly scoped and documented.
Review details
Suppressed comments (1)
crates/cargo-anvil/templates/github/setup-action.yml:116
- This changes the generated GitHub setup behavior by making the release install non-fatal and adding a source-build fallback, but the PR description explicitly says installer behavior and workflow setup remain unchanged while the title scopes this change to doctest selection. Please either remove this unrelated fallback and its generated/docs changes, or update the PR description/scope so this behavior change is intentional and reviewable separately.
id: cargo-binstall
continue-on-error: true
uses: cargo-bins/cargo-binstall@v1.21.0 # pinned by tag: this release is an immutable release (GitHub locks the tag to a commit)
with:
version: "1.21.0"
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.
What this changes
anvil-doc-testnow filters explicit affected-package selections to workspace members with library targets. GitHub setup now builds the pinnedcargo-binstallfrom source if its release action fails.Details
rlib,dylib,cdylib, andstaticlibtargets with version-qualified package specs.--workspace.cargo-gamma-libdoctest bodies on theinternalsfeature that exposes their imports.cargo-binstallrelease action as the fast path. Build version1.21.0with--forceafter a release failure.Effects
cargo metadata --lockedinvocation before the two doctest runs.packages.cargo-binstallrelease download starts a slower source build instead of stopping setup.cargo-binstallexecutable before continuing.cargo-gamma-libexamples compile only wheninternalsexposes their imports.