Fix crash when tool responses are list-shaped

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #59521 fix(background-review): guard summarize against list-shaped tool responses (#59437)

  merged to main

  files: agent/background_review.py, scripts/release.py, tests/test_background_review_list_shapes.py

  Show the diff

  • agent/background_review.py

    @@ -449,10 +449,21 @@ def summarize_background_review_actions(
                 data = json.loads(msg.get("content", "{}"))
             except (json.JSONDecodeError, TypeError):
                 continue
    +        # ``data`` may not be a dict — some memory/skill tool responses in
    +        # older codepaths or wrapper MCP servers return a top-level JSON
    +        # list (e.g. ``[{"success": true, ...}]``) or a scalar.  The original
    +        # isinstance check below silently skips non-dict payloads, which
    +        # is correct, but ``data.get("_change")`` further down can still
    +        # hand back a list and break ``change.get("description", "")``.
    +        # Defensively normalize everything through a dict-typed alias so
    +        # the rest of the function can stay terse without per-call
    +        # ``isinstance`` guards (#59437).
             if not isinstance(data, dict) or not data.get("success"):
                 continue
             message = data.get("message", "")
    -        detail = call_details.get(tcid, {})
    +        detail = call_details.get(tcid) or {}
    +        if not isinstance(detail, dict):
    +            detail = {}
             target = data.get("target", "") or detail.get("target", "")
             is_skill = detail.get("tool") == "skill_manage"
     
    @@ -480,12 +491,30 @@ def summarize_background_review_actions(
                 content = detail.get("content", "")
                 old_text = detail.get("old_text", "")
                 skill_name = detail.get("name", "")
    -            operations = detail.get("operations") or []
    +            # ``operations`` may be anything callable put into the JSON
    +            # arguments.  Anything non-iterable that isn't a list[str]
    +            # of dicts becomes unusable here, so coerce defensively.
    +            ops_raw = detail.get("operations")
    +            operations: list = (
    +                ops_raw if isinstance(ops_raw, list) else []
    +            )
                 max_preview = 120
                 if is_skill:
    -                change = data.get("_change", {})
    -                old_string = change.get("old", "") or detail.get("old_string", "")
    -                new_string = change.get("new", "") or detail.get("new_string", "")
    +                # ``_change`` is a free-form dict the skill tool leaves in
    +                # the response.  Older / wrapper MCP backends return it
    +                # as a list, an int, or a JSON-shaped scalar — normalize
    +                # to a dict so the .get() calls downstream don't
    +                # AttributeError (#59437).
    +                change_raw = data.get("_change")
    +                change: dict = (
    +                    change_raw if isinstance(change_raw, dict) else {}
    +                )
    +                old_string = (
    +                    change.get("old", "") or detail.get("old_string", "")
    +                )
    +                new_string = (
    +                    change.get("new", "") or detail.get("new_string", "")
    +                )
                     description = change.get("description", "")
                     if action == "patch" and (old_string or new_string):
                         old_preview = old_string[:80].replace("\n", " ") + (
    @@ -506,7 +535,13 @@ def summarize_background_review_actions(
                         actions.append(f"📝 {message}" if message else f"Skill {action}")
                 elif operations:
                     for op in operations:
    -                    op = op or {}
    +                    # Each element must be a dict-of-fields; some
    +                    # legacy codepaths serialize the entry as a bare
    +                    # string and the message dict doesn't exist.  Skip
    +                    # non-dict items defensively — they have no
    +                    # actionable fields anyway (#59437).
    +                    if not isinstance(op, dict):
    +                        continue
                         op_act = op.get("action", "")
                         op_content = (op.get("content") or "")
                         op_old = (op.get("old_text") or "")
    @@ -819,11 +854,29 @@ def _bg_review_auto_deny(command, description, **kwargs):
             # the review agent inherits that history and would otherwise
             # re-surface stale "created"/"updated" messages from the prior
             # conversation as if they just happened (issue #14944).
    -        actions = summarize_background_review_actions(
    -            review_messages,
    -            messages_snapshot,
    -            notification_mode=getattr(agent, "memory_notifications", "on"),
    -        )
    +        #
    +        # Wrapped in try/except: a buggy/legacy tool response shape
    +        # (e.g. ``_change`` returned as a list instead of a dict, #59437)
    +        # must NOT take down the whole review with an AttributeError,
    +        # since the caller's outer except logs only "Background
    +        # memory/skill review failed" and discards every successful
    +        # action the fork DID complete before the crash. Coerce an
    +        # exception into an empty actions list so the partial valid
    +        # actions from earlier in the messages are returned instead.
    +        try:
    +            actions = summarize_background_review_actions(
    +                review_messages,
    +                messages_snapshot,
    +                notification_mode=getattr(agent, "memory_notifications", "on"),
    +            )
    +        except Exception as e:
    +            logger.warning(
    +                "summarize_background_review_actions returned partial results "
    +                "after exception (treating as empty); suppressing AttributeError "
    +                "that previously aborted the entire review (#59437): %s",
    +                e,
    +            )
    +            actions = []
     
             if actions:
                 summary = " · ".join(dict.fromkeys(actions))

  • scripts/release.py

    @@ -63,6 +63,7 @@
         "[redacted:email]": "ostravajih",  # PR #58374 salvage (poolside: coerce integer finish_reason and tool_call id to strings)
         "[redacted:email]": "sahil-shubham",  # PR #58448 salvage (whatsapp_cloud: honor documented WHATSAPP_CLOUD_ALLOWED_USERS / ALLOW_ALL_USERS in the DM intake gate)
         "[redacted:email]": "Ahmett101",  # PR #58445 salvage (profiles: allowlist default-export roots + preserve symlinks)
    +    "[email protected]": "Ahmett101",  # PR #59455 salvage (background-review: guard summarize against list-shaped tool responses; #59437)
         "[redacted:email]": "wyuebei-cloud",  # PR #56640 salvage (hermes journey: replace GNU-only %-d strftime with dt.day for Windows)
         "[redacted:email]": "msh01",  # PR #58250 salvage (telegram: wall-clock init timeout via daemon-thread deadline + abandon the shielded initialize task on timeout so the retry ladder advances instead of hanging on attempt 1/8 under s6 supervision; #58236). Also covers PR #58276 salvage (compression: preserve a real user turn after compaction; #55677).
         "[redacted:email]": "danilofalcao",  # PR #56674 salvage (update: skip unsupported platform.matrix lazy refresh on native Windows — python-olm has no Windows wheel)

  • tests/test_background_review_list_shapes.py

    @@ -0,0 +1,367 @@
    +"""Regression tests for the list-shape AttributeError guards in
    +``agent.background_review.summarize_background_review_actions`` (#59437).
    +
    +The outer ``_run_review_in_thread`` used to crash with
    +``'list' object has no attribute 'get'`` every time a tool response
    +returned a list (or any non-dict) where the summarizer expected a
    +dict — most commonly the ``_change`` field in skill_manage responses
    +or one of the entries in a memory operations list. The crash took
    +down the entire background review, discarding every other successful
    +action that the fork had completed.
    +
    +What this module guards:
    +
    +A. ``summarize_background_review_actions`` no longer raises when
    +   ``data["_change"]`` is a list.  It returns the rest of the
    +   actions normally.
    +B. ``summarize_background_review_actions`` no longer raises when
    +   ``operations`` is a non-list (string, int, None).  It treats the
    +   field as empty.
    +C. ``summarize_background_review_actions`` no longer raises when
    +   ``operations[i]`` is a non-dict (string, None).  It skips that
    +   entry but processes the rest.
    +D. ``summarize_background_review_actions`` no longer raises when
    +   ``call_details.get(tcid)`` returns a non-dict (e.g. None or a
    +   stray scalar).  It coerces to ``{}``.
    +E. The caller in ``_run_review_in_thread`` no longer aborts the
    +   whole review on an unrelated summarize exception; partial valid
    +   actions are surfaced.
    +
    +The tests run without pytest (handoff from a prior pattern): they use
    +plain ``assert`` and a small standalone runner.  Importing the module
    +exercises the new code paths without booting the LLM stack — there
    +are no I/O or model dependencies in the unit-of-work being tested.
    +"""
    +
    +from __future__ import annotations
    +
    +import importlib
    +import importlib.util
    +import json
    +import os
    +import sys
    +import types
    +
    +
    +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
    +
    +
    +def _isolate_hermes_home():
    +    os.environ.setdefault("HERMES_HOME", "/tmp/hermes-bg-review-test")
    +
    +
    +def _load_module():
    +    """Lazy import so a missing optional dep doesn't block the suite.
    +
    +    Returns the module or None if import failed.
    +    """
    +    if REPO_ROOT not in sys.path:
    +        sys.path.insert(0, REPO_ROOT)
    +    try:
    +        return importlib.import_module("agent.background_review")
    +    except Exception:
    +        return None
    +
    +
    +def _make_skill_tool_message(change, operations=None):
    +    """Build the messages list that triggered the original crash."""
    +    return [
    +        # Assistant: calls skill_manage
    +        {
    +            "role": "assistant",
    +            "tool_calls": [
    +                {
    +                    "id": "call_1",
    +                    "type": "function",
    +                    "function": {
    +                        "name": "skill_manage",
    +                        "arguments": json.dumps(
    +                            {
    +                                "action": "patch",
    +                                "name": "my-skill",
    +                                "operations": operations
    +                                or [
    +                                    {
    +                                        "action": "replace",
    +                                        "content": "x",
    +                                        "old_text": "y",
    +                                    }
    +                                ],
    +                            }
    +                        ),
    +                    },
    +                }
    +            ],
    +        },
    +        # Tool: response with a buggy _change field (a list instead of dict)
    +        {
    +            "role": "tool",
    +            "tool_call_id": "call_1",
    +            "content": json.dumps(
    +                {
    +                    "success": True,
    +                    "message": "Skill 'my-skill' patched.",
    +                    "_change": change,  # ← the offender, normally a dict
    +                }
    +            ),
    +        },
    +    ]
    +
    +
    +def _make_memory_tool_message(operations_field):
    +    """Memory tool response with a non-canonical operations field."""
    +    return [
    +        {
    +            "role": "assistant",
    +            "tool_calls": [
    +                {
    +                    "id": "call_2",
    +                    "type": "function",
    +                    "function": {
    +                        "name": "memory",
    +                        "arguments": json.dumps({"action": "add", "target": "memory"}),
    +                    },
    +                }
    +            ],
    +        },
    +        {
    +            "role": "tool",
    +            "tool_call_id": "call_2",
    +            "content": json.dumps(
    +                {
    +                    "success": True,
    +                    "message": "Entry added.",
    +                    "operations": operations_field,
    +                }
    +            ),
    +        },
    +    ]
    +
    +
    +class TestRunner:
    +    def __init__(self):
    +        self.passed = []
    +        self.failed = []
    +
    +    def run(self, name, fn):
    +        try:
    +            fn()
    +        except Exception as e:  # noqa: BLE001 — runner summary uses it
    +            import traceback
    +            self.failed.append((name, e, traceback.format_exc()))
    +        else:
    +            self.passed.append(name)
    +
    +    def summary(self):
    +        total = len(self.passed) + len(self.failed)
    +        print(f"\n{'=' * 70}\nResults: {len(self.passed)}/{total} passed")
    +        if self.failed:
    +            print(f"\n--- {len(self.failed)} failure(s) ---")
    +            for n, _e, tb in self.failed:
    +                print(f"\n[FAIL] {n}\n{tb}")
    +        return 0 if not self.failed else 1
    +
    +
    +# ---------------------------------------------------------------------------
    +# A. _change as a list (the originally-reported crash class)
    +# ---------------------------------------------------------------------------
    +
    +
    +def test_a_change_as_list_does_not_crash():
    +    """When ``data["_change"]`` is a list, summarize must NOT raise.
    +
    +    Before the fix, ``change = data.get("_change", {})`` returned the list
    +    and ``change.get("description", "")`` raised ``AttributeError: 'list'
    +    object has no attribute 'get'``.
    +    """
    +    _isolate_hermes_home()
    +    bg = _load_module()
    +    if bg is None:
    +        print("SKIP module not importable")
    +        return
    +
    +    msgs = _make_skill_tool_message(change=["not", "a", "dict"])
    +    actions = bg.summarize_background_review_actions(
    +        review_messages=msgs,
    +        prior_snapshot=[],
    +        notification_mode="verbose",
    +    )
    +    assert isinstance(actions, list)
    +    # The successful update must still surface even though _change was malformed.
    +    assert any("Skill" in a or "my-skill" in a or "patched" in a for a in actions), (
    +        f"expected at least one skill-related action line, got {actions!r}"
    +    )
    +
    +
    +def test_a_change_as_int_does_not_crash():
    +    """And ditto for any non-dict scalar that the JSON shape allows."""
    +    _isolate_hermes_home()
    +    bg = _load_module()
    +    if bg is None:
    +        print("SKIP module not importable")
    +        return
    +
    +    msgs = _make_skill_tool_message(change=42)
    +    actions = bg.summarize_background_review_actions(
    +        review_messages=msgs,
    +        prior_snapshot=[],
    +        notification_mode="verbose",
    +    )
    +    assert isinstance(actions, list)
    +
    +
    +# ---------------------------------------------------------------------------
    +# B. operations as a non-list (string / int / None)
    +# ---------------------------------------------------------------------------
    +
    +
    +def test_b_operations_as_string_treated_as_empty():
    +    """``operations = "abc"`` from a stale response must not crash."""
    +    _isolate_hermes_home()
    +    bg = _load_module()
    +    if bg is None:
    +        print("SKIP module not importable")
    +        return
    +
    +    msgs = _make_memory_tool_message(operations_field="legacy-string-shape")
    +    actions = bg.summarize_background_review_actions(
    +        review_messages=msgs,
    +        prior_snapshot=[],
    +        notification_mode="verbose",
    +    )
    +    assert isinstance(actions, list)
    +
    +
    +def test_b_operations_as_none_treated_as_empty():
    +    """``operations = None`` (missing key, JSON null) is still safe."""
    +    _isolate_hermes_home()
    +    bg = _load_module()
    +    if bg is None:
    +        print("SKIP module not importable")
    +        return
    +
    +    msgs = _make_memory_tool_message(operations_field=None)
    +    actions = bg.summarize_background_review_actions(
    +        review_messages=msgs,
    +        prior_snapshot=[],
    +        notification_mode="verbose",
    +    )
    +    assert isinstance(actions, list)
    +
    +
    +# ---------------------------------------------------------------------------
    +# C. operations[i] as a non-dict (str / None)
    +# ---------------------------------------------------------------------------
    +
    +
    +def test_c_operations_contains_non_dict_entries():
    +    """A legacy/half-typed operations list with string entries short-circuits.
    +
    +    In ``verbose`` mode the function should produce the valid entries and
    +    silently skip the non-dict ones without ``AttributeError``. In
    +    non-verbose mode it falls back to a generic "Memory updated" string,
    +    so this test exercises the verbose branch where iteration over
    +    per-entry fields actually happens.
    +    """
    +    _isolate_hermes_home()
    +    bg = _load_module()
    +    if bg is None:
    +        print("SKIP module not importable")
    +        return
    +
    +    msgs = _make_memory_tool_message(
    +        operations_field=[
    +            "raw-string-no-fields",
    +            {"action": "add", "content": "valid entry"},
    +            None,
    +            {"action": "replace", "content": "another", "old_text": "thing"},
    +        ]
    +    )
    +    actions = bg.summarize_background_review_actions(
    +        review_messages=msgs,
    +        prior_snapshot=[],
    +        notification_mode="verbose",
    +    )
    +    assert isinstance(actions, list)
    +    # ``notification_mode='verbose'`` walks per-entry fields; the two
    +    # dict-shaped entries produce action lines, the string and None
    +    # entries are skipped via the isinstance guard. The exact wording is
    +    # not asserted (memory module shapes may vary) but at least one
    +    # action line must be present.
    +    assert len(actions) >= 1, f"expected at least one action line, got {actions!r}"
    +
    +
    +# ---------------------------------------------------------------------------
    +# D. detail comes back non-dict (None / stale value)
    +# ---------------------------------------------------------------------------
    +
    +
    +def test_d_detail_non_dict_replaced_with_empty():
    +    """When ``call_details.get(tcid)`` returns None, summarize must coerce
    +    it to ``{}`` rather than calling ``.get(...)`` on ``None``.
    +    """
    +    _isolate_hermes_home()
    +    bg = _load_module()
    +    if bg is None:
    +        print("SKIP module not importable")
    +        return
    +
    +    # Build a tool-only message whose tcid does NOT have an assistant tool_call.
    +    msgs = _make_skill_tool_message(change={})
    +    # Drop the assistant message so call_details is empty for tcid=call_1.
    +    msgs = [m for m in msgs if m.get("role") != "assistant"]
    +
    +    actions = bg.summarize_background_review_actions(
    +        review_messages=msgs,
    +        prior_snapshot=[],
    +        notification_mode="verbose",
    +    )
    +    assert isinstance(actions, list)
    +
    +
    +# ---------------------------------------------------------------------------
    +# E. Caller defends against summarize raising
    +# ---------------------------------------------------------------------------
    +
    +
    +def test_e_call_does_not_unwind_module_callables():
    +    """Structural: the new defensive try/except around the summarize
    +    call is in place. Caught here rather than via a partial mocking
    +    cascade because monkeypatching the AIAgent is too brittle for a
    +    blind regression test — keeping it text-anchored guards the
    +    ``_run_review_in_thread`` invariant without a real LLM.
    +    """
    +    src_path = os.path.join(REPO_ROOT, "agent", "background_review.py")
    +    src = open(src_path, encoding="utf-8").read()
    +    # The fix added: ``try: actions = summarize_background_review_actions(...)``
    +    # followed by ``except Exception as e: ... actions = []``.
    +    assert "actions = summarize_background_review_actions(" in src
    +    assert (
    +        "summarize_background_review_actions returned partial results"
    +        in src
    +    ), "expected partial-results guard message present"
    +    # And the prior-tonon-dict guard for the call_details lookup.
    +    assert "if not isinstance(detail, dict):" in src
    +    assert "if isinstance(ops_raw, list)" in src
    +    assert "if isinstance(change_raw, dict)" in src
    +
    +
    +# ---------------------------------------------------------------------------
    +# Runner
    +# ---------------------------------------------------------------------------
    +
    +
    +def main():
    +    runner = TestRunner()
    +    runner.run("a_change_as_list_does_not_crash", test_a_change_as_list_does_not_crash)
    +    runner.run("a_change_as_int_does_not_crash", test_a_change_as_int_does_not_crash)
    +    runner.run("b_operations_as_string_treated_as_empty", test_b_operations_as_string_treated_as_empty)
    +    runner.run("b_operations_as_none_treated_as_empty", test_b_operations_as_none_treated_as_empty)
    +    runner.run("c_operations_contains_non_dict_entries", test_c_operations_contains_non_dict_entries)
    +    runner.run("d_detail_non_dict_replaced_with_empty", test_d_detail_non_dict_replaced_with_empty)
    +    runner.run("e_call_defends_via_try_except", test_e_call_does_not_unwind_module_callables)
    +    return runner.summary()
    +
    +
    +if __name__ == "__main__":
    +    sys.exit(main())