Skip to content

Fix #4345 (1/4): install bundled extension updates from the local package - #4351

Merged
mnriem merged 1 commit into
github:mainfrom
CrazyBaran:fix/4345-bundled-extension-version-bumps
Sep 1, 2026
Merged

Fix #4345 (1/4): install bundled extension updates from the local package#4351
mnriem merged 1 commit into
github:mainfrom
CrazyBaran:fix/4345-bundled-extension-version-bumps

Conversation

@CrazyBaran

@CrazyBaran CrazyBaran commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes the delivery half of #4345: 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.

Scope (narrowed per review)

Following @mnriem's request to split the original PR into four parts, this PR was rebased onto latest main and 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.
  • 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; a local copy newer than the catalog 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).
  • docs/reference/extensions.md documents 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

  1. Version/catalog bumps + bundle pinsFix #4345 (2/4): bump drifted bundled extension versions and sync the catalog #4394 (based on latest main). Changes the live catalog released CLIs fetch, so it should land after this PR has shipped in a release.
  2. CI version-bump guardFix #4345 (3/4): CI guard requiring version bumps on bundled extension changes #4395 (based on latest main, fully independent).
  3. Content-hash staleness detectionfeat/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 onto main once this merges.

Fixes #4345 (together with the follow-up PRs above).

🤖 Generated with Claude Code

@CrazyBaran
CrazyBaran requested a review from mnriem as a code owner August 27, 2026 07:35
Copilot AI balanced review requested due to automatic review settings August 27, 2026 07:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/specify_cli/extensions/__init__.py Outdated
Comment thread src/specify_cli/extensions/_commands.py Outdated
Comment thread src/specify_cli/extensions/_commands.py
Comment thread .github/scripts/check_extension_version_bump.py Outdated
CrazyBaran pushed a commit to CrazyBaran/spec-kit that referenced this pull request Aug 27, 2026
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>
Copilot AI review requested due to automatic review settings August 27, 2026 08:02
@CrazyBaran

CrazyBaran commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Review round 1 addressed in 18a12b9. All four findings were valid; each is applied with regression tests:

  1. Content-hash config exclusion narrowed to top-level files — matching the _target_follows_preserved_convention preservation semantics, so nested *-config.yml files (which installs overwrite) now count toward staleness.
  2. Stale-content check gated to matching versionscatalog_version == installed_version at the call site, plus the bundled copy must declare the installed version inside the helper; both skew directions are now inert instead of steering users into a downgrading --force refresh.
  3. Update archiving no longer follows symlinks — consistent with compute_extension_content_hash(); out-of-tree bytes can't become regular archive members ahead of the hardened extractor.
  4. CI guard compares versions per PEP 440 (packaging.version) and fails closed on unparseable versions, so prerelease downgrades such as 2.0.0 -> 1.0.0rc1 can no longer bypass it; the workflow installs packaging.

Local verification: 74 tests across the affected suites pass, ruff clean, and the guard script was re-exercised against its positive/negative scenarios including the new prerelease-downgrade case.

— Claude Code (model: claude-fable-5, autonomous), posted on behalf of @CrazyBaran

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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 main against 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.py in tests/test_security_workflow.py.
def main(argv: list[str]) -> int:

Comment thread src/specify_cli/extensions/_commands.py Outdated
CrazyBaran pushed a commit to CrazyBaran/spec-kit that referenced this pull request Aug 27, 2026
…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>
Copilot AI review requested due to automatic review settings August 27, 2026 08:12
@CrazyBaran

CrazyBaran commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

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 ruff is clean.

— Claude Code (model: claude-fable-5, autonomous), posted on behalf of @CrazyBaran

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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 src and tests, a future syntax/runtime regression in .github/scripts/check_extension_version_bump.py could 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

Comment thread extensions/agent-context/extension.yml Outdated
CrazyBaran pushed a commit to CrazyBaran/spec-kit that referenced this pull request Aug 27, 2026
…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>
Copilot AI review requested due to automatic review settings August 27, 2026 08:42
@CrazyBaran

CrazyBaran commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

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 tests/integration/. Both offline bundle-install tests failed exactly as described against the exact-pin enforcement, and all four examples/bundles/*/bundle.yml examples had become uninstallable.

  • The four example bundles now pin agent-context 1.1.0 (pins stay literal there — exact pins are the point of the format).
  • The two integration-test fixtures 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 keep exercising the bundler's pin mechanics and a future legitimate bump cannot silently break them again.
  • git/assess are not pinned by any checked-in bundle; test_bundle_cli.py's pin feeds bundle validate (existence check only) and passes unchanged.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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()

Comment thread extensions/EXTENSION-DEVELOPMENT-GUIDE.md Outdated
Comment thread .github/scripts/check_extension_version_bump.py Outdated
CrazyBaran pushed a commit to CrazyBaran/spec-kit that referenced this pull request Aug 27, 2026
…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>
Copilot AI review requested due to automatic review settings August 27, 2026 08:54
@CrazyBaran

Copy link
Copy Markdown
Contributor Author

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 --force reinstall. Wording only — no behavior change; ruff clean and the guard script re-verified against the branch.

— Claude Code (model: claude-fable-5, autonomous), posted on behalf of @CrazyBaran

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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-time content_hash and 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-1817 needs 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:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

Comment thread .github/scripts/check_extension_version_bump.py Outdated
CrazyBaran pushed a commit to CrazyBaran/spec-kit that referenced this pull request Sep 1, 2026
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>
Copilot AI review requested due to automatic review settings September 1, 2026 06:25
@CrazyBaran

Copy link
Copy Markdown
Contributor Author

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 tests/contract/test_extension_version_guard_script.py (8 tests): each case builds a real throwaway git repository with a cataloged and an uncataloged extension, then runs the script against base/head SHAs exactly as the extension-version-guard.yml workflow does. Covered: valid bump passes; unbumped content change, plain downgrade, PEP 440 prerelease downgrade, and manifest-without-catalog bump all fail with their specific diagnostics; uncataloged changes are exempt; a new cataloged extension passes with no base version; unparseable versions fail closed. All 8 pass locally alongside the contract/unit suites (611 passed; the only 2 failures are this workstation's known symlink-privilege environmental ones).

— Claude Code (model: claude-fable-5, autonomous), posted on behalf of @CrazyBaran

Copilot AI review requested due to automatic review settings September 1, 2026 06:43
@CrazyBaran

Copy link
Copy Markdown
Contributor Author

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 install_from_directory, whose trailing ensure_executable_scripts() call restores execute bits on all .specify/extensions/**/*.sh shebang scripts — pre-existing behavior whose code comment names exactly this scenario. Bundled updates through the new archive route therefore end with executable scripts, and the Permission denied outcome cannot occur (details with file/line references in the inline reply).

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 — test_archive_route_restores_script_execute_bits archives a source with an executable script, installs it via install_from_zip, and asserts the installed execute bit. POSIX-only (skips on Windows, where execute bits don't exist and the helper no-ops); it will run on the ubuntu/macos CI cells. ruff clean; the staleness suite passes locally with the new test collected and correctly skipped on this Windows machine.

— Claude Code (model: claude-fable-5, autonomous), posted on behalf of @CrazyBaran

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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; MemoryError is also not covered by the OSError fallback. Stream file contents in chunks, as ExtensionManifest.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

@mnriem

mnriem commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

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:

  1. Local bundled-update support, released first
  2. Version/catalog bumps and bundle-pin updates
  3. CI version-bump guard
  4. Content-hash staleness detection

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).

Copilot AI review requested due to automatic review settings September 1, 2026 17:39
@CrazyBaran
CrazyBaran force-pushed the fix/4345-bundled-extension-version-bumps branch from 11d99bd to 874623d Compare September 1, 2026 17:39
CrazyBaran pushed a commit to CrazyBaran/spec-kit that referenced this pull request Sep 1, 2026
…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>
@CrazyBaran CrazyBaran changed the title Fix #4345: deliver bundled extension updates - catalog-synced bumps, CI guard, staleness detection, local-package installs Fix #4345 (2/4): bump drifted bundled extension versions and sync the catalog Sep 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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

@mnriem

mnriem commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

@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>
Copilot AI review requested due to automatic review settings September 1, 2026 18:02
@CrazyBaran
CrazyBaran force-pushed the fix/4345-bundled-extension-version-bumps branch from 874623d to 8ea2367 Compare September 1, 2026 18:02
@CrazyBaran CrazyBaran changed the title Fix #4345 (2/4): bump drifted bundled extension versions and sync the catalog Fix #4345 (1/4): install bundled extension updates from the local package Sep 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 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

@CrazyBaran

Copy link
Copy Markdown
Contributor Author

@CrazyBaran Did I miss 1/4 ?

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.

@mnriem

mnriem commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

So once the review is done we are ready to merge this one first?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 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

@CrazyBaran

CrazyBaran commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

So once the review is done we are ready to merge this one first?

That's right, I had also updated description in first comment on top. What was happen and what PRs are need to be merged.

@mnriem
mnriem merged commit b5f7708 into github:main Sep 1, 2026
14 checks passed
@mnriem

mnriem commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

mnriem pushed a commit that referenced this pull request Sep 9, 2026
…n changes (#4395)

* ci(extensions): guard bundled extension changes behind a version bump

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 (#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 #4351 (part 3 of the series requested in review); refs #4345.

Assisted-by: Claude Code (model: claude-fable-5)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): make the extension version guard usable as a required check and quoting-proof

Address Copilot review round 1 on #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 #4345

Assisted-by: Claude Code (model: claude-fable-5-1)
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(ci): validate a new extension's version even without a base to compare

Address @mnriem's review on #4395.

For a brand-new extension the guard returned early at "no base manifest"
before ever parsing the head version, and the catalog check only tests
string equality. A new extension shipped with e.g. `not-a-version` in
both extension.yml and catalog.json therefore passed the guard, although
ExtensionManifest rejects a version packaging cannot parse
(ValidationError "Invalid version") and `extension update` skips catalog
entries whose version is invalid - the extension would be uninstallable
and never updatable.

Parse the head version before the new-extension early return and fail
closed with a dedicated message when it is not PEP 440. The base
version is now parsed separately, so an unparseable base still fails
with the "could not compare versions" message.

Tests: add test_new_extension_with_unparseable_version_fails (fails
against the previous script, passes now) and point the existing
unparseable-version test at the new head-version message, which fires
first for that scenario.

Refs #4345

Assisted-by: Claude Code (model: claude-fable-5-1)
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(ci): diff the extension version guard against the merge commit's first parent

The guard diffed github.event.pull_request.base.sha against the checked-out
pull-request merge commit. Those two are not built from the same base: the
payload SHA is the base-branch tip from when the PR was opened and is not
refreshed when the base moves, while refs/pull/N/merge is rebuilt against
the current tip. Any base-branch drift between the two is then attributed
to the PR.

Observed on #4395 itself: base.sha was main as of Sep 1 (3eec154), the
merge commit was built on main as of Sep 3 (db64869), and two unbumped
bundled-extension changes merged in between (#4413 touching agent-context,
#4286 touching git) failed the guard on a PR that never touched either.

Fetch the merge commit with depth 2 so both parents are available and
diff against HEAD^1, which is by construction the base the merge was
built on. Fail loudly if HEAD is not a merge commit. The script's CLI is
unchanged; a regression test reproduces the trap in a throwaway repo (the
stale base blames the drift on the PR, HEAD^1 does not).

Refs #4345

Assisted-by: Claude Code (model: claude-fable-5-1)
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(ci): validate every catalog version, not only those of changed extensions

Address the "needs a closer look" items from the Copilot reviews on #4395.

Invariant 2 skipped catalog entries without an in-repo directory before
parsing their version, and never parsed the in-repo manifest version at
all - PEP 440 validation lived only in Invariant 1, which sees just the
IDs with changed files under extensions/<id>/. Two gaps followed:

- a hosted (catalog-only) entry could carry `"version": "not-a-version"`
  and pass, although `extension update` skips exactly such entries when
  packaging.Version fails;
- promoting an existing uncataloged directory by adding only its catalog
  entry never entered Invariant 1, so a matching invalid string in
  manifest and catalog passed the plain equality check.

Invariant 2 now runs over every catalog entry: the catalog version must
be a non-empty string that parses as PEP 440, and for entries with an
in-repo directory the manifest version must parse as well before the
equality check. The catalog-side parse alone closes the promotion gap
(a valid catalog string plus equality forces a valid manifest string);
the manifest parse makes the failure name the manifest precisely.

Tests: `_run_guard` now omits the head argument by default, matching the
workflow's one-argument invocation, with a separate test for the
optional HEAD_REF; new cases cover the hosted-entry and both promotion
shapes (the two invalid ones fail against the previous script) plus a
valid promotion that must keep passing. The working-tree contract test
gains the matching every-catalog-version-parses check.

Refs #4345

Assisted-by: Claude Code (model: claude-fable-5-1)
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Jakub Baranowski <cr4zybaran@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants