Skip to content

feat(cargo-anvil): tell the ADO job wrapper which check group it renders - #85

Open
Evgenii (Vaiz) wants to merge 2 commits into
mainfrom
user/vaiz/anvil-job-group-parameter
Open

feat(cargo-anvil): tell the ADO job wrapper which check group it renders#85
Evgenii (Vaiz) wants to merge 2 commits into
mainfrom
user/vaiz/anvil-job-group-parameter

Conversation

@Vaiz

Copy link
Copy Markdown
Contributor

Problem

The ADO stages templates name every per-OS job linux / windows, so a job wrapper cannot tell a pr_mutants job from a pr_fast one.

That matters for extension-template wrappers (1ESPT, SubstratePT, M365PT). Those repositories take ownership of steps/job.yml and add a per-job templateContext: block, and some groups genuinely need different settings than others. Two escape hatches do not work:

  • parameters.name is linux / windows for four different stages, so it does not identify the group.
  • templateContext: is consumed at template-expansion time, so a runtime condition on System.StageName is not available either.

The concrete case that prompted this: a pr-mutants job cannot produce a Cargo SBOM, because cargo-mutants compiles inside its own scratch copy of the tree. Component Governance therefore falls back to parsing Cargo.lock and reports optional backends the build never selects. A wrapper that sets failOnAlert: true needs to exempt exactly that one group, and today it has no way to name it.

Change

Add an optional group parameter to the steps/job.yml contract, and pass the check-group name from every steps/job.yml invocation in pr-stages.yml and scheduled-stages.yml:

- template: steps/job.yml
  parameters:
    name: linux
    group: pr-mutants
    pool: ${{ parameters.linuxPool }}

The default wrapper ignores it -- it exists purely so downstream wrappers can discriminate. It defaults to '', so a wrapper that does not declare it keeps its current behaviour.

Effects

  • steps/job.yml accepts a new optional group parameter; the default wrapper's rendered output is unchanged.
  • All 18 steps/job.yml invocations across the two stages templates now pass a group.
  • New test stages_identify_the_check_group_for_every_job asserts each group appears on both per-OS jobs, and that no steps/job.yml invocation is left without a group.
  • job_wrapper_declares_expected_contract now also pins name: group.
  • The ado_backend snapshot is updated.
  • Adopter action required. ADO rejects a parameter the target template does not declare. A repository that already owns steps/job.yml must take the proposed wrapper update (which adds the group parameter) in the same change as this stages-template update, or its pipeline fails to compile. Flagging this explicitly in case reviewers would rather sequence it differently.

Validation

cargo test -p cargo-anvil (289 + integration suites), cargo clippy --all-targets -D warnings, cargo fmt --check -- all green.

The ADO stages templates name every per-OS job `linux` / `windows`, so a
job wrapper cannot tell a `pr_mutants` job from a `pr_fast` one. That
matters for extension-template wrappers (1ESPT, SubstratePT, M365PT),
which take ownership of `steps/job.yml` and set a per-job
`templateContext:` block: some groups need different SDL settings than
others. `templateContext:` is consumed at template-expansion time, so a
runtime condition on `System.StageName` is not an option either, and the
job name is the only compile-time signal the wrapper gets.

Add an optional `group` parameter to the wrapper contract and pass the
check-group name from every `steps/job.yml` invocation in pr-stages.yml
and scheduled-stages.yml. The default wrapper ignores it; it exists so
downstream wrappers can discriminate. It defaults to '' so a wrapper
that does not declare it keeps its current behaviour.

Note for adopters: ADO rejects a parameter the target template does not
declare, so a repository that already owns `steps/job.yml` must take the
proposed wrapper update (which adds the `group` parameter) together with
this stages-template update.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 18, 2026 12:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the Azure DevOps (ADO) job-wrapper contract emitted by cargo-anvil so downstream “extension template” wrappers (e.g., 1ESPT/SubstratePT/M365PT) can distinguish which anvil check group a linux/windows job belongs to at template-expansion time.

Changes:

  • Add an optional group string parameter to templates/ado/steps/job.yml.
  • Pass group: <check-group> from every steps/job.yml invocation in pr-stages.yml and scheduled-stages.yml.
  • Add a unit test to assert each expected group is present on both per-OS jobs and that every steps/job.yml invocation supplies a group; update the ADO backend snapshot accordingly.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
crates/cargo-anvil/templates/ado/steps/job.yml Adds optional group parameter to the job-wrapper contract.
crates/cargo-anvil/templates/ado/pr-stages.yml Passes group for each PR-tier job-wrapper invocation.
crates/cargo-anvil/templates/ado/scheduled-stages.yml Passes group for each scheduled-tier job-wrapper invocation.
crates/cargo-anvil/src/anvil/artifacts/ado.rs Adds tests validating the group contract and its presence in stage templates.
crates/cargo-anvil/tests/snapshots/snapshots__ado_backend.snap Updates snapshot output to include the new group parameter and passed values.
Suppressed comments (3)

crates/cargo-anvil/templates/ado/steps/job.yml:28

  • The comment says the parameter "defaults to '' so wrappers written against the older contract keep working", but wrappers that predate this parameter will fail to compile when called with group:. Consider rewording to avoid implying backward compatibility without updating the wrapper contract.
#                          not available either. Defaults to '' so wrappers
#                          written against the older contract keep working.

crates/cargo-anvil/tests/snapshots/snapshots__ado_backend.snap:1907

  • This snapshot still says "Defaults to '' so wrappers written against the older contract keep working", which is misleading now that stage templates always pass group. Reword to describe that wrappers/callers that ignore/omit the value keep the same behavior.
#                          not available either. Defaults to '' so wrappers
#                          written against the older contract keep working.

crates/cargo-anvil/templates/ado/steps/job.yml:45

  • The ADO design doc documents the job-wrapper parameter contract (currently name/pool/steps/artifacts), but this PR adds a new group parameter. Please update crates/cargo-anvil/docs/design/ado.md (wrapper contract table and the example wrapper snippet) to include group so the design stays in lockstep with the implementation.
  - name: group
    type: string
    default: ''

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/cargo-anvil/templates/ado/steps/job.yml Outdated
Comment thread crates/cargo-anvil/tests/snapshots/snapshots__ado_backend.snap Outdated
Comment thread crates/cargo-anvil/src/anvil/artifacts/ado.rs Outdated
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (0b690f1) to head (546f91b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #85   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files          58       58           
  Lines        3277     3277           
=======================================
  Hits         3277     3277           
Flag Coverage Δ
linux 100.0% <ø> (ø)
linux-arm 100.0% <ø> (ø)
scheduled ?
windows 100.0% <ø> (ø)

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.

The wrapper contract comment claimed the stages templates name every per-OS
job `linux` / `windows`, but the impact stage renders `compute_linux` /
`compute_windows`. The argument is unchanged -- per-OS names repeat across
stages, so `name` cannot identify a check group -- but the wording now says
that accurately, in `steps/job.yml`, in the test rationale and in the
snapshot.

It also claimed the `''` default lets wrappers written against the older
contract keep working. ADO rejects a parameter the target template does not
declare, so that is wrong and contradicted the adopter-compat note in the PR
description: the default only lets a CALLER omit the value.

Add `group` to the wrapper contract table and the default-wrapper snippet in
docs/design/ado.md so the design stays in lockstep with the template.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 18, 2026 13:01
@Vaiz

Copy link
Copy Markdown
Contributor Author

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

Answering the three suppressed comments in the review body — all three were correct and all three are fixed in 546f91b.

1 & 2 — "Defaults to '' so wrappers written against the older contract keep working" (steps/job.yml:28, snapshot line 1907). Right, and this contradicted the adopter-compat note already in the PR description: ADO rejects a parameter the target template does not declare, so a wrapper that predates group does not keep working once the stages templates pass it. The '' default only lets a caller omit the value. Reworded:

Defaults to '' so a caller may omit it. A wrapper that does not DECLARE the parameter is rejected by ADO once the stages templates pass it, so an adopter who owns this file takes the wrapper and stages updates together.

3 — docs/design/ado.md out of lockstep. Correct, the §4.1 wrapper-contract table and the default-wrapper snippet both predated this parameter. Added a group row (type string, required no) carrying the same rationale, and added - { name: group, type: string, default: '' } to the snippet.

No behaviour change in any of this: the default wrapper still ignores group, and its rendered output is byte-identical. Gates re-run after the edits — cargo test -p cargo-anvil (289 unit + all integration + snapshots) green, cargo clippy -p cargo-anvil --all-targets -D warnings clean, just anvil-fmt and just anvil-spellcheck clean. The snapshot was re-accepted with cargo insta, not hand-edited.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@Vaiz
Evgenii (Vaiz) enabled auto-merge (squash) August 18, 2026 13:48
@Vaiz

Copy link
Copy Markdown
Contributor Author

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

CI status on 546f91b: 42 success, 2 failing, 0 pending. Both failures are the ARM pr-fast legs, and neither is caused by this branch — both fail in the anvil-spellcheck step for environment reasons, in two different ways:

  • pr-fast (linux-arm)cargo-spellcheck cannot be installed: clang-sys v1.7.0 build script panics with couldn't find any valid shared libraries matching: ['libclang.so', 'libclang-*.so'], set the LIBCLANG_PATH environment variable. The runner image has no libclang.
  • pr-fast (windows-arm)cargo-spellcheck installs and then crashes: recipe anvil-spellcheck failed with exit code -1073741819 (0xC0000005, access violation) / Segmentation fault … exit code 139. No spelling diagnostics are emitted before the crash, so this is a tool crash, not a content finding.

Evidence that this is agent-image breakage rather than this PR:

  • Both legs fail the same way on an unrelated branch: run 32098550248 (new-tools, earlier today) has pr-fast (linux-arm) and pr-fast (windows-arm) both red.
  • Both were already red on this PR's previous head 8316050 (run 32138764838), before the doc-wording commit.
  • just anvil-spellcheck and just anvil-fmt both pass locally on this exact tree, along with cargo test -p cargo-anvil (289 unit + all integration + snapshots) and cargo clippy -p cargo-anvil --all-targets -D warnings.

There appears to be a branch already addressing the ARM runners (u/mhavelka/unblock-arm-pipelines), so I am not touching it from here — fixing the runner image is out of scope for this PR.

Everything raised in review has been addressed and all three threads are resolved. This is now waiting on human review/approval only; merge state is BLOCKED for want of an approving review. I am stopping the automated driver here rather than polling further.

| Parameter | Type | Required | Meaning |
|-------------|------------|----------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `name` | `string` | yes | Job name; ADO derives the display name from it. |
| `group` | `string` | no | The anvil check group this job runs (`impact`, `pr-fast`, `pr-test`, `pr-runtime-analysis`, `pr-mutants`, `scheduled-*`). The default wrapper ignores it; extension-template wrappers use it to vary per-job `templateContext:`. Per-OS job names repeat across stages (`linux` / `windows` in every pr-\* and scheduled-\* stage, `compute_linux` / `compute_windows` in impact), so `name` alone cannot identify the group, and `templateContext:` is evaluated at template-expansion time so a runtime stage-name condition is not available either. Defaults to `''` so a caller may omit it — but a wrapper that does not *declare* the parameter is rejected by ADO once the stages templates pass it, so an adopter who owns this file takes the wrapper and stages updates together. |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖: Non-blocking — This new row is the only place the widened contract and its adopter coupling are written down; five mirrors of the old contract elsewhere now contradict it:

  • ado.md:186 — "exposes through its name/pool/steps/artifacts parameter contract".
  • ado.md:207-208 — the artifact tree: "takes name, pool, steps, artifacts".
  • ado.md:228-230 — "The stages templates address the wrapper only via its parameter contract (name, pool, steps, artifacts), so the wrapper can diverge arbitrarily without blocking stage-shape updates."
  • ado.md:427-431 — "stages updates flow through without merging, and the user's wrapper changes survive every anvil upgrade."
  • ado.md:466-471 — the §4.2 sample stages template calls steps/job.yml with no group:, while stages_identify_the_check_group_for_every_job now requires one on every invocation.

The 228-230 / 427-431 claims are the ones that matter: what stays true is that the wrapper body can diverge arbitrarily, but a growth of the parameter contract now does block a stage-shape update for exactly the adopters the wrapper exists for. Worth adding group to the two enumerations, qualifying the divergence/merge-free-update promise to the wrapper body, and adding group: impact / group: pr-fast to the §4.2 sample.

Same stale claim in templates/ado/steps/job.yml:9-11 ("continue to update normally because they only know about this wrapper's parameter contract"), 18 lines above the new note that says otherwise — and that is the file adopters take ownership of.

# Contract (intentionally small and stable):
# - name (string) Job name; ADO derives the display name from it.
# - group (string) Optional. The anvil check group this job runs
# (`impact`, `pr-fast`, `pr-mutants`, ...). The

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖: Non-blocking — Two things about this parameter's vocabulary, in the one emitted file adopters hand-edit and branch on.

(1) group already means something else one directory over. steps/setup.yml:8-19 takes a parameter also called group, with a different domain: '' = install the full catalog, none = skip, otherwise a check-group name. Both are visible inside the same job — the impact job passes group: impact to this wrapper while steps/impact.yml:27 passes group: none to setup. A wrapper author will reasonably assume one vocabulary. Either name this one for the domain it actually carries (stage — every value passed is the ADO stage identifier, which also makes impact natural where it is not an anvil check group per GROUPS, ado.rs:48-57), or add a line here saying this group is the job's check group plus impact, and is unrelated to setup.yml's installer selector. Worth settling before it becomes part of an "intentionally stable" contract.

(2) The value set is elided here ("impact, pr-fast, pr-mutants, ...") but a wrapper author has to write ${{ if eq(parameters.group, '...') }} against exact strings, and the exhaustive list lives only in docs/design/ado.md:371. Listing all nine would make the contract self-contained in the file people read.

parameters:
- name: name
type: string
- name: group

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖: Non-blocking — This declaration is where the adopter-visible break lands: once the stages templates pass group unconditionally (pr-stages.yml:86 onward), a repository owning a customized steps/job.yml gets a committed pipeline that fails at ADO template expansion until it merges the .proposed wrapper — anvil overwrites the owned stages files but only proposes the wrapper change. The commit body states the migration precisely, which is most of the work; the gap is that it says it in prose on a feat(cargo-anvil): subject, so the changelog tooling classifies it as an ordinary feature and the one line adopters need never reaches the release notes. Consider marking the squash commit breaking (feat(cargo-anvil)!: or a BREAKING CHANGE: trailer), or carrying the migration line into the 0.4.0 CHANGELOG's 📝 Notes section: "if you own .pipelines/anvil/steps/job.yml, take the proposed wrapper update in the same change as the regenerated pr.yml / scheduled.yml." No code change implied.

for (template, groups) in [
(
PR_STAGES,
&["impact", "pr-fast", "pr-test", "pr-runtime-analysis", "pr-mutants"][..],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖: Non-blocking — These expected group names are a third hardcoded copy alongside GROUPS (ado.rs:48-57) and GROUP_STEPS (ado.rs:155-167), which the suite already cross-checks against each other. That makes this test narrower than its name: it catches a steps/job.yml invocation added without a group, but a group added to GROUPS/GROUP_STEPS and never threaded into the stages templates passes silently — the exact regression the test exists to prevent. GROUPS is #[cfg(test)] in this module and already in scope via use super::*, so deriving both lists from it (PR = once("impact").chain(GROUPS[..4]), scheduled = GROUPS[4..], or a split on the pr- / scheduled- prefix) would make adding a group fail here until the stages templates thread it, and drops the third copy.

@martin-kolinek martin-kolinek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖: Approved.

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