Skip to content

Preserve the original file when in-place CLI replacement fails - #232

Merged
stefankoegl merged 1 commit into
stefankoegl:masterfrom
xingzhao-ji:fix-cli-preserve-original-on-replace-error
Oct 19, 2026
Merged

stefankoegl merged 1 commit into
stefankoegl:masterfrom
xingzhao-ji:fix-cli-preserve-original-on-replace-error

Conversation

@xingzhao-ji

@xingzhao-ji xingzhao-ji commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

The in-place CLI currently catches any error from creating a backup, copying permissions, or renaming the replacement, then unlinks the original and retries. A repeated rename failure leaves the original file missing. A failed backup or chmod can also be silently ignored.

Use os.replace to overwrite the destination on POSIX and Windows without unlinking it first. If a preparation or replacement step fails, keep the original, clean up the temporary file, and propagate the error. An existing .orig backup now causes the command to fail instead of silently modifying the original without the requested backup.

Regression coverage checks a persistent replacement error, a chmod error, and a pre-existing backup. All three fail against the original source and pass with this change. The existing input-handle regression now checks the replacement call.

Validation:

  • python tests.py: 212 tests pass on Python 3.12.14 and 3.13.5.
  • Ruff syntax/undefined-name checks and git diff --check pass.
  • The separate Hypothesis property suite was not run because Hypothesis is unavailable in the test environment. Windows was not run locally.

@stefankoegl

Copy link
Copy Markdown
Owner

Looks good to me. @fhgffy you touched that part of the code recently. What do you think?

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 100.0%. remained the same — xingzhao-ji:fix-cli-preserve-original-on-replace-error into stefankoegl:master

@xingzhao-ji
xingzhao-ji marked this pull request as ready for review October 14, 2026 17:49
@stefankoegl
stefankoegl requested a balanced review from Copilot October 19, 2026 10:40

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.

🟢 Approval recommended

The focused change removes the destructive fallback and includes regression coverage for the reported failure paths.

No open issues 🎉

What changed in this PR

This PR makes in-place CLI updates safer by preserving the original file when preparation or replacement fails.

Changes:

  • Uses os.replace instead of deleting the original and retrying.
  • Cleans up temporary files and propagates failures.
  • Adds failure-path regressions and updates the input-handle check.
File Description
tests.py Tests original-file preservation, temporary-file cleanup, and input closure before replacement.
jsonpatch_cli.py Removes the destructive fallback and uses os.replace.

Review effort: Balanced

@stefankoegl
stefankoegl merged commit 592fbe8 into stefankoegl:master Oct 19, 2026
12 checks passed
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.

4 participants