From 6783aaf81d601ca9b616bc67208fc17175255b70 Mon Sep 17 00:00:00 2001 From: dovvnloading <157447210+dovvnloading@users.noreply.github.com> Date: Fri, 4 Sep 2026 06:56:51 -0400 Subject: [PATCH] Stop Review Lens losing files and nodes without saying so Two ways a Review Lens node could quietly hold less than it claimed. A pull request of more than 100 changed files was truncated to its first 100 with files_truncated left False. The flag was only ever set from inside the row loop, and the `while len(files) < MAX_PR_FILES` header above it ended the loop first whenever a page filled the list exactly to the cap. Since _build_overview_markdown gates its "File list truncated" line on that flag, a 150-file PR rendered a header reading "Files changed: 150" over a walkthrough that covered 100 of them. The loop now runs to the end of the listing and the flag is derived from two independent signals: whether the cap cut a page short, and whether the PR's own changed_files count exceeds what we kept. Telling "exactly 100" apart from "the first 100 of more" costs one extra, empty page request. Separately, _restore_code_review_payload read eight numeric fields with a bare int(). One non-numeric value - "n/a" in a saved scorecard - raised ValueError, and _restore_node catches every restorer exception and drops the node, so the PR URL, stored diff, findings, scorecard and Q&A history vanished from the canvas with nothing logged. The plan restorer had the same gap: it already guarded activity elapsedMs against exactly this, then read six budget fields with a bare int() six lines further down. Both now use _non_negative_int, the integer sibling of the _finite_float helper already in this module and written for the same reason - a saved chat row is hostile data on disk. `or 0` was never a substitute: it guards None and "" but passes "n/a" straight into int(). The plan node's caps keep falling back to their real defaults rather than zero, since a restored plan with max_steps=0 could never run again. The silent drop itself is now logged, matching the warning the neighbouring "no restorer registered" branch already emits. One bad row still must not fail the whole load, but a restorer bug should not erase a node without a trace. Test plan: - 4 new diff_fetch cases covering both sides of the 100-file boundary, a full page landing exactly on the cap, and rows dropped as unusable. - 4 new session_load cases: code_review survives a non-numeric score and non-numeric counts, plan survives non-numeric budget fields, and a raising restorer is logged while the rest of the canvas still loads. - Full suite: 3145 passed, 19 skipped. ruff clean. Co-Authored-By: Claude Opus 5 --- backend/session_load.py | 61 +++++++++++----- backend/tests/test_review_lens_domain.py | 51 +++++++++++++ backend/tests/test_session_load.py | 81 +++++++++++++++++++++ graphlink_plugins/review_lens/diff_fetch.py | 36 ++++++--- 4 files changed, 202 insertions(+), 27 deletions(-) diff --git a/backend/session_load.py b/backend/session_load.py index c10bc407..1a26706b 100644 --- a/backend/session_load.py +++ b/backend/session_load.py @@ -367,6 +367,25 @@ def _finite_float(value: Any, default: float = 0.0) -> float: return result if math.isfinite(result) else default +def _non_negative_int(value: Any, default: int = 0) -> int: + """_finite_float's integer sibling, and for the same reason: a saved + chat row is hostile-data-on-disk, so a bare `int(value)` on anything + read out of one is a crash waiting for a corrupt row. + + It matters more here than the raw exception suggests. _restore_node + wraps every restorer in `except Exception: return None, None`, so a + restorer that raises does not surface an error - the node is dropped + from the canvas entirely. One non-numeric value in a saved payload + used to take the whole node with it. + + `or 0` is not a substitute: it guards None and "" but passes "n/a" + straight into int(), which is exactly the shape that raises.""" + try: + return max(0, int(value)) + except (TypeError, ValueError): + return default + + def _position(payload: dict[str, Any]) -> tuple[float, float]: position = payload.get("position") if isinstance(position, dict): @@ -689,11 +708,8 @@ def _dict_list(value): return [dict(item) for item in value] if isinstance(value, list) else [] scores = review.get("scores") - scores = {str(k): int(v) for k, v in scores.items()} if isinstance(scores, dict) else {} - try: - pr_number = max(0, int(pr_state.get("number", 0))) - except (TypeError, ValueError): - pr_number = 0 + scores = {str(k): _non_negative_int(v) for k, v in scores.items()} if isinstance(scores, dict) else {} + pr_number = _non_negative_int(pr_state.get("number")) dismissed = review.get("dismissed_ids") dismissed_ids = ( [str(item) for item in dismissed if str(item)] if isinstance(dismissed, list) else [] @@ -710,15 +726,15 @@ def _dict_list(value): code_review_pr_html_url=str(pr_state.get("html_url", "")), code_review_base_ref=str(pr_state.get("base_ref", "")), code_review_head_ref=str(pr_state.get("head_ref", "")), - code_review_additions=max(0, int(payload.get("additions", 0) or 0)), - code_review_deletions=max(0, int(payload.get("deletions", 0) or 0)), - code_review_changed_files=max(0, int(payload.get("changed_files", 0) or 0)), + code_review_additions=_non_negative_int(payload.get("additions")), + code_review_deletions=_non_negative_int(payload.get("deletions")), + code_review_changed_files=_non_negative_int(payload.get("changed_files")), code_review_files=_dict_list(payload.get("files")), code_review_files_truncated=bool(payload.get("files_truncated", False)), code_review_diff_text=str(payload.get("diff_text", "")), code_review_diff_truncated=bool(payload.get("diff_truncated", False)), - code_review_diff_chars=max(0, int(payload.get("diff_chars", 0) or 0)), - code_review_diff_version=max(0, int(payload.get("diff_version", 0) or 0)), + code_review_diff_chars=_non_negative_int(payload.get("diff_chars")), + code_review_diff_version=_non_negative_int(payload.get("diff_version")), code_review_walkthrough=_dict_list(review.get("walkthrough")), code_review_findings=_dict_list(review.get("findings")), code_review_errors=_dict_list(review.get("errors")), @@ -727,7 +743,7 @@ def _dict_list(value): code_review_overview=str(review.get("overview", "")), code_review_confidence=str(review.get("confidence", "")), code_review_scores=scores, - code_review_quality_score=max(0, int(review.get("quality_score", 0) or 0)), + code_review_quality_score=_non_negative_int(review.get("quality_score")), code_review_verdict=str(review.get("verdict", "none") or "none"), code_review_risk=str(review.get("risk", "")), code_review_quality_summary=str(review.get("quality_summary", "")), @@ -874,12 +890,12 @@ def _restore_plan_payload(payload: dict[str, Any]) -> SceneNode: builder_status=status, builder_mode=mode if mode in ("copilot", "autopilot") else "copilot", builder_run_id=str(payload.get("builder_run_id", "")), - builder_max_steps=int(payload.get("max_steps", 12) or 12), - builder_max_tokens=int(payload.get("max_tokens", 150_000) or 150_000), - builder_max_wall_seconds=int(payload.get("max_wall_seconds", 900) or 900), - builder_spent_steps=int(payload.get("spent_steps", 0) or 0), - builder_spent_tokens=int(payload.get("spent_tokens", 0) or 0), - builder_spent_wall_seconds=int(payload.get("spent_wall_seconds", 0) or 0), + builder_max_steps=_non_negative_int(payload.get("max_steps") or 12, 12), + builder_max_tokens=_non_negative_int(payload.get("max_tokens") or 150_000, 150_000), + builder_max_wall_seconds=_non_negative_int(payload.get("max_wall_seconds") or 900, 900), + builder_spent_steps=_non_negative_int(payload.get("spent_steps")), + builder_spent_tokens=_non_negative_int(payload.get("spent_tokens")), + builder_spent_wall_seconds=_non_negative_int(payload.get("spent_wall_seconds")), builder_status_detail=( str(payload.get("status_detail", "")) if raw_status == status @@ -1121,6 +1137,17 @@ def _restore_node( try: node = restorer(payload, document) except Exception: + # Same reasoning as the "no restorer registered" warning above, for + # the other way a node can vanish: swallowing the exception keeps one + # bad row from failing the whole session load, which is right - but + # doing it silently meant a restorer bug erased the node from the + # canvas with no trace anywhere. The row on disk is untouched, so a + # fixed restorer brings the node back; this log is what makes that + # diagnosable instead of a mystery. + logger.exception( + "session load: restorer for node kind %r raised - the node stays in the saved " + "row but is not on this canvas", node_type, + ) return None, None return document.register_restored_node(node), parent_new_id diff --git a/backend/tests/test_review_lens_domain.py b/backend/tests/test_review_lens_domain.py index 3fd588ed..9c7139c3 100644 --- a/backend/tests/test_review_lens_domain.py +++ b/backend/tests/test_review_lens_domain.py @@ -158,6 +158,57 @@ def test_fetch_bundle_caps_file_pages_and_flags_truncation(monkeypatch): assert bundle["files_truncated"] is True +def _file_rows(start, count): + return [ + {"filename": f"f{i}.py", "status": "modified", "additions": 1, "deletions": 0, "patch": "@@ x"} + for i in range(start, start + count) + ] + + +def test_fetch_bundle_flags_truncation_when_a_full_page_lands_exactly_on_the_cap(monkeypatch): + """The boundary the cap check used to miss entirely. + + The old `while len(files) < MAX_PR_FILES` header ended the loop the moment + page 1 filled the list, BEFORE anything could set the truncation flag - so + a 150-file PR returned 100 files with files_truncated=False, and the review + header said "Files changed: 150" while the walkthrough covered 100 of them. + """ + cap = diff_fetch_module.MAX_PR_FILES + client = _FakeClient( + _metadata(changed_files=150), + [_file_rows(0, cap), _file_rows(cap, 50)], + ) + bundle = _run_bundle(monkeypatch, client, diff_text="x") + assert len(bundle["files"]) == cap + assert bundle["changed_files"] == 150 + assert bundle["files_truncated"] is True + + +def test_fetch_bundle_does_not_claim_truncation_for_exactly_the_cap(monkeypatch): + """The other half of the same boundary: a PR of exactly MAX_PR_FILES files + is fully covered, and must not be reported as truncated. Costs one extra + (empty) page request, which is the only way to tell it apart from the + first 100 of more.""" + cap = diff_fetch_module.MAX_PR_FILES + client = _FakeClient(_metadata(changed_files=cap), [_file_rows(0, cap), []]) + bundle = _run_bundle(monkeypatch, client, diff_text="x") + assert len(bundle["files"]) == cap + assert bundle["files_truncated"] is False + + +def test_fetch_bundle_flags_truncation_when_rows_were_dropped_as_unusable(monkeypatch): + """The declared-count half of the flag, independent of the page cap: rows + _normalize_file_entry rejects still leave the review with fewer files than + the PR changed, and the flag has to say so.""" + client = _FakeClient( + _metadata(changed_files=3), + [[{"filename": "kept.py", "status": "modified"}, {"filename": " "}, {}]], + ) + bundle = _run_bundle(monkeypatch, client, diff_text="x") + assert len(bundle["files"]) == 1 + assert bundle["files_truncated"] is True + + def test_fetch_bundle_maps_diff_download_failures_to_display_errors(monkeypatch): client = _FakeClient(_metadata(), [[]]) monkeypatch.setattr( diff --git a/backend/tests/test_session_load.py b/backend/tests/test_session_load.py index 35e59cdd..4352de13 100644 --- a/backend/tests/test_session_load.py +++ b/backend/tests/test_session_load.py @@ -73,6 +73,87 @@ def test_unrecognized_node_type_is_skipped_not_raised(): assert len(document.nodes) == 1 +# -- hostile numeric fields must not cost the whole node --------------------- +# _restore_node swallows any restorer exception and drops the node, so a bare +# int() anywhere in a restorer turns one corrupt value into a vanished node - +# PR URL, stored diff, findings, scorecard and Q&A gone with no error shown. +# _non_negative_int is _finite_float's integer sibling and exists for exactly +# this; these tests pin the shape rather than the individual call sites. + + +def _code_review(**extra): + # code_review is a _PARENT_NODE_INDEX_KINDS kind: without a resolvable + # parent the node is skipped before its restorer is ever reached, so every + # payload here is paired with _chat("parent") at index 0. + payload = {"node_type": "code_review", "position": {"x": 0.0, "y": 0.0}, + "parent_node_index": 0, "pr_url": "https://github.com/o/r/pull/3"} + payload.update(extra) + return payload + + +def test_code_review_node_survives_a_non_numeric_score(): + document = _restore(nodes=[_chat("parent"), + _code_review(review={"scores": {"correctness": "n/a"}})]) + node = next(n for n in document.nodes.values() if n.kind == "code_review") + assert node.state.code_review_pr_url == "https://github.com/o/r/pull/3" + assert node.state.code_review_scores == {"correctness": 0} + + +def test_code_review_node_survives_non_numeric_counts(): + document = _restore(nodes=[_chat("parent"), _code_review( + additions="lots", deletions=None, changed_files="many", + diff_chars="?", diff_version="v2", + pr_state={"number": "twelve"}, review={"quality_score": "high"}, + )]) + node = next(n for n in document.nodes.values() if n.kind == "code_review") + assert node.state.code_review_additions == 0 + assert node.state.code_review_changed_files == 0 + assert node.state.code_review_pr_number == 0 + assert node.state.code_review_quality_score == 0 + + +def test_plan_node_survives_non_numeric_budget_fields(): + """The same bug class in the plan restorer: it already guarded activity + elapsedMs against exactly this, then read six budget fields with a bare + int() six lines further down.""" + document = _restore(nodes=[{ + "node_type": "plan", "position": {"x": 0.0, "y": 0.0}, "goal": "ship it", + "max_steps": "twelve", "max_tokens": "lots", "max_wall_seconds": "soon", + "spent_steps": "some", "spent_tokens": "many", "spent_wall_seconds": "a while", + }]) + node = next(n for n in document.nodes.values() if n.kind == "plan") + assert node.state.plan_goal == "ship it" + # Unreadable caps fall back to their documented defaults, not to zero - + # a restored plan with max_steps=0 could never run again. + assert node.state.builder_max_steps == 12 + assert node.state.builder_max_tokens == 150_000 + assert node.state.builder_max_wall_seconds == 900 + assert node.state.builder_spent_steps == 0 + + +def test_a_restorer_that_raises_is_logged_not_swallowed_in_silence(caplog): + """The node is still dropped - one bad row must not fail the whole load - + but it no longer disappears without a trace.""" + import logging + + import backend.session_load as session_load + + def _boom(payload, document): + raise RuntimeError("restorer exploded") + + original = session_load._NODE_RESTORERS["code_review"] + session_load._NODE_RESTORERS["code_review"] = _boom + try: + with caplog.at_level(logging.ERROR, logger="backend.session_load"): + document = _restore(nodes=[_chat("parent"), _code_review()]) + finally: + session_load._NODE_RESTORERS["code_review"] = original + + assert len(document.nodes) == 1 # the parent chat still loaded + assert "code_review" in caplog.text + assert "restorer exploded" in caplog.text + + # -- parent-required kinds: parent_content_node_index ------------------------ diff --git a/graphlink_plugins/review_lens/diff_fetch.py b/graphlink_plugins/review_lens/diff_fetch.py index d394bb61..9632c3ea 100644 --- a/graphlink_plugins/review_lens/diff_fetch.py +++ b/graphlink_plugins/review_lens/diff_fetch.py @@ -122,28 +122,44 @@ def _int(value: Any) -> int: except (TypeError, ValueError): return 0 + declared_changed_files = _int(metadata.get("changed_files")) + + # The loop header is `while True`, deliberately. It used to be + # `while len(files) < MAX_PR_FILES`, which pre-empted the cap check inside + # the row loop below: a page that filled the list EXACTLY to the cap ended + # the loop with hit_cap still False, so a 150-file PR reported + # files_truncated=False while carrying only its first 100 files. Since + # _build_overview_markdown gates its "File list truncated" line on that + # flag, the review then claimed to cover a change it had only half read. + # + # Letting the loop ask for the next page instead costs one extra request + # for a PR of exactly 100 files (that page comes back empty) and is the + # only way to tell "exactly 100" from "the first 100 of more". files: list[dict[str, Any]] = [] - files_truncated = False + hit_cap = False page = 1 - while len(files) < MAX_PR_FILES: + while True: rows = client.request(metadata_url + "/files", params={"per_page": 100, "page": page}) if not isinstance(rows, list) or not rows: break for row in rows: if len(files) >= MAX_PR_FILES: - files_truncated = True + hit_cap = True # rows left unread on the page we stopped on break normalized = _normalize_file_entry(row) if normalized: files.append(normalized) - if len(rows) < 100 or files_truncated: + if hit_cap or len(rows) < 100: break page += 1 - if not files_truncated and page > 1: - # We stopped because the listing ended, not because of the cap - - # files_truncated stays False. (The flag is only set above, at the - # cap; this branch exists to say so explicitly.) - pass + + # Two independent signals, because either alone has a blind spot: hit_cap + # catches the listing being cut short, and the declared count catches + # everything we dropped for any other reason (an unusable row that + # _normalize_file_entry rejected, a listing that disagrees with the PR's + # own metadata). Both point the same way - fewer files in hand than the PR + # actually changed - and the flag exists to say exactly that. + files_truncated = hit_cap or declared_changed_files > len(files) diff_text, diff_truncated = _fetch_unified_diff(client, metadata_url) @@ -159,7 +175,7 @@ def _int(value: Any) -> int: "head_ref": str(head.get("ref") or "").strip(), "additions": _int(metadata.get("additions")), "deletions": _int(metadata.get("deletions")), - "changed_files": _int(metadata.get("changed_files")) or len(files), + "changed_files": declared_changed_files or len(files), "files": files, "files_truncated": files_truncated, "diff_text": diff_text,