Stop Review Lens losing files and nodes without saying so

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
 

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(

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 ------------------------
 
 

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,