Repository navigation
emrg: one producer for the directory a session lives in - #2474
Conversation
Read the guard, and measured what it missesNot a vote — CI is still running on The change is sound in the part that matters: What the guard does not reach. I extracted
The last one is the interesting one, because it is the same operator chain merely grouped: the Why this is a note and not a veto. Every spelling that actually existed in the tree was a If you want the concept rather than the family, cheap additions that stay in the same idiom: One more, offered as an observation rather than a request: the |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261022-155832
Read the whole diff (4 files). emrg/session.py gains session_dir(cwd, session_id), Session.__init__ takes self._dir from it, and the eight former copies — Session.delete, five sites in emrg/server/daemon.py (delete, get-or-create, the resume ghost check, and both memory handlers), two in emrg/client/app.py (paste and /image) — now call it. Every call site keeps its behaviour: a subagent's slot cwd, the boss's cwd and the <id>/images directory land exactly where they did.
The guards are what makes this worth landing rather than a tidy-up. test_the_directory_a_session_lives_in_has_one_producer reads the syntax tree instead of the text, so a copy under another local name is still caught and the producer's own docstring is not counted as a second producer. test_the_electron_client_builds_the_same_path_in_one_place covers the half the AST guard cannot reach: the Electron main process builds the same <project>/.emrg/sessions/<id>/images in main.js, shares a filesystem with the daemon rather than a runtime, and therefore cannot delegate — so one code site, and only one, is allowed to spell it. Its skip predicate compares a path against the root it walked from; an absolute Path.parts there would let an ancestor directory named node_modules silence the walk and report a clean reading of nothing.
Verified on this head: check-merge-freshness.py FRESH — base 0cccf946 is master's tip and head 90445e1e carries a passing run — and check-merge-plan-suite.py OK, 5596 passed / 35 skipped on the landing tree d9aa258d.
Correctly out of scope and said so in the body: the sessions root without an id is a different value with its own three sites, one of them behind a circular import.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261022-161949
Read the whole diff (4 files, +210/−26) and re-measured it on its own tree rather than taking the body's word for it.
The change is a delegation, not a rewrite. emrg/session.py gains session_dir(cwd, session_id); Session.__init__ assigns self._dir from it, so the class and the rule cannot disagree; Session.delete and the five daemon.py sites (session delete, get-or-create, the resume ghost check, both memory handlers) call it, and the two client/app.py paste sites and the two child-cwd sites (sub-agent/sub<N>, sub-agent/sub-boss) do too. Renaming the five daemon locals from session_dir to session_path is what keeps the name meaning one thing. Behaviour at every call site is unchanged — the subagent and boss slots and <id>/images land exactly where they did.
The guards are why this is worth landing. Both directions hold when I break them, not just when I run them:
- I recomputed the syntax-tree guard's own count on the PR tree:
session.pyholds exactly one leaf-producing expression (line 357, the producer'sreturn). The twosessions_dir = cwd / ".emrg" / "sessions"sites (lines 55, 1108) are the id-less root, which the guard admits by design and the body names as out of scope — a leaf needs an operand after"sessions". - Arm, run in a detached worktree at
90445e1e: I rewrote bothclient/app.pypaste sites toimages_dir = Path(cwd) / ".emrg" / "sessions" / session_id / "images".test_the_directory_a_session_lives_in_has_one_producerwent red namingclient/app.py:2722. (First attempt at this arm asserted on a single occurrence and silently did not apply — 2 sites carry that line — which is why the mutated lines are printed before the suite.) - Independent of the guard:
git grepover the PR tree for other spellings (emrg/sessions,"emrg", "sessions") finds code in exactly one place,gui/main.js:436; every other hit is a comment or docstring, which is what the JavaScript scan's comment-blanking exists to exclude. Pathis still used 11 times inclient/app.py, so removing the two inline builds leaves no dead import behind.
Readings on this head (90445e1e, base 0cccf946): check-merge-freshness.py FRESH — the merge base is master's tip, behind_by=0, and the head carries a passing run. check-merge-plan-suite.py OK — final tree d9aa258d, 5596 passed, 35 skipped. The PR's own tests/test_session.py passes on that tree, 89 tests.
Correctly out of scope and said so: the sessions root without an id is a different value, still spelled at three sites, one behind a circular import.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261022-161052
Measured on this head, not read from the body.
The landing tree. check-merge-freshness.py reads FRESH — base 0cccf946 is master's tip and head 90445e1e carries a passing run. check-merge-plan-suite.py on the tree this PR would land, d9aa258d9473ace3393de63df17b84450a92cb0a: 5122 passed, 508 skipped. The first attempt answered rc 2 "could not measure" because merge-tree could not read 5daf3f4a, an intermediate commit whose tree this clone had never materialised; materialising the PR's three commits fixed the reading. A missing object, not a red tree.
One producer, and every former copy. The diff converts all ten spellings the issue lists: Session.__init__ now assigns from session_dir, and Session.delete, the five daemon sites (delete, get-or-create, the resume ghost check, both memory handlers), the two client sites (paste and /image) and the two delegation sites (the subagent slot cwd, the boss's cwd) call it. Renaming the local to session_path where the name would have shadowed the imported function is right, and every call site keeps its value.
The Python guard, both directions. I extracted _slash_chain and _spells_a_session_directory from this head and ran them over two trees and eight synthetic spellings:
| Arm | Reading |
|---|---|
| master (control — must fail) | producer 2, offenders 13 |
| this head (must pass) | producer 1 (session.py:357), offenders 0 |
| a copy under a different local name | flagged |
the copy that existed, … / sid / "images" |
flagged |
cwd / ".emrg" / "sessions" with no id |
quiet — the boundary the docstring names |
The len(producer) == 1 arm is what stops a vacuous pass when nothing is found anywhere, and it does its job.
The Electron half, both directions. One site at this head (main.js:436); after injecting a second path.join(base, ".emrg", "sessions", id, "images") into a copy of the tree, two. It discriminates, and blanking comments rather than counting lines is the right shape for a file whose prose names the same path.
Note, not a veto — the four spellings the predicate does not reach. Confirmed by running it, not by reading:
| Spelling | Result |
|---|---|
cwd / f".emrg/sessions/{sid}" |
not flagged |
cwd.joinpath(".emrg", "sessions", sid) |
not flagged |
os.path.join(cwd, ".emrg", "sessions", sid) |
not flagged |
(cwd / ".emrg") / ("sessions" / sid) |
not flagged |
The last is not a different spelling family: it is the same / chain parenthesised, and it drops out because the flattener walks left-hand Div nodes only, so "sessions" / sid sits where the "sessions" constant is expected. So "expressions that build <cwd>/.emrg/sessions/<id>" reads wider than the predicate walks. That matters for the claim rather than for this tree — every copy that existed was a / chain, so the regression the guard was written for is covered, and the issue's own acceptance item (a copy under a different local name) is measured above as met. Should someone want the concept rather than the family, both additions stay in the same idiom: flatten through a right-hand Div too, and flag a Call to joinpath or os.path.join whose arguments carry the two constants. Neither needs the text scan the docstring rightly rejects.
This vote makes 2/3, so the merge waits for a third cycle.
Closes #2473
What changed
One path rule —
<cwd>/.emrg/sessions/<id>— was spelled out inside ten pieces of code. Nine ofthem are Python. One of those nine was the rule (
Session.__init__inemrg/session.py); the othereight were copies that happened to agree with it:
Session.delete, five sites inemrg/server/daemon.py(session deletion, get-or-create, the resume ghost check, and the two memoryhandlers), and two in
emrg/client/app.py(the clipboard-image directory under paste and/image).The tenth is JavaScript, and it is the one this branch's second pass found:
emrg/gui/main.jsbuilds the same directory to drop a pasted image where the daemon will look for it.
emrg/session.pynow holds the single producer,session_dir(cwd, session_id). Every one of thoseeight Python sites asks it, and
Session.__init__assignsself._dirfrom it, so the class and therule cannot disagree.
The copies were worth removing precisely because they were correct: a copy that computes the right
answer is invisible to every behavioural test, so a layout change would have to find them by search.
Two of them came in with the delegation features (#2454, #2470) — five survived the first pass of
this branch, and the JavaScript site survived the second, which is why the guards below exist rather
than a promise.
The other language cannot delegate, so it keeps one copy
The Electron main process shares a filesystem with the daemon, not a runtime: it cannot call
session_dir. Delegation is not available on that half, so what is available is that the copy staysone copy, and that is what the companion guard pins.
test_the_electron_client_builds_the_same_path_in_one_placescans the client's JavaScript withcomments blanked — the path is also spelled in comments in
main.jsandpreload.js, and a commentis not a producer — and asserts exactly one code site builds it. Measured when written: one, at
emrg/gui/main.js:436.The scan compares each candidate path against the root it walked from, rather than reading
Path.partsof an absolute path. The first version did the latter, andtests/test_walk_skips_read_their_root.py::TestThisTree::test_no_walk_reads_an_absolute_pathcaughtit: an ancestor directory of the checkout named
node_moduleswould have silenced the walk and letthe guard report a clean reading of nothing. The guard is right and the new test was wrong; the fix
is one line in the skip predicate.
Why the Python guard reads the syntax tree, not the text
test_the_directory_a_session_lives_in_has_one_producerwalks every module in the package, flattenseach
/chain, and reports any expression that carries the constants".emrg","sessions"andan operand after them — the leaf, not the directory the sessions live in. A text scan would fail
both ways: it counts the producer's own docstring as a second producer, and it passes an author who
writes the same path with a different local name. The arm confirms the second half — reintroducing
the expression as
sid_dir = cwd / ".emrg" / "sessions" / session_idreddens the guard.Two further pins on the rule itself, which the guard cannot give:
test_session_dir_names_the_directory_a_session_actually_fillscreates a session with a non-emptyid, asserts on disk that the directory exists, and only then asserts the rule names that same
directory — two spellings of one string compare equal however wrong the rule is, so the subject has
to be real.
test_session_dir_keeps_the_workspace_it_was_givenpins the rule's own shape: arelative workspace stays relative, so the caller's cwd still decides.
Every change here is a delegation, not a rewrite:
Path(cwd)moved inside the producer, and thefive daemon locals were renamed from
session_dirtosession_pathbecause the name now belongs tothe function they call.
Verification
Measured on this head, which also carries master
0cccf946:uv run pytest tests/— 5597 passed, 34 skippeduv run python -c "from emrg.client.app import run_client"— okuv run python -m emrg --help— oktest_the_directory_a_session_lives_in_has_one_producerred, naming the file and linevacuous
conn-manager.js—test_the_electron_client_builds_the_same_path_in_one_placered, namingconn-manager.js:282andmain.js:436tests/test_boss_tool.pyandtests/test_subagent_tool.py, which place a child's cwd throughthese paths, pass unchanged
Not in scope, and named so the claim stays honest: the
sessionsroot (without an id) is adifferent value, still spelled at three sites (
generate_session_id,Session.list_all, andemrg/sessions_index.py). One of them sits behind a circular import, so it is its own change ratherthan a rider on this one.