fix(review): salvage findings from nested-object fences + bare arrays + unfenced tail JSON
The canalhandia PR review lost all findings because the agent ran out of
context before emitting the closing json fence. Three failure modes hit
the old regex \{.*?\}:
* nested objects inside the fence truncated at the first }
* bare arrays (no {summary, findings} wrapper) returned []
* unfenced JSON in the prose tail was never reached (first not last)
Replace the regex with a balanced-brace scanner:
* _last_json_block walks the fence contents with a depth counter so
nested objects survive
* _last_balanced_json + _balanced_json_substring handle bare arrays and
prose-tail JSON when no fence is present
* _parse_json_tolerant returns list as well as dict; parse_findings and
parse_review_output accept a bare array as the outer value
Agent prompt tightened: reserve the final step for emitting the JSON
block so the analysis isn't lost when context runs out.
10 new tests in tests/pilot/test_ai_review.py cover the new shapes.
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -11,6 +11,9 @@ sys.path.insert(0, os.path.join(ROOT, "pilot"))
|
||||
|
||||
import ai_review # noqa: E402
|
||||
from ai_review import ( # noqa: E402
|
||||
_balanced_json_substring,
|
||||
_extract_first_json_object,
|
||||
_last_balanced_json,
|
||||
build_user_prompt,
|
||||
compute_attribution,
|
||||
format_review_body,
|
||||
@@ -408,6 +411,108 @@ def test_parse_review_output_uses_last_json_block():
|
||||
assert fs[0]["path"] == "y"
|
||||
|
||||
|
||||
def test_parse_findings_fenced_json_with_nested_object():
|
||||
# Real-world regression: agent emits a fence whose inner JSON has nested
|
||||
# objects. The old regex `\{.*?\}` matched only the first `}`, truncating
|
||||
# the JSON. Now we balance braces inside the fence.
|
||||
txt = (
|
||||
"```json\n"
|
||||
'{"summary":"x","findings":[{"severity":"high","path":"a.py","line":1,'
|
||||
'"problem":"p","fix":"f","suggestion":"","reference":""}],"meta":{"engine":"opencode"}}\n'
|
||||
"```"
|
||||
)
|
||||
fs = parse_findings(txt)
|
||||
assert len(fs) == 1
|
||||
assert fs[0]["path"] == "a.py"
|
||||
|
||||
|
||||
def test_parse_findings_unfenced_at_tail():
|
||||
# No fence at all. Agent wrote the JSON inline at the very end of its
|
||||
# prose. The old first-balanced regex caught the FIRST `{`, not this one.
|
||||
txt = (
|
||||
"I considered the diff carefully. Two findings stand out:\n"
|
||||
"First one is just text.\n"
|
||||
'{"findings":[{"severity":"critical","path":"x","line":1,"problem":"p","fix":"f"}]}'
|
||||
)
|
||||
fs = parse_findings(txt)
|
||||
assert len(fs) == 1
|
||||
assert fs[0]["severity"] == "critical"
|
||||
|
||||
|
||||
def test_parse_findings_bare_array():
|
||||
# Some agents skip the `{"summary":..., "findings":[...]}` wrapper and
|
||||
# emit just the array.
|
||||
txt = (
|
||||
"Here are my findings:\n"
|
||||
"```json\n"
|
||||
'[{"severity":"low","path":"a","line":1,"problem":"p","fix":"f","suggestion":"","reference":""}]\n'
|
||||
"```"
|
||||
)
|
||||
fs = parse_findings(txt)
|
||||
assert len(fs) == 1
|
||||
assert fs[0]["path"] == "a"
|
||||
|
||||
|
||||
def test_parse_review_output_unfenced_at_tail():
|
||||
# The exact shape canalhandia produced: long prose, JSON at the very end,
|
||||
# no fence. Old parser returned ([], salvage) — now we recover findings.
|
||||
txt = (
|
||||
"Let me refine the fix: should call a dedicated `setPermanent`.\n"
|
||||
"Let me finalize. Let me also double-check the `find` thread-safety.\n"
|
||||
'{"summary":"Adds void protection; one critical race.","findings":['
|
||||
'{"severity":"high","path":"VoidProtection.java","line":162,'
|
||||
'"problem":"drop duplication race","fix":"use ItemMeta","suggestion":"","reference":""}]}'
|
||||
)
|
||||
summary, fs = parse_review_output(txt)
|
||||
assert "void protection" in summary.lower()
|
||||
assert len(fs) == 1
|
||||
assert fs[0]["path"] == "VoidProtection.java"
|
||||
|
||||
|
||||
def test_parse_review_output_bare_array_at_tail():
|
||||
txt = (
|
||||
"All wrapped up.\n"
|
||||
'[{"severity":"low","path":"a","line":1,"problem":"p","fix":"","suggestion":"","reference":""}]'
|
||||
)
|
||||
summary, fs = parse_review_output(txt)
|
||||
assert summary == ""
|
||||
assert len(fs) == 1
|
||||
|
||||
|
||||
def test_scan_balanced_handles_braces_in_strings():
|
||||
# The JSON scanner must not be fooled by `{` or `}` inside string literals.
|
||||
s = '{"a":"contains { and }","b":1}'
|
||||
obj = _extract_first_json_object(s)
|
||||
assert obj == s
|
||||
d = json.loads(obj)
|
||||
assert d["a"] == "contains { and }"
|
||||
|
||||
|
||||
def test_last_balanced_json_picks_latest():
|
||||
s = '{"a":1} some text {"b":2,"nested":{"c":3}} trailing'
|
||||
out = _last_balanced_json(s)
|
||||
assert out is not None
|
||||
d = json.loads(out)
|
||||
assert d == {"b": 2, "nested": {"c": 3}}
|
||||
|
||||
|
||||
def test_last_balanced_json_no_json():
|
||||
assert _last_balanced_json("nothing here") is None
|
||||
assert _last_balanced_json("") is None
|
||||
|
||||
|
||||
def test_balanced_json_substring_skips_leading_prose():
|
||||
s = 'preamble {"a":1} more prose {"b":2}'
|
||||
out = _balanced_json_substring(s)
|
||||
assert out == '{"a":1}'
|
||||
|
||||
|
||||
def test_balanced_json_substring_handles_array():
|
||||
s = '[{"a":1},{"b":2}]'
|
||||
out = _balanced_json_substring(s)
|
||||
assert out == s
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# reference rendering in inline_comment_body + summary_bullets + summary section
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user