-
Notifications
You must be signed in to change notification settings - Fork 60
fix: improve root cause detector parsing returned json #401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |||||||||||||
| import json | ||||||||||||||
| import logging | ||||||||||||||
| import re | ||||||||||||||
| from collections.abc import Iterator | ||||||||||||||
|
|
||||||||||||||
| from pydantic import ValidationError | ||||||||||||||
| from strands.models.model import Model | ||||||||||||||
|
|
@@ -176,13 +177,110 @@ async def _stream() -> str: | |||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _extract_json(text: str) -> str: | ||||||||||||||
| """Extract JSON from LLM response, stripping markdown fences if present.""" | ||||||||||||||
| """Extract JSON from an LLM response. | ||||||||||||||
|
|
||||||||||||||
| Handles three shapes the model emits in practice: | ||||||||||||||
| 1. A fenced block: ```json ... ``` (or a bare ``` ... ``` fence). | ||||||||||||||
| 2. A prose preamble followed by a bare JSON object/array, e.g. | ||||||||||||||
| "Looking at the session, here are the failures:\n{ ... }". | ||||||||||||||
| 3. Clean JSON with no wrapping. | ||||||||||||||
|
|
||||||||||||||
| For (2) we scan for balanced `{...}`/`[...]` spans (tracking string | ||||||||||||||
| literals and escapes so brackets inside strings don't fool the matcher) | ||||||||||||||
| and pick the one most likely to be the detector payload: a JSON object | ||||||||||||||
| carrying the top-level `"errors"` key wins over one that merely parses | ||||||||||||||
| as JSON, so a decoy object in the model's prose (e.g. a format example | ||||||||||||||
| like `{"category": "tool_error"}`) doesn't shadow the real result. | ||||||||||||||
| Candidates that don't parse — e.g. a regex like `[a-z0-9-]` the model | ||||||||||||||
| mentioned in its prose — are skipped. Objects are preferred over arrays | ||||||||||||||
| since the detector schema is a JSON object. Falls back to the stripped | ||||||||||||||
| text so the caller's json parser produces the original, actionable | ||||||||||||||
| error if nothing JSON-like is found. | ||||||||||||||
| """ | ||||||||||||||
| match = re.search(r"```(?:json)?\s*(.*?)```", text, re.DOTALL) | ||||||||||||||
| if match: | ||||||||||||||
| return match.group(1).strip() | ||||||||||||||
|
|
||||||||||||||
| extracted = _extract_balanced_json(text) | ||||||||||||||
| if extracted is not None: | ||||||||||||||
| return extracted | ||||||||||||||
|
|
||||||||||||||
| return text.strip() | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _iter_balanced_spans(text: str, open_ch: str, close_ch: str) -> Iterator[str]: | ||||||||||||||
| """Yield every balanced `open_ch`…`close_ch` substring in `text`. | ||||||||||||||
|
|
||||||||||||||
| Respects string literals and escape sequences so brackets inside quoted | ||||||||||||||
| strings don't affect nesting. Handles multiple, possibly nested spans; | ||||||||||||||
| truncated (never-closing) spans are simply not yielded. | ||||||||||||||
| """ | ||||||||||||||
| i = 0 | ||||||||||||||
| n = len(text) | ||||||||||||||
| while i < n: | ||||||||||||||
| if text[i] != open_ch: | ||||||||||||||
| i += 1 | ||||||||||||||
| continue | ||||||||||||||
| depth = 0 | ||||||||||||||
| in_string = False | ||||||||||||||
| escaped = False | ||||||||||||||
| for j in range(i, n): | ||||||||||||||
| ch = text[j] | ||||||||||||||
| if in_string: | ||||||||||||||
| if escaped: | ||||||||||||||
| escaped = False | ||||||||||||||
| elif ch == "\\": | ||||||||||||||
| escaped = True | ||||||||||||||
| elif ch == '"': | ||||||||||||||
| in_string = False | ||||||||||||||
| continue | ||||||||||||||
| if ch == '"': | ||||||||||||||
| in_string = True | ||||||||||||||
| elif ch == open_ch: | ||||||||||||||
| depth += 1 | ||||||||||||||
| elif ch == close_ch: | ||||||||||||||
| depth -= 1 | ||||||||||||||
| if depth == 0: | ||||||||||||||
| yield text[i : j + 1] | ||||||||||||||
| break | ||||||||||||||
| # Advance past this opener regardless of whether it balanced, so we | ||||||||||||||
| # keep looking for a later, valid span. | ||||||||||||||
| i += 1 | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _extract_balanced_json(text: str) -> str | None: | ||||||||||||||
| """Return the balanced JSON value embedded in `text` most likely to be the payload. | ||||||||||||||
|
|
||||||||||||||
| Scans for balanced `{...}` spans first (the detector's structured | ||||||||||||||
| output is a JSON object), then `[...]` spans. Among the spans that | ||||||||||||||
| parse as JSON, one shaped like the detector payload — a JSON object | ||||||||||||||
| carrying the top-level `"errors"` key — is preferred over one that | ||||||||||||||
| merely parses. That way a decoy object earlier in the model's prose | ||||||||||||||
| (e.g. a format example like `{"category": "tool_error"}`) doesn't | ||||||||||||||
| shadow the real result and reintroduce a silent-empty diagnosis. The | ||||||||||||||
| `"errors"` key (rather than full `FailureDetectionStructuredOutput` | ||||||||||||||
| validation) is used as the discriminator so extraction stays tolerant | ||||||||||||||
| of minor per-entry issues and leaves strict validation to | ||||||||||||||
| `_parse_text_result`. If nothing carries the key, the first parseable | ||||||||||||||
| candidate is returned so the caller still gets a concrete value to | ||||||||||||||
| surface an actionable error. Spans that don't parse — e.g. a regex like | ||||||||||||||
| `[a-z0-9-]` — are skipped. Returns None if no candidate parses. | ||||||||||||||
| """ | ||||||||||||||
| first_parseable: str | None = None | ||||||||||||||
| for open_ch, close_ch in (("{", "}"), ("[", "]")): | ||||||||||||||
| for span in _iter_balanced_spans(text, open_ch, close_ch): | ||||||||||||||
| candidate = span.strip() | ||||||||||||||
| try: | ||||||||||||||
| parsed = json.loads(candidate) | ||||||||||||||
| except json.JSONDecodeError: | ||||||||||||||
| continue | ||||||||||||||
| if first_parseable is None: | ||||||||||||||
| first_parseable = candidate | ||||||||||||||
| if isinstance(parsed, dict) and "errors" in parsed: | ||||||||||||||
| return candidate | ||||||||||||||
| return first_parseable | ||||||||||||||
|
Comment on lines
+279
to
+281
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First The prompt contains three Suggestion: keep the last errors-bearing span (a preamble is the documented failure mode; a trailing recap isn't). 107 tests pass with this.
Suggested change
(plus |
||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _parse_text_result(text: str) -> list[FailureItem]: | ||||||||||||||
| """Parse raw LLM text response into list[FailureItem]. | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -103,6 +103,45 @@ def test_extract_json_with_surrounding_text(): | |||||||||||
| assert '"errors"' in _extract_json(text) | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_extract_json_bare_object_after_prose(): | ||||||||||||
| """A bare JSON object following a prose preamble (no fence) is extracted.""" | ||||||||||||
| text = 'Looking at the session, here are the failures:\n{"errors": []}' | ||||||||||||
| assert _extract_json(text) == '{"errors": []}' | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_extract_json_prefers_schema_over_decoy(): | ||||||||||||
| """A decoy JSON object in the prose must not shadow the real payload. | ||||||||||||
|
|
||||||||||||
| The detector schema is well-known, so the span that validates against | ||||||||||||
| it wins over an earlier span that merely parses as JSON. Otherwise the | ||||||||||||
| decoy fails schema validation downstream and reintroduces the | ||||||||||||
| silent-empty diagnosis this logic exists to prevent. | ||||||||||||
| """ | ||||||||||||
| real_payload = ( | ||||||||||||
| '{"errors": [{"location": "s1", "category": ["err"], ' | ||||||||||||
| '"confidence": ["high"], "evidence": ["ev"]}]}' | ||||||||||||
| ) | ||||||||||||
|
Comment on lines
+120
to
+123
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| text = f'Example format {{"category": "tool_error"}}. Now the real one: {real_payload}' | ||||||||||||
| extracted = _extract_json(text) | ||||||||||||
| assert extracted == real_payload | ||||||||||||
| # And it survives the full parse rather than collapsing to []. | ||||||||||||
| result = _parse_text_result(text) | ||||||||||||
| assert len(result) == 1 | ||||||||||||
| assert result[0].span_id == "s1" | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_extract_json_skips_regex_in_prose(): | ||||||||||||
| """Bracketed prose that isn't JSON (e.g. a regex) is skipped, not returned.""" | ||||||||||||
| text = 'The ids match [a-z0-9-]. Here is the result: {"errors": []}' | ||||||||||||
| assert _extract_json(text) == '{"errors": []}' | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_extract_json_falls_back_to_first_parseable(): | ||||||||||||
| """When nothing validates against the schema, the first parseable span is returned.""" | ||||||||||||
| text = 'Here is some data: {"foo": 1} and more.' | ||||||||||||
| assert _extract_json(text) == '{"foo": 1}' | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_parse_text_result_basic(): | ||||||||||||
| text = _make_json_response( | ||||||||||||
| [ | ||||||||||||
|
|
||||||||||||
Uh oh!
There was an error while loading. Please reload this page.