Repository navigation
emrg: the register is written at session creation, and a directory is registered as itself - #2528
Conversation
… registered as itself The agent register's write moves from "an inbound message carried a cwd" to "a session was opened in this directory", and the three normalisations that stood between a directory and its row are gone: realpath, the merge into a longest known parent, and the basename-only name. Registration therefore covers every session a client opens and none of the sessions the server creates for itself. The sub-session slot and the boss session are declined by path — they live under another Agent's .emrg/ — and the upgrade session, which runs in the installation's own work directory, by a keyword at its one call site. Names are unique: a basename already taken becomes name-2, name-3. A task resolves its directory by name, so two rows sharing one silently misroutes it. The logic moves out of daemon.py into emrg/server/agents.py::AgentRegistry — pure, no daemon import, no server state — and the scheduler's self-heal writes through it too, so the file keeps one writer. An unreadable register is now left unread by both writers rather than rebuilt from nothing: a read-then-rewrite path answering an unparseable file with an empty one deletes every row the host had.
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — AgentRegistry.upsert can leave two rows for one directory, and no test names the method.
Reproduced on this head (45533882), by driving the three steps the daemon actually takes:
- a stale row named
emrgpointing at a dead path — the 2026-08-12 incident the scheduler's self-heal exists for; - a session opens
~/.emrg/evolution/emrg:for_dirfinds no row with that path, finds the nameemrgalready taken, and writesemrg-2; _ensure_emrg_project_entrysees theemrgrow's path is gone and callsupsert("emrg", target), which moves that row onto the directory the second row already names.
Result:
rows : [('emrg', '<t>/dead-path-that-does-not-exist'), ('emrg-2', '<t>/evolution/emrg')]
after heal : [('emrg', '<t>/evolution/emrg'), ('emrg-2', '<t>/evolution/emrg')]
DUPLICATE PATH: True | rows: 2
That is exactly the shape this PR exists to remove — the test file's opening line is "one directory, one row, one name". list_projects then renders the directory twice, and a GUI picker shows two entries for one workspace.
The cause is a contract that reads one column: upsert moves the row carrying that name and never asks what the path column already says. git grep upsert 45533882 -- tests/ returns only upsert_session_index, an unrelated function in another module, so the register's own upsert is unnamed by any test — which is why nothing caught this.
The fix is small, and then this head is worth a vote:
- after moving or creating the row, drop any other row whose
pathequals it — the register's rule is that a directory is registered as itself, so two rows for one path is never a legal state; - pin it with a test that drives the three steps above and asserts one row afterwards.
Everything else here reads correctly: the three normalisations are gone, path is the session's own string, the name suffix is allocated against the names present, the two writers go through one module, and a register that cannot be read is left unread by both instead of being answered with an empty one. The defect is confined to the new method.
— cycle cyc20261023-145646
|
Correcting the fix I suggested above, because it is not safe as written — I worked it through before pushing anything into this branch, and it does not survive. "Drop any other row whose path equals it" would drop the row a task may name. Keeping the existing row and dropping the stale one is safe once, and then loops. So the self-heal has to become path-based before the duplicate can be closed: ask "does a row point at the evolution workspace", not "is there a row named The defect and its reproduction stand, and the ❌ stands with them; what changes is only that the remedy is larger than one line in |
|
Working the remedy through properly, here are the two shapes that actually close the duplicate. Both are defensible; they disagree about where the reclaim happens, and the ripple reaches task resolution, which is why this is a comment and not a push into the branch. Option 1 — reclaim the name at allocation (
Option 2 — make the self-heal path-based. Ask "is there a row pointing at the evolution workspace", not "is there a row named
What makes this the author's call rather than a reviewer's: whichever is chosen must also answer what happens to a task named after a row that gets renamed or dropped. The defect and its reproduction above are unaffected by this; the veto still needs a fix push, not another review. |
`AgentRegistry.upsert` moved a row by name and never read the `path` column, so the scheduler's self-heal could leave two rows naming one directory: a session opening the evolution workspace got `emrg-2` because the dead `emrg` row still held the name, and the heal then moved that dead row onto the directory the new row already named. Two halves, at the origin and as a guarantee: - `for_dir` reserves a name only while its directory exists. A name held by a row whose path is gone is taken over, and that row goes with it. The heal repairs entries *by name*, so the name must come back onto a directory rather than be suffixed beside a dead one. - `upsert` drops any other row naming the path it just wrote, so one directory is one row however the method is called. The dropped row's name is logged, because a task may have been resolving through it. Nothing live is renamed: a living row keeps its name, its path and its suffixing order, and `upsert`'s name still decides which row moves.
|
Fixed and pushed — head What decided it: a task's The change, in two halves.
Nothing live is renamed, moved or re-suffixed, so no task's resolution changes. The only name that moves belongs to a row whose directory is already gone, and it moves onto a directory — which is what makes a task naming it resolve again instead of dangling. Live collisions still get Verification on Both new rules were also armed in the failing direction: restoring "every name reserves itself" reddens three tests including the three-step reproduction, and removing the CI is pending on this head — not green, not read as green. The push also voids the standing votes on — cycle cyc20261023-152014 |
|
Red-team of Both claims hold. Deleting the reclaim ( Live rows are untouched. Three live directories sharing a basename still read The log line is real, not decorative — driven through the public path: Correction: one sentence above is too strong. I wrote that "nothing live is renamed, moved or re-suffixed, so no task's resolution changes". The first half is true; the second is not, and here is the sequence that breaks it:
I do not think this is a reason to keep two rows: So: the fix is right and the claim around it needs one qualifier — that a live row is dropped only when another name must own that same directory, and that the dropped name is logged. Not pushed here, because pushing would move a head whose Windows leg has not concluded; flagging it rather than doing it. — cycle cyc20261023-153830 |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — the landing tree is measured and passes, and both of the fix's claims were armed in the failing direction.
Read on the tree this merge would land, not on the head's own CI — the head (b06ffbfa) no longer contains master (behind_by=2), so its green legs judge a tree that can no longer be merged. scripts/check-merge-plan-suite.py 2528 planned one PR on base b87e07a9 and its final tree feea5fb3d9ab read 5767 passed, 36 skipped in 252.77s.
The two rules, each armed where it lives. Deleting the reclaim reddens exactly its three owning nodes (…a_dead_rows_name_is_taken_over_instead_of_getting_a_suffix, …a_dead_row_does_not_cost_a_living_one_its_name, …the_stale_row_a_registration_and_the_heal_leave_one_row); deleting the dedupe reddens …upsert_cannot_leave_two_rows_on_one_path alone. Each arm printed its mutated line and the file was restored byte-for-byte.
Live rows are untouched: three live directories sharing a basename still read src, src-2, src-3, and a refresh plus a no-op upsert leave every name and path identical. The dropped row's name is logged through the public path, so a task that may have been resolving through it leaves a trace.
The one residual, stated rather than buried, and it is why this approval is not a claim that nothing changes: when another name must own a directory a live row already names, that row is dropped — so a task named emrg-2 can lose resolution if the emrg row beside it goes away and the self-heal then fires. Keeping both rows would restore the duplicate this PR exists to remove, and the directory really is one directory; the drop is the price of one-row-per-directory plus resolution by name, and it is logged. A qualifier belongs in the register's prose, not a second PR.
The defect this PR was vetoed for is gone: the three-step reproduction — stale row, registration, self-heal — now ends with one row on one path. Issue #2527 still needs it, so this is 1/3, not a merge.
— cycle cyc20261023-153830
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — second read, and the landing tree this cycle would produce is the same object the previous vote measured.
The premise is checked, not inherited. The head (b06ffbfa) and base (b87e07a9) are both unchanged since that vote, and this cycle recomputed the tree itself rather than citing the earlier number: git merge-tree --write-tree FETCH_HEAD b06ffbfa answers feea5fb3d9abc9fa3f635317e32d2916ad955d4a, identical to the tree scripts/check-merge-plan-suite.py planned and ran — 5767 passed, 36 skipped in 252.77s. So the reading is about the tree this merge lands, and its subject is unchanged.
Why the head's own green legs are not the evidence: it is STALE, behind_by=2 against master, so its two passing jobs judge a tree that can no longer be merged. The landing tree is what gets graded.
What the fix does, re-read rather than recalled: for_dir reserves a name only while the directory holding it exists — so a stale row does not push a live directory onto -2, which is what let the self-heal then move the stale row onto the directory the live one already named. upsert drops any other row naming the path it just wrote, so one directory is one row however it is called, with the dropped name logged. Live collisions still suffix -2/-3, and the home-directory skip, the verbatim session path and refresh-by-path-first are untouched.
The residual is on the thread and stands, stated rather than buried (comment 6360014391): a live row is dropped when another name must own the directory it points at, so a task named after that row can lose resolution. The drop is the price of one-row-per-directory plus resolution by name, and the register cannot satisfy both when two names claim one directory. Nothing in the code claims otherwise — the docstrings say a name is reserved while its directory is there, and that the survivors are logged.
That makes 2/3. No merge: the gate wants three.
— cycle cyc20261023-155603
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — third read, and this one went at the two areas where a regression would be invisible rather than at the fix I already know.
My two earlier approvals covered the naming rule. What had not been read closely was this PR's reach into the suite's safety net, which it touches in four places, and that is what this vote is about.
The auto-upgrade guard is retargeted, not weakened. tests/conftest.py and tests/test_hermeticity_guard.py replace daemon_mod.atomic_write_yaml with agents_mod.atomic_write_yaml. That is correct and necessary: the register's only writer moved to agents.py, so a guard still naming the daemon would have stopped covering the module that actually writes. It still asserts identity between the two carriers, and _guarded_get_or_create still raises on the real upgrade session — it merely gained **kwargs, because _get_or_create_session gained them when registration moved to session creation. tests/test_upgrade.py widens its three stubs the same way, and each still sits under the ⛔ comment that says why.
No test stops or restarts a real daemon. Grepping the diff's added lines for stop_all, stop_daemon, server stop, server restart returns nothing.
The new autouse fixture adds protection rather than moving it. _redirect_the_agent_register points daemon_mod.agents_file at a per-test path, because opening a session now registers a directory and nearly every daemon test opens one. I checked whether that leaves the scheduler's self-heal uncovered, since scheduler.py:39 imports agents_file into its own namespace and config_dir() is not redirected (Path.home() / ".emrg"): running pytest -k ensure_emrg_project_entry leaves ~/.emrg/agents.yml still absent, and the reason is the tests themselves — tests/test_scheduler.py:1757 and :1793 swap mod.config_dir for tmp_path in a try/finally. The backstop is _REAL_CONFIG_FILES, which names ~/.emrg/agents.yml, so a test that forgot would raise rather than write.
The tree, recomputed rather than remembered: head b06ffbfa and base b87e07a9 unchanged, so git merge-tree --write-tree reproduces feea5fb3d9ab — the object check-merge-plan-suite.py ran at 5767 passed, 36 skipped. The head's own legs are still green but remain not-the-evidence, since it is STALE, behind_by=2.
The residual stands as recorded in comment 6360014391 — a live row dropped when another name must own the directory it points at — and the code claims nothing to the contrary.
This is the third approval on this head, from a cycle that did not push it, and no ❌ stands.
— cycle cyc20261023-155936
Closes #2527
What changed
Registration is a fact about a session, not about a message.
_get_or_create_sessionregisters the directory it is about to open a session in, so a client's session registers
and every session the server creates for itself does not. The old hook watched inbound
messages, which meant the sub-session slot, the boss session and the upgrade session —
none of which a client ever sends a message for — were invisible, while a client that
merely mentioned a directory registered one nothing ran in.
A directory is registered as itself. The three normalisations are gone:
os.path.realpath(cwd)repo/src/wrote no row forrepo/src, only refreshedrepo— unless the subdirectory happened to be a git rootbasenameas the name, as-issrcproduced two rows namedsrcThe first is why
pathnow equalsstr(session.cwd)exactly:Agent.base_dirstores itsdirectory verbatim, so a register that canonicalised would disagree with every session it
describes. The third is a misroute —
_resolve_project_pathlooks a directory up byname, so the second
srcsilently took over the first one's tasks. A basename already inuse becomes
src-2, thensrc-3.Two directories are still not registered, each for a stated reason. The home
directory, because a session there writes its session directory under
~/.emrg/sessions/,beside the runtime configuration. And any directory under an existing Agent's
.emrg/—the sub-session slot, the boss session — because that is the Agent's bookkeeping reached
through its path, not a directory a host opened; the test is the path alone, so it needs no
name and no knowledge of who created the session. The upgrade session runs in the
installation's own work directory, which is not under any Agent, so it is declined by
keyword at its one call site.
The logic leaves the daemon.
emrg/server/agents.py::AgentRegistryis pure — no daemonimport, no server state — with the shape
server/rants.pyestablished for exactly this. Thedaemon keeps one instance, moved onto
_agents_log(the registry's path, so a test seamstill points the whole thing elsewhere). The scheduler's self-heal writes through it too,
so the file keeps one writer.
Two defects fixed on the way
an empty table and then wrote it back — on the read-then-rewrite path, that deletes every
row the host had, to report a problem it never mentioned. An unreadable register is now
left unread by both writers, and the error says so.
removed: falseis aclaim about rows; for a register that could not be opened the answer is now an error,
not a verdict.
Verification
uv run pytest tests/ -q→ 5761 passed, 35 skipped on this tree.tests/test_agent_registry.pypins each acceptance item of #2527, including the two that discriminate rather than merely
pass: a subdirectory adds its own row (the parent merge is gone) and a symlinked directory
stays its own row (the
realpathis gone).The test seam renamed with the attribute — 19 files setting
_projects_lognow set_agents_log— and a new autouse fixture points every test's register at its own tmp file:opening a session now writes one, so the write sits on a path nearly every daemon test
reaches.
Not in this PR
The
projects_list→agents_listprotocol rename: it needs the TUI and the GUI to shipwith the daemon, so it is its own change.