fix(review): refuse a default choice the saver anchor contradicts (#390)
Apply re-resolves a default item with the file's name: when its saver anchors the branch, no candidate is offered and the choice is refused. The scan's anchor check skips bytes carrying a decision and remembers unanchored file states per process, so a rescan does not inspect an unchanged file again.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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"]
|
||||
|
||||
@@ -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")
|
||||
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
+13
-3
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user