Add Review Lens: guided pull-request review node - #392
Conversation
New first-party code_review node + Review Lens picker entry (Validation & Delivery): fetch a GitHub PR diff, guided walkthrough, severity-tiered findings with deterministic scorecard, and follow-up Q&A over the stored diff. Ports the retired Code Review plugin's rubric to multi-file diffs; read-only by default, no auto-post. Also bumps locked pypdf 6.15.0 -> 6.16.1 (pip-audit CVEs).
e4cb5bc to
4c84c05
Compare
dovvnloading
left a comment
There was a problem hiding this comment.
Code review - Review Lens (agent/review-lens-code-review)
Verdict: looks good to merge. No blocking issues. All four CI jobs are green on the latest push (Python checks incl. pip-audit on the bumped pypdf lock, Frontend checks, E2E 5/5, Build check). I reviewed the full diff (44 files, +5617/-26) against the repo's own conventions: first-party-kind ladder, SDK hatch discipline, undo-classification gate, wire-contract/codegen flow, and the Gitlink card patterns the UI mirrors.
What holds up well
- Correct layering throughout: pure domain package (\graphlink_plugins/review_lens/), dispatch mixin with the established deferred-import patch seams, thin intent wrappers, domain-only mutations. The placeholder-claim race discipline and cooperative-cancel shape are faithfully carried over from Gitlink.
- Undo posture is exactly right (URL set + dismissal = A with
ecord_command; fetch/run/ask/cancel = B run-lifecycle), and the classification gate + count pin were updated, not bypassed. - Wire discipline is principled: full diff text off-wire with a monotonic version key (the FIX-6 precedent), scores coerced to the string-valued-dict lane codegen admits, save/load round trip covered by test.
- UI mirrors the Gitlink card chrome faithfully (shell/menu/tabs/draft discipline/lazy-fetch guards/memo comparators), severity tiers render as red/yellow/gray with the precise severity kept in metadata, and clipboard/diff rendering follow the inert-text pipeline rules.
- Pinned-guard amendments (intent count, wire keys + fallbacks, wire budget, bundle ceilings, lock bump) are all commented with measured numbers - the ratchet discipline intact.
Findings (all non-blocking, suggested follow-ups)
- (Medium, follow-up) \codeReviewFiles[].patch\ rides the wire with no reader. Each per-file patch is capped (6KB) but there can be up to 100 files, so a maxed-out node puts ~600KB of patch text on every scene snapshot - the same snapshot-cost class \codeReviewDiffText\ was kept off-wire to avoid. Nothing renders it (Walkthrough shows groups/paths; the diff viewer uses the lazy fetch). Only server-side reader is the fallback heuristic's _first_path_matching. Suggestion: project \patch\ out of the wire row (keep it in state + save file), or cap aggregate patch bytes at fetch time.
- (Nit)
eview_engine.py::_normalize_findings\ builds \known_paths\ (L395-398) but never reads it - dead code, remove or use it to drop findings pointing at unknown paths. - (Nit) \diff_fetch.py\ L142-146 (\if not files_truncated and page > 1: pass) is a no-op block that only restates the flag's default - delete it.
- (Nit) \ est_review_lens_domain.py::test_walkthrough_caps_groups_and_paths\ asserts \len(groups) <= MAX_PR_FILES\ (100) next to the real <= 8\ bound - the first clause is vacuous, drop it.
- (Note, accepted) Fetch fires \onSetPrUrl\ (undoable) then \onFetchDiff\ (run-lifecycle, not undoable), so undo after a fetch reverts the URL but keeps the fetched diff. Same shape as Gitlink's local-root + tree calls - consistent with precedent, just naming it.
- (Note, accepted) \session_load\ restores lists uncapped (caps live in \complete_code_review_run). Save path always caps, so only a hand-edited save file could exceed them - matches how every other kind's restorer trusts the saver.
Security posture - read-only default, per-finding dismissal instead of bulk actions, no GitHub writes, token via the shared allowlisted client, prompt-injection rule stated in both agent prompts, secret material capped and flagged when truncated. The pypdf bump (pre-existing CVE finding, verified hashes) is appropriately scoped into this PR rather than left red.
No action required from me on merge strategy - squash-merge per repo convention.
Summary
Adds Review Lens, a first-party guided code review node: paste a GitHub PR URL, fetch its unified diff, read it as a guided group-by-group walkthrough, and work severity-tiered findings (red/yellow/gray) with a deterministic weighted scorecard - plus follow-up Q&A answered over the stored diff. Read-only by default; nothing posts to GitHub.
What Changed
Why
Code review - not generation - is the bottleneck on agent-authored diffs (large PRs get rubber-stamped instead of read). GraphLink had the retired single-file reviewer and Gitlink's change proposals, but no PR-diff review surface. This closes that gap following the repo's own first-party-kind conventions.
Validation
px playwright test\ passes (5/5, incl. plugin-picker node creation)
UI Notes
Additional Context
Follow-ups deliberately deferred: per-finding Post-as-PR-comment (needs inline-comment position mapping + approval UX), local \git diff\ as a diff source alongside the GitHub API. Dismissal is server-persisted and undoable; re-fetch supersedes the old review rather than merging.