diff --git a/emrg/config.py b/emrg/config.py index 1064cbfa..f896514a 100644 --- a/emrg/config.py +++ b/emrg/config.py @@ -247,6 +247,41 @@ def agents_file(base: Path | None = None) -> Path: return directory / "agents.yml" +def register_for_reading(base: Path | None = None) -> Path: + """Returns the register a *reader* should open (#2545). + + :func:`agents_file` names where the register belongs; this answers where it + actually is. :func:`migrate_legacy_registry` moves a register still at the + pre-rename name, and it is documented to leave the file exactly where it is + when the move cannot happen — a read-only home, a permission boundary, a + concurrent caller that got there first. A checkout also runs ahead of the + daemon that still holds the register at the old name. In both states + :func:`agents_file` names a file that does not exist, and a reader asking + only that one answers an **empty register** while the real one sits one + filename away — silently, because "nothing there" and "it is at the other + name" print the same. + + The new name governs whenever it exists, so a legacy file that an older + daemon re-created cannot shadow the current register. When neither name + exists the new one comes back: the absent file the caller would have opened + anyway. + + **Why this is not folded into** :func:`agents_file`: the migration takes + that function's answer as its move *target*, so a fallback inside it would + hand the migration the legacy path as the target, ``target.exists()`` would + be true, and the move would answer ``False`` on every host for ever — the + register would never reach the new name at all. + + Reading never moves anything: this stats the two names and opens neither. + """ + directory = config_dir() if base is None else base + current = agents_file(directory) + if current.exists(): + return current + legacy = directory / _LEGACY_AGENTS_NAME + return legacy if legacy.exists() else current + + def migrate_legacy_registry(base: Path | None = None) -> bool: """Move a register still at the pre-rename name onto the new one. diff --git a/emrg/server/scheduler.py b/emrg/server/scheduler.py index d9548e93..574e11b5 100644 --- a/emrg/server/scheduler.py +++ b/emrg/server/scheduler.py @@ -36,7 +36,7 @@ from pathlib import Path import yaml -from emrg.config import agents_file, config_dir +from emrg.config import agents_file, config_dir, register_for_reading from emrg.connect import connect_to_server from emrg.memory import NotARegularFile from emrg.tools.base import non_regular_kind @@ -993,8 +993,13 @@ def _resolve_task_template(task_type: str) -> Path: def _resolve_agent_path(name: str) -> str | None: - """Resolve an agent name to its path from agents.yml.""" - register = agents_file(config_dir()) + """Resolve an agent name to its path from the register. + + The register is opened through :func:`emrg.config.register_for_reading`, so a + host whose file is still at the pre-rename name resolves here instead of + answering `None` (#2545). + """ + register = register_for_reading(config_dir()) if not register.exists(): return None try: @@ -1010,14 +1015,14 @@ def _resolve_agent_path(name: str) -> str | None: def _load_agent_config(name: str, source_dir: str) -> dict: - """Return the full agents.yml entry matching the current agent. + """Return the full register entry matching the current agent. Matches by ``name`` first, then by ``path`` against ``source_dir``. Returns an empty dict when no match is found. Custom fields added by the user in agents.yml are preserved, so templates can reference them via ``{{ agent. }}`` without code changes. """ - register = agents_file(config_dir()) + register = register_for_reading(config_dir()) if not register.exists(): return {} try: diff --git a/emrg/tools/file_policy.py b/emrg/tools/file_policy.py index ea677f16..a15129a6 100644 --- a/emrg/tools/file_policy.py +++ b/emrg/tools/file_policy.py @@ -47,8 +47,9 @@ #: Daemon state files a sandboxed tool call must never write, at any tier. Each is #: a file the host owns and the daemon reads at startup. The agent registry is -#: not spelled here: its path is derived (:func:`emrg.config.agents_file`), so it -#: joins this list where names become paths. +#: not spelled here: its path is derived (:func:`emrg.config.register_for_reading`, +#: which answers whichever name the register currently holds, #2545), so it joins +#: this list where names become paths. PROTECTED_FILES = ( "~/.emrg/config.toml", "~/.emrg/emrgd.token", @@ -62,10 +63,10 @@ def protected_paths() -> list[str]: # Imported at call time, not module scope: `emrg.config` imports # `emrg.tools.base`, and this module is reached from the tool packages that # sit beside it (#2521). - from emrg.config import agents_file + from emrg.config import register_for_reading out: list[str] = [] - for p in (*PROTECTED_FILES, str(agents_file())): + for p in (*PROTECTED_FILES, str(register_for_reading())): try: out.append(os.path.realpath(os.path.expanduser(p))) except OSError: diff --git a/emrg/tools/submit_rant_tool.py b/emrg/tools/submit_rant_tool.py index 44a8e3b8..c594e723 100644 --- a/emrg/tools/submit_rant_tool.py +++ b/emrg/tools/submit_rant_tool.py @@ -316,10 +316,14 @@ def _execute_submit(self, arguments: dict) -> ToolResult: ) def _registered_agent_names(self) -> list[str]: - """Short names registered in ~/.emrg/agents.yml (config dir). + """Short names registered in the register (config dir). The daemon registers a directory there on first use; the list is the - authoritative source for the rant ``agent`` field. Any read error + authoritative source for the rant ``agent`` field. The path is asked of + :func:`emrg.config.register_for_reading`, which answers the pre-rename + name while that is the file that holds the register (#2545) — a reader + that asked only for the new name saw an empty register, and the warning + below is gated on a non-empty list, so it never fired. Any read error degrades to "no candidates" (validation is advisory, never blocking). The subject's kind is asked before it is opened: `exists()` is true for a FIFO, and @@ -331,9 +335,9 @@ def _registered_agent_names(self) -> list[str]: try: import yaml - from emrg.config import agents_file + from emrg.config import register_for_reading - p = agents_file(self._rants_log().parent) + p = register_for_reading(self._rants_log().parent) try: kind = non_regular_kind(p) except OSError: diff --git a/tests/test_agents_registry_migration.py b/tests/test_agents_registry_migration.py index c39016a5..55c1959c 100644 --- a/tests/test_agents_registry_migration.py +++ b/tests/test_agents_registry_migration.py @@ -15,7 +15,7 @@ import pytest import yaml -from emrg.config import agents_file, migrate_legacy_registry +from emrg.config import agents_file, migrate_legacy_registry, register_for_reading #: A register with what the daemon cares about: a `name`/`path` pair, and the #: `last_active` stamp the pickers sort by. The second entry carries a custom @@ -193,3 +193,85 @@ def test_the_old_name_is_spelled_only_where_the_move_is(): assert not offenders, "the register's path is built outside its one producer:\n" + "\n".join( offenders ) + + +# ── The register read path (#2545) ──────────────────────────────────────────── +# +# The move above is the *writer's* business. These pin the reader's: while the +# file is still at the old name — because the move cannot happen on this host, +# or because a checkout runs ahead of the daemon that performs it — every reader +# must read the register that is actually there, and must not answer "empty". + + +def test_a_register_at_the_old_name_is_read_by_every_reader(tmp_path, monkeypatch): + """The measured defect, one reader per assertion. + + With only the pre-rename file present, each of these answered the empty + answer — `[]`, `None`, `{}` — while the register held two entries: silent, + because "nothing there" and "it is at the other name" read alike. The + candidate list is the one with a consequence: `submit_rant`'s warning about a + mis-spelled agent is gated on that list being non-empty, so it never fired. + """ + from emrg.server.scheduler import _load_agent_config, _resolve_agent_path + from emrg.tools.file_policy import protected_paths + from emrg.tools.submit_rant_tool import SubmitRantTool + + legacy = _legacy(tmp_path) + # Each module holds its own bound copy of `config_dir` (`from emrg.config + # import config_dir`), so patching the definition does not rebind them — a + # seam that patches only the library leaves the caller reading the host's + # real register, which is host state in a test. + monkeypatch.setattr("emrg.config.config_dir", lambda: tmp_path) + monkeypatch.setattr("emrg.server.scheduler.config_dir", lambda: tmp_path) + + assert register_for_reading(tmp_path) == legacy + + assert sorted(SubmitRantTool()._registered_agent_names()) == ["aitokenpool", "emrg"] + + assert _resolve_agent_path("emrg") == "/home/h/evolution/emrg" + + entry = _load_agent_config("emrg", "/home/h/evolution/emrg") + assert entry.get("name") == "emrg", entry + assert entry.get("path") == "/home/h/evolution/emrg", entry + + assert str(legacy.resolve()) in protected_paths(), ( + "the register a sandboxed write must not touch is the one that is live" + ) + + +def test_the_new_name_governs_when_both_are_present(tmp_path): + """A legacy file an older daemon re-created cannot shadow the register.""" + _legacy(tmp_path) + current = tmp_path / "agents.yml" + current.write_text(yaml.safe_dump([{"name": "only-new"}]), encoding="utf-8") + + assert register_for_reading(tmp_path) == current + + +def test_the_read_path_never_moves_the_register(tmp_path): + """Asking a reader where the register is must not perform the migration.""" + legacy = _legacy(tmp_path) + + assert register_for_reading(tmp_path) == legacy + + assert legacy.exists(), "reading the register moved it" + assert not (tmp_path / "agents.yml").exists() + + +def test_the_read_fallback_cannot_shadow_the_migration_target(tmp_path): + """The reason the fallback is its own function and not part of `agents_file`. + + `migrate_legacy_registry` takes `agents_file()` as the move *target*. A + fallback inside that function would hand it the legacy path as the target, + `target.exists()` would be true, and the move would answer "already done" on + every host for ever. Read the fallback first, then migrate, and the move + still happens. + """ + legacy = _legacy(tmp_path) + + assert register_for_reading(tmp_path) == legacy + assert migrate_legacy_registry(tmp_path) is True, "the fallback made the move a no-op" + + assert not legacy.exists() + assert agents_file(tmp_path).exists() + assert register_for_reading(tmp_path) == agents_file(tmp_path)