From bfa652fae87fd7c5c01161188f97a8ed68b5a762 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 2 Nov 2026 19:13:47 -0800 Subject: [PATCH] fix(pm): an automatic sync never undoes a completed `--without` Review finding: configured-platform repair reads the declined extras, then asks PM for the configured extra with explicit=True. A `--without` completing in between was erased: the stale request rebuilt the extra and its opt-back-in clear emptied the decline record. sync_venv now takes `automatic`: the entries of `extras` Hermes chose on the user's behalf. Under the dependency lock a declined automatic extra is left out, and only the remaining (user-requested) extras clear a decline. Every caller is classified: configured-platform repair, the update's and launch migration's legacy carry, the container baseline restore and the carried part of pm install's first selection are automatic; setup flows and `--extra NAME` stay requests. --- hermes_cli/_update_takeover.py | 4 ++- hermes_cli/main_install_repair.py | 7 ++-- hermes_cli/venv_sync.py | 4 ++- pm/cli.py | 14 +++++--- pm/client.py | 8 +++-- pm/install.py | 29 +++++++++++++--- pm/recovery.py | 7 ++-- .../test_update_missing_configured_deps.py | 3 +- tests/pm/test_baked_selection.py | 2 +- tests/pm/test_declined_extras.py | 34 +++++++++++++++++++ 10 files changed, 91 insertions(+), 21 deletions(-) diff --git a/hermes_cli/_update_takeover.py b/hermes_cli/_update_takeover.py index f4b3fad025..c8756e3890 100644 --- a/hermes_cli/_update_takeover.py +++ b/hermes_cli/_update_takeover.py @@ -51,8 +51,10 @@ def prepare(request: dict) -> tuple[Path, dict[str, str]]: # an ordinary sync, which builds and validates a fresh generation too. repair = repair_marker.is_file() and venv_is_current(project_root=root) # An update never fails because of a plugin: misfits are disabled and reported. + # The old venv's extras are carried, not asked for: none of them undoes a `--without`. + automatic = {"automatic": extras} if not repair and extras and extras != ["all"] else {} sync_venv(None if repair else extras, explicit=True, project_root=root, repair=repair, - evict_incompatible_plugins=not repair) + evict_incompatible_plugins=not repair, **automatic) collect_superseded_generations(root) request["pm_receipt"] = receipt.last_for_update(correlation) publish_launchers(root) diff --git a/hermes_cli/main_install_repair.py b/hermes_cli/main_install_repair.py index 0bc23e7457..60c67b9739 100644 --- a/hermes_cli/main_install_repair.py +++ b/hermes_cli/main_install_repair.py @@ -297,11 +297,14 @@ def _install_configured_features_missing_deps(project_root: Path) -> None: if (extra := name.replace("_", "-")) in declared and extra_supported(extra)}) if extras: try: - pm.sync_venv(extras, explicit=True, project_root=project_root, evict_incompatible_plugins=True) + # Config asked for these, not the user: a `--without` landing before the PM lock still wins. + pm.sync_venv(extras, explicit=True, project_root=project_root, evict_incompatible_plugins=True, + automatic=extras) except Exception as exc: print(f" ⚠ Could not install {', '.join(extras)} for configured features: {exc}") else: - missing = [row for row in missing if row[2].replace("_", "-") not in extras] + installed = set(extras) - declined_extras(project_root) + missing = [row for row in missing if row[2].replace("_", "-") not in installed] if missing: print(" ⚠ Configured features whose dependencies are still missing — the gateway will fail to load them on restart:") for feature, hint, _extra in missing: diff --git a/hermes_cli/venv_sync.py b/hermes_cli/venv_sync.py index e21eb7c137..161e7d344d 100644 --- a/hermes_cli/venv_sync.py +++ b/hermes_cli/venv_sync.py @@ -670,7 +670,9 @@ def _sync_source_dependencies(root: Path, *, arm: bool, borrowed_from: Path | No # Same order as `hermes update`: an interrupted update or a hand-run # `git pull` leaves this tree's lockfile ahead of the installed tools. ensure_tools_for_sync() - pm.sync_venv(extras, explicit=True, project_root=root, evict_incompatible_plugins=True) + # The old venv's extras are carried, not asked for: none of them undoes a `--without`. + automatic = {"automatic": extras} if extras and extras != ["all"] else {} + pm.sync_venv(extras, explicit=True, project_root=root, evict_incompatible_plugins=True, **automatic) collect_superseded_generations(root) if borrowed_from is not None: return # The checkout's markers describe the owner's environment, not this one. diff --git a/pm/cli.py b/pm/cli.py index efec56cdf1..74c547b17c 100644 --- a/pm/cli.py +++ b/pm/cli.py @@ -308,21 +308,24 @@ def _install_defaults(names: list[str], *, verify: bool) -> None: f"`hermes pm install {name}` succeeds", flush=True) -def _first_selection(extras: list[str]) -> list[str]: - """The extras this install's sync requests: the caller's, else ``[all]``. +def _first_selection(extras: list[str]) -> tuple[list[str], list[str]]: + """``(extras this install's sync requests, the ones it carries)``: the caller's, else ``[all]``. Before PM has recorded a selection (a first migration that failed before writing facts), the main-era venv's extras are carried too, minus any dropped with ``--without``, exactly as the launch migration would; otherwise this sync's facts would end the re-scan without them. + The carried ones are not a request, so they never opt a declined extra back in. """ from pm.environments import runtime_facts_path from pm.extras import legacy_selection requested = extras or ["all"] if runtime_facts_path(repo_root()).is_file(): - return requested + return requested, [] carried = legacy_selection(repo_root()) - return requested if carried == ["all"] else sorted(set(carried) | set(extras)) + if carried == ["all"]: + return requested, [] + return sorted(set(carried) | set(extras)), sorted(set(carried) - set(requested)) def _install_python_environments(extras: list[str], *, sync: bool, test_environment) -> int: @@ -337,7 +340,8 @@ def _install_python_environments(extras: list[str], *, sync: bool, test_environm # the installers' old `--extra all` did. sync_venv unions, so # any lazy extras already recorded survive this; it only makes # a fresh bootstrap match what the first update would do. - sync_venv(_first_selection(extras), explicit=True) + selection, carried = _first_selection(extras) + sync_venv(selection, explicit=True, **({"automatic": carried} if carried else {})) print(f"✓ venv{' +' + ' +'.join(extras) if extras else ''}") except InstallError as e: print(f"✗ {e}") diff --git a/pm/client.py b/pm/client.py index b79d0c12de..89e432b1a5 100644 --- a/pm/client.py +++ b/pm/client.py @@ -215,7 +215,8 @@ def ensure(name, *, base_env=None, explicit=False, verify=True, progress=None, p def sync_venv(extras=None, *, explicit=False, plugins: PluginInput | None = None, repair=False, - project_root: Path | None = None, evict_incompatible_plugins: bool = False) -> None: + project_root: Path | None = None, evict_incompatible_plugins: bool = False, + automatic: list[str] | None = None) -> None: from pm.environments import running_from_selected_environment if extras and not explicit and not repair and not running_from_selected_environment( @@ -238,10 +239,11 @@ def sync_venv(extras=None, *, explicit=False, plugins: PluginInput | None = None if is_runtime() and not foreign: from pm.install import sync_venv as direct return direct(extras, explicit=explicit, plugins=plugins, repair=repair, - evict_incompatible_plugins=evict_incompatible_plugins) + evict_incompatible_plugins=evict_incompatible_plugins, automatic=automatic) _request("sync_venv", {"extras": extras, "explicit": explicit, "repair": repair, "plugins": plugin_inputs.encode(plugins), - "evict_incompatible_plugins": evict_incompatible_plugins}, project_root=project_root) + "evict_incompatible_plugins": evict_incompatible_plugins, "automatic": automatic}, + project_root=project_root) def ensure_tools_for_sync() -> None: diff --git a/pm/install.py b/pm/install.py index 778ce42be6..69bdac6e81 100644 --- a/pm/install.py +++ b/pm/install.py @@ -859,6 +859,22 @@ def _refuse_declined(package, extras: Optional[list[str]], *, explicit: bool) -> "run `" + "`, `".join(install_hint(extra) for extra in dropped) + "` to enable them again") +def _without_new_declines(package, extras: Optional[list[str]], automatic) -> Optional[list[str]]: + """*extras* minus the *automatic* entries the user has declined, read under the dependency lock. + + An automatic entry (configured-platform repair, the main-era venv carry, a container's shipped + baseline) was chosen before this sync took the lock, from a decline record a concurrent + `hermes pm install --without NAME` may since have changed; only the lock-held read counts. + """ + if not extras or not automatic: + return extras + from pm.extras import declined_extras + + refused = declined_extras(package.project_root()) + derived = {_extra_key(extra) for extra in automatic} + return [extra for extra in extras if _extra_key(extra) not in derived or _extra_key(extra) not in refused] + + def decline_extras(names: list[str]) -> None: """Record ``hermes pm install --without NAME`` for extras, under the dependency lock: an in-flight `--extra NAME` sync clears its decline before releasing it, so an unlocked write @@ -873,7 +889,7 @@ def decline_extras(names: list[str]) -> None: def sync_venv(extras: Optional[list[str]] = None, *, explicit: bool = False, plugins: PluginInput | None = None, repair: bool = False, - evict_incompatible_plugins: bool = False) -> None: + evict_incompatible_plugins: bool = False, automatic: Optional[list[str]] = None) -> None: """Make the venv match uv.lock + the enabled extras. Extras union into the installed state (one ledger); no-op when the stamp already matches. ``repair`` restores the recorded dependency graph into a fresh generation, @@ -884,7 +900,9 @@ def sync_venv(extras: Optional[list[str]] = None, *, explicit: bool = False, source of plugin members (see pm.plugin_inputs); None discovers them from config. ``evict_incompatible_plugins`` is the update's contract: a discovered plugin that keeps the environment from building is disabled instead of failing the sync - (see pm.plugin_eviction). + (see pm.plugin_eviction). ``automatic`` names the entries of ``extras`` Hermes chose + on the user's behalf: one the user declined is left out, and none of them opts a + declined extra back in; every other requested extra does (``--extra NAME``). Lazy installs OFF = the frozen feature set: when security.allow_lazy_installs is false AND the bundle's @@ -912,6 +930,7 @@ def sync_venv(extras: Optional[list[str]] = None, *, explicit: bool = False, from hermes_cli.runtime_state import recover_publication from pm.publication import StagedPlugin with _venv_install_lock(patient=explicit or repair): + extras = _without_new_declines(package, extras, automatic) _refuse_declined(package, extras, explicit=explicit) recover_publication(paths.repo_root()) change = _publication(plugins) @@ -935,12 +954,14 @@ def sync_venv(extras: Optional[list[str]] = None, *, explicit: bool = False, receipt.record_feature_list(enabled) _commit_selection(package, facts, change, enabled=enabled, stamp=stamp, inputs=inputs, current=current, repair=repair, explicit=explicit) - if extras and explicit and not repair: + derived = {_extra_key(extra) for extra in automatic or ()} + opted_in = [extra for extra in extras or () if _extra_key(extra) not in derived] + if opted_in and explicit and not repair: from pm.extras import record_declined_extras # An explicit request is the opt-back-in. Cleared before the lock is released, or a # sync queued behind this one still reads the decline and publishes without the extra. - record_declined_extras(remove=extras, project_root=package.project_root()) + record_declined_extras(remove=opted_in, project_root=package.project_root()) outcome = "ok" except BaseException as exc: receipt.record_step("dependency-sync", False, f"{type(exc).__name__}: {exc}") diff --git a/pm/recovery.py b/pm/recovery.py index aa931b0618..a7defac8bf 100644 --- a/pm/recovery.py +++ b/pm/recovery.py @@ -71,7 +71,7 @@ def _baseline_gaps(root: Path) -> list[str]: image's environment with one missing everything the image ships. """ from pm.environments import runtime_facts_path - from pm.install import _extra_key, _facts, _still_declared + from pm.install import _carried, _extra_key, _facts from pm.lock import Facts from pm.packages import Venv @@ -86,7 +86,7 @@ def _baseline_gaps(root: Path) -> list[str]: raise ValueError("invalid recorded dependency state") shipped = (_facts().get("venv") or {}).get("extras") or [] have = {_extra_key(extra) for extra in recorded} - return sorted({extra for extra in _still_declared(Venv(root), shipped) if _extra_key(extra) not in have}) + return sorted({extra for extra in _carried(Venv(root), shipped) if _extra_key(extra) not in have}) def refresh_dependencies(project_root: Path) -> str: @@ -115,7 +115,8 @@ def refresh_dependencies(project_root: Path) -> str: return "current" try: with contextlib.redirect_stdout(sys.stderr): - sync_venv(missing or None, explicit=True) + # The image's baseline, not a user request: it never undoes a `--without`. + sync_venv(missing or None, explicit=True, automatic=missing) return f"restored {', '.join(missing)}" if missing else "rebuilt" except Exception as exc: print(f"dependency refresh failed: {exc}", file=sys.stderr) diff --git a/tests/hermes_cli/test_update_missing_configured_deps.py b/tests/hermes_cli/test_update_missing_configured_deps.py index b0bb32a338..22cd3a66d5 100644 --- a/tests/hermes_cli/test_update_missing_configured_deps.py +++ b/tests/hermes_cli/test_update_missing_configured_deps.py @@ -130,7 +130,8 @@ def test_update_installs_the_extras_configured_features_need(monkeypatch, tmp_pa monkeypatch.setattr(pm, "sync_venv", lambda extras, **kwargs: calls.append((extras, kwargs))) repair._install_configured_features_missing_deps(tmp_path) assert calls == [(["discord", "google-chat", "mcp"], - {"explicit": True, "project_root": tmp_path, "evict_incompatible_plugins": True})] + {"explicit": True, "project_root": tmp_path, "evict_incompatible_plugins": True, + "automatic": ["discord", "google-chat", "mcp"]})] out = capsys.readouterr().out assert "IRC: irc hint" in out assert "Discord" not in out and "MCP servers" not in out diff --git a/tests/pm/test_baked_selection.py b/tests/pm/test_baked_selection.py index 639f72e930..e01313baf5 100644 --- a/tests/pm/test_baked_selection.py +++ b/tests/pm/test_baked_selection.py @@ -93,7 +93,7 @@ def test_refresh_restores_baked_extras_a_volume_lost(install, monkeypatch, recor monkeypatch.setattr(pm.install, "venv_is_current", lambda **kwargs: True) assert refresh_dependencies(root) == "restored all, messaging, otlp" - assert calls == [(["all", "messaging", "otlp"], {"explicit": True})] + assert calls == [(["all", "messaging", "otlp"], {"explicit": True, "automatic": ["all", "messaging", "otlp"]})] def test_refresh_leaves_a_complete_selection_alone(install, monkeypatch): diff --git a/tests/pm/test_declined_extras.py b/tests/pm/test_declined_extras.py index 3fd8e4ca59..b7ef44d6dd 100644 --- a/tests/pm/test_declined_extras.py +++ b/tests/pm/test_declined_extras.py @@ -214,3 +214,37 @@ def test_without_after_a_failed_first_migration_keeps_the_other_legacy_extras(mo assert calls == [["all", "fal"]] assert extras.declined_extras(root) == frozenset({"matrix"}) + + +def test_automatic_repair_never_undoes_a_without_that_landed_after_its_read(monkeypatch): + """Configured-platform repair reads the declines, then asks PM for the configured extra. A + `--without matrix` completing between that read and PM's lock wins: repair's request was + Hermes's choice, not the user's, so it neither re-adds matrix nor clears the new decline.""" + import pm.client + from hermes_cli import main_install_repair + + monkeypatch.setattr(install, "_feature_policy", lambda extras, *, repair: (None, None)) + package = _BlockingVenv() + monkeypatch.setattr(install, "get_package", lambda name: package) + monkeypatch.setattr(pm.client, "is_runtime", lambda: True) + monkeypatch.setattr(main_install_repair, "_configured_features_missing_deps", + lambda: [("Matrix", "Run `hermes setup`.", "matrix")]) + install.sync_venv(["all"], explicit=True) + + real_lock, opted_out = install._venv_install_lock, [] + + def lock(**kwargs): + if not opted_out: + # Repair has read the declines and is about to take the lock: the opt-out lands first. + opted_out.append(True) + install.decline_extras(["matrix"]) + install.sync_venv(["all"], explicit=True) + return real_lock(**kwargs) + + monkeypatch.setattr(install, "_venv_install_lock", lock) + main_install_repair._install_configured_features_missing_deps(repo_root()) + + assert opted_out + assert extras.declined_extras() == frozenset({"matrix"}) + assert _recorded() == ["all"] + assert all("matrix" not in built for built in package.built)