Fix #4345 (1/4): install bundled extension updates from the local package - #4351
Conversation
There was a problem hiding this comment.
Pull request overview
Adds reliable bundled-extension updates through synchronized versions, content-staleness detection, and local-package installation.
Changes:
- Bumps bundled extension versions and synchronizes the catalog.
- Adds content hashing and local bundled-update support.
- Adds CI guards, contract tests, and documentation.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/extensions/__init__.py |
Adds extension content hashing and registry persistence. |
src/specify_cli/extensions/_commands.py |
Detects stale content and installs bundled updates locally. |
.github/scripts/check_extension_version_bump.py |
Validates version bumps and catalog synchronization. |
.github/workflows/extension-version-guard.yml |
Runs the version guard for extension changes. |
extensions/catalog.json |
Synchronizes bumped extension versions. |
extensions/agent-context/extension.yml |
Bumps agent-context to 1.1.0. |
extensions/assess/extension.yml |
Bumps assess to 1.0.1. |
extensions/git/extension.yml |
Bumps git to 1.1.0. |
extensions/EXTENSION-DEVELOPMENT-GUIDE.md |
Documents version-bump requirements. |
docs/reference/extensions.md |
Documents bundled updates and stale-content recovery. |
tests/test_extensions.py |
Tests local bundled-update behavior. |
tests/test_extension_content_staleness.py |
Tests hashing and stale-content detection. |
tests/contract/test_bundled_extension_versions.py |
Enforces catalog/manifest version parity. |
tests/extensions/git/test_git_extension.py |
Updates the expected git version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
All four review findings were valid; each is applied with regression tests: 1. compute_extension_content_hash() excluded *-config.yml / *-config.local.yml at every depth, but the remove/backup/restore machinery only preserves top-level config files (_target_follows_preserved_convention), so a changed nested shipped file like templates/foo-config.yml was overwritten by installation yet invisible to staleness detection. The exclusion now applies only to direct children of the extension directory. 2. The stale-content check ran on every catalog_version <= installed_version outcome. When the installed copy is newer than the catalog or the running release's bundled copy (e.g. written by a newer CLI), a hash difference is version skew, not unbumped drift - and the suggested `extension add --force` would downgrade the installation. The check is now gated on catalog_version == installed_version at the call site AND on the bundled copy declaring the same version as the installed one inside the helper (the second guard also covers an older CLI run against an up-to-date project, which the call-site gate alone would miss). 3. _archive_extension_directory() followed file symlinks (is_file() + ZipFile.write() dereference), so a symlink in a source directory could turn out-of-tree bytes into a regular archive member before the hardened extractor sees it. Symlinks are now skipped, matching the rule in compute_extension_content_hash(). 4. The CI guard's fallback comparison only required inequality for non-dotted-numeric versions, so a PEP 440 prerelease downgrade like 2.0.0 -> 1.0.0rc1 passed. The script now compares with packaging.version.Version - the same semantics extension update/install use - and fails closed on unparseable versions; the workflow installs packaging alongside pyyaml. Verified locally that 1.1.0 -> 1.0.0rc1 with content changes is now rejected. Refs github#4345 Assisted-by: Claude Code (model: claude-fable-5, autonomous) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review round 1 addressed in 18a12b9. All four findings were valid; each is applied with regression tests:
Local verification: 74 tests across the affected suites pass, — Claude Code (model: claude-fable-5, autonomous), posted on behalf of @CrazyBaran |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/scripts/check_extension_version_bump.py:71
- The new CI guard is not exercised by an automated test, so regressions in its diff parsing or version comparisons could silently disable the recurrence protection. Add tests that run
mainagainst temporary git histories for an unbumped content change, downgrade, catalog mismatch, valid bump, new extension, removal, and uncataloged exemption; the repository already tests the analogous.github/scripts/check_security_requirements.pyintests/test_security_workflow.py.
def main(argv: list[str]) -> int:
…he catalog Copilot review round 2 on github#4351: the offer gate only blocked a bundled copy that was no newer than the installation. With installed v1, locally bundled v2, and catalog v3, the command offered and installed v2 and reported success - leaving the project lagging the catalog with no mention of it, contrary to the documented "the update is reported as requiring a spec-kit upgrade first" behavior (and re-nagging about the upgrade on every subsequent run). Compare the bundled version against the catalog version instead: any older local copy is blocked with the upgrade-spec-kit guidance. This subsumes the previous gate (inside the catalog > installed branch, bundled <= installed implies bundled < catalog). A local copy at or above the catalog version (dev/source checkouts) is still offered and installed. Tests added for the intermediate-version block and the newer-than-catalog install. Refs github#4345 Assisted-by: Claude Code (model: claude-fable-5, autonomous) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review round 2 addressed in 002443a. The finding was valid: with installed v1, locally bundled v2, and catalog v3, the offer gate installed the intermediate v2 and reported success, contrary to the documented "upgrade spec-kit first" behavior. The gate now blocks whenever the local bundled copy lags the catalog version (subsuming the previous installation-lagging check), while a local copy at or above the catalog version — a dev/source checkout — is still offered and installed. Regression tests added for both sides; 67 tests across the affected suites pass and — Claude Code (model: claude-fable-5, autonomous), posted on behalf of @CrazyBaran |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
.github/workflows/extension-version-guard.yml:15
- This filter does not run the guard when its Python implementation or workflow is modified. Because the regular lint job only checks
srcandtests, a future syntax/runtime regression in.github/scripts/check_extension_version_bump.pycould merge without this job ever executing. Include the guard script and workflow paths so changes to the protection validate themselves.
paths:
- "extensions/**"
.github/scripts/check_extension_version_bump.py:90
- The new recurrence guard has no automated tests for its rejection paths; this workflow only exercises it against the current passing diff. Add tests for an unbumped content change, downgrade/prerelease, manifest/catalog desync, new extension, removal, and uncataloged exemption so changes cannot silently weaken the merge guard.
# -- Invariant 1: content change requires a version bump ---------------
changed = _git(
"diff", "--name-only", "--no-renames", base_ref, head_ref, "--", EXTENSIONS_ROOT
).splitlines()
extensions/EXTENSION-DEVELOPMENT-GUIDE.md:629
- This newly added rationale says the update command compares versions only, but this PR also makes it compare content hashes and report stale bundled copies. Clarify that automatic update installation remains version-driven while hash detection is advisory and requires a forced refresh; otherwise this guide contradicts the new reference documentation.
- **Bump on every content change**: `specify extension update` compares
versions only, so a content change shipped without a version bump never
reaches already-installed copies. For the bundled extensions in this
…em in fixtures Copilot review round 3 on github#4351: the agent-context bump left every checked-in bundle pinned to 1.0.0. BundleExtensionPrimitive enforces exact pins against the bundled manifest, so the offline installs in tests/integration/test_bundler_local_install.py and test_bundler_init_install.py failed, and all four examples/bundles/*/bundle.yml examples stopped being installable. - examples/bundles/{business-analyst,developer,product-manager, security-researcher}/bundle.yml: agent-context pin 1.0.0 -> 1.1.0 (exact pins are the point of the example format, so they stay literal). - The two integration-test fixtures now resolve the pin through a new tests/bundler_helpers.bundled_extension_version() helper, which reads the version via the same _locate_bundled_extension lookup the primitive enforces against - so the fixtures test the bundler's pin mechanics rather than a frozen version literal, and the next legitimate extension bump cannot silently break them again. The git and assess extensions are not pinned by any checked-in bundle; tests/contract/test_bundle_cli.py's 1.0.0 pin feeds `bundle validate`, which checks existence only, and keeps passing unchanged. Refs github#4345 Assisted-by: Claude Code (model: claude-fable-5, autonomous) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review round 3 addressed in edb30a4. The finding was valid — and it caught a real hole in my earlier verification, which had run the extension suites but not
This round the full local suite was run, not just the affected subsets: 6751 passed; the 68 failures on this Windows workstation are all pre-existing environmental issues (symlink tests requiring developer-mode privileges, plus four setup-tasks tests tripping over an unquoted space-containing user path), none in code this PR touches. — Claude Code (model: claude-fable-5, autonomous), posted on behalf of @CrazyBaran |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.github/workflows/extension-version-guard.yml:15
- The workflow does not run when its checker or workflow definition is changed, because the path filter only includes
extensions/**. A checker-only PR can therefore introduce a syntax/runtime regression without exercising this guard. Include both guard files in the trigger paths.
paths:
- "extensions/**"
.github/scripts/check_extension_version_bump.py:90
- This critical recurrence guard has no automated coverage for its diff/version branches; the scenarios in the PR description were only exercised manually. The repository tests the analogous GitHub workflow checker in
tests/test_security_workflow.py, so add tests using temporary Git repositories for unbumped changes, downgrades, catalog mismatches, additions/removals, and exemptions to prevent the guard itself from silently regressing.
# -- Invariant 1: content change requires a version bump ---------------
changed = _git(
"diff", "--name-only", "--no-renames", base_ref, head_ref, "--", EXTENSIONS_ROOT
).splitlines()
…leness Copilot review round 4 on github#4351: two documentation spots still described `specify extension update` as comparing "versions only" / "purely" by semver, with unbumped content reporting "Up to date" forever - wording this PR itself made stale when it added the content-hash check. Clarify in extensions/EXTENSION-DEVELOPMENT-GUIDE.md (Versioning) and the .github/scripts/check_extension_version_bump.py module docstring that update offers remain version-driven - a bump is still required for automatic delivery - while the content-hash check on bundled extensions is only an advisory stale-content warning pointing at a manual --force reinstall. Wording only; no behavior change. Refs github#4345 Assisted-by: Claude Code (model: claude-fable-5, autonomous) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review round 4 addressed in 16c5642. Both findings were valid documentation inconsistencies this PR introduced against itself: the development guide's Versioning bullet and the guard script's module docstring still described update discovery as comparing "versions only" / "purely" by semver, which stopped being the whole story once the content-hash check landed. Both spots now distinguish the two mechanisms: update offers remain version-driven (a bump is required for automatic delivery), while the content-hash check on bundled extensions is an advisory stale-content warning pointing at a manual — Claude Code (model: claude-fable-5, autonomous), posted on behalf of @CrazyBaran |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
docs/reference/extensions.md:80
- This describes a live comparison of the installed files, but
_bundled_content_is_stale()normally compares the registry's install-timecontent_hashand only hashes the installed directory for legacy entries without that field. Consequently, editing or deleting a shipped non-config file after a current install still reports “Up to date,” while the same edit on a legacy registry entry is flagged. Either hash the installed directory consistently or document that this checks the content snapshot recorded at installation; the CLI message at_commands.py:1816-1817needs the same clarification.
When an installed bundled extension's files differ from the copy shipped with your spec-kit release even though the versions match (content that shipped without a version bump), the check flags it as stale content and points to the refresh command:
.github/scripts/check_extension_version_bump.py:75
- The new guard's decision logic has no automated tests; the contract test only checks manifest/catalog synchronization and never exercises the PR-diff version-bump rule. Add tests for an unbumped content change, downgrade/prerelease comparison, new/removed or uncataloged extensions, and catalog desynchronization. Similar GitHub support scripts are loaded and tested directly in
tests/test_security_workflow.py, so relying on manual scenarios leaves this recurrence-prevention path itself unprotected.
def main(argv: list[str]) -> int:
There was a problem hiding this comment.
🟡 Changes recommended
The primary CI guard lacks automated tests for its rejection paths and exemptions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Balanced
Copilot review round 5 on github#4351: the guard script (.github/scripts/check_extension_version_bump.py) is the primary regression prevention in this series, but its failure behavior was only ever exercised manually against throwaway commits - a change to its diff or parsing logic could have silently disabled it while CI stayed green. The existing contract test only checks the working tree's manifest/catalog sync, never the base/head diff logic. Add tests/contract/test_extension_version_guard_script.py: each case builds a real throwaway git repository (one cataloged and one uncataloged extension at base) and runs the script against base/head SHAs exactly as the extension-version-guard.yml workflow does, asserting exit codes and diagnostics for: - valid bump (content + manifest + catalog) -> pass - unbumped content change -> fail ("did not increase") - plain downgrade and PEP 440 prerelease downgrade -> fail - manifest bump without catalog sync -> fail ("must move together") - uncataloged extension change -> exempt, pass - new cataloged extension with no base version -> pass - unparseable version -> fails closed Refs github#4345 Assisted-by: Claude Code (model: claude-fable-5, autonomous) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review round 5 addressed in 6a7c475. The finding was valid: the guard script is this series' primary regression prevention, yet its failure behavior had only been verified manually — a change to its diff or parsing logic could have silently disabled it while CI stayed green. New — Claude Code (model: claude-fable-5, autonomous), posted on behalf of @CrazyBaran |
There was a problem hiding this comment.
🟡 Changes recommended
The new ZIP-based bundled update path strips executable permissions from installed shell scripts on POSIX systems.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Balanced
…ate route Copilot review round 6 on github#4351 reported that bundled updates lose executable permissions on POSIX because safe_extract_archive writes ZIP members without reapplying their recorded modes. The premise about the extractor is accurate, but the conclusion does not hold: every archive install funnels into install_from_directory (install_from_zip -> install_from_archive -> install_from_directory), whose trailing ensure_executable_scripts() call re-establishes execute bits on every .specify/extensions/**/*.sh shebang script - pre-existing behavior whose code comment describes exactly this stripped-mode scenario, and which covers all POSIX-invoked scripts the bundled extensions ship (.ps1 and .py files are interpreter-invoked and need no execute bit). What the report does expose is that nothing pinned this round trip: deleting the restoration call would have broken bundled updates exactly as described while CI stayed green. Add the suggested regression test - build an archive from a source with an executable script via _archive_extension_directory, install it through install_from_zip, and assert the installed script's execute bit. POSIX-only (execute bits do not exist on Windows; the helper no-ops there). Refs github#4345 Assisted-by: Claude Code (model: claude-fable-5, autonomous) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review round 6 addressed in 11d99bd. This finding is the first of the series that did not survive verification: the extractor does drop ZIP modes as described, but every archive install funnels into The valuable half of the report was the missing coverage: nothing pinned that round trip, so removing the restoration call would have broken bundled updates silently. The suggested regression test is adopted — — Claude Code (model: claude-fable-5, autonomous), posted on behalf of @CrazyBaran |
There was a problem hiding this comment.
🔵 Needs a closer look
Content hashing reads arbitrary local extension files fully into memory after installation has already modified project state.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/extensions/init.py:782
read_bytes()loads each file completely into memory. Local-directory installs are not subject to the archive member-size limits, so a large extension asset can exhaust memory here after the destination and registrations have already been modified;MemoryErroris also not covered by theOSErrorfallback. Stream file contents in chunks, asExtensionManifest.get_hash()already does.
data = entry.read_bytes()
h.update(entry.relative_to(ext_dir).as_posix().encode("utf-8"))
h.update(b"\x00")
h.update(len(data).to_bytes(8, "big"))
h.update(data)
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Thanks for the thorough work here. This PR has grown to cover four distinct concerns: local bundled updates, version/catalog bumps, CI enforcement, and content-hash staleness detection. Would you be open to splitting these into separate PRs? A natural sequence might be:
This would reduce the blast radius, simplify review and rollback, and allow the live catalog change to be sequenced safely. The hash mechanism is also defense-in-depth rather than required for the immediate fix, so it can be evaluated independently. Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol). |
11d99bd to
874623d
Compare
…em in fixtures Copilot review round 3 on github#4351: the agent-context bump left every checked-in bundle pinned to 1.0.0. BundleExtensionPrimitive enforces exact pins against the bundled manifest, so the offline installs in tests/integration/test_bundler_local_install.py and test_bundler_init_install.py failed, and all four examples/bundles/*/bundle.yml examples stopped being installable. - examples/bundles/{business-analyst,developer,product-manager, security-researcher}/bundle.yml: agent-context pin 1.0.0 -> 1.1.0 (exact pins are the point of the example format, so they stay literal). - The two integration-test fixtures now resolve the pin through a new tests/bundler_helpers.bundled_extension_version() helper, which reads the version via the same _locate_bundled_extension lookup the primitive enforces against - so the fixtures test the bundler's pin mechanics rather than a frozen version literal, and the next legitimate extension bump cannot silently break them again. The git and assess extensions are not pinned by any checked-in bundle; tests/contract/test_bundle_cli.py's 1.0.0 pin feeds `bundle validate`, which checks existence only, and keeps passing unchanged. Refs github#4345 Assisted-by: Claude Code (model: claude-fable-5, autonomous) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Publishing the live catalog requires coordination with the prerequisite bundled-update release to avoid advertising uninstallable updates.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
@CrazyBaran Did I miss 1/4 ? |
…kage Bundled extensions (agent-context, git, assess) have no download URL, so `specify extension update` could offer a version bump it then failed to install: step 5 unconditionally called catalog.download_extension(), which errors out for catalog entries without a URL (github#4345). Resolve the update source for bundled extensions from the copy shipped with the running spec-kit release instead: - `_bundled_update_source()` locates the local bundled copy and parses its manifest version. - `_archive_extension_directory()` packages that copy as a ZIP so the update flows through the identical hardened archive pipeline (bounded extraction, manifest preflight, ID/version checks, backup/rollback) rather than growing a second install path. Symlinks are never followed into the archive. - When the local copy lags the catalog (or is missing), the update is blocked with an explicit "upgrade spec-kit, then rerun" message instead of installing an intermediate version or crashing; when the local copy is newer than the catalog, it installs the local version. Tests pin the install-from-local-copy route, every blocked-update branch, the newer-local-copy case, archive content/symlink behavior, and execute-bit restoration through the archive install route (POSIX-only; install_from_directory's trailing ensure_executable_scripts() re-establishes modes that ZIP extraction drops). Part 1 of the series requested in review on github#4351; refs github#4345. Assisted-by: Claude Code (model: claude-fable-5) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
874623d to
8ea2367
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation reuses existing validation and rollback paths with comprehensive coverage of its failure modes.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
No no, I thought it can be merged version fix first, but indeed need to be in that order. Let me prepare PR part 2 and 3. |
|
So once the review is done we are ready to merge this one first? |
There was a problem hiding this comment.
🟢 Approval recommended
The implementation follows the existing hardened installation pipeline and adequately covers its new behavior and edge cases.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
That's right, I had also updated description in first comment on top. What was happen and what PRs are need to be merged. |
|
Thank you! |
Fixes the delivery half of #4345: bundled extensions (
agent-context,git,assess) have no download URL, sospecify extension updatecould offer a version bump it then failed to install — step 5 unconditionally calledcatalog.download_extension(), which errors out for catalog entries without a URL.Scope (narrowed per review)
Following @mnriem's request to split the original PR into four parts, this PR was rebased onto latest
mainand reduced to part 1 — local bundled-update support, the piece suggested to be released first:_bundled_update_source()resolves a bundled extension's update source from the copy shipped with the running spec-kit release and parses its manifest version._archive_extension_directory()packages that copy as a ZIP so the update flows through the identical hardened archive pipeline downloads use (bounded extraction, manifest preflight, ID/version checks, backup/rollback) — no second install code path. Symlinks are never followed into the archive.docs/reference/extensions.mddocuments where bundled updates come from.All content is byte-identical to what was already reviewed here through the previous rounds — only re-partitioned.
The other parts
main). Changes the live catalog released CLIs fetch, so it should land after this PR has shipped in a release.main, fully independent).feat/4345-content-staleness-detection, stacked on this PR's commit because_bundled_content_is_stale()builds on_bundled_update_source()and the shared test scaffolding introduced here; it will be rebased ontomainonce this merges.Fixes #4345 (together with the follow-up PRs above).
🤖 Generated with Claude Code