From abe1e83df62fcdf47a5b1e60ed6b977e5e67c74b Mon Sep 17 00:00:00 2001 From: chelsealong Date: Sun, 13 Sep 2026 02:15:47 +0000 Subject: [PATCH 1/2] fix: skip codex config.toml rewrite when there's nothing to merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _merge_toml_fragment() always rewrote .codex/config.toml, even when the event fragment was empty and there were no Specify-owned hook blocks to remove. That unconditional rewrite appended stray blank lines and, via Python's text-mode newline translation on read/write, silently changed the file's line-ending convention (LF -> CRLF on Windows) — turning a no-op install into a spurious, unmanifested diff on a pre-existing tracked file. Now the merge is skipped (and the file left untouched) when there is no fragment to add and no owned blocks to remove, matching the existing S5 tracking convention used by the other native-format mergers in this file. --- src/specify_cli/events.py | 11 ++++++++--- tests/integrations/test_events.py | 33 +++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/events.py b/src/specify_cli/events.py index 17a9d7ffbe..3acea8bcae 100644 --- a/src/specify_cli/events.py +++ b/src/specify_cli/events.py @@ -2129,7 +2129,10 @@ def _merge_toml_fragment(dst: Path, fragment: str) -> bool: 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). + (S5) — including when there is no fragment to add and no owned blocks to + remove, so a no-op install doesn't rewrite (and, via text-mode newline + translation, mangle the line endings of) an untouched pre-existing file + (#4563). """ _ensure_safe_destination(dst) existing = "" @@ -2144,14 +2147,16 @@ def _merge_toml_fragment(dst: Path, fragment: str) -> bool: ) logger.debug("Read error detail: %s", exc) return False - existing = re.sub( + stripped = re.sub( r'\[\[hooks\.\w+\]\]\n(?:(?!\[\[hooks\.\w+\]\]).)*?speckit_marker = true\n*', "", existing, flags=re.DOTALL, ) + if not fragment and stripped == existing: + return False dst.parent.mkdir(parents=True, exist_ok=True) - dst.write_text(existing.rstrip() + "\n\n" + fragment + "\n", encoding="utf-8") + dst.write_text(stripped.rstrip() + "\n\n" + fragment + "\n", encoding="utf-8") return True diff --git a/tests/integrations/test_events.py b/tests/integrations/test_events.py index 0168f4302c..5936d896ae 100644 --- a/tests/integrations/test_events.py +++ b/tests/integrations/test_events.py @@ -937,6 +937,39 @@ def test_teardown_skips_unreadable_config_and_preserves_bytes(self, tmp_path): assert config_path.read_bytes() == user_bytes +class TestTomlNoOpMerge: + """#4563: a merge with no fragment to add and no owned blocks to remove + must leave a pre-existing config.toml byte-for-byte untouched. + + Previously the merge unconditionally rewrote the file via + ``existing.rstrip() + "\\n\\n" + fragment + "\\n"`` even when ``fragment`` + was empty, which both appended stray blank lines and (through Python's + text-mode newline translation on read/write) silently changed the file's + line-ending convention — turning a clean install into a spurious git diff + with no semantic change. + """ + + def test_no_handlers_leaves_existing_config_untouched(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) + original_bytes = b"project_doc_max_bytes = 200000" + config_path.write_bytes(original_bytes) + + # An event key resolves for Codex but carries no handlers, so there + # is no hook fragment to merge in. + install_integration_events( + integration, tmp_path, manifest, + {"pre_tool_use": []}, + ) + + assert config_path.read_bytes() == original_bytes + manifest.record_existing.assert_not_called() + + # -- Opencode TS Plugin merging --------------------------------------------- class TestOpencodePluginMerging: From 6cf4b3fdb0d1f257e447a42323473dc3b3771b70 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Sun, 13 Sep 2026 02:32:59 +0000 Subject: [PATCH 2/2] fix: skip unconditional rewrite in _remove_toml_entries for real no-op path Review found the prior fix patched the wrong function: the real "specify integration install codex" repro (no Codex event hooks configured) resolves to events={}, which routes through install_integration_events's empty-map branch into _remove_native_event_hooks -> _remove_toml_entries, never touching _merge_toml_fragment. _remove_toml_entries still rewrote the file unconditionally even when the regex strip was a no-op, which (via text-mode newline translation) mangles line endings on Windows. Adds the same cleaned == existing guard to _remove_toml_entries, and replaces the regression test with one that drives the real install_integration_events(..., events={}) path instead of a synthetic events map no caller can produce. --- src/specify_cli/events.py | 7 ++++++ tests/integrations/test_events.py | 41 +++++++++++++++++++++---------- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/specify_cli/events.py b/src/specify_cli/events.py index 3acea8bcae..f61995c895 100644 --- a/src/specify_cli/events.py +++ b/src/specify_cli/events.py @@ -2200,6 +2200,11 @@ def _remove_toml_entries(dst: Path) -> bool: """Remove Specify-marked TOML entries; delete the file if now empty (#14). Returns True if the file was deleted (no user content remained). + + Leaves the file untouched (no write) when there are no Specify-owned + blocks to strip, so a no-op teardown/install doesn't rewrite (and, via + text-mode newline translation, mangle the line endings of) an untouched + pre-existing file (#4563). """ if not dst.exists(): return False @@ -2235,6 +2240,8 @@ def _remove_toml_entries(dst: Path) -> bool: if not stripped: dst.unlink(missing_ok=True) return True + if cleaned == existing: + return False dst.write_text(cleaned, encoding="utf-8") return False diff --git a/tests/integrations/test_events.py b/tests/integrations/test_events.py index 5936d896ae..7476ec6531 100644 --- a/tests/integrations/test_events.py +++ b/tests/integrations/test_events.py @@ -938,18 +938,25 @@ def test_teardown_skips_unreadable_config_and_preserves_bytes(self, tmp_path): class TestTomlNoOpMerge: - """#4563: a merge with no fragment to add and no owned blocks to remove - must leave a pre-existing config.toml byte-for-byte untouched. - - Previously the merge unconditionally rewrote the file via - ``existing.rstrip() + "\\n\\n" + fragment + "\\n"`` even when ``fragment`` - was empty, which both appended stray blank lines and (through Python's - text-mode newline translation on read/write) silently changed the file's - line-ending convention — turning a clean install into a spurious git diff - with no semantic change. + """#4563: installing with no resolved events must leave a pre-existing, + Specify-unowned config.toml byte-for-byte untouched. + + This is the real ``specify integration install codex`` repro: a project + with no Codex-specific event hooks configured resolves to ``events={}`` + (see ``resolve_events``), which routes through + ``install_integration_events``'s empty-map branch into + ``_remove_native_event_hooks`` -> ``_remove_toml_entries`` — not through + ``_merge_toml_fragment``, which only runs when there is at least one + supported, non-empty event to merge. ``_remove_toml_entries`` computed + ``cleaned`` via a regex strip and then unconditionally called + ``dst.write_text(cleaned, ...)`` even when ``cleaned == existing`` (no + Specify-marked blocks present), which (through Python's text-mode + newline translation on read/write) silently changed the file's + line-ending convention on Windows — turning a clean install into a + spurious git diff with no semantic change. """ - def test_no_handlers_leaves_existing_config_untouched(self, tmp_path): + def test_no_events_leaves_existing_config_untouched(self, tmp_path): from specify_cli.integrations.codex import CodexIntegration integration = CodexIntegration() @@ -958,15 +965,23 @@ def test_no_handlers_leaves_existing_config_untouched(self, tmp_path): config_path.parent.mkdir(parents=True) original_bytes = b"project_doc_max_bytes = 200000" config_path.write_bytes(original_bytes) + mtime_before = config_path.stat().st_mtime_ns - # An event key resolves for Codex but carries no handlers, so there - # is no hook fragment to merge in. + # The real no-extensions-installed shape: resolve_events() returns an + # empty map when no built-in defaults, extensions, or overrides + # contribute any handlers. install_integration_events( integration, tmp_path, manifest, - {"pre_tool_use": []}, + {}, ) assert config_path.read_bytes() == original_bytes + # An unconditional rewrite can reproduce identical bytes on Linux + # (text-mode newline translation is a no-op when the platform line + # separator is already "\n"), so byte equality alone doesn't catch + # the defect here; assert the file was never even opened for + # writing, which is what actually mangles line endings on Windows. + assert config_path.stat().st_mtime_ns == mtime_before manifest.record_existing.assert_not_called()