From 0362b85098c582fe68e5b0763c79adeeca21fd4f Mon Sep 17 00:00:00 2001 From: dovvnloading <157447210+dovvnloading@users.noreply.github.com> Date: Fri, 4 Sep 2026 10:07:37 -0400 Subject: [PATCH] Finish the Review Lens fallback fix, and undo the hole the last one dug Two defects in the fix that shipped in #394. A verification pass measured both against the merged code; neither was theoretical. The fabricated grade was never actually closed. #394 gated the fallback on `json.loads` RAISING, which catches "the model said something unparseable" but not "the model said nothing usable". A reply that parses fine and carries no review sailed straight past it: model returns {} -> needs_revision 72/100 medium model returns {"error": "I cannot"} -> needs_revision 72/100 medium model returns null, or a list -> needs_revision 72/100 medium _normalize_scores defaults all eight categories to 72, so the node rendered a full verdict banner for a change no model had read - the exact defect #394 was written to remove, reached by a different door. A refusal is the likeliest non-happy-path reply any provider gives. looks_like_a_review() now decides it: a real review has an overview, or a scorecard naming a known category, or a non-empty walkthrough/findings/ errors list. A clean review still has the first two, so a genuine "nothing wrong here" is not discarded. Second, #394's narrowed shell pattern silently stopped detecting the common real shapes. Replacing `.*`+DOTALL with a flat `[^)]*` bounded the span to one call, but `[^)]` cannot cross the `)` that closes a nested call - and the ordinary way to write this puts a call in the argument list: subprocess.run(cmd, shell=True) DETECTED subprocess.run(cmd, env=os.environ.copy(), shell=True) MISSED subprocess.run(cmd, cwd=str(path), shell=True) MISSED subprocess.run(" ".join(parts), shell=True) MISSED subprocess.run(\n build(cmd),\n shell=True,\n) MISSED Four of six real shapes. The pre-#394 pattern caught all of them; the control test I added used the one multi-line shape with no nested parens, which is why it passed. The span now allows one level of nesting (`(?:[^()]|\([^()]*\))*?`) and still cannot escape the call. Test plan: - 6 parametrised cases pinning the contentless-reply shapes, and 3 pinning that a thin-but-real review (overview only, scores only, findings only) is still graded. - 7 parametrised cases for shell shapes that must be detected, 4 for shapes that must not be - including a later statement in the same file, the false positive the bound exists for. - Full suite: 3183 passed, 19 skipped. ruff clean. Co-Authored-By: Claude Opus 5 --- backend/tests/test_review_lens_domain.py | 91 +++++++++++++++++-- .../review_lens/review_engine.py | 67 ++++++++++++-- 2 files changed, 143 insertions(+), 15 deletions(-) diff --git a/backend/tests/test_review_lens_domain.py b/backend/tests/test_review_lens_domain.py index 18c9c5e2..22006e4e 100644 --- a/backend/tests/test_review_lens_domain.py +++ b/backend/tests/test_review_lens_domain.py @@ -413,18 +413,49 @@ def test_fallback_does_not_pair_a_call_in_one_file_with_text_in_another(): assert fallback["category_scores"]["security"] == 82 -def test_fallback_still_flags_a_real_shell_true_call_across_lines(): - """The control for the test above: narrowing the pattern to one call's own - argument list must not cost the genuine multi-line case.""" +@pytest.mark.parametrize( + "label, patch", + [ + ("plain", "@@\n+subprocess.run(cmd, shell=True)\n"), + # Every one of these puts a CALL in the argument list, and every one of + # them went undetected when the pattern was a flat `[^)]*` - which + # cannot cross the `)` that closes the inner call. They are the common + # real shapes, and the original control case (multi-line, no nested + # parens) was the one shape that happened to survive. + ("env=", "@@\n+subprocess.run(cmd, env=os.environ.copy(), shell=True)\n"), + ("cwd=", "@@\n+subprocess.run(cmd, cwd=str(path), shell=True)\n"), + ("joined args", '@@\n+subprocess.run(" ".join(parts), shell=True)\n'), + ("multiline", "@@\n+subprocess.run(\n+ cmd,\n+ shell=True,\n+)\n"), + ("multiline nested", "@@\n+subprocess.run(\n+ build(cmd),\n+ shell=True,\n+)\n"), + ("os.system", "@@\n+os.system(cmd)\n"), + ], +) +def test_fallback_flags_every_real_shell_execution_shape(label, patch): agent = ReviewLensAgent() - fallback = agent._fallback_review(_patched( - ("c/run.py", "@@\n+subprocess.run(\n+ cmd,\n+ shell=True,\n+)\n"), - )) + fallback = agent._fallback_review(_patched(("c/run.py", patch))) titles = [finding["title"] for finding in fallback["review_findings"]] - assert any("Shell execution" in title for title in titles) + assert any("Shell execution" in title for title in titles), label assert fallback["review_findings"][0]["path"] == "c/run.py" +@pytest.mark.parametrize( + "label, patch", + [ + ("text in a string", "@@\n+DOC = 'never pass shell=True here'\n"), + ("shell=False", "@@\n+subprocess.run(cmd, shell=False) # not shell=True\n"), + # The span must not escape one call to reach a later statement, which + # is the false positive the per-file scoping and this bound exist for. + ("later statement", "@@\n+subprocess.run(cmd)\n+SHELL_DOC = 'never shell=True'\n"), + ("two calls then text", "@@\n+subprocess.run(a)\n+subprocess.run(b)\n+x = 'shell=True'\n"), + ], +) +def test_fallback_does_not_invent_a_shell_finding(label, patch): + agent = ReviewLensAgent() + fallback = agent._fallback_review(_patched(("c/run.py", patch))) + titles = [finding["title"] for finding in fallback["review_findings"]] + assert not any("Shell execution" in title for title in titles), label + + def test_fallback_detects_a_bare_except_on_the_last_added_line(): """Added lines are joined WITHOUT a trailing newline, so the old `except\\s*:\\s*\\n` pattern could never match the final added line.""" @@ -479,6 +510,52 @@ def test_an_unparseable_model_reply_also_reports_no_verdict(monkeypatch): assert result["quality_score"] == 0 +@pytest.mark.parametrize( + "label, reply", + [ + ("empty object", "{}"), + # The likeliest non-happy-path reply any provider gives, and the one + # that made this a live defect rather than a theoretical one. + ("refusal object", '{"error": "I cannot review this"}'), + ("bare null", "null"), + ("a list", "[1, 2, 3]"), + ("fenced empty object", "```json\n{}\n```"), + ("wrong shape entirely", '{"unrelated": "payload"}'), + ], +) +def test_a_reply_that_parses_but_carries_no_review_reports_no_verdict(monkeypatch, label, reply): + """Gating the fallback on `json.loads` RAISING was not enough. Each of + these parses cleanly, so it reached _normalize_response, where + _normalize_scores defaulted all eight categories to 72 - and the node + rendered "Needs Revision, 72/100, Release risk: Medium" for a change no + model had read.""" + monkeypatch.setattr(api_provider, "chat", lambda **kwargs: {"message": {"content": reply}}) + agent = ReviewLensAgent() + result = agent.get_response(_payload()) + assert result["fallback"] is True, label + assert result["verdict"] == "none", label + assert result["quality_score"] == 0, label + + +@pytest.mark.parametrize( + "label, reply", + [ + ("overview only", '{"overview": "All good."}'), + ("scores only", '{"category_scores": {"correctness": 90}}'), + ("findings only", '{"review_findings": [{"title": "T", "evidence": "E"}]}'), + ], +) +def test_a_thin_but_real_review_is_still_graded(monkeypatch, label, reply): + """The other side of the same gate: a genuine review with nothing wrong to + report still has an overview and a scorecard, and must NOT be discarded as + contentless.""" + monkeypatch.setattr(api_provider, "chat", lambda **kwargs: {"message": {"content": reply}}) + agent = ReviewLensAgent() + result = agent.get_response(_payload()) + assert result["fallback"] is False, label + assert result["verdict"] != "none", label + + def test_a_real_model_reply_is_still_graded_normally(monkeypatch): monkeypatch.setattr( api_provider, "chat", diff --git a/graphlink_plugins/review_lens/review_engine.py b/graphlink_plugins/review_lens/review_engine.py index 8d9936f6..936b7cf9 100644 --- a/graphlink_plugins/review_lens/review_engine.py +++ b/graphlink_plugins/review_lens/review_engine.py @@ -205,6 +205,38 @@ def _added_lines(diff_text): return added +def looks_like_a_review(parsed): + """True only if the model actually returned a review. + + The distinction the fallback has to key on, and previously did not. It + keyed on `json.loads` RAISING, which conflates "the model said nothing + usable" with "the model said something unparseable" - and only catches the + second. A reply that parses fine but carries no review at all (`{}`, a + refusal object like {"error": "I cannot review this"}, a bare `null`, a + JSON list) sailed past that check into _normalize_response, where + _normalize_scores defaults all eight categories to 72 and the node + rendered "Verdict: Needs Revision, 72/100, Release risk: Medium" for a + change no model had read. A refusal is the single likeliest non-happy-path + reply any provider gives. + + A real review has SOMETHING to say: an overview, a scorecard naming at + least one known category, or a non-empty walkthrough/findings/errors list. + A clean review still has the first two - only the lists may be empty - so + this does not reject a genuine "nothing wrong here" verdict. + """ + if not isinstance(parsed, dict): + return False + if _clean_text(parsed.get("overview")): + return True + scores = parsed.get("category_scores") + if isinstance(scores, dict) and any(key in REVIEW_CATEGORY_WEIGHTS for key in scores): + return True + return any( + isinstance(parsed.get(key), list) and parsed.get(key) + for key in ("walkthrough", "review_findings", "errors_found") + ) + + def _added_sections(payload): """[(path, that file's added-line text)] - the unit every fallback heuristic scans. @@ -552,15 +584,27 @@ def add_error(severity, kind, path, title, evidence, fix): ) scores["security"] = min(scores["security"], 40) - # `[^)]*` keeps the match inside one call's own argument list, and - # re.DOTALL is gone. Together they were the worst of these: `.*` under - # DOTALL ran across every file's added lines at once, so a benign - # `subprocess.run(["ls"])` in one file plus the words `shell=True` in - # a string literal in another produced a HIGH-severity finding and - # dropped the security score to 45. `[^)]` still spans newlines, so a - # genuine multi-line call is still caught. + # The span stays inside one call's own argument list - `[^()]` cannot + # cross either bracket, and the `\([^()]*\)` alternative lets ONE level + # of nesting through so an argument that is itself a call does not end + # the match early. re.DOTALL is gone. + # + # Both halves are load-bearing and each was wrong once. The original + # `.*` under DOTALL ran across every file's added lines at a time, so a + # benign `subprocess.run(["ls"])` in one file plus the words + # `shell=True` in a string literal in ANOTHER scored a HIGH finding. + # The first fix for that used a flat `[^)]*`, which cannot cross a + # `)` at all - so it silently stopped detecting the most common real + # shapes, every one of which puts a call in the argument list: + # `env=os.environ.copy()`, `cwd=str(path)`, `" ".join(parts)`, + # `build(cmd)`. Four of six real shapes went undetected until a + # verification pass measured them. The cases are pinned in + # backend/tests/test_review_lens_domain.py. path = _first_section_matching( - sections, r"subprocess\.(?:Popen|run)\([^)]*shell\s*=\s*True|os\.system\(", re.IGNORECASE) + sections, + r"subprocess\.(?:Popen|run)\((?:[^()]|\([^()]*\))*?shell\s*=\s*True|os\.system\(", + re.IGNORECASE, + ) if path is not None: add_finding( "high", "Security", path, @@ -836,10 +880,17 @@ def get_response(self, payload): {"role": "system", "content": self.SYSTEM_PROMPT}, {"role": "user", "content": user_prompt}, ] + raw_text = None + parsed = None try: raw_text = api_provider.chat(task=config.TASK_CHAT, messages=messages)["message"]["content"] parsed = json.loads(self._extract_json(raw_text)) except Exception: + parsed = None + + # Two ways to end up without a review, and BOTH have to land here. The + # try/except above only catches the second. See looks_like_a_review. + if not looks_like_a_review(parsed): fallback = self._fallback_review(payload) return self._normalize_response({ "title": f"Review of {payload.get('repo', '')}#{payload.get('pr_number', '')}",