Skip to content

fix: require explicit refresh for bundle version changes - #4477

Open
rksharma-owg wants to merge 3 commits into
github:mainfrom
rksharma-owg:codex/fix/4434-bundle-version-guard
Open

fix: require explicit refresh for bundle version changes#4477
rksharma-owg wants to merge 3 commits into
github:mainfrom
rksharma-owg:codex/fix/4434-bundle-version-guard

Conversation

@rksharma-owg

@rksharma-owg rksharma-owg commented Sep 8, 2026

Copy link
Copy Markdown

Refs #4434

Description

A normal bundle install could advance an existing bundle record to a new version while leaving its owned components unchanged. Version-changing normal installs now fail before primitive work; same-version installs remain idempotent.

Local bundles have an explicit upgrade route: specify bundle install <path> --refresh. The path can be a manifest file, directory, or ZIP. The flag uses the existing refresh implementation, reapplies owned components before advancing the record, and reports refreshed and removed counts. Catalog bundles can continue using specify bundle update <id>. Local sources supply manifests, not component payloads: catalog-backed components still need network access. Offline failures now identify the component and instruct users to retry without --offline, rather than suggesting an ineffective pre-install workaround.

This addresses the existing-bundle version-change case in #4434. Independently installed components remain untouched and unowned under the existing no-collateral-removal policy; the independently-installed-components case remains open for a separate follow-up. Refresh retains the existing bounded rollback contract: previously installed components modified before a failure are not rolled back.

Related changes

Checked #4466#4470. Only #4469 changes the same installer and lifecycle-test files; it moves record persistence into the rollback boundary, while this PR guards version changes and exposes the existing refresh path. Its patch passes git apply --check against this working tree. The other four PRs change separate reference, manifest, workflow, and primitive-version-validation paths.

Testing

  • 115 focused primitive, install-flow, local-source, and bundle CLI tests passed. New manifest/directory/ZIP cases use the real extension installer with mocked catalog I/O, prove offline failure leaves the existing payload and record unchanged, and prove a retry refreshes the catalog-backed component and record to the requested version.
  • New CLI regressions cover manifest, directory, and ZIP upgrades. They fail on the prior commit because --refresh is unavailable, verify ordinary installs preserve the record and installed versions, and verify refresh applies all owned pins before advancing the record.
  • uvx ruff@0.15.0 check src tests: passed.
  • npx --yes markdownlint-cli2 docs/reference/bundles.md: passed.
  • .venv/bin/specify bundle install --help: passed; the refresh option is visible.
  • git diff --check: passed.
  • Earlier full-suite run (not repeated for this review update): 7,525 passed, 195 skipped, 11 failed. The failures are in the PowerShell launcher and composed-template parity tests, the same local environment/platform categories reported before this update; the full local suite is not completely green.

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance

OpenAI Codex worked autonomously on behalf of @rksharma-owg to investigate the feedback, implement the refresh option and regression tests, check related PRs, and run validation. The changes have not received human line-by-line review from the contributor.

@rksharma-owg
rksharma-owg requested a review from mnriem as a code owner September 8, 2026 19:50
@mnriem mnriem added author-awaiting Waiting on author response triage-must-have Verdict: high-value, important work for Spec Kit — do first labels Sep 9, 2026
@mnriem
mnriem requested a balanced review from Copilot September 9, 2026 14:19
@mnriem

mnriem commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks — this correctly fixes the primary #4434 failure (a version-changing normal install advancing the record past stale components), with a clean guard and regression test, and the disclosure is appreciated. One thing before merge: #4434 reported a second case that this doesn't cover — components installed individually before the bundle, where every primitive is classified "already present" so the new bundle record ends up with no contributed-component entries. As written, Closes #4434 would auto-close the issue while that half remains. Please either extend this to handle the individually-installed case (with a test), or change Closes #4434 to Refs #4434 and I'll keep the issue open for a follow-up. Also heads-up: #4466#4470 ([@marcelsafin](https://github.com/marcelsafin)) touch the same installer.py version-integrity path — worth confirming no overlap. I'll trigger the automated review in parallel.

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

Local bundles cannot use the recommended update command, leaving them without an upgrade path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Prevents normal bundle installs from advancing records across bundle-version changes while components remain stale.

Changes:

  • Rejects version-changing installs unless refresh mode is enabled.
  • Adds regression coverage ensuring records and primitives remain unchanged.
File summaries
File Description
src/specify_cli/bundler/services/installer.py Adds the version-change guard and guidance.
tests/integration/test_bundler_install_flow.py Tests rejection and preserved state.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/specify_cli/bundler/services/installer.py Outdated
Assisted-by: OpenAI Codex (autonomous)
@rksharma-owg rksharma-owg changed the title fix: reject bundle version changes during install fix: require explicit refresh for bundle version changes Sep 9, 2026
@mnriem

mnriem commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

The automated review surfaced a bigger issue than my earlier note: rejecting a version-changing bundle install and pointing users to bundle update breaks the exact case #4434 was reported for — local bundles. bundle update only resolves catalog bundles; it can't update a local/path-installed bundle (_local_manifest_source), and the original repro was bundle install <local bundle.yml> --offline. So as written, local-bundle users go from "silent stale record" to "no upgrade path at all," which is worse for them.

Please make the local-bundle case work end to end — either extend bundle update to accept a local path/manifest, or have the version-change guard perform the refresh for local bundles rather than just rejecting. And (from before) confirm whether #4434's second case is in scope — individually-installed components producing a record with no contributed-component entries; if not, change Closes #4434 to Refs #4434 so the issue stays open. Re-request once addressed.

@mnriem
mnriem requested a balanced review from Copilot September 9, 2026 17:25
@mnriem

mnriem commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks — --refresh for local sources is exactly the right fix; the per-source-type error message pointing to bundle update <id> vs install <path> --refresh resolves the dead-end cleanly. (My 17:16 comment crossed with your 15:00 commit — you'd already addressed it.) Re-requesting review/CI on 3b6d00c5; given it touches the bundler, I want the macOS/Windows matrix green before merging. One thing still open: does this cover #4434's second case — components installed individually before the bundle producing a record with no contributed-component entries? If not, let's change Closes #4434 to Refs #4434 and track that separately.

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 documented offline refresh route fails for non-bundled components that require catalog resolution.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread docs/reference/bundles.md Outdated
A normal install rejects a change to an already-recorded bundle's version. To upgrade a local bundle without adding it to a catalog, pass the newer source with `--refresh`:

```bash
specify bundle install ./new-release/bundle.yml --refresh --offline
@mnriem

mnriem commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

The --refresh addition resolves the local dead-end well. One more edge from the re-review: the offline refresh route (bundle install <path> --refresh --offline) fails for owned components that require catalog resolution — DefaultPrimitiveInstaller(allow_network=False) routes refresh through managers that still need the catalog. Since offline/local is exactly #4434's motivating case, this should work end-to-end (or fail with a clear "this component needs network to refresh" message rather than a confusing error). Re-request once handled.

Exercise local manifest, directory, and ZIP refresh through the real extension installer with deterministic catalog artifacts. Preserve state on offline failure and verify the online retry refreshes the owned version.

Assisted-by: OpenAI Codex (model: GPT-6 Astra, autonomous)
@rksharma-owg

rksharma-owg commented Sep 9, 2026

Copy link
Copy Markdown
Author

Posted on behalf of @rksharma-owg by OpenAI Codex (GPT-6 Astra, autonomous).

Updated in 2fd5c88: local-source refresh documentation now separates the manifest from catalog-backed component payloads. Offline errors identify the component and direct users to retry without --offline; they no longer suggest pre-installing an already-owned component. Three new CLI cases exercise manifest, directory, and ZIP sources through the real extension installer, with only catalog I/O substituted by local artifacts. They verify unchanged payload/record on offline failure and successful version refresh on retry.

All 115 focused tests and Ruff passed. The PR continues to use Refs #4434; the independently-installed-component ownership case remains a separate follow-up. The fixes are pushed, but GitHub rejected my review re-request because this account lacks RequestReviewsByLogin permission; a maintainer needs to trigger re-review/CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-awaiting Waiting on author response triage-must-have Verdict: high-value, important work for Spec Kit — do first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants