fix: prevent data loss in clipboard PNG conversion when ImageMagick fails
_convert_to_png() renamed the original file to .bmp before calling ImageMagick convert, then unconditionally deleted the .bmp regardless of whether convert succeeded. If convert failed, both files were gone. - Only delete .bmp after confirmed successful conversion - Restore original file on convert failure, timeout, or missing binary - Add 3 tests covering failure, not-installed, and timeout scenarios
This commit is contained in:
parent
41877183bc
commit
ee7d8c56c7
2 changed files with 54 additions and 2 deletions
|
|
@ -285,20 +285,27 @@ def _convert_to_png(path: Path) -> bool:
|
||||||
logger.debug("Pillow BMP→PNG conversion failed: %s", e)
|
logger.debug("Pillow BMP→PNG conversion failed: %s", e)
|
||||||
|
|
||||||
# Fall back to ImageMagick convert
|
# Fall back to ImageMagick convert
|
||||||
|
tmp = path.with_suffix(".bmp")
|
||||||
try:
|
try:
|
||||||
tmp = path.with_suffix(".bmp")
|
|
||||||
path.rename(tmp)
|
path.rename(tmp)
|
||||||
r = subprocess.run(
|
r = subprocess.run(
|
||||||
["convert", str(tmp), "png:" + str(path)],
|
["convert", str(tmp), "png:" + str(path)],
|
||||||
capture_output=True, timeout=5,
|
capture_output=True, timeout=5,
|
||||||
)
|
)
|
||||||
tmp.unlink(missing_ok=True)
|
|
||||||
if r.returncode == 0 and path.exists() and path.stat().st_size > 0:
|
if r.returncode == 0 and path.exists() and path.stat().st_size > 0:
|
||||||
|
tmp.unlink(missing_ok=True)
|
||||||
return True
|
return True
|
||||||
|
else:
|
||||||
|
# Convert failed — restore the original file
|
||||||
|
tmp.rename(path)
|
||||||
except FileNotFoundError:
|
except FileNotFoundError:
|
||||||
logger.debug("ImageMagick not installed — cannot convert BMP to PNG")
|
logger.debug("ImageMagick not installed — cannot convert BMP to PNG")
|
||||||
|
if not path.exists() and tmp.exists():
|
||||||
|
tmp.rename(path)
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.debug("ImageMagick BMP→PNG conversion failed: %s", e)
|
logger.debug("ImageMagick BMP→PNG conversion failed: %s", e)
|
||||||
|
if not path.exists() and tmp.exists():
|
||||||
|
tmp.rename(path)
|
||||||
|
|
||||||
# Can't convert — BMP is still usable as-is for most APIs
|
# Can't convert — BMP is still usable as-is for most APIs
|
||||||
return path.exists() and path.stat().st_size > 0
|
return path.exists() and path.stat().st_size > 0
|
||||||
|
|
|
||||||
|
|
@ -559,6 +559,51 @@ class TestConvertToPng:
|
||||||
# (raw BMP is better than nothing)
|
# (raw BMP is better than nothing)
|
||||||
assert dest.exists() and dest.stat().st_size > 0
|
assert dest.exists() and dest.stat().st_size > 0
|
||||||
|
|
||||||
|
def test_imagemagick_failure_preserves_original(self, tmp_path):
|
||||||
|
"""When ImageMagick convert fails, the original file must not be lost."""
|
||||||
|
dest = tmp_path / "img.png"
|
||||||
|
original_data = FAKE_BMP
|
||||||
|
dest.write_bytes(original_data)
|
||||||
|
|
||||||
|
def fake_run_fail(cmd, **kw):
|
||||||
|
# Simulate convert failing without producing output
|
||||||
|
return MagicMock(returncode=1)
|
||||||
|
|
||||||
|
with patch.dict(sys.modules, {"PIL": None, "PIL.Image": None}):
|
||||||
|
with patch("hermes_cli.clipboard.subprocess.run", side_effect=fake_run_fail):
|
||||||
|
_convert_to_png(dest)
|
||||||
|
|
||||||
|
# Original file must still exist with original content
|
||||||
|
assert dest.exists(), "Original file was lost after failed conversion"
|
||||||
|
assert dest.read_bytes() == original_data
|
||||||
|
|
||||||
|
def test_imagemagick_not_installed_preserves_original(self, tmp_path):
|
||||||
|
"""When ImageMagick is not installed, the original file must not be lost."""
|
||||||
|
dest = tmp_path / "img.png"
|
||||||
|
original_data = FAKE_BMP
|
||||||
|
dest.write_bytes(original_data)
|
||||||
|
|
||||||
|
with patch.dict(sys.modules, {"PIL": None, "PIL.Image": None}):
|
||||||
|
with patch("hermes_cli.clipboard.subprocess.run", side_effect=FileNotFoundError):
|
||||||
|
_convert_to_png(dest)
|
||||||
|
|
||||||
|
assert dest.exists(), "Original file was lost when ImageMagick not installed"
|
||||||
|
assert dest.read_bytes() == original_data
|
||||||
|
|
||||||
|
def test_imagemagick_timeout_preserves_original(self, tmp_path):
|
||||||
|
"""When ImageMagick times out, the original file must not be lost."""
|
||||||
|
import subprocess
|
||||||
|
dest = tmp_path / "img.png"
|
||||||
|
original_data = FAKE_BMP
|
||||||
|
dest.write_bytes(original_data)
|
||||||
|
|
||||||
|
with patch.dict(sys.modules, {"PIL": None, "PIL.Image": None}):
|
||||||
|
with patch("hermes_cli.clipboard.subprocess.run", side_effect=subprocess.TimeoutExpired("convert", 5)):
|
||||||
|
_convert_to_png(dest)
|
||||||
|
|
||||||
|
assert dest.exists(), "Original file was lost after timeout"
|
||||||
|
assert dest.read_bytes() == original_data
|
||||||
|
|
||||||
|
|
||||||
# ── has_clipboard_image dispatch ─────────────────────────────────────────
|
# ── has_clipboard_image dispatch ─────────────────────────────────────────
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue