Stop Review Lens losing files and nodes without saying so

[dovvnloading]

Problem

Two ways a Review Lens node could quietly hold less than it claimed. Both were found by executing the code, and both are reproduced by the tests added here.

A pull request of more than 100 changed files was silently truncated. files_truncated 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:

PR has  99 files -> kept= 99  changed_files= 99  files_truncated=False
PR has 100 files -> kept=100  changed_files=100  files_truncated=False
PR has 150 files -> kept=100  changed_files=150  files_truncated=False   <-- wrong
PR has 250 files -> kept=100  changed_files=250  files_truncated=False   <-- wrong

_build_overview_markdown gates its "File list truncated" line on that flag, so a 150-file PR rendered a header reading "Files changed: 150" above a walkthrough covering 100 of them, with nothing saying so. The walkthrough grouping and every fallback heuristic saw the partial set too. diff_fetch.py's own module docstring promises the opposite ("rather than silently reviewing half a diff"), and CODE_REVIEW_DIFF_TIMEOUT_SECONDS' justification in backend/agents.py describes "up to two pages of file-listing GETs" — a second page the loop could never reach.

One corrupt number deleted an entire node on load. _restore_code_review_payload read eight numeric fields with a bare int():

_restore_code_review_payload({"review": {"scores": {"correctness": "n/a"}}})
  -> ValueError: invalid literal for int() with base 10: 'n/a'

_restore_node catches every restorer exception and returns None, so that ValueError did not surface as an error — the node was dropped from the canvas, taking the PR URL, the stored diff, every finding, the scorecard and the Q&A history with it, and logging nothing.

The plan restorer has the same gap. It already guards activity elapsedMs against exactly this failure, with a comment saying so, and then reads six builder budget fields with a bare int() six lines further down.

Change

files_truncated is now derived from two independent signals rather than set mid-loop: whether the cap cut a page short, and whether the PR's own changed_files count exceeds what was kept. Either alone has a blind spot — the first misses a page that ends exactly on the cap, the second misses a listing that disagrees with its own metadata. Distinguishing "exactly 100 files" from "the first 100 of more" costs one extra page request, which comes back empty; there is no way to tell them apart without asking.

Both restorers now use _non_negative_int, added here as the integer sibling of the _finite_float helper already in this module and written for the same stated reason — a saved chat row is hostile data on disk. or 0 was never a substitute: it guards None and "", then passes "n/a" straight into int(). The plan node's caps still fall back to their real defaults rather than to zero, since a restored plan with max_steps=0 could never run again.

The silent drop 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 be able to erase a node with no trace anywhere.

Test plan

• 4 new diff_fetch cases: both sides of the 100-file boundary, a full page landing exactly on the cap, and rows dropped as unusable. The pre-existing truncation test (120 rows on one page) still passes — it covered the mid-page cut, which always worked.
• 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 (up from 3138). ruff check . clean.

🤖 Generated with Claude Code