Skip to content

feat(cargo-anvil): make impact cache inputs injectable - #169

Merged
Evgenii (Vaiz) merged 8 commits into
mainfrom
u/vaiz/2026/09/08/anvil-impact-msrv-flake
Sep 9, 2026
Merged

feat(cargo-anvil): make impact cache inputs injectable#169
Evgenii (Vaiz) merged 8 commits into
mainfrom
u/vaiz/2026/09/08/anvil-impact-msrv-flake

Conversation

@Vaiz

@Vaiz Evgenii (Vaiz) commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.

Add ANVIL_IMPACT_INPUT_DIR to select a read-only cache in consume mode. Cache validation and scope reads use the selected directory. Missing inputs fail instead of falling back to another cache.

The MSRV test reads its committed fixture directly, without copying files or computing impact. Existing assertions for both feature modes and no-MSRV skipping remain.

Unset or empty uses target/anvil/impact/. Compute and off modes ignore the override. Update the design documentation and regenerate the repository recipe and lock checksums.

This removes the setup dependency behind the failure on #162; it does not establish why impact analysis originally returned success without the cache file.

Failure: https://github.com/microsoft/ox-tools/actions/runs/34223762035/job/102053921477

@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.5%. Comparing base (3bf8d54) to head (dc8f970).
⚠️ Report is 2 commits behind head on main.

❌ Your project status has failed because the head coverage (97.5%) 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    #169     +/-   ##
=======================================
- Coverage   97.5%   97.5%   -0.1%     
=======================================
  Files        300     300             
  Lines      68538   68538             
=======================================
- Hits       66876   66875      -1     
- Misses      1662    1663      +1     
Flag Coverage Δ
linux 97.5% <ø> (ø)
linux-arm 97.5% <ø> (+<0.1%) ⬆️
scheduled ?
windows 97.7% <ø> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@Vaiz Evgenii (Vaiz) changed the title test(cargo-anvil): isolate MSRV cache-consumer setup feat(cargo-anvil): make impact cache inputs injectable Sep 8, 2026
@Vaiz
Evgenii (Vaiz) force-pushed the u/vaiz/2026/09/08/anvil-impact-msrv-flake branch from e82e190 to 89f8d4b Compare September 8, 2026 14:07
@Vaiz
Evgenii (Vaiz) marked this pull request as ready for review September 8, 2026 15:31
Copilot AI lite review requested due to automatic review settings September 8, 2026 15:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes are internally consistent across recipe/template/docs and are backed by targeted test updates that exercise the new consume-mode input selection and failure behavior.

Pull request overview

This PR extends cargo-anvil’s impact-scoping “consume” mode to allow selecting an alternate, read-only impact cache directory via ANVIL_IMPACT_INPUT_DIR (e.g., a checked-in fixture), and updates tests/docs/lock artifacts accordingly.

Changes:

  • Add ANVIL_IMPACT_INPUT_DIR support in impact cache validation and _anvil-impact-include reads (consume mode only), and fail loudly when the selected cache is missing.
  • Update MSRV test flow to consume a committed impact-cache fixture directly (no copying / no impact computation) and strengthen assertions around produced scopes and command invocations.
  • Refresh design documentation, generated recipe snapshots, fixtures, and .anvil.lock checksums.
File summaries
File Description
justfiles/anvil/impact.just Add injectable input dir for consume-mode cache validation and include-file reads; improve missing-cache diagnostics.
crates/cargo-anvil/templates/justfiles/anvil/impact.just Mirror the same consume-mode injectable input-dir behavior in the generated template source.
crates/cargo-anvil/docs/design/local.md Document ANVIL_IMPACT_INPUT_DIR behavior, constraints, and consume-mode semantics.
crates/cargo-anvil/tests/impact.rs Adjust MSRV test to inject a checked-in cache; add assertions preventing fallback/local-cache creation; add explicit failure coverage for missing injected input.
crates/cargo-anvil/tests/fixtures/msrv-impact-cache/include_required.txt Add committed fixture include file for required tier.
crates/cargo-anvil/tests/fixtures/msrv-impact-cache/include_modified.txt Add committed fixture include file for modified tier.
crates/cargo-anvil/tests/fixtures/msrv-impact-cache/include_affected.txt Add committed fixture include file for affected tier.
crates/cargo-anvil/tests/snapshots/snapshots__local_only.snap Update generated snapshot to reflect new ANVIL_IMPACT_INPUT_DIR logic and diagnostics.
crates/cargo-anvil/tests/snapshots/snapshots__github_backend.snap Update generated snapshot to reflect new ANVIL_IMPACT_INPUT_DIR logic and diagnostics.
crates/cargo-anvil/tests/snapshots/snapshots__ado_backend.snap Update generated snapshot to reflect new ANVIL_IMPACT_INPUT_DIR logic and diagnostics.
.anvil.lock Update catalog and file checksums after recipe/template regeneration.
Review details
  • Files reviewed: 10/11 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@Vaiz
Evgenii (Vaiz) force-pushed the u/vaiz/2026/09/08/anvil-impact-msrv-flake branch from 89f8d4b to 2124527 Compare September 8, 2026 16:06
Copilot AI review requested due to automatic review settings September 8, 2026 16:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes are consistently applied across the recipe source/template, tests and design docs, and the updated tests cover both the new override behavior and the “missing explicit input must fail” contract.

Review details
  • Files reviewed: 10/11 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@Vaiz
Evgenii (Vaiz) force-pushed the u/vaiz/2026/09/08/anvil-impact-msrv-flake branch from 2124527 to f3d7d32 Compare September 9, 2026 02:57
Copilot AI review requested due to automatic review settings September 9, 2026 02:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The behavioral change is scoped and consistently applied across recipe/template/docs/tests, with only minor diagnostic clarity suggestions remaining.

Review details

Suppressed comments (1)

crates/cargo-anvil/templates/justfiles/anvil/impact.just:634

  • When _anvil-impact-include fails in consume mode, the message names the missing file but doesn’t call out that ANVIL_IMPACT_INPUT_DIR may be redirecting reads, nor what constitutes a “complete” cache. Mentioning the override and required include files would better guide users to the fix.
    } elseif ($env:ANVIL_IMPACT -eq 'consume') {
        Write-Error "anvil-impact: ANVIL_IMPACT=consume but the input impact cache is missing ($file). Supply a complete impact cache before scoped checks run."
        exit 1
  • Files reviewed: 10/11 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/cargo-anvil/templates/justfiles/anvil/impact.just Outdated
Copilot AI review requested due to automatic review settings September 9, 2026 06:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes are internally consistent (template + generated recipe + docs + tests) and the new override behavior is guarded by targeted test coverage.

Review details
  • Files reviewed: 10/11 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Comment thread crates/cargo-anvil/templates/justfiles/anvil/impact.just
Comment thread crates/cargo-anvil/templates/justfiles/anvil/impact.just
@Vaiz
Evgenii (Vaiz) force-pushed the u/vaiz/2026/09/08/anvil-impact-msrv-flake branch from afff1d9 to a591584 Compare September 9, 2026 07:58
Copilot AI review requested due to automatic review settings September 9, 2026 07:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes consistently apply the new consume-mode input override across validation and reads, include clear failure diagnostics, and are backed by targeted test updates and design doc updates.

Review details
  • Files reviewed: 16/17 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@Vaiz
Evgenii (Vaiz) enabled auto-merge (squash) September 9, 2026 09:20
Evgenii (Vaiz) and others added 4 commits September 9, 2026 10:44
Seed the downloaded impact cache instead of computing it in the MSRV
recipe test. Keep the real recipe and assert both feature-mode commands.
Report captured output if impact-production tests miss a tier file.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy the cache files from tests/fixtures/msrv-impact-cache instead of
constructing their contents inside the test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Honor ANVIL_IMPACT_INPUT_DIR in consume mode for cache validation and
scope reads. Preserve the default cache path and computation behavior.
Read the MSRV fixture in place without creating or copying a cache.
Reject missing explicit inputs rather than falling back to default scope.
Regenerate the in-tree recipe and document the input contract.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…pact input

The snapshots tests capture the emitted backend trees, which embed
anvil-impact. Refresh them for the ANVIL_IMPACT_INPUT_DIR recipe change and
regenerate .anvil.lock after rebasing onto main.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Evgenii (Vaiz) and others added 3 commits September 9, 2026 10:44
PR 169 has been green on all 28 checks and carries two approving automated
reviews, but GitHub reported `mergeable_state: dirty` and it could not merge.
The 2026-09-08 diary entry recorded exactly that state -- "both automated
reviews recommend approval, but the branch is currently dirty against `main`"
-- and left it as the outstanding blocker.

The conflict was entirely in `.anvil.lock`. `main` released cargo-anvil 0.9.0
(3bf8d54) with a new `catalog_checksum`, while this branch still carried the
0.8.0 header plus its own checksum for `justfiles/anvil/impact.just`, which the
branch regenerates because it edits
`crates/cargo-anvil/templates/justfiles/anvil/impact.just`. Both sides touched
the same two lines.

`.anvil.lock` is generated, so it was NOT hand-merged. At each conflicting step
the lock was taken from the rebase target and the final `chore: regenerate
.anvil.lock after rebase` commit -- whose entire content was the stale lock --
was dropped, then the file was rewritten by the tool itself with
`cargo run -p cargo-anvil -- anvil`. Hand-resolving the hunk was rejected: the
`catalog_checksum` is derived from the whole template catalog, so a plausible
manual merge can be textually reasonable and still wrong.

Verified rather than assumed:
- `cargo run -p cargo-anvil -- anvil --dry-run` before the regen reported
  "Manifest update required: .anvil.lock" -- i.e. the exact `regenerate-check`
  gate in `.github/workflows/regenerate-check.yml` would have failed.
- The regen produced only the two expected lines: the 0.9.0 catalog checksum
  and the `justfiles/anvil/impact.just` checksum. No generated file content
  changed, confirming the rebase preserved the reviewed behaviour.
- `--dry-run` afterwards reports 92 unchanged and no manifest update.
- `cargo +1.97 test -p cargo-anvil --all-features --locked`: 415 unit tests and
  all integration suites pass, including the 21 impact tests this branch
  rewrites.
- `just anvil-clippy` clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…agnostics

Both consume-mode failures now state which include files a complete cache
holds and that ANVIL_IMPACT_INPUT_DIR selects the read directory, so an
accidentally set override is diagnosable from the message alone. Pin the new
wording in the missing-explicit-input test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…oundary

ANVIL_IMPACT_INPUT_DIR names a host directory, so -e cannot carry it into a
container correctly, and dropping it silently would let a containerized check
fall back to target/anvil/impact and scope off a different package list than the
native run while still reporting green. anvil-container now fails fast when the
override is set, and the boundary test guards both halves of that contract.

Also correct the _anvil-impact-include contract comment, which still claimed a
missing include file falls back to the tier default -- under consume it exits 1
-- and the matching helpers.just wording. That block ships verbatim into adopter
repositories.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 9, 2026 09:53
@Vaiz
Evgenii (Vaiz) force-pushed the u/vaiz/2026/09/08/anvil-impact-msrv-flake branch from a591584 to f733f78 Compare September 9, 2026 09:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

anvil-container currently rejects ANVIL_IMPACT_INPUT_DIR even when ANVIL_IMPACT is not consume, which conflicts with the stated “ignored outside consume mode” contract and unnecessarily blocks containerized runs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 16/17 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/cargo-anvil/templates/justfiles/anvil/container.just
The boundary rejected ANVIL_IMPACT_INPUT_DIR whatever ANVIL_IMPACT was, which
contradicted the documented contract that the override is ignored outside
consume: a containerized run with impact off or unset cannot diverge, so there
was nothing to reject and the guard only blocked a well-defined run. Gate it on
consume and assert that gating.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 9, 2026 10:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The updated MSRV test assertion compares mismatched types for the captured invocations, so the check needs to be adjusted to compare like-for-like.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/cargo-anvil/tests/impact.rs:1052

  • test_invocations is collected as Vec<&str>, but it is compared against expected, which is an array of Strings. Align the types (and ideally compare two Vec<String>s) so the assertion is well-defined and checks the exact invocations in order.
  • Files reviewed: 16/17 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@Vaiz

Copy link
Copy Markdown
Contributor Author

On the suppressed comment about crates/cargo-anvil/tests/impact.rs:1052 — I checked this against the code and I don't think there is anything to fix, so I'm leaving it as is.

The types are mismatched in the sense the comment describes: test_invocations is a Vec<&str> and expected is a [String; 2]. But that comparison is well-defined, not accidental. The standard library implements PartialEq<String> for &str, and Vec<A> implements PartialEq<[B; N]> wherever A: PartialEq<B>, so the two compose and assert_eq! compares the elements pairwise, in order, for exact string equality — which is precisely what the test is asserting.

The decisive evidence is that it compiles. Rust performs no implicit conversion here; if the operands did not have a PartialEq impl relating them, this would be a compile error rather than a silently weak assertion. The file builds and the test passes as part of cargo +1.95 test -p cargo-anvil --all-features (21/21 in the impact suite on the current head), so the assertion is both well-typed and exercised.

Collecting into a Vec<String> would allocate two Strings per line to reach the same verdict, so I would rather keep the borrowed form. Happy to change it if a reviewer prefers the symmetry as a readability matter, but I don't want to record it as a correctness fix, because the current assertion already checks the exact invocations in order.

@Vaiz
Evgenii (Vaiz) merged commit b0afae0 into main Sep 9, 2026
29 checks passed
@Vaiz
Evgenii (Vaiz) deleted the u/vaiz/2026/09/08/anvil-impact-msrv-flake branch September 9, 2026 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants