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', '')}",