Finish the Review Lens fallback fix, and undo the hole the last one dug

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

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