From beaa1a08e6abf2fb8efff0b05da8857bef21ce1f Mon Sep 17 00:00:00 2001
From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com>
Date: Sun, 5 Jul 2026 22:53:25 +0530
Subject: [PATCH] fix(config): guard xai migration writer + drop gratuitous
 annotation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Phase-2 review follow-ups on the unreadable-config chokepoint work:

- hermes_cli/xai_retirement.py apply_migration() is a full-file config.yaml
  rewriter (ruamel round-trip + plain open("w")) that lives outside the
  atomic_yaml_write path, so the chokepoint didn't cover it. It reads the
  file first (which already fails closed on an unreadable file), but add
  require_readable_config_before_write() right before the write as a
  backstop for the read-then-write window, and a regression test asserting
  the original bytes survive an unreadable config.
- Drop the unnecessary "Path" string quotes on atomic_config_write's
  annotation — Path is imported eagerly at module top, no forward ref needed.

auth.py _update_config_for_provider / _reset_config_provider intentionally
keep their standalone require_readable_config_before_write guard + bare
atomic_yaml_write: the guard must fire BEFORE the read (fail-fast) at those
read-then-write sites, and a test pins the atomic_yaml_write call. Both are
already fully guarded against the bug; routing them through the wrapper
would move the check to write time for no benefit.
---
 hermes_cli/config.py                 |  2 +-
 hermes_cli/xai_retirement.py         |  3 +++
 tests/hermes_cli/test_migrate_xai.py | 27 +++++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/hermes_cli/config.py b/hermes_cli/config.py
index fb408026867..53c88c69332 100644
--- a/hermes_cli/config.py
+++ b/hermes_cli/config.py
@@ -6523,7 +6523,7 @@ def require_readable_config_before_write(config_path: Optional[Path] = None) ->
         ) from exc
 
 
-def atomic_config_write(config_path: "Path", data: Any, **kwargs: Any) -> None:
+def atomic_config_write(config_path: Path, data: Any, **kwargs: Any) -> None:
     """Fail-closed atomic write for ``config.yaml``.
 
     The single chokepoint every config-update path should use instead of
diff --git a/hermes_cli/xai_retirement.py b/hermes_cli/xai_retirement.py
index 02ad903f7b3..0dc8a45c6f7 100644
--- a/hermes_cli/xai_retirement.py
+++ b/hermes_cli/xai_retirement.py
@@ -242,6 +242,9 @@ def apply_migration(
         )
         shutil.copy2(config_path, backup_path)
 
+    from hermes_cli.config import require_readable_config_before_write
+
+    require_readable_config_before_write(config_path)
     with config_path.open("w", encoding="utf-8") as fh:
         yaml.dump(doc, fh)
 
diff --git a/tests/hermes_cli/test_migrate_xai.py b/tests/hermes_cli/test_migrate_xai.py
index 8a913e98bf2..32162b2064e 100644
--- a/tests/hermes_cli/test_migrate_xai.py
+++ b/tests/hermes_cli/test_migrate_xai.py
@@ -221,3 +221,30 @@ class TestIdempotence:
         assert issues_2 == []
         result_2 = apply_migration(trap_config, issues_2)
         assert result_2.config_changed is False
+
+
+# ---------------------------------------------------------------------------
+# Fail-closed on unreadable existing config
+# ---------------------------------------------------------------------------
+
+class TestUnreadableExistingConfig:
+    def test_apply_refuses_to_overwrite_unreadable_config(self, trap_config: Path):
+        """apply_migration must not clobber an existing config.yaml it can't
+        read. It reads the file first (which raises on an unreadable file), and
+        the require_readable_config_before_write guard before the write is a
+        belt-and-suspenders backstop for the read-then-write window. Either way
+        the original bytes must survive."""
+        import os
+
+        issues = find_retired_xai_refs(_parse(trap_config))
+        assert issues  # sanity: trap_config has retired refs
+        original = trap_config.read_bytes()
+
+        os.chmod(trap_config, 0o000)
+        try:
+            with pytest.raises((PermissionError, RuntimeError, OSError)):
+                apply_migration(trap_config, issues, backup=False)
+        finally:
+            os.chmod(trap_config, 0o644)
+
+        assert trap_config.read_bytes() == original