mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-11-06 05:17:15 +00:00
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.
This commit is contained in:
1 parent
385e6f3d6a
commit
bfa652fae8
10 files changed
+91
-21
No files matched your search
@@ -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)
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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}")
|
||||
|
||||
+5
-3
@@ -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:
|
||||
|
||||
+25
-4
@@ -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}")
|
||||
|
||||
+4
-3
@@ -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)
|
||||
|
||||
@@ -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
|
||||
@@ -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):
|
||||
|
||||
@@ -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)
|
||||
Reference in new issue
Block a user