fix: skip codex config.toml rewrite when there's nothing to merge - #4564
Open
chelsealong wants to merge 2 commits into
Open
fix: skip codex config.toml rewrite when there's nothing to merge#4564chelsealong wants to merge 2 commits into
chelsealong wants to merge 2 commits into
Conversation
_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.
…p 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4563
Problem
When Codex has no Specify-managed event hooks configured (the exact
scenario in #4563: a fresh
specify integration install codexwith noextensions/overrides declaring any handlers),
resolve_events()returns{}, andinstall_integration_events()routes through itsempty-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 whenstripping 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 thissilently turned an LF-terminated pre-existing
.codex/config.tomlintoCRLF, 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 --jsonreports a clean/healthystate while
git diffshows the file as modified — exactly as describedin #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(), thefunction that is actually executed on the issue's repro path (confirmed
by tracing every caller of
install_integration_events/resolve_eventsand by instrumented runs of the CLI's
specify init/specify integration install codexagainst a real.codex/config.toml).Fix
_remove_toml_entries()now skips the write (return False, leaving thefile 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 isleft in place (it doesn't hurt, and does fix a separate reachable no-op
case where a fragment is being merged but the file already contains it
identically).
Test
Replaced
TestTomlNoOpMergewithtest_no_events_leaves_existing_config_untouched, which drives the realproduction shape:
install_integration_events(integration, tmp_path, manifest, {})— the empty resolved-events mapresolve_events()actuallyreturns when no hooks are configured — against a pre-existing
.codex/config.tomlwith no Specify-owned content. It asserts both thatthe file's bytes are unchanged and that the file's mtime is unchanged
(byte-equality alone doesn't catch an unconditional rewrite on Linux,
where the platform line separator is already
\n; the mtime checkverifies no write occurs at all, which is what actually mangles line
endings on Windows).
Confirmed the test fails without this revision's fix (
git checkout HEAD -- src/specify_cli/events.pyto restore the pre-this-fix version, whichstill has the previous PR's
_merge_toml_fragment-only patch):With the fix applied:
Full suite (
python3 -m pytest tests -q):8054 passed, 12 skipped.ruff checkon the changed files shows the same pre-existing findings asbefore this change; none are on the added/modified lines.
AI assistance disclosure
This PR was authored by an autonomous AI coding agent (Claude Code). An
independent review (also AI-assisted) found that the original version of
this PR patched the wrong function; this revision traces the issue's
actual repro path, fixes the function that is really executed
(
_remove_toml_entries), and rewrites the regression test to exercisethat real path instead of a synthetic input no production caller
produces.