Review — end-to-end ✅ (approved after atomic-write fix pushed)

What this ships

@bergeouss's fix for #1164 — .env corruption when WebUI and Telegram bot/CLI write the shared ~/.hermes/.env concurrently:

1. Removed duplicate _write_env_file in api/onboarding.py; onboarding now imports the locked version from api/providers.py (preserves the api.onboarding._write_env_file name for monkey-patching tests).
2. Refactored _write_env_file in api/providers.py to preserve comments, blank lines, and original key order via line-based update instead of dict-rebuild-and-sort.

Traced against upstream hermes-agent

Pulled fresh nousresearch/hermes-agent tarball.

The shared file is ~/.hermes/.env. Both processes touch it:

• Agent CLI / Telegram: writes via hermes_cli/config.py:save_env_value — uses tempfile.mkstemp + os.replace (atomic on POSIX).
• Agent CLI: reads via load_env which runs _sanitize_env_lines on every read to fix exactly this corruption pattern: "Concatenated KEY=VALUE pairs on a single line (missing newline between entries)." The agent's sanitizer existing is a strong signal that this race occurs in practice.

So the agent does atomic writes. The WebUI's pre-PR _write_env_file used os.open(..., O_TRUNC) — non-atomic. Between the truncate and the complete write, a concurrent reader (or agent writer) sees a partial / interleaved file.

What I caught — within-process is fixed, but cross-process race remains

The PR closes the within-process leg correctly:

• _ENV_LOCK from api.streaming (a real threading.Lock at streaming.py:34) wraps the entire load → modify → write cycle ✅
• onboarding's duplicate is gone, so both code paths share the lock ✅
• comment/order/blank-line preservation prevents loss-on-save corruption ✅

But it leaves the cross-process leg open: the WebUI was still doing os.open(..., O_WRONLY | O_CREAT | O_TRUNC) inside the lock. Threads can't observe the race, but a separate process (Telegram bot, CLI) can — and the agent's _sanitize_env_lines exists specifically because this race happens.

The actual user-reported symptom in #1164 ("config.yaml overwrite or .env corruption causes No LLM provider configured") is the cross-process leg.

What I pushed — 7e5cdfa

(Pushed to @bergeouss's fork via maintainer-edit access.)

Replaced the O_TRUNC write with tempfile.mkstemp + os.replace — same pattern as hermes_cli/config.py:save_env_value:

_tmp_fd, _tmp_path = _tempfile.mkstemp(
    dir=str(env_path.parent), prefix=".env_", suffix=".tmp"
)
try:
    with os.fdopen(_tmp_fd, "w", encoding="utf-8") as _f:
        _f.write(content)
        _f.flush()
        os.fsync(_f.fileno())
    os.chmod(_tmp_path, _mode)  # tighten before rename so readers see 0600
    os.replace(_tmp_path, env_path)
except BaseException:
    try:
        os.unlink(_tmp_path)
    except OSError:
        pass
    raise

Key properties:

• Tempfile is in the same directory → same filesystem → os.replace is atomic on POSIX.
• fsync before rename so the new content is durable.
• chmod 0600 before rename so readers never see a 0644 window.
• Cleanup on exception to avoid leaving stray .env_*.tmp files.

Plus a regression test (test_providers_write_env_uses_atomic_rename) asserting:

• tempfile is referenced in the function body
• os.replace( is called
• O_TRUNC is NOT present (locks the bug from regressing)

CI green on 7e5cdfa: 3.11/3.12/3.13 all ✅. Local full suite: 2595 passed.

End-to-end trace

Comment & key-order preservation (PR's contribution)

providers.py:104-150 — uses existing_lines array indexed by key. Updates rewrite the existing line in place:

if key in existing_key_indices:
    output_lines[existing_key_indices[key]] = f"{key}={clean}"
else:
    new_keys.append(f"{key}={clean}")

New keys appended after a blank-line separator. Deletions use a None sentinel filtered out before write. ✅ All 6 preservation tests pass.

Onboarding delegation (PR's contribution)

onboarding.py:26:

from api.providers import _write_env_file  # shared impl with _ENV_LOCK (#1164)

api.onboarding._write_env_file and api.providers._write_env_file are now the same object — verified by test_onboarding_imports_write_env_from_providers (assertIs). Existing monkey-patching tests that patch api.onboarding._write_env_file continue to work because the name still resolves at the onboarding module level (re-export semantics). ✅

Atomic write (my contribution)

After my push, the write sequence is atomic. Cross-process readers (Telegram bot, agent CLI) see either the OLD file or the NEW file — never partial. Inotify-style watchers fire once, on the rename. ✅ Matches hermes_cli/config.py:save_env_value so both processes use the same shape for atomicity.

Edge-case trace

Tests

• PR's tests (8) + my added test (1): 9/9 pass.
• Local full suite: 2595 passed, 47 skipped, 1 PR-unrelated pre-existing failure (macOS-only test_sprint3 system-paths quirk; verified pre-existing on master).
• CI on PR after my push: ✅ test (3.11), ✅ test (3.12), ✅ test (3.13).

Other audit — confirmed correct

• Permissions: 0600 set on tempfile via os.chmod BEFORE rename, so the file is never world-readable even briefly. The trailing env_path.chmod(_mode) is a belt-and-suspenders for the case where the original file had wider perms and os.replace somehow preserved them.
• Newline injection rejection: if "\n" in clean or "\r" in clean: raise ValueError preserved at line 134 — still blocks .env injection from API key fields.
• os.environ mutation: still inside the lock — concurrent streaming sessions reading env vars see consistent state.
• Backward compat with monkey-patching: api.onboarding._write_env_file is api.providers._write_env_file after the import. mock.patch('api.onboarding._write_env_file') continues to work because Python attribute lookup resolves at the module level.

Minor observations (non-blocking)

• The _load_env_file helper in api/onboarding.py is now unused (the duplicate _write_env_file was its only caller). Leaving it for now since _load_env_file may be referenced by tests; harmless dead code.
• The PR doesn't update ~/.hermes/config.yaml writes (save_settings and friends). That file has its own write path; if it has the same O_TRUNC problem, it'd warrant a similar pass. Out of scope for #1164 which is specifically about .env.
• The agent's _sanitize_env_lines workaround can be safely removed in a future agent release once enough time passes for in-the-wild WebUIs to upgrade. Worth a follow-up note in the agent repo.

Recommendation

Approved (after atomic-rename fix pushed). Within-process correctness from the contributor's PR is solid (lock, comment preservation, key order). I added cross-process safety via tempfile + os.replace to match the pattern hermes_cli uses for the same shared file — that closes the actual root cause symptom in the user report. Existing 8 PR tests preserved + 1 regression test for the atomic invariant. CI green on 3.11/3.12/3.13. Parked at approval — ready for the release agent's merge/tag pipeline.