Skip to content

fix: make _merge_toml_fragment and _remove_toml_entries use atomic writes - #3972

Open
Quratulain-bilal wants to merge 1 commit into
github:mainfrom
Quratulain-bilal:fix/toml-merge-atomic-write
Open

fix: make _merge_toml_fragment and _remove_toml_entries use atomic writes#3972
Quratulain-bilal wants to merge 1 commit into
github:mainfrom
Quratulain-bilal:fix/toml-merge-atomic-write

Conversation

@Quratulain-bilal

Copy link
Copy Markdown
Contributor

Problem

Both _merge_toml_fragment and _remove_toml_entries used write_text() which truncates the file before writing. A crash or power loss mid-write leaves a partial TOML file.

Fix

Now uses empfile.mkstemp + os.replace for atomic writes, matching the pattern used elsewhere in the codebase.

…ites

Both functions used write_text() which truncates before writing. A crash
or power loss mid-write leaves a partial TOML file. Now uses
tempfile.mkstemp + os.replace for atomic writes.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates TOML event configuration writes to use temporary files and atomic replacement.

Changes:

  • Adds tempfile support.
  • Converts TOML merge/removal rewrites to mkstemp and os.replace.
  • Cleans up staged files on failure.
Show a summary per file
File Description
src/specify_cli/events.py Adds atomic TOML rewrite paths.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Suppressed comments (2)

src/specify_cli/events.py:1775

  • This teardown path also replaces an existing TOML file with mkstemp's 0600 mode. Removing Spec Kit entries from a group-readable or group-writable config therefore changes access for unrelated user content. Preserve the destination's existing mode before os.replace, as the workflow registry writer does in src/specify_cli/workflows/catalog.py:162-188.
    fd, tmp = tempfile.mkstemp(
        dir=str(dst.parent), prefix=f".{dst.name}.", suffix=".tmp"
    )

src/specify_cli/events.py:1779

  • The removal rewrite has the same durability gap: os.replace is namespace-atomic, but the staged bytes and rename are not made durable before returning. Because this PR explicitly claims protection from power loss, sync the staged file before replacement and the parent directory afterward via the same platform-aware helper.
        with os.fdopen(fd, "w", encoding="utf-8") as f:
            f.write(cleaned)
        os.replace(tmp, dst)
  • Files reviewed: 1/1 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread src/specify_cli/events.py
Comment on lines +1734 to +1735
try:
with os.fdopen(fd, "w", encoding="utf-8") as f:
Comment thread src/specify_cli/events.py
Comment on lines +1735 to +1737
with os.fdopen(fd, "w", encoding="utf-8") as f:
f.write(existing.rstrip() + "\n\n" + fragment + "\n")
os.replace(tmp, dst)
Comment thread src/specify_cli/events.py
Comment on lines +1731 to +1737
fd, tmp = tempfile.mkstemp(
dir=str(dst.parent), prefix=f".{dst.name}.", suffix=".tmp"
)
try:
with os.fdopen(fd, "w", encoding="utf-8") as f:
f.write(existing.rstrip() + "\n\n" + fragment + "\n")
os.replace(tmp, dst)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants