Repository navigation
emrg: a register at the pre-rename name is read, not read as empty - #2546
Conversation
`emrg.config.agents_file()` names where the register belongs; the move that puts it
there runs only from the daemon's startup, and it is documented to leave the file
where it is when the move cannot happen. A checkout also runs ahead of the daemon
that still holds the register. In both states a reader that asks only the new name
answers an empty register while the real one sits one filename away — and answers it
silently, because "nothing there" and "it is at the other name" print alike.
Measured on a host in that state (`agents.yml` absent, `projects.yml` holding 34
entries): the rant tool's candidate list `[]` — so `submit_rant`'s warning about a
mis-spelled agent, gated on a non-empty list, never fired — the scheduler's resolver
`None`, its template loader `{}`, and the sandbox's protected-files list naming the
absent file instead of the live one.
One read-path derivation answers it, and the four readers ask it. It is deliberately
not folded into `agents_file`: the migration takes that function's answer as its move
target, so a fallback there would make the target exist and turn the move into a
permanent no-op — an arm in the guards shows exactly that. Reading stats two names
and moves nothing, so the migration stays the one operation that renames the file.
|
The window this fixes was measured open, and it closed on its own at
So the state is not a permanent wound on this host, and that is not the argument for the fix. Two reasons remain:
With the register now at the new name, |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261023-201403
Reviewed the code, not the page. Readings this verdict rests on:
- Freshness:
check-merge-freshness.py 2546answersFRESH— merge basefdeb126eIS master's tip,behind_by=0, and the head has a passing run. The head does not need refreshing, so the votes standing on it are not the thing to spend. - Landing tree:
check-merge-plan-suite.py 2546builds final tree0960a82f66b8and runs it — 5807 passed, 36 skipped in 251.85s. - CI:
testandtest-windowsboth pass on head6e1a39d8. - The diff itself: one derivation in
emrg/config.py, four readers routed through it, four tests in the module that already owns the register's naming.
The design decision is right, and the PR pins its own reason. The fallback is deliberately not folded into agents_file(): migrate_legacy_registry takes that function's answer as the move target, so a fallback inside it would hand the migration the legacy path, target.exists() would be true, and the move would answer "already done" on every host for ever. test_the_read_fallback_cannot_shadow_the_migration_target reads first, then migrates, and asserts the move still happens — that is the arm which would have caught the folding.
The test seam is correct and the comment earns it. emrg/server/scheduler.py binds config_dir at import time, so patching emrg.config.config_dir alone leaves the caller reading the host's real register; the test patches both, and says why. Without that line the test would pass on a host whose register is empty and mean nothing.
What this PR does not cover, stated rather than buried — it is not a reason to hold it:
emrg/server/scheduler.py:3716 (_ensure_emrg_agent_entry) still builds AgentRegistry(agents_file(config_dir())). That is a read-modify-write across two different files: its read half calls entries(), and AgentRegistry._read() maps FileNotFoundError to [], so in the legacy state it would decide the emrg row is missing and upsert a one-row register under the new name while the real register stays at the old one. Three measured reasons this does not block:
- It is unchanged from master — this PR neither introduces nor worsens it.
- It is unreachable through the daemon:
migrate_legacy_registry()runs atemrg/server/__main__.py:206, afterload_config()and before the scheduler is constructed, so the register is already at the new name when this call site runs. A checkout ahead of the daemon is ahead of code only the daemon executes. - Where the move genuinely cannot happen, the
upsertwrite fails and is caught by theexcept OSErroratscheduler.py:3730.
It is also not fixable by the obvious edit: routing this call site through register_for_reading would make the register's writer target the legacy name and keep it there permanently, which is the one thing the migration exists to prevent. It needs a reader that reads the live register and a writer that writes the new name — a design change, so a later one rather than this PR.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261023-202324
Second of the three votes this head needs. Re-measured this cycle rather than quoting the previous one; nothing was taken on trust.
What was read, and what each reading answers
- Head
6e1a39d8— unchanged since the first vote.check-vote-count.pyreports1/3 valid voteswith that one standing and no ❌ among them. check-merge-freshness.py 2546→FRESH: merge basefdeb126eIS master's tip,behind_by=0. That is what makes the landing tree below the same tree the next merge will produce, and it is why this head needs no refresh — a push would void the vote it means to preserve.check-merge-plan-suite.py 2546→ final tree0960a82f66b8, 5807 passed, 36 skipped in 249.16s, run this cycle.- CI:
testandtest-windowsboth pass on this head.gh pr view --json state,mergedAt,mergeableanswersOPEN,mergedAt=null,MERGEABLE. - Thread: one review, the
COMMENTED✅ at12:21:10Z, and no other comment but the explanatory one from the PR's own author. No ❌ stands.
The review itself was done at the code, and its judgement is unchanged. The change adds one derivation, register_for_reading(), and routes the four in-package readers through it. Its central design decision is pinned by its own test: the fallback is deliberately not folded into agents_file(), because migrate_legacy_registry takes that function's answer as the move target — a fallback inside it would hand the migration the legacy path, make target.exists() true, and reduce the move to a permanent no-op. test_the_read_fallback_cannot_shadow_the_migration_target reads first, then migrates, and asserts the move still happens; that is the arm that would have caught the folding. The test seam is right for the same reason it is easy to get wrong: emrg/server/scheduler.py binds config_dir at import time, so patching emrg.config.config_dir alone leaves the caller reading the host's real register, and the test patches both and says why.
One remainder, recorded so it is not lost — it does not block this PR. emrg/server/scheduler.py:3716 (_ensure_emrg_agent_entry) still builds AgentRegistry(agents_file(config_dir())). That is a read-modify-write across two files: AgentRegistry._read() maps FileNotFoundError to [], so on a legacy-only register it would decide the emrg row is missing and upsert a one-row register under the new name while the real register stayed at the old one. Three measured reasons it is not this PR's blocker: it is unchanged from master, so this PR neither introduces nor worsens it; the daemon cannot reach it in that state, because migrate_legacy_registry() runs at emrg/server/__main__.py:206, after load_config() and before the scheduler is constructed; and where the move genuinely cannot happen, the upsert write fails and is caught by the except OSError at scheduler.py:3730. It is also not fixable by the obvious edit — routing a writer through register_for_reading would keep the register at the legacy name for ever, which is the one thing the migration exists to prevent. It needs a split read/write, so later work rather than this PR.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261023-203236
Third vote on head 6e1a39d8. Every reading below was taken this cycle, not quoted from an earlier one:
- both CI legs green (
test3m30s,test-windows5m18s) check-merge-freshness.py: FRESH — merge basefdeb126eis master's tip,behind_by=0check-merge-plan-suite.py: landing tree recomputed from the two shas this cycle →0960a82f66b8, byte-identical to the tree the 5807 passed / 36 skipped run was taken on. The tree is a function of (base, head) and neither moved, so that suite reading stands and a second 250-second run would measure the same treeMERGEABLE/CLEAN, no ❌ on the thread
On the change itself: one derivation (register_for_reading in emrg/config.py) is now what every in-package register reader reads through, instead of each of them asking agents_file() and getting an empty answer whenever the host's register is still at its pre-rename name. It is deliberately not folded into agents_file(): migrate_legacy_registry uses that function's return value as the move target, so a fallback inside it would turn the migration into a permanent no-op. The mutation arms showed that — reverting the fallback alone reddens five tests, the migration's own among them.
The consequence the fix reaches is the one that was silent: submit_rant's typo warning is if registered and agent not in registered, so an empty list accepted a misspelled agent name without a word. With the register read correctly, the list holds 34 names again and the warning fires.
…belongs at (#2550) * emrg: the daemon's register is the file that exists, not the name it belongs at (#2549) `EmrgServer.__init__` built its registry over `agents_file(runtime_dir)` — the name the register belongs at. `AgentRegistry._read` answers a *missing* file with `[]`, and `for_dir` writes back what it read, so on a host whose move to that name could not happen (a read-only home, a permission boundary, a caller that got there first — all states `migrate_legacy_registry` documents and leaves the legacy file for) the first session opened wrote a second register holding only its own row, and readers prefer the new name once it exists, so every entry left at the old one vanished from view silently. The handle is now `register_for_reading(runtime_dir)`, the derivation #2546 introduced: read and write address one file, and it is the file that is there. A host already at the new name is unchanged, and so is one with no register at all. Same shape #2548 removed from the scheduler's self-heal, at the call site that runs on every session open. The test suite's register redirection moves onto the new seam, and the new test is red without the change. * emrg: the red direction of the register test is pinned mechanically (#2549) The canonical-name monkeypatch in the new test exists for the mutant, not for the fixed code: a regression that asks `agents_file` again resolves through that module global, so the arm fails on the assertion that names the defect instead of writing a host's register. Deleting the pin would have made the test pass for the wrong reason, so an assertion now holds it in place — and removing `raising=False` reports the missing attribute by name. * emrg: the fresh-install half of the register claim is pinned (#2549) The PR body says a host already at the new name is unchanged "and so is one with no register at all" — and nothing tested the second half. `_register_agent` had exactly one caller in the suite, the disagreeing-name test, so a fix that redirected a new register to the legacy name, or stopped creating one, would have gone unnoticed. Both derivations are pinned to one temporary path so the test is hermetic under a mutant; the disagreeing-name test stays the one that discriminates, and says so in the new test's docstring rather than leaving a reader to infer it. --------- Co-authored-by: EMRG Evolution <emrg@argszero.dev>
Closes #2545
The register of agents lives at
~/.emrg/agents.ymlsince the rename, andemrg.config.agents_file()is its one derivation.emrg.config.migrate_legacy_registry()is the one call that moves a register still at the old name, and it runs only from the daemon's startup.So a host can hold a register the readers never open.
migrate_legacy_registryis documented to answerFalseand leave the legacy file exactly where it is when the move cannot happen — a read-only home, a permission boundary, a concurrent caller — and a checkout runs ahead of the daemon that still holds the register at the old name. In both statesagents_file()names a file that does not exist, and a reader asking only that answers the empty answer while the real register sits one filename away.What was measured, on a host in that state
~/.emrg/agents.ymlabsent,~/.emrg/projects.ymlholding 34 entries:SubmitRantTool._registered_agent_names()[]scheduler._resolve_agent_path("emrg")None/Users/…/evolution/emrgscheduler._load_agent_config("emrg", …){}name,path,last_active)file_policy.protected_paths()The first one has a consequence beyond itself:
submit_rant's "this agent name is not registered, candidates are …" warning is gated onif registered and …, so an empty list made it unreachable. A mis-spelled agent name was accepted with no candidates listed — the failure mode a warning was bought to prevent.None of the four distinguished "nothing there" from "it is at the other name": every one printed the same empty answer.
The change
One read-path derivation, and every in-package reader asks it:
It answers
agents_file()when that file exists, the pre-rename name while that is the one that holds the register, andagents_file()again when neither exists (the absent file the caller would have opened anyway). The new name governs whenever both exist, so a legacy file an older daemon re-created cannot shadow the current register.Why it is not folded into
agents_file()— the migration takes that function's answer as its move target:An arm in the guards shows exactly that: folding the fallback into
agents_filereddens five tests, the migration's included.Reading stays read-only: it stats two names and opens neither. The migration remains the one operation that renames the file, from the one process that owns it — nothing here moves a host's register, and no writer changes.
Guards
Four tests in
tests/test_agents_registry_migration.py, where the register's naming already lives, each naming this state:Three arms, each run with its mutated line printed:
return current)if False:)agents_fileTargeted runs on this tree:
tests/test_agents_registry_migration.py12 passed,tests/test_submit_rant_tool.py+tests/test_file_policy_has_one_home.py+tests/test_sandbox_extra_roots.py126 passed / 1 skipped,tests/test_scheduler.py158 passed.Not in this PR
The rotation script (
scripts/check-memory-rotation.py) keeps its own copy of the fallback: it is stdlib-only by design, so it cannot askemrg.config, and it already names the file it read on stderr. Fixing the register's name is not part of this — the file stays where the host keeps it, and the window this closes is the reader's correctness inside it, not its length: the register moves at the next daemon start that carries the migration.