From cdfa23a626268a099fb7a2450fe5ecdbfcf46e69 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Tue, 27 Oct 2026 19:37:24 -0700 Subject: [PATCH] fix(review): upgrade old default items of anchored files, tag anchored results (#390) A pending default item recorded before the saver anchor no longer shortcuts the rescan when only the anchor resolves the file: the anchored item supersedes it. Apply accepts a resolved re-resolution only when it is tagged as the saver anchor's, the anchored pair must carry a positive text to choose, and limited anchored evidence still counts as a choice. TESTING.md describes the ruling. --- TESTING.md | 25 +++++++----- database/review_schema.py | 6 +-- py/api/review.py | 4 +- tests/test_review_saver_anchor.py | 43 ++++++++++++++++++++ utils/review_capture.py | 67 +++++++++++++++++++++++-------- 5 files changed, 112 insertions(+), 33 deletions(-) diff --git a/TESTING.md b/TESTING.md index 73c4177a..9cc651b0 100644 --- a/TESTING.md +++ b/TESTING.md @@ -4595,17 +4595,20 @@ pending, undecided item of bytes now untraced leaves the queue (`settle_stale_review_items`), so an item never stays stuck at 409. Saver anchor (#390, `tests/test_review_saver_anchor.py`): the scan and Apply resolve with the file's name as the workspace inspector does, so the one saver -whose `filename_prefix` names the file anchors its branch. A file whose branch -that anchor resolves is never queued: from the inspection profile it offers no -choice; a default-ambiguous file with a choice asks `saver_anchored` (the named -run resolves, the unnamed one does not) and is reported `unresolved`, never -linked. Its pending, undecided items leave on the next scan -(`REVIEW_RULE_VERSION` is folded into inspection item versions; a default item -is settled directly). No match, several matches or an unfinished walk anchor -nothing, and the file stays queued as before. Apply refuses a choice the anchor -contradicts. Measured on the maintainer's 40,890 outputs: the queue is -unchanged (249 files); the 242 `gpu_unlock_bench` files the inspector resolves -are linked by the parser floor, so they were never queued. The 16-candidate limited +whose `filename_prefix` names the file anchors its branch. A file is never both +unlinked and unqueued (maintainer ruling): when only that anchor resolves it +(the named inspection run resolves, the unnamed one does not) and neither the +default profile nor the parser floor links it, the scan queues it with the +anchor's pair as its one candidate (evidence `anchor: "saver"`; the page +preselects it), never linking it. Such an item counts as a choice for the +untraced settle. Files already linked or resolved by the default path are +unchanged. No match, several matches or an unfinished walk anchor nothing, and +the file is queued as before. `REVIEW_RULE_VERSION` is folded into inspection +item versions, so older inspection items are recorded again once. Apply accepts +the anchored pair and refuses a choice the anchor contradicts. Measured on the +maintainer's 40,890 outputs: the queue is unchanged (249 files); the 242 +`gpu_unlock_bench` files the inspector resolves are linked by the parser +floor. The 16-candidate limited rule and the 8 MiB bound apply unchanged. The queue heals itself: after each batch, the scan removes the pending items of every listed file (read or skipped as known) that a saved row now links, and the monitor does the same for each ambiguous output it links diff --git a/database/review_schema.py b/database/review_schema.py index 94ed1064..c0c45d76 100644 --- a/database/review_schema.py +++ b/database/review_schema.py @@ -229,14 +229,14 @@ def evidence_offers_choice(evidence: Any) -> bool: """ if not isinstance(evidence, dict): return False + # The saver anchor's one pair (#390) is a suggestion to confirm. + needed = 1 if evidence.get("anchor") == "saver" else MIN_CHOOSABLE if evidence.get("limited"): total = evidence.get("candidate_total") - return type(total) is int and total >= MIN_CHOOSABLE + return type(total) is int and total >= needed candidates = evidence.get("candidates") if not isinstance(candidates, list): return False - # The saver anchor's one pair (#390) is a suggestion to confirm. - needed = 1 if evidence.get("anchor") == "saver" else MIN_CHOOSABLE return sum(1 for c in candidates if choosable(c)) >= needed diff --git a/py/api/review.py b/py/api/review.py index 0e49f6dc..a07c02ce 100644 --- a/py/api/review.py +++ b/py/api/review.py @@ -730,8 +730,8 @@ def _candidate_still_offered(resolution: dict, choice: str) -> bool: that rewords the top-level reason or reorders candidates leaves a pending item applicable. """ - if isinstance(resolution, dict) and resolution.get("state") == "resolved": - # The bytes now name one pair (the saver anchor, #390): only it. + if isinstance(resolution, dict) and resolution.get("anchor") == "saver": + # Only the saver anchor resolves the bytes (#390): its pair alone. resolution = {"state": "ambiguous", "candidates": [resolution]} if not isinstance(resolution, dict) or resolution.get("state") != "ambiguous": return False diff --git a/tests/test_review_saver_anchor.py b/tests/test_review_saver_anchor.py index f7e146fa..c61d8112 100644 --- a/tests/test_review_saver_anchor.py +++ b/tests/test_review_saver_anchor.py @@ -19,6 +19,7 @@ try: from tests.test_review import unlinkable_graph, write_png from tests.test_review_inspection import ( PROMPTS, + _counted, InspectionTestCase, qwen_edit_graph, ) @@ -26,6 +27,7 @@ except ImportError: # discovered with tests/ as the top-level directory from test_review import unlinkable_graph, write_png from test_review_inspection import ( PROMPTS, + _counted, InspectionTestCase, qwen_edit_graph, ) @@ -265,6 +267,47 @@ class TestDefaultItemsAnchor(InspectionTestCase): self.assertEqual(data["code"], "review-conflict", data) self.assertIsNone(self._linked_text(path)) + async def test_old_default_item_is_replaced_by_the_anchor_choice(self): + self._roots() + write_png( + self.output_dir / "side_0_00001_.png", + prompt=saved_unlinkable_graph("cat", "dog"), + ) + with patch.object(review_capture, "_captured_inspection", return_value=None): + await self._scan() # queued from the default profile, as before #390 + (old,) = self._items() + self.assertEqual(old["resolver_version"], review_capture.RESOLVER_VERSION) + + await self._scan("all") + + (item,) = self._items() + self.assertNotEqual(item["id"], old["id"]) + self.assertEqual(_only_candidate(self, item), "cat") + + with _counted(review_capture, "resolve_inspection") as calls: + await self._scan("all") # now queued under its version: no re-read + self.assertEqual(calls, []) + self.assertEqual([i["id"] for i in self._items()], [item["id"]]) + + async def test_decided_anchored_item_is_kept(self): + self._require_routes() + self._roots() + write_png( + self.output_dir / "side_0_00001_.png", + prompt=saved_unlinkable_graph("cat", "dog"), + ) + await self._scan() + item, detail, observed = await self._open() + status, data = await self._decide( + item["id"], self._body(detail, observed, "unresolved") + ) + self.assertEqual(status, 200, data) + before = (self._items(), self._decisions()) + + await self._scan("all") + + self.assertEqual((self._items(), self._decisions()), before) + async def test_apply_accepts_an_old_default_choice_the_anchor_names(self): self._require_routes() self._roots() diff --git a/utils/review_capture.py b/utils/review_capture.py index 44bfd987..8fadab72 100644 --- a/utils/review_capture.py +++ b/utils/review_capture.py @@ -213,17 +213,28 @@ def inspection_choice(metadata: dict, file_name: str | None = None) -> dict | No return None resolution = resolve_inspection(metadata, file_name) if resolution.get("state") == "resolved": - return resolution if _anchor_alone(metadata, file_name) else None + return _anchor_of(metadata, file_name, resolution) if resolution.get("state") != "ambiguous" or not reviewable(resolution): return None return resolution -def _anchor_alone(metadata: Any, file_name: str | None) -> bool: - """True when ``metadata`` does not resolve without ``file_name``.""" - if not file_name: - return False - return resolve_inspection(metadata).get("state") != "resolved" +def _anchor_of(metadata: Any, file_name: str | None, named: dict) -> dict | None: + """``named`` tagged ``anchor: "saver"`` when only the saver anchor + resolves the file, to a pair with a positive text to choose; else None. + + ``named`` is ``metadata`` resolved with ``file_name``; the profile runs + once more without the name only when it is resolved and choosable. + """ + if not file_name or named.get("state") != "resolved": + return None + try: + offered = choosable(_projection(named)) + except (KeyError, TypeError): + return None + if not offered or resolve_inspection(metadata).get("state") == "resolved": + return None + return {**named, "anchor": SAVER_ANCHOR} def saver_anchored(metadata: Any, file_name: str | None) -> bool: @@ -245,10 +256,7 @@ def _anchored(metadata: Any, file_name: str | None) -> dict | None: return None if inspection_review_version(metadata) is None: return None - resolution = resolve_inspection(metadata, file_name) - if resolution.get("state") != "resolved": - return None - return resolution if _anchor_alone(metadata, file_name) else None + return _anchor_of(metadata, file_name, resolve_inspection(metadata, file_name)) def item_profile(evidence: Any) -> str: @@ -311,7 +319,9 @@ def record_ambiguous_file( """ versions = item_versions(scanned[0] if scanned is not None else None) spelled_path, queued = _queued_spelling(db, root, relative, versions) - if queued is not None: + if queued is not None and not ( + inspection and _default_item_to_anchor(db, queued, scanned, relative) + ): return _already_queued(db, *queued, versions) epoch = db.get_execution_epoch() try: @@ -338,7 +348,7 @@ def record_ambiguous_file( return "skipped" profile, version = DEFAULT, RESOLVER_VERSION named = _captured_inspection(captured[0], relative, scanned) if inspection else None - anchored = named is not None and named[0].get("state") == "resolved" + anchored = named is not None and named[0].get("anchor") == SAVER_ANCHOR if anchored: # Only the saver anchor resolves it (#390): offer that one pair. profile, version = INSPECTION, named[1] @@ -378,13 +388,31 @@ def record_ambiguous_file( return "limited" if limited else "recorded" +def _default_item_to_anchor(db, queued: tuple, scanned, relative: str) -> bool: + """True when the bytes' only pending item is a default item recorded + before #390 and the saver anchor alone resolves the scanned records. + + The file is then captured again so the anchored item supersedes that + pending, undecided item. Decided bytes and bytes already queued under + the current inspection version are never re-read. + """ + if scanned is None or scanned[0] is None: + return False + version = inspection_review_version(scanned[0]) + if version is None or db.review_pending_at(*queued, version): + return False + if not db.review_pending_at(*queued, RESOLVER_VERSION): + return False + return _anchored(scanned[0], _file_name(relative)) is not None + + def _captured_inspection(metadata: dict, relative: str, scanned) -> tuple | None: """``(resolution, item_version)`` of the captured records under the inspection profile, anchored on the file's name; None when the table is unusable. - A ``resolved`` result means the saver anchor alone resolves the file - (#390); a result that also resolves without the name is returned as + A result tagged ``anchor: "saver"`` means the saver anchor alone + resolves the file (#390); any other resolved result is returned as ``unresolved`` (no review case, as before). The scan's result is reused when the capture holds the same records (``inspection_choice`` applies the same rule). @@ -400,8 +428,12 @@ def _captured_inspection(metadata: dict, relative: str, scanned) -> tuple | None return scanned[1], version # the same records: no second run name = _file_name(relative) resolution = resolve_inspection(metadata, name) - if resolution.get("state") == "resolved" and not _anchor_alone(metadata, name): - resolution = {**resolution, "state": "unresolved"} + if resolution.get("state") == "resolved": + # Resolved without the anchor too: no review case, as before. + resolution = _anchor_of(metadata, name, resolution) or { + **resolution, + "state": "unresolved", + } return resolution, version @@ -491,7 +523,8 @@ def captured_review( info["format"] = read.format canonical["file_info"] = info if profile == INSPECTION: - return canonical, resolve_inspection(canonical, file_name) + named = resolve_inspection(canonical, file_name) + return canonical, _anchor_of(canonical, file_name, named) or named resolution = resolve_prompt_pair(canonical) if file_name is not None and resolution.get("state") == "ambiguous": anchored = _anchored(canonical, file_name)