Fix #4345 (3/4): CI guard requiring version bumps on bundled extension changes - #4395
Open
CrazyBaran wants to merge 2 commits into
Open
Fix #4345 (3/4): CI guard requiring version bumps on bundled extension changes#4395CrazyBaran wants to merge 2 commits into
CrazyBaran wants to merge 2 commits into
Conversation
Bundled extensions only reach existing installs through a version bump: `specify extension update` compares the semver in extensions/catalog.json against the installed copy and reports "Up to date" whenever they match. Content changes shipped without a bump go silently stale on every project that already installed the extension (github#4345). This guard turns "please remember to bump" into a merge requirement. - `.github/scripts/check_extension_version_bump.py` fails a PR that changes files under `extensions/<id>/` for a catalog-listed extension without increasing that extension's `extension.yml` version (PEP 440 comparison, the same semantics `extension update` uses), and requires `extensions/catalog.json` to stay in sync with each manifest. Non-catalog extensions (the `selftest` fixture and the `template` scaffold) are exempt. - `extension-version-guard.yml` runs the check on pull requests touching `extensions/**`, diffing the PR base against HEAD. - Contract tests pin the working-tree half of the invariant (catalog/manifest version sync, bundled entries shipping a directory); guard-script tests pin the failure behavior against real throwaway git repositories so a parsing change cannot silently disable the guard while CI stays green. Split out of github#4351 (part 3 of the series requested in review); refs github#4345. Assisted-by: Claude Code (model: claude-fable-5) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Sep 1, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Path quoting can bypass change detection, and the path-filtered workflow cannot reliably serve as a required check.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds CI enforcement to prevent bundled extension changes from becoming stale due to missing version bumps.
Changes:
- Adds the version-bump guard and workflow.
- Adds contract and integration-style guard tests.
- Documents bundled extension versioning requirements.
File summaries
| File | Description |
|---|---|
.github/scripts/check_extension_version_bump.py |
Validates changed extension versions and catalog synchronization. |
.github/workflows/extension-version-guard.yml |
Runs the guard in pull requests. |
tests/contract/test_extension_version_guard_script.py |
Tests guard success and failure scenarios. |
tests/contract/test_bundled_extension_versions.py |
Enforces catalog, manifest, and bundled-directory consistency. |
extensions/EXTENSION-DEVELOPMENT-GUIDE.md |
Documents mandatory version bumps. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…and quoting-proof Address Copilot review round 1 on github#4395. Workflow: drop the `paths: extensions/**` filter. A required status check that is skipped by path filtering stays in "Expected" state and blocks every PR that does not touch extensions/**, which defeats the point of making the guard a merge requirement. The job now runs on every pull request; the script already reports success when nothing under extensions/ changed, so unrelated PRs pass in one short job. Script: read the changed-path list with `git diff --name-only -z`. With git's default core.quotePath, a path containing non-ASCII or control characters is C-quoted with the quotes included (`"extensions/demo/caf\303\251.txt"`), so its first component was no longer `extensions` and an unbumped change to such a file escaped the guard. NUL-delimited output is emitted verbatim; paths are decoded with surrogateescape so an undecodable byte cannot crash the check, and only the ASCII `extensions/<id>/` prefix is ever interpreted. Tests: pin both behaviors. The non-ASCII case fails against the previous script and passes now; the no-extension-changes case backs the workflow change. core.quotePath is pinned to true in the fixture repo so the regression exercises the quoting path even where a developer's global config disables it. Refs github#4345 Assisted-by: Claude Code (model: claude-fable-5-1) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
New cataloged extensions can bypass version parsing and accept invalid versions.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/scripts/check_extension_version_bump.py:124
- New cataloged extensions skip the only
Version(...)parsing in this function, so a manifest and catalog both set tonot-a-versionpass the guard even though the CLI rejects that version and the guard promises that unparseable versions fail closed. Parse the head version before accepting the no-base-manifest case, and add this variant to the regression test.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Collaborator
|
Is this one ready? |
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.
Prevents the recurrence half of #4345: bundled extensions only reach existing installs through a version bump —
specify extension updatecompares the semver inextensions/catalog.jsonagainst the installed copy and reports "Up to date" whenever they match. Content changes shipped without a bump go silently stale on every project that already installed the extension, which is exactly howagent-contextandgitdrifted 15 and 23 commits behind their1.0.0manifests. This PR turns "please remember to bump" into a merge requirement.Scope (part 3 of the split requested in review)
Following @mnriem's request to split the original #4351 into four parts, this PR carries part 3 — the CI version-bump guard, based on latest
main:.github/scripts/check_extension_version_bump.pyfails a PR that changes files underextensions/<id>/for a catalog-listed extension without increasing that extension'sextension.ymlversion (PEP 440 comparison — the same semanticsextension updateuses), and requiresextensions/catalog.jsonto stay in sync with each manifest (the update preflight rejects a manifest whose version differs from the catalog's). Non-catalog extensions (theselftestfixture and thetemplatescaffold) are exempt — no update flow is driven by their versions..github/workflows/extension-version-guard.ymlruns the check on pull requests touchingextensions/**, diffing the PR base against HEAD (same fetch pattern aslint.yml; pinned actions,contents: readonly).tests/contract/test_bundled_extension_versions.pypins the working-tree half of the invariant: catalog/manifest version sync, and everybundledcatalog entry shipping an in-repo extension directory.tests/contract/test_extension_version_guard_script.pypins the guard's failure behavior against real throwaway git repositories, invoked exactly as the workflow does — so a change to the script's diff or parsing logic can't silently disable the guard while CI stays green.extensions/EXTENSION-DEVELOPMENT-GUIDE.mdgains a versioning bullet documenting the bump-on-every-content-change rule and its CI enforcement.All content is byte-identical to what was already reviewed on #4351 through the previous rounds, except two doc sentences that referenced the staleness detection (part 4) across PR boundaries — trimmed so this PR documents only what it ships.
Sequencing: independent
No ordering constraint — this can land before or after #4351 and #4394. A nice property of landing it early: the guard will then validate #4394's bump/catalog sync as its first real customer.
The other parts
feat/4345-content-staleness-detection, stacked on Fix #4345 (1/4): install bundled extension updates from the local package #4351 (PR to follow once it merges)Refs #4345.
🤖 Generated with Claude Code