Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 38 additions & 8 deletions src/specify_cli/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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*',
"",
Expand All @@ -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:
Expand All @@ -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*',
"",
Expand Down
53 changes: 53 additions & 0 deletions tests/integrations/test_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down