mirrors/hermes-agent
1
0
Fork 0
mirror of https://github.com/NousResearch/hermes-agent.git synced 2026-07-08 13:12:08 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 6

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

Browse source 
`summarize_background_review_actions` was structured on the assumption
that every parsed tool response is a fully-typed dict-of-fields. In
practice the memory/skill tools — and their wrappers over Mem0 OSS and
the skill_manage MCP server — sometimes serialize `_change` as a list
or scalar, and clamp `operations` to a single string when the field
came in via a partial JSON bridge.

The original code did the equivalent of:
    change = data.get("_change", {})
    change.get("description", "")

so when `_change` was a list the inner .get crashed with
`AttributeError: 'list' object has no attribute 'get'`, every ~10
turns the user saw the entire background review collapse.

Three defensive guards in summarize_background_review_actions:

- `call_details.get(tcid, {})` → `call_details.get(tcid) or {}` plus
  `isinstance(detail, dict)` coercion. Catches stale scalar/None
  values when a fork inherits partial state from a stale tool_call_id.
- `operations = detail.get("operations") or []` → `isinstance(ops_raw, list)`
  coerce, then per-entry isinstance check before `.get()`. Skips
  non-dict items without raising; an entire surrounding review no
  longer goes down because one entry was malformed.
- `change = data.get("_change", {})` → `isinstance(change_raw, dict)`
  coerce. The originally-reported crash class for skill_manage with
  list-shaped _change now falls through to the generic summary path.

And the caller in `_run_review_in_thread` is wrapped in a try/except
that maps any residual summarize exception to `actions = []` and
emits a 'partial results' warning, so even an entirely unanticipated
shape won't take down the outer review — the user only sees
'Background memory/skill review failed' instead of the prior hard
crash that lost every successful action the fork had completed.

Tests: tests/test_background_review_list_shapes.py — standalone
pytest-free runner, 7/7 PASS:
  a_change_as_list_does_not_crash       (originally-reported shape)
  a_change_as_int_does_not_crash        (scalar fallback)
  b_operations_as_string_treated_as_empty
  b_operations_as_none_treated_as_empty
  c_operations_contains_non_dict_entries (verbose-mode per-entry filter)
  d_detail_non_dict_replaced_with_empty
  e_call_defends_via_try_except         (structural anchor)

Refs NousResearch/hermes-agent#59437
This commit is contained in:
Ahmett101 2026-07-06 10:15:29 +03:00 • committed by Teknium
parent 4a80e27bba
commit 2e828d4b75
2 changed files with 431 additions and 11 deletions
Download patch file Download diff file Expand all files Collapse all files

75
agent/background_review.py
View file

@ -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 _run_review_in_thread(
# 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))

367
tests/test_background_review_list_shapes.py Normal file
View file

@ -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())