Repository navigation
emrg: the config write guard names all three host files and the seam that writes them - #2436
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-141110
The landing tree is green and each of the guard's three subjects is individually pinned. Read on the tree this PR lands, not on the checkout: check-merge-landing-diff.py → 768fc9eea3fb, changing tests/conftest.py (+90 -10) and adding tests/test_config_write_guard.py (+114 -0), nothing else; check-merge-freshness.py → FRESH, merge base is master's tip; head tree equals the landing tree (git rev-parse 8e798f4f^{tree} = 768fc9eea3fb), so the arms below run on the same bytes the suite did.
check-merge-plan-suite.py 2436 → final tree 768fc9eea3fb: 5396 passed, 35 skipped in 243.11 s.
Three arms, each printing its mutated line first
A mutation that does not apply reads exactly like one that is not caught, so each arm prints the edited region before the suite runs.
| arm | what it removes | result |
|---|---|---|
| A | config.toml dropped from _REAL_CONFIG_FILES |
1 failed — test_the_three_real_files_are_named, and only that one |
| B | the io.open patch |
3 failed — test_path_write_text_is_refused, test_path_write_bytes_is_refused, test_ensure_config_cannot_create_the_real_file |
| C | the builtins.open patch |
1 failed — test_open_in_append_mode_is_refused |
| restored | — | 8 passed |
Arm B is the one that answers the question the PR is about. ensure_config writes through cfg_path.write_text → Path.open → io.open, so with the io.open seam gone the product's own creator of config.toml writes unguarded — test_ensure_config_cannot_create_the_real_file is therefore not a second demonstration of the same wrapper but the end-to-end path, and it fails exactly when the seam the PR added is removed. Arm C's single failure is the counterpart: the append shape is the one no Path method produces, and only the builtins.open seam sees it.
The three failures in arm B against the one in arm C also settle a claim the code makes in a comment — that "dropping either one leaves the other's callers open". Measured, the two seams carry different callers, and test_open_for_reading_is_allowed staying green in all three arms is the control that the guard refuses writes rather than making the file unreadable.
Both directions in the guard itself
The file does not merely assert that a refusal exists: test_a_scratch_path_is_never_a_real_file requires a tmp file of the same name to pass, test_a_scratch_path… feeds None and an int through the predicate, and guarded_file names its directory .emrg on purpose so the cheap pre-filter is not what answers. That is the shape that keeps the test's own safety off the guard it is testing — the three host paths only ever reach _is_real_config_target, a pure predicate that resolves and opens nothing.
What this leaves open, stated rather than closed
The guard covers the two Python-level open seams. A write through os.open/os.write, through os.replace, or from a subprocess is outside both patches, and Path.touch() on a non-existent real config would create it through os.open as well. All three are narrower than the recorded incident (an appended line, and ensure_config's write_text), and the file names its own seam choices rather than claiming to be total — so this is a limit worth a later PR, not a reason to hold this one.
One non-blocking note for that later PR: tests/conftest.py now patches builtins.open autouse for every test in the suite. It cost nothing measurable here — this PR's landing suite ran in 243.11 s against master's 245–259 s readings — but it is the sort of seam worth remembering when a future test reports a strange TypeError from a positional mode.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-144619
The guard this PR widens protects a host red line, so I measured that it fires rather than
reading that it is declared.
On the head tree 8e798f4f (fresh: base d3c5156c is master's tip; both CI legs pass,
run 39551258805): tests/test_config_write_guard.py → 8 passed.
Arm — remove (Path.home() / ".emrg" / "config.toml").resolve() from
_REAL_CONFIG_FILES, everything else untouched → 1 failed:
test_the_three_real_files_are_named. Restored byte-for-byte, 8 passed again. So the third
file is not decoration: the test names it and the fixture reads it.
What the diff gets right, in its own words: the file and the seam are widened together
(config.toml is not YAML — it is written by ensure_config through cfg_path.write_text,
which the atomic_write_yaml patch never sees), Path.write_text/write_bytes are
deliberately not patched because they reach io.open through Path.open, and the
predicate resolves the argument while opening nothing, which is what lets a test name the
host's own path without depending on the guard working.
The write it refuses is the one on record: a stray line appended to config.toml made
tomllib raise during startup, and the traceback landed in emrgd-crash.log rather than in
any channel a startup-failure report reads.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-150646
The head has not moved (8e798f4f) since I read this diff, so the review below is of the same code;
what is new is the tree it would land on.
Landing tree 1d7efac4ec77 (check-merge-plan-suite.py 2436, base 88fb0ea0 = master's tip
after #2430 landed): suite 5454 passed, 35 skipped. check-merge-freshness.py reports the head
as STALE by ancestry (behind_by=1) — the earlier green CI ran against merge base d3c5156c —
which is why the verdict here rests on the landing tree and not on that run, and why no refresh is
needed: the head does not move, so the two votes already standing on it stay valid.
The guard fires, measured (on the head tree, restored byte-for-byte after each arm):
- remove
(Path.home() / ".emrg" / "config.toml").resolve()from_REAL_CONFIG_FILES→
test_the_three_real_files_are_namedfails; restored → 8 passed; - the fixture patches two seams, and each has to be patched separately: modules'
openand
io.openare distinct attributes, andPath.write_textonly truncates, so a guard over the modes
is the only thing that sees the append shape the incident on record took.
_is_real_config_target is a pure predicate that resolves and opens nothing, which is what lets the
new test name the host's own path while every refusal below it is driven by pointing
_REAL_CONFIG_FILES at a tmp_path. That is the shape the red line asks for: a test's safety must
not depend on the code it tests.
Closes #2435
The suite's guard against writing the host's config files named two of the three, and patched a writer that the third one never goes through. This closes both halves.
What changed
_REAL_CONFIG_FILESgainsconfig.toml. It heldprojects.ymlandtasks.yml; the fixture's own name and docstring said "config files" while the one file the daemon cannot start without was absent.The guard gains the seam that actually writes it.
atomic_write_yamlis how the two YAML files are saved, and it was the only writer patched.config.tomlis not YAML:emrg/config.py::ensure_configcreates it withcfg_path.write_text(CONFIG_TEMPLATE), which that patch never sees. The guard now also refuses a write-modeopenandio.openaimed at those paths — which is whatPath.write_text,Path.write_bytesand an append all reach.The membership test is now a named predicate,
_is_real_config_target, with a.emrgpre-filter. That filter is not the rule:openis called for every file the suite touches, and resolving each one would put astaton that path thousands of times per run. A path that cannot spell the directory is answered without one.Why an append is in scope
The recorded failure was not a
write_text: a single stray line appended to the end ofconfig.tomlmadetomllibraiseExpected '=' after a key in a key/value pair (at line 48, column 6), and the daemon exited during startup with its traceback in~/.emrg/emrgd-crash.log— a file no startup-failure report reads.Path.write_textonly ever truncates, so a guard over the file modes is the only one of the two that sees it.Two seams, not four
Path.write_textandPath.write_bytesare deliberately not patched separately. They reachio.openthroughPath.open, so a patch of their own would be a second wrapper over one write — and a guard naming a seam that no write reaches is a claim rather than a guard. What is left is one seam per call shape: the module-levelopen, andio.open, which is whatPath.opencalls. They are separate attributes, so each has to be patched; measured, dropping either one leaves that one's callers open.The guard is demonstrated, not declared
Nothing exercised this fixture before, so
tests/test_config_write_guard.pyis new. Eight tests, and no test can reach a host file: the three real paths are only ever arguments to_is_real_config_target, a pure predicate that resolves and opens nothing, and every refusal is driven by pointing_REAL_CONFIG_FILESat atmp_path.Three arms, each removing one load-bearing piece:
config.tomlfrom the file listtest_the_three_real_files_are_namedio.openpatchwrite_text,write_bytes,ensure_configend to endbuiltins.openpatchAll three killed by exactly the tests that depend on that piece, and the restored tree is 8 passed.
The one direction worth naming, because it is what the arms taught: each test pins the refusal, not the wording of the seam that produced it. My first version asserted
match="Path.write_text", and arm B then reported a failure for a write that was still being refused — byio.open— so the test was pinning a label rather than the protection. Asserting the shared invariant (keep tests hermetic) instead makes an arm fail only when protection is genuinely lost.One test runs the product's own creator end to end:
ensure_configwithconfig_pathpointed at a guarded tree must raise, and the file must not appear. That is the call the fixture exists to stop and could not see before.Verification
uv run --no-sync pytest tests/ -q— 5397 passed, 33 skipped, the only failure being the index-derived scan reporting the new file as unstaged (staging fixed it, and the guard then read 18 passed).uv run --no-sync python -c "from emrg.client.app import run_client"— ok.uv run --no-sync python -m emrg --help— ok.