From 1ca37437ad56da47147bad9f5a711f3745a45c25 Mon Sep 17 00:00:00 2001 From: wang-qisen Date: Sun, 6 Sep 2026 13:36:04 +0800 Subject: [PATCH] fix: preserve resource image detail and linked daily ownership --- reme/steps/evolve/auto_image_resource.py | 22 ++- reme/steps/evolve/base_auto_resource.py | 17 ++- tests/unit/test_auto_image_steps.py | 76 ++++++++++- .../test_auto_resource_review_regressions.py | 126 +++++++++++++++++- 4 files changed, 224 insertions(+), 17 deletions(-) diff --git a/reme/steps/evolve/auto_image_resource.py b/reme/steps/evolve/auto_image_resource.py index 6180e71a..b59c5834 100644 --- a/reme/steps/evolve/auto_image_resource.py +++ b/reme/steps/evolve/auto_image_resource.py @@ -158,19 +158,26 @@ def _normalize_image_bytes( needs_orientation = orientation in range(2, 9) if not needs_resize and not needs_convert and not needs_orientation: return None, source_mime, source_mime + resize_frame = None try: + frame = image if needs_resize: - # Resize the decoded source before color conversion so large - # non-JPEG images do not require a second full-size frame. + # Pillow forces NEAREST for palette and bilevel images, even + # when LANCZOS is requested. Expand these modes within the + # checked pixel budget so resizing retains fine strokes and + # palette transparency. Other modes resize before conversion. + if image.mode in ("P", "1"): + resize_frame = image.convert("RGBA" if image.mode == "P" else "L") + frame = resize_frame # Pillow 10 cannot apply LANCZOS directly to 16-bit integer # modes; NEAREST keeps that path bounded without a full-size # RGB conversion first. resize_filter = image_module.Resampling.LANCZOS - if image.mode.startswith("I;16"): + if frame.mode.startswith("I;16"): resize_filter = image_module.Resampling.NEAREST - image.thumbnail((MAX_IMAGE_REQUEST_DIMENSION, MAX_IMAGE_REQUEST_DIMENSION), resize_filter) - has_alpha = image.mode in ("RGBA", "LA", "P") - frame = image.convert("RGBA" if has_alpha else "RGB") + frame.thumbnail((MAX_IMAGE_REQUEST_DIMENSION, MAX_IMAGE_REQUEST_DIMENSION), resize_filter) + has_alpha = frame.mode in ("RGBA", "LA", "P") + frame = frame.convert("RGBA" if has_alpha else "RGB") try: buffer = io.BytesIO() if frame.mode == "RGBA": @@ -182,6 +189,9 @@ def _normalize_image_bytes( frame.close() except Exception as exc: # pylint: disable=broad-except raise RuntimeError(f"Failed to convert/resize image ({suffix or 'unknown suffix'}): {exc}") from exc + finally: + if resize_frame is not None: + resize_frame.close() def _build_image_request_payload( diff --git a/reme/steps/evolve/base_auto_resource.py b/reme/steps/evolve/base_auto_resource.py index 1020a183..f14c80ca 100644 --- a/reme/steps/evolve/base_auto_resource.py +++ b/reme/steps/evolve/base_auto_resource.py @@ -245,11 +245,18 @@ class BaseAutoResourceStep(BaseStep): raise ValueError(f"invalid daily_dir {daily_dir!r}: {error or 'cannot resolve path'}") if not daily_root.is_dir(): return [] - return sorted( - entry.name - for entry in daily_root.iterdir() - if _DATE_RE.fullmatch(entry.name) and entry.is_dir() and not entry.is_symlink() - ) + days = [] + for entry in daily_root.iterdir(): + if not _DATE_RE.fullmatch(entry.name): + continue + try: + resolved, path_error = resolve_path(workspace, f"{daily_dir}/{entry.name}") + if not path_error and resolved is not None and resolved.is_dir(): + days.append(entry.name) + except (OSError, RuntimeError): + # Broken or cyclic links must not prevent lookup in other days. + continue + return sorted(days) async def _find_loose_resource_day(self, file_path: str) -> str | None: """Find the single daily-card owner for a root-level resource.""" diff --git a/tests/unit/test_auto_image_steps.py b/tests/unit/test_auto_image_steps.py index e7af5a96..f4eccff2 100644 --- a/tests/unit/test_auto_image_steps.py +++ b/tests/unit/test_auto_image_steps.py @@ -237,15 +237,81 @@ def test_image_preprocessing_resizes_16_bit_tiff_before_rgb_conversion(): assert max(sent.size) <= 2048 +@pytest.mark.parametrize(("mode", "transparent"), [("P", False), ("1", False), ("P", True)]) +def test_image_preprocessing_retains_thin_strokes_and_palette_alpha(mode, transparent): + """Filtered downscaling retains lines that nearest-neighbor sampling drops.""" + image = Image.new(mode, (4096, 256), 1 if mode == "1" else 0) + if mode == "P": + image.putpalette([255, 255, 255, 0, 0, 0] + [0] * 762) + if transparent: + image.info["transparency"] = 0 + image.paste(0 if mode == "1" else 1, (0, 0, 1, 256)) + buffer = io.BytesIO() + image.save(buffer, format="PNG") + image.close() + + payload = _build_image_request_payload(buffer.getvalue(), ".png") + + assert payload["source_mime"] == "image/png" + with Image.open(io.BytesIO(base64.b64decode(payload["data_b64"]))) as sent: + assert sent.size == (2048, 128) + if transparent: + assert payload["mime"] == "image/png" + assert sent.mode == "RGBA" + alpha_min, alpha_max = sent.getextrema()[3] + assert alpha_min == 0 + assert 0 < alpha_max < 255 + else: + assert sent.getextrema()[0][0] < 240 + assert sent.getpixel((sent.width - 1, 0))[:3] == (255, 255, 255) + + +@pytest.mark.parametrize("mode", ["P", "1"]) +@pytest.mark.parametrize("failing_method", ["thumbnail", "save"]) +def test_image_preprocessing_closes_expanded_frames_on_failure(mode, failing_method): + """Failure after mode expansion releases the temporary resize frame.""" + image = Image.new(mode, (2049, 2)) + buffer = io.BytesIO() + image.save(buffer, format="PNG") + image.close() + converted_frames = [] + original_convert = Image.Image.convert + + def record_convert(frame, *args, **kwargs): + converted = original_convert(frame, *args, **kwargs) + if frame.mode == mode: + converted_frames.append(converted) + return converted + + with ( + patch.object(Image.Image, "convert", new=record_convert), + patch.object(Image.Image, failing_method, side_effect=OSError(f"{failing_method} failed")), + pytest.raises(RuntimeError, match="Failed to convert/resize image"), + ): + _build_image_request_payload(buffer.getvalue(), ".png") + + assert len(converted_frames) == 1 + for frame in converted_frames: + with pytest.raises(ValueError, match="closed image"): + frame.getpixel((0, 0)) + + +@pytest.mark.parametrize( + ("source_size", "request_size"), + [((3000, 1000), (683, 2048)), ((4096, 1024), (512, 2048))], + ids=["resize-and-rotate", "decoder-downsample-and-rotate"], +) @pytest.mark.asyncio -async def test_auto_image_applies_exif_orientation_before_resizing(auto_resource_env): +async def test_auto_image_applies_exif_orientation_before_resizing(source_size, request_size, auto_resource_env): """A rotated phone JPEG is normalized upright for the VLM without touching its source.""" env = auto_resource_env - image = Image.new("RGB", (3000, 1000), (40, 80, 120)) + image = Image.new("RGB", source_size, (255, 0, 0)) + image.paste((0, 0, 255), (image.width // 2, 0, image.width, image.height)) exif = Image.Exif() exif[274] = 6 buffer = io.BytesIO() image.save(buffer, format="JPEG", exif=exif) + image.close() source = env.write_binary("resource/2026-01-01/phone.jpg", buffer.getvalue()) stored_bytes = source.read_bytes() model = _FakeVisionModel(_caption_json("upright-phone-photo", "Upright", "An upright phone photo.")) @@ -256,8 +322,12 @@ async def test_auto_image_applies_exif_orientation_before_resizing(auto_resource data_block = model.calls[0][0].content[1] assert data_block.source.media_type == "image/jpeg" with Image.open(io.BytesIO(base64.b64decode(data_block.source.data))) as sent: - assert sent.size == (683, 2048) + assert sent.size == request_size assert sent.getexif().get(274) is None + top = sent.getpixel((sent.width // 2, sent.height // 4)) + bottom = sent.getpixel((sent.width // 2, 3 * sent.height // 4)) + assert top[0] > 240 and top[2] < 15 + assert bottom[2] > 240 and bottom[0] < 15 assert source.read_bytes() == stored_bytes assert (env.workspace / "daily/2026-01-01/upright-phone-photo.md").is_file() diff --git a/tests/unit/test_auto_resource_review_regressions.py b/tests/unit/test_auto_resource_review_regressions.py index 9fa866c7..58293804 100644 --- a/tests/unit/test_auto_resource_review_regressions.py +++ b/tests/unit/test_auto_resource_review_regressions.py @@ -1,13 +1,18 @@ """Regression tests for the safety findings from the Auto Resource PR review.""" -from unittest.mock import patch +from pathlib import Path +from unittest.mock import AsyncMock, patch import frontmatter import pytest +from reme.components import R +from reme.steps.evolve.auto_resource import AutoResourceStep +from reme.steps.evolve.auto_text_resource import AutoTextResourceStep from reme.steps.evolve.base_auto_resource import BaseAutoResourceStep from .auto_resource_test_support import ( + FakeAgentWrapper, FakeVisionModel, StructuredVisionModel, caption_json, @@ -20,6 +25,16 @@ pytest_plugins = ("unit.auto_resource_test_plugin",) pytestmark = pytest.mark.asyncio +def _link_daily_directory(workspace: Path, day: str, target: Path) -> None: + """Expose a fixture directory through the supported daily layout.""" + link = workspace / "daily" / day + link.parent.mkdir(parents=True, exist_ok=True) + try: + link.symlink_to(target, target_is_directory=True) + except OSError as exc: + pytest.skip(f"symlinks unavailable: {exc}") + + @pytest.mark.parametrize("routed", [False, True], ids=["image", "unified-router"]) async def test_image_rejects_paths_outside_the_resource_tree(routed, auto_resource_env, tmp_path): """Traversal and external paths fail before image reads.""" @@ -174,9 +189,14 @@ async def test_blank_plain_caption_does_not_create_or_overwrite_note(routed, pla @pytest.mark.parametrize("routed", [False, True], ids=["image", "unified-router"]) -async def test_loose_root_image_keeps_original_daily_card_across_days(routed, auto_resource_env): +@pytest.mark.parametrize("linked_day", [False, True], ids=["directory", "internal-symlink"]) +async def test_loose_root_image_keeps_original_daily_card_across_days(routed, linked_day, auto_resource_env): """Later updates and deletion keep a loose resource's first daily-card ownership.""" env = auto_resource_env + if linked_day: + archive = env.workspace / "archive-day" + archive.mkdir() + _link_daily_directory(env.workspace, "2026-01-01", archive) source = env.write_binary("resource/photo.png", image_bytes(color=(200, 30, 30))) initial_model = FakeVisionModel(caption_json("original-card", "Original", "first-day caption")) with patch.object(BaseAutoResourceStep, "_today", return_value="2026-01-01"): @@ -261,9 +281,14 @@ async def test_loose_root_image_keeps_original_daily_card_across_days(routed, au @pytest.mark.parametrize("routed", [False, True], ids=["image", "unified-router"]) -async def test_loose_root_image_duplicate_daily_owners_fail_closed(routed, auto_resource_env): +@pytest.mark.parametrize("linked_day", [False, True], ids=["directory", "internal-symlink"]) +async def test_loose_root_image_duplicate_daily_owners_fail_closed(routed, linked_day, auto_resource_env): """Ambiguous exact ownership is reported without reading the image model or changing notes.""" env = auto_resource_env + if linked_day: + archive = env.workspace / "archive-day" + archive.mkdir() + _link_daily_directory(env.workspace, "2026-01-01", archive) source = env.write_binary("resource/duplicate.png", image_bytes()) first_note = env.write_note("daily/2026-01-01/first.md", "[[resource/duplicate.png]]", body="first owner") second_note = env.write_note("daily/2026-01-02/second.md", "[[resource/duplicate.png]]", body="second owner") @@ -288,3 +313,98 @@ async def test_loose_root_image_duplicate_daily_owners_fail_closed(routed, auto_ assert not (env.workspace / "daily/2026-01-01.md").exists() assert not (env.workspace / "daily/2026-01-02.md").exists() assert not (env.workspace / "daily/2026-01-03.md").exists() + + +@pytest.mark.parametrize("routed", [False, True], ids=["image", "unified-router"]) +async def test_loose_root_lookup_ignores_unsafe_daily_links(routed, auto_resource_env, tmp_path): + """Outside, missing, and cyclic directories cannot expose notes or block a safe owner.""" + env = auto_resource_env + outside = write_note(tmp_path / "outside-day/claim.md", "[[resource/photo.png]]", body="private outside note") + outside_before = outside.read_bytes() + _link_daily_directory(env.workspace, "2025-12-01", outside.parent) + _link_daily_directory(env.workspace, "2025-12-02", env.workspace / "missing-day") + _link_daily_directory(env.workspace, "2025-12-03", env.workspace / "daily/2025-12-03") + source = env.write_binary("resource/photo.png", image_bytes()) + owned_note = env.write_note("daily/2026-01-01/original.md", "[[resource/photo.png]]") + model = FakeVisionModel(caption_json("original", "Updated", "Updated inside workspace.")) + original_read_text = Path.read_text + outside_reads = [] + + def guarded_read_text(path, *args, **kwargs): + if path.resolve() == outside.resolve(): + outside_reads.append(path) + raise AssertionError("cross-date lookup read a note outside the workspace") + return original_read_text(path, *args, **kwargs) + + with patch.object(Path, "read_text", guarded_read_text): + with patch.object(BaseAutoResourceStep, "_today", return_value="2026-01-02"): + updated = await env.run(env.processor(model, routed=routed), [{"change": "modified", "path": str(source)}]) + assert updated.success is True + assert updated.metadata["results"][0]["metadata"]["path"] == "daily/2026-01-01/original.md" + assert "Updated inside workspace." in owned_note.read_text(encoding="utf-8") + source.unlink() + with patch.object(BaseAutoResourceStep, "_today", return_value="2026-01-03"): + deleted = await env.run(env.processor(model, routed=routed), [{"change": "deleted", "path": str(source)}]) + + assert deleted.success is True + assert deleted.metadata["results"][0]["metadata"]["action"] == "deleted" + assert not owned_note.exists() + assert not outside_reads + assert outside.read_bytes() == outside_before + assert len(model.calls) == 1 + assert not (env.workspace / "daily/2026-01-02").exists() + assert not (env.workspace / "daily/2026-01-03").exists() + + +@pytest.mark.parametrize("routed", [False, True], ids=["text", "unified-router"]) +async def test_loose_root_text_updates_and_deletes_original_daily_card(routed, auto_resource_env): + """Text processing also uses exact cross-date ownership for its prompt and lifecycle.""" + env = auto_resource_env + source = env.write_binary("resource/report.txt", b"Updated report text.") + note_rel = "daily/2026-01-01/original.md" + owned_note = env.write_note(note_rel, "[[resource/report.txt]]", body="Original report text.") + unrelated = env.write_note("daily/2026-01-02/report.md", "[[resource/other.txt]]", body="Keep unrelated report.") + unrelated_before = unrelated.read_bytes() + wrapper = FakeAgentWrapper() + + async def update_note(inputs, **kwargs): + assert "Date: 2026-01-01" in inputs + assert f"Target note path: {note_rel}" in inputs + assert "Updated report text." in inputs + assert "read" in kwargs["job_tools"] + response = await env.app_context.jobs["write"]( + path=note_rel, + name="suggested-rename", + description="Updated report", + content="Updated report text.", + metadata={"source_resource": "[[resource/report.txt]]"}, + ) + assert response.success is True + return {"result": "Updated original report."} + + env.app_context.registry = R + step_cls = AutoResourceStep if routed else AutoTextResourceStep + step = step_cls(app_context=env.app_context, file_store=env.file_store, agent_wrapper=wrapper, language="en") + with patch.object(wrapper, "reply", new=AsyncMock(side_effect=update_note)) as reply: + with patch.object(BaseAutoResourceStep, "_today", return_value="2026-01-02"): + updated = await env.run(step, [{"change": "modified", "path": str(source)}]) + assert updated.success is True + result = updated.metadata["results"][0]["metadata"] + assert result["path"] == note_rel + assert result["created"] is False + assert result["action"] == "modified" + assert result["index"]["date"] == "2026-01-01" + assert "Updated report text." in owned_note.read_text(encoding="utf-8") + source.unlink() + with patch.object(BaseAutoResourceStep, "_today", return_value="2026-01-03"): + deleted = await env.run(step, [{"change": "deleted", "path": str(source)}]) + reply.assert_awaited_once() + + assert deleted.success is True + assert deleted.metadata["results"][0]["metadata"]["path"] == note_rel + assert deleted.metadata["results"][0]["metadata"]["action"] == "deleted" + assert deleted.metadata["results"][0]["metadata"]["index"]["date"] == "2026-01-01" + assert not owned_note.exists() + assert unrelated.read_bytes() == unrelated_before + assert list((env.workspace / "daily/2026-01-02").glob("*.md")) == [unrelated] + assert not (env.workspace / "daily/2026-01-03").exists()