mirror of
https://github.com/github/spec-kit.git
synced 2026-08-15 14:02:04 +02:00
fix(events): preserve a non-UTF-8 config.toml on hook install/teardown (#3963)
_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>
This commit is contained in:
@@ -1342,11 +1342,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
|
||||
@@ -1932,11 +1933,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*',
|
||||
"",
|
||||
@@ -1945,6 +1962,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:
|
||||
@@ -1958,7 +1976,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*',
|
||||
"",
|
||||
|
||||
@@ -868,6 +868,59 @@ class TestTomlMatcherEscaping:
|
||||
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:
|
||||
|
||||
Reference in New Issue
Block a user