feat(cargo-anvil): tell the ADO job wrapper which check group it renders - #85
feat(cargo-anvil): tell the ADO job wrapper which check group it renders#85Evgenii (Vaiz) wants to merge 2 commits into
Conversation
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>
There was a problem hiding this comment.
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
groupstring parameter totemplates/ado/steps/job.yml. - Pass
group: <check-group>from everysteps/job.ymlinvocation inpr-stages.ymlandscheduled-stages.yml. - Add a unit test to assert each expected group is present on both per-OS jobs and that every
steps/job.ymlinvocation supplies agroup; 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 newgroupparameter. Please updatecrates/cargo-anvil/docs/design/ado.md(wrapper contract table and the example wrapper snippet) to includegroupso 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 58 58
Lines 3277 3277
=======================================
Hits 3277 3277
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:
|
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>
|
🤖 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
3 — No behaviour change in any of this: the default wrapper still ignores |
|
🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting. CI status on
Evidence that this is agent-image breakage rather than this PR:
There appears to be a branch already addressing the ARM runners ( 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. | |
There was a problem hiding this comment.
🤖: 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 itsname/pool/steps/artifactsparameter contract".ado.md:207-208— the artifact tree: "takesname,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 callssteps/job.ymlwith nogroup:, whilestages_identify_the_check_group_for_every_jobnow 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 |
There was a problem hiding this comment.
🤖: 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 |
There was a problem hiding this comment.
🤖: 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"][..], |
There was a problem hiding this comment.
🤖: 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.
Problem
The ADO stages templates name every per-OS job
linux/windows, so a job wrapper cannot tell apr_mutantsjob from apr_fastone.That matters for extension-template wrappers (1ESPT, SubstratePT, M365PT). Those repositories take ownership of
steps/job.ymland add a per-jobtemplateContext:block, and some groups genuinely need different settings than others. Two escape hatches do not work:parameters.nameislinux/windowsfor four different stages, so it does not identify the group.templateContext:is consumed at template-expansion time, so a runtime condition onSystem.StageNameis not available either.The concrete case that prompted this: a
pr-mutantsjob cannot produce a Cargo SBOM, because cargo-mutants compiles inside its own scratch copy of the tree. Component Governance therefore falls back to parsingCargo.lockand reports optional backends the build never selects. A wrapper that setsfailOnAlert: trueneeds to exempt exactly that one group, and today it has no way to name it.Change
Add an optional
groupparameter to thesteps/job.ymlcontract, and pass the check-group name from everysteps/job.ymlinvocation inpr-stages.ymlandscheduled-stages.yml: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.ymlaccepts a new optionalgroupparameter; the default wrapper's rendered output is unchanged.steps/job.ymlinvocations across the two stages templates now pass a group.stages_identify_the_check_group_for_every_jobasserts each group appears on both per-OS jobs, and that nosteps/job.ymlinvocation is left without a group.job_wrapper_declares_expected_contractnow also pinsname: group.ado_backendsnapshot is updated.steps/job.ymlmust take the proposed wrapper update (which adds thegroupparameter) 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.