From 720057bc72834049aa6279d054104d39a1404c24 Mon Sep 17 00:00:00 2001 From: Chaim Date: Wed, 5 Aug 2026 09:58:48 +0000 Subject: [PATCH] =?UTF-8?q?fix(aggregator):=20=D7=A6=D7=93=20=D7=A9=D7=9C?= =?UTF-8?q?=D7=9D=20=D7=A0=D7=9E=D7=97=D7=A7=20=D7=91=D7=A9=D7=A7=D7=98=20?= =?UTF-8?q?=E2=80=94=20chunking=20=D7=9C=D7=A4=D7=99=20=D7=92=D7=95=D7=93?= =?UTF-8?q?=D7=9C=20+=20=D7=9B=D7=A9=D7=9C=20=D7=A9=D7=9E=D7=93=D7=95?= =?UTF-8?q?=D7=95=D7=97=20(#233)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit בתיק 1069-04-26 נשלחו 310 טענות עוררים בקריאת-Claude אחת. הקריאה החזירה לא-JSON, הקוד רשם warning והחזיר [], והפעולה דיווחה status=completed עם אפס טיעונים לצד המרכזי בערר. 495 propositions_processed — כלומר "עיבדתי הכל". שתי תקלות מובחנות, שתיהן מתוקנות: 1. **הקריאה גדולה מדי.** הפיצול הפר-כתב-טענות שנוסף למשיבים (#224) הסתיר את זה במקרה — הוא שמר על קריאות קטנות — אבל עוררים וּועדה מדברים בקול אחד ואינם מפוצלים לעולם, כך שערר גדול יוצא בקריאה אחת ענקית. ההערה בקוד כבר תיעדה בדיוק את הכשל הזה אצל המשיבים; התיקון פשוט לא הוחל על העוררים. עכשיו כל צד מעל MAX_PROPS_PER_CALL=80 נצבר בכמה קריאות ומשורשר. הפיצול הוא trade-off ולא רווח חינם: כל chunk מקובץ בבידוד, ולכן צד שמתפצל עלול לקבל יותר טיעונים (וחופפים במקצת) מאשר במעבר יחיד. לאבד ליטיגנט שלם גרוע יותר, והחלופה — פרומפט קטן יותר לכל פרופוזיציה — הייתה מנוונת כל תיק כדי לתקן את הגדולים. הסדר נשמר בחיתוך, כי טענות מגיעות ממוינות לפי claim_index ושכנות שייכות בד"כ לאותו ראש-טיעון. 2. **הכשל נבלע.** החזרת [] על תשובה לא-רשימה אינה ניתנת להבחנה מ"לצד הזה אין טיעונים". עכשיו נזרקת AggregationFailed, הקורא רושם אותה ב-errors, והסטטוס יורד ל-completed_with_errors (כלל-הנדסה §6). ההודעה נוקבת בשם הצד ובמספר הפרופוזיציות, אחרת מפעיל שרואה completed_with_errors לא יודע איזה ליטיגנט נעלם. מבחני רגרסיה: chunking לא מאבד ולא מסדר-מחדש (310→4 קריאות, רצף נשמר); תשובה לא-רשימה זורקת ומזכירה את שם הצד. אם המבחן השני יחזור אי-פעם לטעון == [] — באג הבליעה הוחזר. invariants: כלל-הנדסה §6 — אין בליעה שקטה. G1 — תיקון במקור (גודל הקריאה) ולא בקריאה. G2/G12 — לא נגועים; שני השערים ירוקים. Co-Authored-By: Claude Opus 5 --- .../legal_mcp/services/argument_aggregator.py | 71 +++++++++++++++++-- mcp-server/tests/test_argument_aggregator.py | 54 ++++++++++++++ 2 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 mcp-server/tests/test_argument_aggregator.py diff --git a/mcp-server/src/legal_mcp/services/argument_aggregator.py b/mcp-server/src/legal_mcp/services/argument_aggregator.py index d820572..89520e4 100644 --- a/mcp-server/src/legal_mcp/services/argument_aggregator.py +++ b/mcp-server/src/legal_mcp/services/argument_aggregator.py @@ -147,6 +147,38 @@ def _normalize_argument(raw: dict, fallback_topic: str = "") -> dict | None: } +class AggregationFailed(RuntimeError): + """One side's aggregation call failed. Never swallowed — see #233.""" + + +#: Largest number of propositions sent to Claude in a single aggregation call. +#: +#: Above roughly this size the model stops returning a JSON array and the whole +#: side is lost. The per-brief split added for the respondent side (#224) hid +#: this by accident — it kept those calls small — but appellant and committee +#: speak with a single voice and are never split by brief, so a big appeal goes +#: out in one oversized call. 1069-04-26 sent 310 propositions and got nothing +#: back (#233). +#: +#: Chunking is a real trade-off, not a free win: each chunk is grouped in +#: isolation, so a side split across chunks can end up with more (and slightly +#: overlapping) arguments than a single pass would have produced. Losing an +#: entire litigant is worse, and the alternative — a smaller prompt per +#: proposition — would degrade every case to fix the large ones. +MAX_PROPS_PER_CALL = 80 + + +def _chunk(propositions: list[dict], size: int) -> list[list[dict]]: + """Split propositions into calls of at most ``size``, keeping claim order. + + Order matters: claims arrive sorted by ``claim_index``, so neighbouring + propositions usually belong to the same head of argument. Slicing in order + keeps related material together instead of scattering one argument across + chunks. + """ + return [propositions[i:i + size] for i in range(0, len(propositions), size)] + + async def _aggregate_party( party: str, propositions: list[dict], party_name: str = "", ) -> list[dict]: @@ -155,9 +187,27 @@ async def _aggregate_party( ``party_name`` names the specific pleading when this is a split side (respondent / permit_applicant brief), so the prompt scopes to that litigant's position only (#224). + + Sides larger than ``MAX_PROPS_PER_CALL`` are aggregated in several calls and + concatenated; a failure in any chunk raises rather than returning a partial + side quietly. """ if not propositions: return [] + + if len(propositions) > MAX_PROPS_PER_CALL: + chunks = _chunk(propositions, MAX_PROPS_PER_CALL) + logger.info( + "argument_aggregator: party '%s'%s has %d propositions — " + "aggregating in %d calls of up to %d", + party, f" ({party_name})" if party_name else "", + len(propositions), len(chunks), MAX_PROPS_PER_CALL, + ) + out: list[dict] = [] + for chunk in chunks: + out.extend(await _aggregate_party(party, chunk, party_name=party_name)) + return out + prompt = _build_prompt(party, propositions, party_name=party_name) try: @@ -171,11 +221,18 @@ async def _aggregate_party( ) from e if not isinstance(raw_result, list): - logger.warning( - "argument_aggregator: Claude returned non-list (%s) for party '%s'", - type(raw_result).__name__, party, + # NOT a silent []: returning empty here is indistinguishable from "this + # side genuinely has no arguments", and the caller would then report + # status=completed while a whole litigant vanished. That is exactly what + # happened to the appellant side of 1069-04-26 — 310 claims in, 0 + # arguments out, "completed" (#233). Raise so the caller records it in + # ``errors`` and the status degrades to completed_with_errors (§6). + raise AggregationFailed( + f"Claude returned {type(raw_result).__name__}, not a list, for party " + f"'{party}'{f' ({party_name})' if party_name else ''} with " + f"{len(propositions)} propositions — the side would otherwise be " + f"dropped without trace", ) - return [] out: list[dict] = [] for entry in raw_result: @@ -276,6 +333,12 @@ async def aggregate_claims_to_arguments( group_key = f"{party}·{party_name}" if party_name else party try: arguments = await _aggregate_party(party, props, party_name=party_name) + except AggregationFailed as e: + # A side that failed is NOT a side with no arguments. Record it so + # the status degrades and the caller can see which litigant is + # missing (#233). + errors.append(f"{group_key}: {e}") + continue except RuntimeError as e: # Most likely cause: Claude CLI not installed (running from # the container). Don't crash — record the gap and continue. diff --git a/mcp-server/tests/test_argument_aggregator.py b/mcp-server/tests/test_argument_aggregator.py new file mode 100644 index 0000000..d677b65 --- /dev/null +++ b/mcp-server/tests/test_argument_aggregator.py @@ -0,0 +1,54 @@ +"""Regression tests for argument aggregation (#233). + +Both tests cover the same 2026-08-05 incident from different angles: the +appellant side of 1069-04-26 sent 310 propositions in one Claude call, the call +came back as something other than a JSON array, and the code logged a warning +and returned ``[]``. The caller could not tell that apart from "this side has no +arguments", so ``aggregate_claims_to_arguments`` reported ``completed`` with the +central litigant of the appeal missing entirely. +""" + +from __future__ import annotations + +import pytest + +from legal_mcp.services.argument_aggregator import ( + MAX_PROPS_PER_CALL, + AggregationFailed, + _aggregate_party, + _chunk, +) + + +def test_chunk_preserves_every_proposition_and_their_order(): + """Chunking must not drop or reorder — losing claims here is invisible.""" + props = [{"i": i} for i in range(310)] + + chunks = _chunk(props, MAX_PROPS_PER_CALL) + + assert sum(len(c) for c in chunks) == 310, "propositions were lost" + assert [p for c in chunks for p in c] == props, "order changed" + assert all(len(c) <= MAX_PROPS_PER_CALL for c in chunks) + + +@pytest.mark.asyncio +async def test_non_list_reply_raises_instead_of_dropping_the_side(monkeypatch): + """A malformed reply must surface, never look like an empty side. + + This is the exact 1069-04-26 failure. If this test ever goes back to + asserting ``== []``, the silent-drop bug has been reintroduced. + """ + async def _query_json(prompt, tools=""): # noqa: ARG001 + return {"error": "not a list"} + + monkeypatch.setattr( + "legal_mcp.services.argument_aggregator.claude_session.query_json", + _query_json, + ) + + with pytest.raises(AggregationFailed) as excinfo: + await _aggregate_party("appellant", [{"id": "x", "claim_text": "t"}]) + + # The message has to name the side, or an operator reading + # completed_with_errors cannot tell which litigant went missing. + assert "appellant" in str(excinfo.value)