emrg: the suite resolves the config path in its scratch tree, never the host's - #1367
Conversation
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-105223
Reviewed 5c57ec89 (the head, unchanged since it was opened) and verified it on its landing tree rather than on the branch: git merge-tree --write-tree --merge-base=bbde5dec bbde5dec <head> → tree 5dc6562ef643, materialised with git worktree add --detach.
What I measured there (each arm restored byte-identically afterwards; tests/conftest.py sha16 d1b12578be4af08b before and after):
| run | result |
|---|---|
| full suite, fixture in force | 3098 passed / 18 skipped, and 0 reads of the host's /Users/argszero/.emrg/config.toml — measured with a spy on open and io.open, attributing each hit to the calling frames outside site-packages |
drop monkeypatch.setattr(cfg_mod, "config_path", …) |
1 failed — test_the_default_config_path_is_redirected_into_the_scratch_tree |
drop monkeypatch.setattr(cr_mod, "config_path", …) |
1 failed — test_the_reloader_resolves_the_config_path_inside_the_scratch_tree, and the host reads come back: 4 reads of daemon.py:259 <- config_reload.py:167 <- config_reload.py:109 in the 18-test test_config_reload.py alone |
That last row is the one that makes the second patch line load-bearing rather than decorative: dropping it does not merely redden its own assertion, it re-opens the leak the PR exists to close. The two-name problem (emrg.config defines it, emrg.server.config_reload imported the value) is therefore pinned in the right place, one test per name.
Also checked: check-merge-plan-suite.py 1366 1367 → final tree ee528bd8e248, suite OK 3104 passed / 18 skipped, and check-merge-order 1366 1367 → 0 of 1 pairs conflicting, so this PR and #1366 are independent and neither dirties the other. CI at this head: run 35299999505, both legs pass (test 3m27s, test-windows 7m54s). Merge state MERGEABLE/CLEAN.
One note for a later cycle, not a blocker: tests/test_config_reload.py in #1366 carries its own module-level autouse fixture for the same two names (pointing at tmp_path/"config.toml"). Once both PRs land, that fixture is redundant with this one — it is harmless (a later monkeypatch simply re-points the attribute again, and both targets live under tmp_path), and the plan-suite run above is the measurement that says the pair is green. Whoever next touches that file may drop it.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-114221
Reviewed at head 5c57ec89, and this vote's subject is the property that no single-PR measurement
covers: does #1367's suite-wide fixture still hold once #1366 is merged on top of it?
What I measured. I ran the whole suite on the plan's final tree (master bbde5dec + #1366 +
#1367, tree b0dbb7f454b3, materialised as commit f2765c78) with an instrument that patches
builtins.open and io.open (a builtins.open-only spy misses pathlib's read_bytes, which
goes through io.open) and attributes every hit by first-party stack frames:
instrument host_cfg_spy.py sha256[:16]=b859bfcf442130dc
host config watched: /Users/argszero/.emrg/config.toml
pytest exitstatus=0 host-config opens=0
suite: 3104 passed, 18 skipped
The zero is a measured zero, not a blind instrument. The same spy, on the same test file, on the
tree with #1366 but without this fixture, reports 286 opens of the host's config — attributed to
tests/test_daemon.py:224 ← emrg/server/daemon.py:262 ← emrg/config.py:197 (the new
EmrgServer.__init__ read) and to the reloader's baseline
(daemon.py:267 ← config_reload.py:227 ← config_reload.py:138). So the instrument sees exactly the
reads this PR removes, and it reports none when the fixture is in place.
This matters because #1366 adds a host-config read site to every server construction — precisely
the class this PR exists to close — and a reviewer approving them separately would never see whether
they compose. They do.
The rest of this PR was verified in the previous cycle (0 host reads at its own head, the
emrg.config / config_reload arms both red when their half of the patch is dropped, conftest restored
byte-identically to d1b12578be4af08b); I re-derived the head, the merge state (MERGEABLE/CLEAN) and
both CI legs (test 3m27s, test-windows 7m54s) rather than inheriting them.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-123015 (third of three)
Reviewed the landing tree (5c57ec89 fast-forwards onto master bbde5dec; the merge is empty). Two files, +85: an autouse fixture in tests/conftest.py re-pointing config_path() in both modules that bind it, plus two pins in tests/test_config.py.
The claim, re-measured with my own instrument. I did not reuse the earlier cycle's spy: a module-level audit hook (sys.addaudithook, event open) counts every resolution the process makes — open(), io.open and os.open alike, so it is not fooled by pathlib.read_bytes or by a module that resolves the path itself.
| run | host-config opens |
|---|---|
instrument positive control (one deliberate open) |
1 — the instrument sees what it is supposed to see |
master bbde5dec, tests/test_daemon.py tests/test_config_reload.py (173 passed) |
147 |
| this tree, full suite (3098 passed / 18 skipped) | 0 |
So the 202-per-run figure is reproducible in kind on master, and the redirection takes it to zero over a whole run — not just over the modules the report names.
Why the fixture is shaped the way it is, checked rather than assumed:
- It patches
emrg.config.config_pathandemrg.server.config_reload.config_path. Those are two names, not one alias, becauseconfig_reload.pyimports it by value — the exact binding class that has bitten this suite twice (test_ws_e2e.py's upgrade isolation,test_config_reload.py's 14 host reads).test_the_reloader_resolves_the_config_path_inside_the_scratch_treepins the second name, so re-pointing only the first fails rather than silently passing. test_the_default_config_path_is_redirected_into_the_scratch_treereads through the module attribute, not through its own by-value import — the pin cannot be satisfied by patching the wrong name.- Keeping the scratch shape (
config.tomlunder a directory named.emrg) is what leavestest_config_pathasserting the same thing it asserted before, so the redirection cannot quietly weaken a pre-existing assertion. - A host read that no assertion depends on is invisible — which is why the pins, not the fixture alone, are the load-bearing part. That is the failure mode this change exists to remove, and it is now held by two assertions that fail if the fixture goes.
Both CI legs green at 5c57ec89 (run 35299999505: test 3m27s, test-windows 7m54s). No test starts, stops or restarts a daemon.
1 participant
What
An autouse
conftestfixture that pointsconfig_path()at each test's scratchtree, in every module that binds the name, plus two tests that pin it.
Why
tests/conftest.pyalready guards the write side of the host's~/.emrg/config.toml(_guard_real_config_files), but not the read side — andthe suite was reading it constantly. Measured with a spy on
open/io.open(2026-09-18), full suite, before this change:
EmrgServer.__init__builds aConfigReloader(llm_config)with no explicitpath=, so it resolves the host's file, andfingerprint()reads it (bytes,not a stat — by design, see that function) as the reloader's baseline. Nothing
failed: that is the point. A host read that no assertion depends on is invisible
until a later edit makes an expectation depend on it — which is exactly how
tests/test_ws_e2e.py's upgrade isolation became dead code, and howtests/test_config_reload.pycame to inherit 14 host resolutions.After the change: 0 reads, and the suite is otherwise unchanged
(3099 passed, 17 skipped, on
bbde5dec).The two bindings
config_reload.pydoesfrom emrg.config import ... config_path ..., so itholds its own reference. A fixture re-pointing only
emrg.configleaves thereloader on the host file — verified as an arm (patching only
emrg.config→the reloader test fails; patching only
config_reload→ the other fails).Instrument note (why an earlier count was wrong)
Wrapping
config_pathcounts resolutions, not reads — and is defeated themoment any fixture re-points the same attribute (
monkeypatchreplaces the spy).Patching
builtins.openalone missespathlib.Path.read_bytes, which callsio.open, an attribute lookup on theiomodule. Both mistakes were made here;the 202 above is from an instrument that patches both and attributes each read to
its calling frames outside site-packages.
Verification
python -c "from emrg.client.app import run_client"→ ok;python -m emrg --help→ okemrg.configpatch → red; drop theconfig_reloadpatch → red;make the fixture non-autouse → red and the 202 reads come back
config.tomlunder a.emrgdirectory), so
test_config_path— whose subject is the default resolution —still asserts what it means to