From e706da2466e545e51ac768122a87f83ec6234c5c Mon Sep 17 00:00:00 2001
From: marcelsafin <179933638+marcelsafin@users.noreply.github.com>
Date: Mon, 3 Aug 2026 21:37:00 +0200
Subject: [PATCH] fix(events): preserve a non-UTF-8 config.toml on hook
 install/teardown
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

_merge_toml_fragment() and _remove_toml_entries() read the user's
config.toml with bare read_text() calls, so a non-UTF-8 (or otherwise
unreadable) file crashed install_integration_events() and
remove_integration_events() with a raw UnicodeDecodeError — and the
merge path regenerates the file from what it read, so it would have
discarded the user's bytes had it not crashed first. Every JSON
merge/remove path already goes through _load_user_json(), which skips
on an unreadable file to preserve user content (#22).

Abort the merge (returning False so the caller skips tracking, S5) and
skip the teardown cleanup with a warning, leaving the user's bytes
untouched in both directions.

Assisted-by: GitHub Copilot (model: claude-fable-5, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 src/specify_cli/events.py         | 46 ++++++++++++++++++++++-----
 tests/integrations/test_events.py | 53 +++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 8 deletions(-)

diff --git a/src/specify_cli/events.py b/src/specify_cli/events.py
index d3002fe805..102e72dc3b 100644
--- a/src/specify_cli/events.py
+++ b/src/specify_cli/events.py
@@ -1193,11 +1193,12 @@ def install_integration_events(
                 lines.append(f'timeout = {_native_timeout(integration, cfg.get("timeout", 60) + EVENT_TIMEOUT_BUFFER)}')
                 lines.append('speckit_marker = true')
                 lines.append('')
-        _merge_toml_fragment(config_path, "\n".join(lines))
-        rel = str(config_path.relative_to(project_root))
-        if rel not in manifest.files:
-            manifest.record_existing(rel)
-        created.append(config_path)
+        # S5: only track when the merge wrote (skips on unreadable file).
+        if _merge_toml_fragment(config_path, "\n".join(lines)):
+            rel = str(config_path.relative_to(project_root))
+            if rel not in manifest.files:
+                manifest.record_existing(rel)
+            created.append(config_path)
 
     elif fmt == "json-flat":
         # Cursor hooks.json custom merge. Flat command-string entries, one
@@ -1715,11 +1716,27 @@ def _remove_opencode_entries(config_path: Path) -> bool:
     return False
 
 
-def _merge_toml_fragment(dst: Path, fragment: str) -> None:
+def _merge_toml_fragment(dst: Path, fragment: str) -> bool:
+    """Merge Specify-owned TOML entries into *dst*, regenerating the file.
+
+    An unreadable or undecodable pre-existing file aborts the merge instead
+    of discarding the user's bytes, mirroring ``_load_user_json`` (#22).
+    Returns False when skipped so callers avoid tracking the untouched file
+    (S5).
+    """
     _ensure_safe_destination(dst)
     existing = ""
     if dst.exists():
-        existing = dst.read_text(encoding="utf-8")
+        try:
+            existing = dst.read_text(encoding="utf-8")
+        except (OSError, UnicodeDecodeError) as exc:
+            logger.warning(
+                "Could not read %s (it may be unreadable or not UTF-8); "
+                "skipping event-config merge to preserve user content.",
+                dst,
+            )
+            logger.debug("Read error detail: %s", exc)
+            return False
     existing = re.sub(
         r'\[\[hooks\.\w+\]\]\n(?:(?!\[\[hooks\.\w+\]\]).)*?speckit_marker = true\n*',
         "",
@@ -1728,6 +1745,7 @@ def _merge_toml_fragment(dst: Path, fragment: str) -> None:
     )
     dst.parent.mkdir(parents=True, exist_ok=True)
     dst.write_text(existing.rstrip() + "\n\n" + fragment + "\n", encoding="utf-8")
+    return True
 
 
 def _remove_toml_entries(dst: Path) -> bool:
@@ -1741,7 +1759,19 @@ def _remove_toml_entries(dst: Path) -> bool:
     # the config after install can't make teardown overwrite a file outside
     # the project (the merge/write path already validates; teardown must too).
     _ensure_safe_destination(dst)
-    existing = dst.read_text(encoding="utf-8")
+    try:
+        existing = dst.read_text(encoding="utf-8")
+    except (OSError, UnicodeDecodeError) as exc:
+        # An unreadable or undecodable file is left untouched rather than
+        # crashing teardown — it contains only user content as far as we can
+        # tell, and the caller drops the manifest claim either way (S9).
+        logger.warning(
+            "Could not read %s (it may be unreadable or not UTF-8); "
+            "skipping event-config cleanup to preserve user content.",
+            dst,
+        )
+        logger.debug("Read error detail: %s", exc)
+        return False
     cleaned = re.sub(
         r'\[\[hooks\.\w+\]\]\n(?:(?!\[\[hooks\.\w+\]\]).)*?speckit_marker = true\n*',
         "",
diff --git a/tests/integrations/test_events.py b/tests/integrations/test_events.py
index 084156a523..b65b8dbfaf 100644
--- a/tests/integrations/test_events.py
+++ b/tests/integrations/test_events.py
@@ -751,6 +751,59 @@ def test_matcher_with_quote_stays_valid_toml(self, tmp_path):
         assert group["matcher"] == 'Ba"sh'
 
 
+class TestTomlUnreadableConfig:
+    """An undecodable user config.toml must not crash install or teardown.
+
+    Every JSON merge/remove path goes through ``_load_user_json``, which
+    skips on an unreadable or malformed file to preserve user content (#22).
+    The TOML merge and remove read the user's config.toml with no boundary,
+    so a non-UTF-8 (or otherwise unreadable) file crashed
+    ``install_integration_events``/``remove_integration_events`` with a raw
+    ``UnicodeDecodeError`` — and the merge path would have regenerated the
+    file, discarding the user's bytes, had it not crashed first.
+    """
+
+    def test_merge_skips_unreadable_config_and_preserves_bytes(self, tmp_path):
+        from specify_cli.integrations.codex import CodexIntegration
+
+        integration = CodexIntegration()
+        manifest = _claude_manifest(tmp_path)
+        config_path = tmp_path / ".codex" / "config.toml"
+        config_path.parent.mkdir(parents=True)
+        user_bytes = b"# codex config \xff\xfe not utf-8\n"
+        config_path.write_bytes(user_bytes)
+
+        install_integration_events(
+            integration, tmp_path, manifest,
+            {"pre_tool_use": [{"command": "speckit.tdd.validate"}]},
+        )
+
+        # User bytes preserved and the skipped file is not tracked (S5).
+        assert config_path.read_bytes() == user_bytes
+        manifest.record_existing.assert_not_called()
+
+    def test_teardown_skips_unreadable_config_and_preserves_bytes(self, tmp_path):
+        from specify_cli.integrations.codex import CodexIntegration
+
+        integration = CodexIntegration()
+        manifest = _claude_manifest(tmp_path)
+        install_integration_events(
+            integration, tmp_path, manifest,
+            {"pre_tool_use": [{"command": "speckit.tdd.validate"}]},
+        )
+        config_path = tmp_path / ".codex" / "config.toml"
+        assert config_path.is_file()
+
+        # The user (or another tool) rewrites the config as non-UTF-8
+        # between install and uninstall.
+        user_bytes = b"# rewritten \xff\xfe not utf-8\n"
+        config_path.write_bytes(user_bytes)
+
+        remove_integration_events(integration, tmp_path, manifest)
+
+        assert config_path.read_bytes() == user_bytes
+
+
 # -- Opencode TS Plugin merging ---------------------------------------------
 
 class TestOpencodePluginMerging: