fix: skip codex config.toml rewrite when there's nothing to merge

[chelsealong]

Fixes #4563

Problem

When Codex has no Specify-managed event hooks configured (the exact
scenario in #4563: a fresh specify integration install codex with no
extensions/overrides declaring any handlers), resolve_events() returns
{}, and install_integration_events() routes through its
empty-resolved-map branch into _remove_native_event_hooks() ->
_remove_toml_entries() — not through _merge_toml_fragment().

_remove_toml_entries() always rewrote the destination file, even when
stripping Specify-owned hook blocks was a no-op (no such blocks were
present) — i.e. a true no-op install/teardown against a config.toml with
no Specify content at all.

That unconditional rewrite went through Python's text-mode read_text()/
write_text(), which perform newline translation. On Windows this
silently turned an LF-terminated pre-existing .codex/config.toml into
CRLF, producing a git-visible diff with no semantic content change.
Because the file is outside the Codex/Spec Kit managed manifests in this
scenario, specify integration status --json reports a clean/healthy
state while git diff shows the file as modified — exactly as described
in #4563.

An earlier version of this PR patched _merge_toml_fragment() instead.
That function does have the identical unconditional-rewrite shape, but it
is only reached when there is at least one supported, non-empty event to
merge — the empty-map/no-op case never calls it, so that fix had no effect
on the actual bug. This revision fixes _remove_toml_entries(), the
function that is actually executed on the issue's repro path (confirmed
by tracing every caller of install_integration_events/resolve_events
and by instrumented runs of the CLI's specify init / specify integration install codex against a real .codex/config.toml).

Fix

_remove_toml_entries() now skips the write (return False, leaving the
file completely untouched — byte-for-byte, including its original line
endings) when stripping Specify-owned blocks left the content unchanged,
mirroring the existing "unreadable file" skip path already in this
function and the analogous guard already present in
_merge_toml_fragment().

The previous, harmless-but-insufficient _merge_toml_fragment() guard is
left in place (it doesn't hurt). To correct an earlier claim in this
description: that guard does not cover "identical nonempty fragment"
merges — it only short-circuits when there is no fragment to add at all
(not fragment) and stripping left the file unchanged. A merge with a
non-empty fragment always writes, even if the file already contains that
exact fragment.

A follow-up review (2026-09-16) found that this fix's own no-op guard in
_remove_toml_entries() ran too late: for a pre-existing blank or
comment-only config.toml with no Specify-owned blocks, the
"nothing-but-whitespace/comments" branch unlinked the file before the
cleaned == existing check was ever reached, deleting user-owned content
on a true no-op path. The unchanged-content check now runs first, so the
file is left untouched whenever stripping made no change; the delete-if-
empty branch below it now only ever fires after stripping has actually
removed a Specify-owned block. Added regression tests for a comments-only
config, a blank config, and confirmed the existing owned-block-deletion
behavior on teardown is unchanged.

Test

TestTomlNoOpMerge in tests/integrations/test_events.py now has four
cases:

• test_no_events_leaves_existing_config_untouched — the original
  regression test: real production shape
  (install_integration_events(integration, tmp_path, manifest, {}))
  against a pre-existing .codex/config.toml with ordinary, non-comment,
  non-Specify content. Asserts bytes and mtime are unchanged.
• test_comments_only_config_untouched_on_teardown (new, this revision)
  — the case the 2026-09-16 review flagged: a pre-existing
  comments-only config.toml. Asserts the file is not deleted and is
  byte/mtime-identical.
• test_blank_config_untouched_on_teardown (new, this revision) — same
  for a whitespace-only pre-existing file.
• test_owned_only_config_still_deleted_on_teardown (new, this revision)
  — guards against the fix over-correcting: when the file contains only a
  Specify-owned block that teardown actually strips, the file must still
  be deleted, not preserved.

Confirmed the new tests fail without this revision's fix (git checkout HEAD~1 -- src/specify_cli/events.py, i.e. the previous, already-merged
ordering):

FAILED tests/integrations/test_events.py::TestTomlNoOpMerge::test_no_events_leaves_existing_config_untouched
AssertionError: assert 1789628920564118373 == 1789628920563545456
FAILED tests/integrations/test_events.py::TestTomlNoOpMerge::test_comments_only_config_untouched_on_teardown
AssertionError: comments-only user config was deleted
FAILED tests/integrations/test_events.py::TestTomlNoOpMerge::test_blank_config_untouched_on_teardown
AssertionError: blank user config was deleted
3 failed, 1 passed, 128 deselected

With the fix applied (git checkout HEAD -- src/specify_cli/events.py):

tests/integrations/test_events.py::TestTomlNoOpMerge .... [100%]
4 passed, 128 deselected in 0.10s

Full suite (python3 -m pytest tests -q): 8057 passed, 12 skipped in 492.05s.

ruff check src/specify_cli/events.py tests/integrations/test_events.py
shows the same pre-existing findings as before this change; none are on
the added/modified lines (verified by diffing the finding line numbers
against the changed line ranges).

AI assistance disclosure

This PR was authored by an autonomous AI coding agent, Claude Code. This
revision (the _remove_toml_entries ordering fix and its tests) was
produced by Claude Code running Claude Sonnet 5 (model ID
claude-sonnet-5). The prior revision that first patched
_remove_toml_entries() was also produced by Claude Code; its exact
underlying model version was not recorded at the time and cannot be
reconstructed after the fact — this note names what is actually known
rather than guessing.

An independent, AI-assisted review found that the original version of
this PR patched the wrong function; that revision traced the issue's
actual repro path, fixed the function that is really executed
(_remove_toml_entries), and rewrote the regression test to exercise
that real path instead of a synthetic input no production caller
produces. A second round of review (2026-09-16) found the fix's own
no-op guard still ran too late for comments-only/blank files; this
revision corrects that ordering (see "Fix" above).

[mnriem]

The revised fix reaches the correct cleanup path, but the no-op guard runs too late. A comments-only .codex/config.toml with no Specify-owned blocks is still deleted before cleaned == existing is checked.

Please check for unchanged content before the empty/comments-only deletion branch. Cover preservation of comments-only and empty unowned files, while retaining cleanup when Specify-owned blocks were actually removed.

Also correct the description’s claim about identical nonempty fragments—the merge guard only handles an empty fragment—and add the model(s) used to the existing AI disclosure.

Drafted for @mnriem with assistance from GitHub Copilot (model: GPT-6 Astra; interactive comment drafting).

---

[Copilot]

🟡 Changes recommended

Blank or comment-only user configurations can still be deleted before the new no-op guard runs.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Prevents Codex event setup from rewriting unchanged user-owned TOML configuration.

Changes:

• Adds no-op guards to TOML merge and cleanup paths.
• Adds a byte/mtime regression test for empty event installation.

File summaries

File	Description
src/specify_cli/events.py	Skips unnecessary TOML writes.
tests/integrations/test_events.py	Tests preservation of existing Codex configuration.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chelsealong]

Fixed: the unchanged-content check in _remove_toml_entries() now runs before the empty/comments-only deletion branch, so a pre-existing comments-only or blank config.toml with no Specify-owned blocks is left untouched instead of being unlinked. The delete-if-empty branch still fires when a Specify-owned block was actually stripped. Added regression tests for comments-only and blank configs (confirmed failing on the prior commit, passing now) plus one guarding that owned-block deletion on teardown still works. Also corrected the PR description's claim about the _merge_toml_fragment guard (it only covers an empty fragment, not identical nonempty fragments) and added the model name (Claude Sonnet 5) to the AI disclosure for this revision.

[Copilot]

🟢 Approval recommended

The targeted fix preserves user files while retaining existing cleanup behavior with appropriate regression coverage.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced