use install.sh from last release, not main + pin deps in installations#2434
Open
evgunter wants to merge 9 commits into
Open
use install.sh from last release, not main + pin deps in installations#2434evgunter wants to merge 9 commits into
evgunter wants to merge 9 commits into
Conversation
…installer Ship a lockfile-derived constraints file so end-user installs resolve to the same third-party versions CI tested, fixing the structural cause of issue #14 (and issue #5, where `mngr plugin add imbue-mngr-azure` pulled an untested azure-mgmt-resource release). Also settle issue #2 by serving both the installer script and the constraints file from a CI-managed `release` branch, so script and constraints are always a matched, published pair. - Generate `libs/mngr/constraints.txt` from the workspace lock via `uv export` (`scripts/make_constraints_file.py`); force-include it into the wheel at `imbue/mngr/constraints.txt`. - Apply it via `uv tool install --constraints` at `mngr plugin add` (packaged, self-contained) and in `scripts/install.sh` (fetched from `release`). - Unify artifact regeneration under `just regenerate` (`scripts/regen.py`) with one drift-guard test covering the CLI docs, agent-capability doc, and the new constraints file. - Fast-forward the `release` branch on each mngr release (publish.yml); repoint the README installer curls at `release`. - Make the release-time exclude-newer advance advisory so the shipped constraints are provably the CI-tested ones. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nstall-script test setup Address review of the pinning changes: - Remove the import-time assert introduced in builtin_help_topics.py. It converted a lazy, localized "one help topic can't find its file" into a hard crash on module import. Revert to the original standalone docs-root resolution (prefer the packaged copy, else the source tree) -- resolve_shipped_path now backs only uv_tool.py, where its None-means-absent contract fits. - Make constraint pinning fail loudly instead of silently skipping. The single lockfile-derived constraints.txt (the whole third-party tree, force-included in the wheel and committed in source) is present in every real install, so its absence signals a broken mngr installation, not a per-plugin fault. with_shipped_ constraints now raises AbortError when the file is missing, for plugin add, the install wizard, and plugin remove alike (a resolver seam keeps this unit-testable without monkeypatch). Fixes the changelog wording that wrongly described the file as generated-on-demand and pinning as best-effort. - Extract a _setup_and_run helper in test_install_script.py so the seven tests no longer each repeat the bin-dir / log-file / uv+mngr+curl mock setup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…est setup The style guide (style_guide.md:1105) says to avoid default arguments and require callers to pass all parameters. Drop the ones added with the pinning work: - with_shipped_constraints no longer takes a `resolve` parameter with a default (it was only a test seam). Split the abort/append decision into a pure helper `_constraints_arg_or_abort(command, constraint_path)` that is testable directly with a None path, and keep with_shipped_constraints a thin resolving wrapper. - test_install_script.py's `_setup_and_run` now requires all four scenario parameters explicitly at each call site, and the `_mngr_mock` / `_curl_mock` builders no longer default their flags. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both parameters (`receipt_path` and `is_plugin_package`) existed only as test injection seams -- the sole production caller (the tab-completion cache writer) always wants the live receipt and the real has_mngr_entry_points check. Per the style guide (avoid default arguments), make the public function take no arguments and move the testable logic into two helpers with required parameters: - `_read_receipt_or_none(receipt_path)` -- parse a receipt, returning None (with a warning) on a garbled/unreadable one, preserving the existing best-effort contract for the background completion writer. - `_plugin_package_names_from_receipt(receipt, is_plugin_package)` -- the pure sorted/deduped/filtered extraction. Tests now exercise those helpers directly instead of injecting into the public function. Behavior is unchanged; the production call site in list.py is untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…in plugin.py resolve_shipped_path had a single caller left (with_shipped_constraints) after builtin_help_topics reverted to its own inline resolution, so the shared-util rationale was gone. Inline it as a private _resolve_shipped_path in uv_tool.py (anchored on this file's own depth), delete utils/paths.py + paths_test.py, and move the three resolution tests into uv_tool_test.py. Also drop plugin.py comments that just restated the next few lines (section-header "Report results" / "Resolve package names" / "Verify all packages" comments, and the reiterating halves of the two constraints comments), keeping the genuine "why" comments (arg-validation ordering, git-diffing rationale). The abort-on-missing detail now lives only in the with_shipped_constraints docstring.
… wording Manual end-to-end testing of the two-step upgrade path revealed a real bug: the constrained `uv tool install imbue-mngr --constraints` after `uv tool upgrade` was run WITHOUT re-listing the installed plugins, so uv reset the tool to just its base package -- silently removing every plugin on upgrade. (`uv tool upgrade` preserves plugins but ignores constraints; a bare constrained install applies constraints but drops extras; only re-listing the extras as `--with` does both.) Fix the upgrade branch to enumerate existing plugins via `uv tool list --show-with` and re-list them as `--with` on the constrained install. Add a regression test (`test_install_sh_relists_plugins_on_upgrade`) exercising the real bash against a mock uv that reports plugins, which the previous mock-based tests could not catch. The mngr-side `plugin add`/`remove` builders already re-list extras, so they were never affected. Also, per review: - Make the installer's user-facing constraints comment a single concise line. - Trim the release.py exclude-newer comments to what they do. - Have the release exclude-newer advisory warn against setting the cutoff newer than the 2-week cooldown without a specific reason (supply-chain/malware guard). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem: scripts/install.sh created the constraints file via mktemp (line 50) but never removed it, so under 'set -euo pipefail' the temp file leaked on every install/upgrade run and on any early exit. Fix: register a 'trap ... EXIT' immediately after mktemp so the temp file is removed reliably on all exit paths (success, error, or set -e abort). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… window) The previous upgrade path ran `uv tool upgrade imbue-mngr` first, which ignores constraints, so it would briefly resolve and install the newest dependency releases from PyPI -- bypassing the tested-versions pin -- before the constrained install could downgrade them. That transient unpinned resolution is an attack surface for a freshly published (e.g. <2-week-old, not-yet-detected) malicious dependency, and it also defeats the exclude-newer cooldown the constraints encode. Replace the two-branch upgrade/install logic with a single `uv tool install imbue-mngr@latest --with <plugins> --constraints <file>`. Verified against real uv that this upgrades mngr to the latest release, applies the pinned constraints, and preserves plugins in ONE constrained resolution -- deps are never resolved unpinned, even transiently -- and that it is idempotent on re-run. Update the tests accordingly (assert no `uv tool upgrade`, expect `@latest`), and reword the `_resolve_shipped_path` docstring: the packaged and source-checkout locations are mutually exclusive per install, so it is a two-location lookup rather than a preference between copies that coexist. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The release branch now governs both the curl|bash installer and the pinned dependency versions for every fresh install. Add an environment: release gate to the update-release-branch job so a repo admin can require human approval before the install-governing branch moves. Until reviewers are configured the environment exists but imposes no gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
apparently
uv.lockwas only working for the dev setup...also fixes #2391 (but we should cut a new release too so that the new install.sh is live, instead of relying on this PR effectively rolling install.sh back)
admin actions that should happen before merge (see "Security follow-ups" below for more context):
release(the branchinstall.shis now read from instead of main). only theupdate-release-branchjob needs to be able to push to releasereleaseenvironmentalso note that the new installer installs imbue-mngr@latest, which technically doesn't match
constraints.txtfor a moment during the release (or potentially longer if there's an error). i can fix this by having it read aversion.txtfile instead of usinglatestif this is undesirableSummary
Ships a lockfile-derived constraints file so end-user installs resolve to the same third-party versions CI tested. Today published wheels carry only loose
Requires-Distranges, so every end-user install andmngr plugin adddoes a fresh, unpinned PyPI resolution -- the repo's tested pins never reach users. This is what letmngr plugin add imbue-mngr-azurepull an untestedazure-mgmt-resourcerelease that devs (who got the locked version) never saw. It also serves both the installer script and the constraints file from a single CI-managedreleasebranch, so the installer and its constraints are always a matched, published pair tied to the last release.Fixes the root cause of #2391: because
install.shis now fetched fromrelease(the last-released commit) instead ofmain, it can never reference CLI subcommands that don't exist in the version it installs -- the version skew that made a fresh install end inError: No such command 'wizard'.What changed
scripts/make_constraints_file.pyrunsuv export --all-packages --no-emit-workspace --no-dev --no-hashesto produce third-party-only pins atlibs/mngr/constraints.txt, force-included into the wheel atimbue/mngr/constraints.txt.mngr plugin add. The wizard andplugin add/removebuilders now append--constraints <shipped file>touv tool install. The file (one overall constraints file, committed and force-included into the wheel) is present in every real install, so if it is ever missing that means the mngr install itself is broken -- add, the wizard, and remove all abort with a clear error rather than silently resolving unpinned.scripts/install.shfetches the constraints from thereleasebranch (required, fails naturally underset -euo pipefail) and installs/upgrades in a single constrained resolution:uv tool install imbue-mngr@latest --with <existing plugins> --constraints <file>. It re-lists existing plugins (enumerated fromuv tool list --show-with) so an upgrade keeps them -- a bareuv tool install imbue-mngrwould reset the tool to just its base package. This deliberately avoids anuv tool upgradefirst:uv tool upgradeignores constraints, so it would briefly resolve and install the newest unpinned dependencies from PyPI (an attack surface for a freshly published malicious release, and a bypass of the exclude-newer cooldown) before a second command could downgrade them. The single constrained install never resolves unpinned, even transiently. Verified against realuv(upgrades base, applies pins, preserves plugins, idempotent).just regenerate(scripts/regen.py) regenerates the CLI docs, agent-capability doc, and constraints file under one drift-guard test (scripts/regen_test.py).releasedelivery pointer.publish.ymlfast-forwardsreleaseto each mngrv*release commit; README installer curls repointed atrelease. The promotion job now runs under a dedicatedreleasedeployment environment (see security follow-ups below).scripts/release.pyno longer auto-advances the cooldown cutoff (now ordinary dependency maintenance via a PR to main); it only warns when stale, alongside the consolidatedrun_advisory_checks(). This keeps the shipped constraints provably identical to what CI tested.Test plan
scripts/regen.py --checkclean; wheel build confirmedimbue/mngr/constraints.txtpresent.uv:uv tool install imbue-mngr@latest --with <plugins> --constraints <file>upgrades the base package, applies the pins, and preserves existing plugins in one resolution (idempotent) with no unconstrained window -- whereas a bare constrained install without--withdrops the plugins. Crystallized astest_install_sh_installs_at_latest_without_unconstrained_upgrade(asserts@latest --constraintsand nouv tool upgrade) andtest_install_sh_relists_plugins_when_already_installed(real bash against a mock uv that reports plugins).uv_tool_test,test_install_script,release_test,regen_test,make_agent_capabilities_doc_test.test_no_type_errorsis left to CI:ty checkOOMs on this local WSL box (5.7 GiB), reporting 0 errors whenever it runs to completion.Security follow-ups
A security pass over this change surfaced a handful of items. The two substantive ones -- a bug where an upgrade dropped all plugins, and an unconstrained
uv tool upgradewindow that briefly installed unpinned deps -- are already fixed (see the base-install bullet above). The rest:release.releasenow governs both thecurl | bashinstaller and the dependency pins for every fresh install. The automated promotion is already as gated as PyPI publishing:update-release-branchruns onlyneeds: publish(which runs in thepypienvironment), and this PR additionally puts it under areleaseenvironment. So it opens no new hole in the CI path. What is newly consequential is a direct push toreleaseby anyone with repo write access, bypassing CI entirely -- now thatreleasedrives installs, that would poison them. An admin should add a branch-protection rule / ruleset onreleasethat restricts direct pushes to the CI actor and blocks force-push and deletion. (I don't have admin on this repo.)v*tag protection. Creating av*tag triggers the publish + release-branch chain. An admin should restrictv*tag creation to release managers.releaseenvironment. This PR addsenvironment: releaseto the promotion job; an admin should add required reviewers to that environment so moving the install-governing branch needs a human approval. Until then the environment exists but imposes no gate.@latest.install.shinstallsimbue-mngr@latestwhile fetching constraints fromrelease. CI advancesreleaseimmediately after the PyPI publish in the same workflow (needs: publish), so the wheel and its constraints are normally a matched pair. The only skew window is brief and self-healing -- it requires the release-branch push to lag the publish -- and if it occurred, a brand-new transitive dependency in the newer wheel could resolve unpinned untilreleasecatches up. We keep@latestrather than parsing an explicit version in the installer, to keep thecurl | bashscript simple for users to read.Bootstrap ordering
The
releasebranch now exists, pointing at the last released tagv0.2.17-- so the repointed README URLs resolve immediately andpublish.yml's fast-forward has a base to advance from (v0.2.17is an ancestor ofmain). Atv0.2.17the branch carries the oldinstall.sh(which does not fetch constraints, so no 404) and noconstraints.txt; both the constraints file and the fetch-enabled installer land onreleasetogether at the next release. Net effect: the#2391fix and constraint pinning for the base installer go fully live at the first release cut after this merges; until then the installer behaves exactly as it does today.🤖 Generated with Claude Code