diff --git a/py/api/review_recording.py b/py/api/review_recording.py index 4e30ceea..c78ef700 100644 --- a/py/api/review_recording.py +++ b/py/api/review_recording.py @@ -216,11 +216,31 @@ class ReviewRecordingMixin: Its current bytes' pending, undecided items then leave the queue (counted in ``review["untraced"]``: nothing to choose). A failure is - logged and counted, and the file is queued as before. + logged and counted, and the file is queued as before. A file state + (size, ``mtime_ns``, item version) found unanchored in this process + is not inspected again, and bytes carrying a decision are not + inspected at all (a decision is never settled). """ + located = self._review_location(media_file, roots) + if canonical is None or located is None: + return False + locator = os.path.normcase(located[3]) try: - if not review_capture.saver_anchored(canonical, media_file.name): - return False + facts = os.lstat(located[3]) + except OSError: + return False # gone: the capture decides as before + if self.db.review_decided_at(locator, facts.st_size, facts.st_mtime_ns): + return False + version = review_capture.inspection_review_version(canonical) + if version is None: + return False + state = _digest(f"anchor:{facts.st_size}:{facts.st_mtime_ns}:{version}") + memory = self._unanchored_memory() + key = _memory_key(locator) + if memory.get(key) == state: + return False + try: + anchored = review_capture.saver_anchored(canonical, media_file.name) except Exception: self.logger.warning( f"{media_file.name} could not be inspected for Needs review", @@ -229,14 +249,16 @@ class ReviewRecordingMixin: if review is not None: review["errors"] += 1 return False - located = self._review_location(media_file, roots) - if located is None: - return True + if not anchored: + memory.pop(key, None) # re-inserted last: the newest state + memory[key] = state + while len(memory) > MAX_UNTRACED_MEMORY: + del memory[next(iter(memory))] + return False try: - facts = os.lstat(located[3]) # No version is current for these bytes: any pending item goes. removed = self.db.settle_stale_review_items( - os.path.normcase(located[3]), facts.st_size, facts.st_mtime_ns, () + locator, facts.st_size, facts.st_mtime_ns, () ) except Exception: self.logger.warning( @@ -250,6 +272,12 @@ class ReviewRecordingMixin: review["untraced"] += removed return True + def _unanchored_memory(self): + memory = getattr(self, "_unanchored_inspections", None) + if memory is None: + memory = self._unanchored_inspections = {} + return memory + def _settle_stale_reviews(self, media_file, roots, canonical, review=None): """Remove the pending, undecided items of an untraced file's current bytes recorded under another version (best effort, counted in diff --git a/tests/test_api_review.py b/tests/test_api_review.py index 6eeaa89d..0e19c6ed 100644 --- a/tests/test_api_review.py +++ b/tests/test_api_review.py @@ -1040,8 +1040,8 @@ class TestApplyRefusals(ReviewRoutesTestCase): _path, item, detail, observed, cat = await self._opened() real = review_capture_module.resolve_prompt_pair - def reworded(metadata): - resolution = real(metadata) + def reworded(metadata, **kwargs): + resolution = real(metadata, **kwargs) return { **resolution, "reason": "a reworded reason from a newer resolver", @@ -1065,8 +1065,8 @@ class TestApplyRefusals(ReviewRoutesTestCase): _path, item, detail, observed, cat = await self._opened() real = review_capture_module.resolve_prompt_pair - def without_cat(metadata): - resolution = real(metadata) + def without_cat(metadata, **kwargs): + resolution = real(metadata, **kwargs) kept = [ c for c in resolution["candidates"] diff --git a/tests/test_review_inspection.py b/tests/test_review_inspection.py index 3a2485e1..ea0774a1 100644 --- a/tests/test_review_inspection.py +++ b/tests/test_review_inspection.py @@ -323,17 +323,24 @@ class TestDefaultProfileStillDecides(InspectionTestCase): self.assertEqual(item["resolver_version"], review_capture.RESOLVER_VERSION) self.assertEqual(_texts(evidence), ["cat", "dog"]) - async def test_apply_of_a_default_item_never_runs_the_inspection_profile(self): + async def test_apply_of_a_default_item_inspects_only_for_the_anchor( + self, + ): self._roots() write_png(self.output_dir / "two.png", prompt=unlinkable_graph("cat", "dog")) await self._scan() - with patch.object( - review_capture, - "resolve_inspection", - side_effect=AssertionError("inspection profile ran"), + with ( + _counted(review_capture, "resolve_inspection") as calls, + patch.object( + review_capture, + "inspection_choice", + side_effect=AssertionError("inspection choices were asked"), + ), ): _item, (status, data) = await self._apply("cat") self.assertEqual(status, 200, data) + # The saver anchor check (#390): with the file's name, nothing matches. + self.assertEqual([c[1] for c in calls], ["two.png"]) self.assertEqual(self._linked_text(self.output_dir / "two.png"), "cat") diff --git a/tests/test_review_saver_anchor.py b/tests/test_review_saver_anchor.py index 3c02f263..696a7451 100644 --- a/tests/test_review_saver_anchor.py +++ b/tests/test_review_saver_anchor.py @@ -17,12 +17,18 @@ try: from tests.test_review import unlinkable_graph, write_png from tests.test_review_inspection import ( PROMPTS, + _counted, InspectionTestCase, qwen_edit_graph, ) except ImportError: # discovered with tests/ as the top-level directory from test_review import unlinkable_graph, write_png - from test_review_inspection import PROMPTS, InspectionTestCase, qwen_edit_graph + from test_review_inspection import ( + PROMPTS, + InspectionTestCase, + _counted, + qwen_edit_graph, + ) from utils import review_capture @@ -186,12 +192,49 @@ class TestDefaultItemsAnchor(InspectionTestCase): with patch.object(review_capture, "saver_anchored", return_value=False): await self._scan() self.assertEqual(len(self._items()), 1) # queued under the old rule + self.api._unanchored_inspections = {} # a restart onto the new rule job = await self._scan("all") self.assertEqual(self._items(), []) self.assertEqual(job["scope"]["review_untraced"], 1) + async def test_apply_refuses_a_default_choice_the_anchor_contradicts(self): + self._require_routes() + self._roots() + path = self.output_dir / "side_0_00001_.png" + write_png(path, prompt=saved_unlinkable_graph("cat", "dog")) + with patch.object(review_capture, "saver_anchored", return_value=False): + await self._scan() + item, detail, observed = await self._open() + # side_0 is the saver of "cat"; choose the other branch. + wrong = self._candidate(detail, "dog") + + status, data = await self._decide( + item["id"], self._body(detail, observed, wrong["hash"]) + ) + + self.assertEqual(status, 409, data) + self.assertEqual(data["code"], "review-conflict", data) + self.assertIsNone(self._linked_text(path)) + + async def test_unanchored_file_is_not_inspected_again_until_it_changes(self): + self._roots() + path = self.output_dir / "unsaved_00001_.png" + write_png(path, prompt=saved_unlinkable_graph("cat", "dog")) + with _counted(review_capture, "saver_anchored") as first: + await self._scan() + self.assertEqual(len(first), 1) + + with _counted(review_capture, "saver_anchored") as again: + await self._scan("all") + self.assertEqual(again, []) + + write_png(path, prompt=saved_unlinkable_graph("cat", "dog"), color=(0, 9, 0)) + with _counted(review_capture, "saver_anchored") as changed: + await self._scan("all") + self.assertEqual(len(changed), 1) + async def test_decided_default_item_of_an_anchored_file_is_kept(self): self._require_routes() self._roots() diff --git a/utils/review_capture.py b/utils/review_capture.py index b635e530..15d33d3d 100644 --- a/utils/review_capture.py +++ b/utils/review_capture.py @@ -411,8 +411,10 @@ def captured_review( JSON-decoded and ``file_info`` (size, dimensions, format) from the capture. ``resolution`` comes from ``profile`` (``DEFAULT`` or ``INSPECTION``; the latter anchored on ``file_name``, the file's own - name, as the item was recorded). None when the capture is gone or - carries no prompt metadata. + name, as the item was recorded). With ``file_name`` (Apply), a default + result its saver anchor resolves (``saver_anchored``) is that anchored + result, so no candidate is offered any more. None when the capture is + gone or carries no prompt metadata. """ if store.acquire(capture.token) is None: return None @@ -441,7 +443,15 @@ def captured_review( canonical["file_info"] = info if profile == INSPECTION: return canonical, resolve_inspection(canonical, file_name) - return canonical, resolve_prompt_pair(canonical) + resolution = resolve_prompt_pair(canonical) + if ( + file_name is not None + and resolution.get("state") == "ambiguous" + and saver_anchored(canonical, file_name) + ): + # Its saver names its branch (#390): no choice is left to apply. + return canonical, resolve_inspection(canonical, file_name) + return canonical, resolution def reviewable(resolution: Any) -> bool: