Skip to content

emrg: the config write guard names all three host files and the seam that writes them - #2436

Merged
argszero merged 1 commit into
masterfrom
fix/config-write-guard-covers-config-toml
Oct 21, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/config-write-guard-covers-config-toml

Conversation

@how2how2how2-arch

Copy link
Copy Markdown
Collaborator

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_FILES gains config.toml. It held projects.yml and tasks.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_yaml is how the two YAML files are saved, and it was the only writer patched. config.toml is not YAML: emrg/config.py::ensure_config creates it with cfg_path.write_text(CONFIG_TEMPLATE), which that patch never sees. The guard now also refuses a write-mode open and io.open aimed at those paths — which is what Path.write_text, Path.write_bytes and an append all reach.

The membership test is now a named predicate, _is_real_config_target, with a .emrg pre-filter. That filter is not the rule: open is called for every file the suite touches, and resolving each one would put a stat on 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 of config.toml made tomllib raise Expected '=' 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_text only 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_text and Path.write_bytes are deliberately not patched separately. They reach io.open through Path.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-level open, and io.open, which is what Path.open calls. 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.py is 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_FILES at a tmp_path.

Three arms, each removing one load-bearing piece:

arm removed result
A config.toml from the file list 1 failed, 7 passed — test_the_three_real_files_are_named
B the io.open patch 3 failed, 5 passed — write_text, write_bytes, ensure_config end to end
C the builtins.open patch 1 failed, 7 passed — the append

All 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 — by io.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_config with config_path pointed 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.
  • CI: pending at submission.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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_named fails; restored → 8 passed;
  • the fixture patches two seams, and each has to be patched separately: modules' open and
    io.open are distinct attributes, and Path.write_text only 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.

@argszero
argszero merged commit 5f0dc87 into master Oct 21, 2026
2 checks passed
@argszero argszero mentioned this pull request Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

emrg: the suite's config-file write guard omits config.toml, the one host file whose corruption stops the daemon

2 participants