Add automated per-plugin versioning (NBGV) with /version-bump + weekly backstop#813
Open
AbhitejJohn wants to merge 3 commits into
Open
Add automated per-plugin versioning (NBGV) with /version-bump + weekly backstop#813AbhitejJohn wants to merge 3 commits into
AbhitejJohn wants to merge 3 commits into
Conversation
…y backstop WHY Tools that surface skills (Copilot CLI, Claude Code, Codex, Cursor) read a plugin's version directly from its checked-in manifest. With no versioning discipline, a plugin's behavior can change while its advertised version stays flat, so clients never learn to re-pull, and there is no human-readable signal of what changed. We want correct, current versions in the repo with minimal manual work and without bloating the marketplace clone. WHAT - Per-plugin semantic versioning via Nerdbank.GitVersioning (NBGV). Each plugin owns a version.json whose pathFilters exclude the generated manifests and the version.json itself, so version height tracks real content changes only. - The computed version is materialized into the checked-in manifests (plugin.json and .codex-plugin/plugin.json) so every consumer reads a current value with no build step on their side. - eng/version/Sync-PluginVersions.ps1 is the single workhorse. It resolves the set of changed plugins from a git diff, computes each version with nbgv (predicting the squash-merge height for PRs), and either reports or stamps. AUTOMATIONS (two, low-touch by design) - /version-bump: an admin/maintainer comments the command on a PR and the affected plugins are stamped on the PR branch. Gated on collaborator permission (admin/write/maintain); forks are rejected before any privileged step. No other PRs are auto-modified. - weekly-version-sync: a Monday backstop (and workflow_dispatch) that stamps any drift on main, opens/updates a single bot PR, and explains the per-plugin reason. This self-heals anything that merged without a bump. We deliberately did NOT auto-edit contributor PRs or add a noisy advisory comment bot; maintainers stay in control and the signal stays clean. SECURITY (multi-model adversarial review: GPT-5.5 + Gemini 3.1 Pro) - Supply chain (High, both models): dotnet tool restore would have honored a nuget.config authored in the PR tree, letting an attacker remap the nbgv package source to a malicious feed and run code in the privileged contents:write context. Mitigated with a trusted eng/version/nuget.config (clear + nuget.org-only + packageSourceMapping), overlaid from main and used via --configfile so PR-supplied configs are ignored. No nuget.config is tracked in the repo today, so this path was genuinely exploitable. - TOCTOU (Medium): /version-bump now checks out the authorized head SHA rather than the mutable branch name; a racing push fails non-fast-forward, which is the safe outcome. - Injection: Set-ManifestVersion uses a MatchEvaluator (not a replacement string) so a "$"-bearing version cannot re-expand, plus a strict major.minor.patch guard that throws on a malformed base, leaving manifests untouched. - A base-only version.json bump (0.1 -> 0.2) is correctly detected and stamped. VERIFIED End-to-end against a real NBGV git harness: content-scoped predict, base-only bump -> x.y.0, docs-only -> [], weekly drift stamping, malformed-base guard, and --configfile restore (exit 0). actionlint passes on both workflows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Skill Coverage Report
Uncovered:
|
Collaborator
Author
|
Curious to hear thoughts on this versioning strategy. Goal was to keep this with as less ceremony as possible. At the moment Claude code users do not really get updates if they've already pulled down a plugin earlier because we haven't changed our versions. |
AbhitejJohn
requested review from
a team,
JanKrivanek,
ManishJayaswal,
Redth,
ViktorHofer,
YuliiaKovalova,
jfversluis and
webreidi
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces automated, per-plugin semantic versioning for the plugins/* tree using Nerdbank.GitVersioning (NBGV), and adds two GitHub Actions automations to materialize the computed versions into the checked-in plugin manifests (so downstream consumers read current versions directly from the repo).
Changes:
- Add per-plugin
plugins/<plugin>/version.jsonfiles to scope NBGV version height to each plugin subtree. - Add
eng/version/Sync-PluginVersions.ps1plus a locked-downeng/version/nuget.configand a local tool manifest fornbgv. - Add two workflows:
/version-bump(issue_comment-driven, same-repo PRs) and a scheduled weekly backstop PR that stamps any drift onmain.
Show a summary per file
| File | Description |
|---|---|
| plugins/dotnet11/version.json | Adds NBGV per-plugin version configuration for dotnet11. |
| plugins/dotnet/version.json | Adds NBGV per-plugin version configuration for dotnet. |
| plugins/dotnet-upgrade/version.json | Adds NBGV per-plugin version configuration for dotnet-upgrade. |
| plugins/dotnet-test/version.json | Adds NBGV per-plugin version configuration for dotnet-test. |
| plugins/dotnet-template-engine/version.json | Adds NBGV per-plugin version configuration for dotnet-template-engine. |
| plugins/dotnet-nuget/version.json | Adds NBGV per-plugin version configuration for dotnet-nuget. |
| plugins/dotnet-msbuild/version.json | Adds NBGV per-plugin version configuration for dotnet-msbuild. |
| plugins/dotnet-maui/version.json | Adds NBGV per-plugin version configuration for dotnet-maui. |
| plugins/dotnet-experimental/version.json | Adds NBGV per-plugin version configuration for dotnet-experimental. |
| plugins/dotnet-diag/version.json | Adds NBGV per-plugin version configuration for dotnet-diag. |
| plugins/dotnet-data/version.json | Adds NBGV per-plugin version configuration for dotnet-data. |
| plugins/dotnet-blazor/version.json | Adds NBGV per-plugin version configuration for dotnet-blazor. |
| plugins/dotnet-aspnetcore/version.json | Adds NBGV per-plugin version configuration for dotnet-aspnetcore. |
| plugins/dotnet-ai/version.json | Adds NBGV per-plugin version configuration for dotnet-ai. |
| eng/version/Sync-PluginVersions.ps1 | Core PowerShell implementation to compute/stamp versions and emit a JSON report. |
| eng/version/nuget.config | Locked-down NuGet config used by workflows to prevent PR-controlled feed remapping during tool restore. |
| .config/dotnet-tools.json | Adds local nbgv tool dependency for consistent version computation in CI. |
| .github/workflows/version-bump-command.yml | Implements the maintainer-triggered /version-bump workflow. |
| .github/workflows/weekly-version-sync.yml | Implements the scheduled backstop that opens/updates a single “weekly sync” PR on drift. |
| CONTRIBUTING.md | Documents the new per-plugin versioning model and contributor expectations. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 20/20 changed files
- Comments generated: 4
Contributor
|
👋 @AbhitejJohn — this PR has 4 unresolved review thread(s). When you're ready, please address the feedback and push an update; the triage bot will pick up the next state automatically. (Add the |
- Add missing plugins/dotnet-test-migration/version.json so it participates in versioning (it was the only plugin without one; manifests are at 0.1.0). - CONTRIBUTING: the two manifests are not byte-identical; say the version is duplicated across two manifest files instead. - weekly-version-sync: include version.json in commit attribution so a base-only bump is explained rather than showing 'no attributable commits'. - Get-NbgvInfo: capture nbgv stderr and include it in the thrown error so CI failures are diagnosable, while keeping stdout clean for JSON parsing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
waiting-on-review
Contributor
|
✅ Evaluation passed for |
Evangelink
reviewed
Jun 25, 2026
Evangelink
left a comment
Evangelink
left a comment
Member
There was a problem hiding this comment.
Review: per-plugin NBGV versioning
Really like the overall design — per-plugin pathFilters, materializing into the checked-in manifests, the opt-in /version-bump + weekly backstop split, and the security hardening on the privileged workflow (SHA-pinned actions, fail-closed permission gate, fork rejection before any privileged step, --configfile source-mapped NuGet, trusted-tooling overlay from main, injection-safe MatchEvaluator). The security posture of version-bump-command.yml looks sound to me.
My concern is that the privileged workflow's git plumbing and the tool runtime pin look like they'll prevent /version-bump (and likely the weekly sync) from actually running. Because both workflows are issue_comment/schedule-triggered they run from main and get no CI signal on this PR, so these can only be caught by review. Two of the inline findings below were reproduced in a scratch repo.
This was a combined pass (two independent reviewers); we converged on the same set with no contradictions. Inline comments have details + suggested fixes. Summary:
Blocking / CI-breaking
git fetch --depth=1 origin mainmakes the whole repo shallow → NBGV refuses to compute height. Reproduced: a depth-limited fetch writes.git/shallowand flips the entire repo to shallow even though the head was checked out withfetch-depth: 0, so everynbgv get-versionin the ordinary-content path fails.git merge-base $BASE_SHA $headreturns empty when the PR is behindmain. Reproduced (PR 3 commits behind → empty merge-base, exit 1 → script throws-PredictSquashMerge requires -BaseCommit). Fixed by the same full-fetch change, plus an empty-merge-base guard.nbgv(net8) underrollForward: falseon a net11-preview-only runtime.setup-dotnetinstalls only theglobal.jsonruntime; a net8-targeted tool won't run, and evenrollForward: truewon't pick a preview runtime withoutDOTNET_ROLL_FORWARD_TO_PRERELEASE=1. Best fix: explicitly install an 8.0.x runtime in both workflows.
Correctness
- Squash prediction over-bumps on
version.json-only (non-base) edits, then the weekly backstop computes the true (lower) height and corrects downward — a non-monotonic version regression visible to consumers.
Minor
- Format guard validates the computed value but not the 2-part base shape in
version.json(a malformed 3-part base slips through the weekly path). - Concurrent
/version-bumpon the same plugin can stamp colliding patch numbers until the weekly sync reconciles — worth a doc note. - PR description says
version.json×14; there are 15 (dotnet-test-migrationwas added in commit 2).
| - name: Pin trusted tooling from main | ||
| if: steps.pr.outputs.is_fork == 'false' | ||
| run: | | ||
| git fetch --no-tags --depth=1 origin main |
Member
There was a problem hiding this comment.
Blocking (CI-breaker, reproduced). git fetch --no-tags --depth=1 origin main writes .git/shallow and turns the entire repository shallow — even though the head was checked out with fetch-depth: 0. NBGV checks Repository.Info.IsShallow at the repo level and throws Shallow clone lacks the objects required to calculate version height, so every nbgv get-version in the ordinary-content path (Sync-PluginVersions.ps1 heightAtBase) fails and /version-bump breaks for the common case.
Reproduced in a scratch repo: is-shallow(before)=false → after this fetch → is-shallow(after)=true.
Fix: drop --depth=1 (the repo is already fully fetched, so git fetch --no-tags origin main is cheap and keeps it non-shallow), or fetch the four tooling blobs without affecting shallow state and git checkout origin/main -- <files>. If a shallow fetch is unavoidable, follow with git fetch --unshallow before invoking NBGV.
| BASE_SHA: ${{ steps.pr.outputs.base_sha }} | ||
| run: | | ||
| $head = git rev-parse HEAD | ||
| $mergeBase = git merge-base $env:BASE_SHA $head |
Member
There was a problem hiding this comment.
Blocking (reproduced). git merge-base $BASE_SHA $head returns empty when main has advanced past the PR's branch point: BASE_SHA (the PR base.sha) isn't an ancestor of head, and the depth-1 fetch above truncates its ancestry at the shallow boundary, so merge-base can't reach the common ancestor (exit 1). $mergeBase becomes empty and the script throws -PredictSquashMerge requires -BaseCommit.
Reproduced (PR 3 commits behind main): merge-base result='' exit=1.
Fix: fetching main at full depth (the fix above) resolves the ancestry; additionally guard against an empty $mergeBase and fail with a clear message rather than passing -BaseCommit ''.
| "commands": [ | ||
| "nbgv" | ||
| ], | ||
| "rollForward": false |
Member
There was a problem hiding this comment.
Blocking. nbgv 3.10.85 targets net8.0, but both workflows setup-dotnet from global.json (11.0.100-preview), which provides only the 11.0 runtime. With rollForward: false the host won't roll an 8.0-targeted tool across majors and fails with The framework 'Microsoft.NETCore.App', version '8.0.0' was not found unless a net8 runtime happens to be pre-installed on the runner (not pinned, not guaranteed).
Note rollForward: true alone is not sufficient here: the only installed runtime is a preview, and roll-forward won't select a prerelease runtime without DOTNET_ROLL_FORWARD_TO_PRERELEASE=1.
Preferred fix: explicitly install a net8 runtime in both workflows (setup-dotnet with an additional dotnet-version: 8.0.x), so nbgv doesn't depend on rolling a release tool onto a preview runtime.
| } | ||
| else { | ||
| $heightAtBase = (Get-NbgvInfo -PluginDir $pluginDir -Commit $BaseCommit).VersionHeight | ||
| $computed = "$newBase.$([int]$heightAtBase + 1)" |
Member
There was a problem hiding this comment.
Correctness (Medium). Get-ChangedPlugins intentionally counts version.json as a change, but each plugin's version.json excludes itself from NBGV height (:!version.json). So a PR that edits version.json without changing the base (e.g. tweaking pathFilters, $schema, or whitespace) has oldBase == newBase and lands here, predicting base.(heightAtBase + 1) — yet the squashed commit only touches height-excluded files, so the true post-merge height stays heightAtBase. The PR gets stamped one patch too high; the weekly backstop later computes the real (lower) height and corrects downward — a non-monotonic version regression consumers can observe.
Fix: in predict mode, check whether the BaseCommit..HeadCommit diff touches any height-bearing file for the plugin (apply the plugin's own pathFilters exclusions, not just the manifest excludes). Only add +1 when a height-bearing file actually changed; otherwise predict base.heightAtBase.
| # Guard against a malformed version.json base (e.g. "0.2$1" or "1.x"): a bad value | ||
| # would otherwise be written verbatim into the manifests. NBGV versions are always | ||
| # numeric major.minor.patch, so anything else means the source data is wrong. | ||
| if ($computed -notmatch '^\d+\.\d+\.\d+$') { |
Member
There was a problem hiding this comment.
Minor. This guard validates the computed value, so a malformed 3-part base in version.json (e.g. "version": "0.1.0") is caught in predict mode (becomes 0.1.0.N → fails the 3-part check) but not in the weekly path: NBGV normalizes it into a 3-part SimpleVersion that passes this guard while silently producing a wrong/fixed version. Consider also asserting the version.json base matches ^\d+\.\d+$ when it's read in Get-VersionBase.
github-actions
Bot
added
waiting-on-author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Every client that surfaces these skills — Copilot CLI, Claude Code, Codex, Cursor — reads a plugin's version directly from its checked‑in manifest (
plugin.json/.codex-plugin/plugin.json). Today those versions are static, so:We want correct, current versions in the repo with minimal manual work, without bloating the marketplace clone and without taking on third‑party/external plugins (out of scope for now).
What this adds
version.jsonwhosepathFiltersexclude the generated manifests andversion.jsonitself, so the computed version height tracks real content changes only (editing a manifest or the version file doesn't inflate the height).eng/version/Sync-PluginVersions.ps1— resolves the changed‑plugin set from a git diff, computes each version withnbgv(predicting the squash‑merge height for PRs), and either reports or stamps.Automations (two, low‑touch by design)
/version-bump/version-bumpon a PRweekly-version-syncworkflow_dispatchmain, opens/updates a single bot PR, and explains the per‑plugin reason. Self‑heals anything merged without a bump.We deliberately did not auto‑edit contributor PRs or add a noisy advisory‑comment bot — maintainers stay in control and the signal stays clean. (No official skills repo we surveyed auto‑mutates contributor PRs for versioning; the opt‑in command + scheduled backstop pattern is the conservative norm.)
Security — multi‑model adversarial review (GPT‑5.5 + Gemini 3.1 Pro)
dotnet tool restorewould have honored anuget.configauthored in the PR tree, letting an attacker remap thenbgvpackage source to a malicious feed and execute code in the privilegedcontents: writecontext. This was genuinely reachable — nonuget.configis tracked anywhere in the repo today, so a PR could add one. Fix: a trustedeng/version/nuget.config(<clear/>+ nuget.org‑only +packageSourceMapping), overlaid frommainand consumed via--configfileso PR‑supplied configs are ignored entirely./version-bumpnow checks out the authorized head SHA rather than the mutable branch name; if the branch advances after authorization, the push fails non‑fast‑forward — the safe outcome.Set-ManifestVersionuses aMatchEvaluator(not a replacement string), so a$‑bearing version can't re‑expand, plus a strict^\d+\.\d+\.\d+$guard that throws on a malformed base and leaves manifests untouched.version.jsonbump (0.1 → 0.2) is now detected and stamped.Verification
Exercised end‑to‑end against a real NBGV git harness (the repo
global.jsonpins an SDK that isn't installable locally, so testing uses an isolated harness):x.y.0;[](no bump);-OnlyChanged→ stamps drift authoritatively;dotnet tool restore --configfile→ exit 0.actionlintpasses on both workflows.Files
eng/version/Sync-PluginVersions.ps1,eng/version/nuget.config,.config/dotnet-tools.json.github/workflows/version-bump-command.yml,.github/workflows/weekly-version-sync.ymlplugins/*/version.json(×14),CONTRIBUTING.mdNotes for reviewers
issue_comment/scheduled workflows run the YAML frommain, so/version-bumpand the weekly job only take effect once this is merged — expected, not a bootstrap bug.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com