Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -923,6 +923,23 @@ After the [litellm supply chain compromise](https://github.com/BerriAI/litellm/i

---

## Error Messages: Name the Cause, Not the Symptom

Every user-facing error must name the **actual cause** and the **next step the reader can take** —
never the proximate symptom that happens to be observable at the failure site. A message that
reports the wrong cause costs more than a vague one: it sends users (and agents) to debug the wrong
subsystem, and the "fix" they land on is often a second bug. So prefer *"no OpenRouter API key is
configured — set `OPENROUTER_API_KEY` or run `hermes providers`"* over *"payment / credit error"*;
*"aiohttp is installed but unusable (no `ClientSession`) — reinstall with
`pip install --force-reinstall aiohttp`"* over *"Bridge HTTP server did not start in 15s"*. If the
cause genuinely cannot be determined at that point, say what was tried and where the detail lives
(log path, `--verbose`), and include the remediation step; a bare "failed" is not acceptable. When a
guard or fallback suppresses an exception, the suppressed reason is exactly what the final message
must carry. Add a regression test asserting the new wording on the touched path — strings plus their
test, nothing more.

---

## Pull Request Process

### Branch naming
Expand Down
4 changes: 4 additions & 0 deletions agent/context_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ def name(self) -> str:
# False keeps successful automatic compaction passes silent (routine background
# maintenance); warnings, errors and manual /compress still surface.
emit_automatic_compaction_status: bool = True
# True for engines whose compaction stays retrievable (originals kept in the store, e.g. LCM):
# the repeated-compaction warning must then not claim accuracy loss — that names a cause the
# engine does not have (#53000).
lossless_compaction: bool = False

@abstractmethod
def update_from_response(self, usage: Dict[str, Any]) -> None:
Expand Down
16 changes: 13 additions & 3 deletions agent/conversation_compression.py
Original file line number Diff line number Diff line change
Expand Up @@ -3104,9 +3104,19 @@ def _finish_compaction_boundary(
compressor = agent.context_compressor
_cc = compressor.compression_count
if _cc >= 2:
_cc_msg = (
f"{agent.log_prefix}⚠️ Session compressed {_cc} times — accuracy may degrade. Consider /new to start fresh."
)
# Engines that keep compacted turns retrievable (LCM-style) declare
# ``lossless_compaction`` on the class; "accuracy may degrade" is then a false cause and
# sends users to /new for nothing (#53000). ``type()`` keeps a mock compressor on the
# default (lossy) wording instead of a truthy auto-attribute.
if bool(getattr(type(compressor), "lossless_compaction", False)):
_cc_msg = (
f"{agent.log_prefix}ℹ️ Session compacted {_cc} times — this engine keeps compacted "
"turns retrievable, so nothing was lost. /new only if you want a clean slate."
)
else:
_cc_msg = (
f"{agent.log_prefix}⚠️ Session compressed {_cc} times — accuracy may degrade. Consider /new to start fresh."
)
agent._compression_warning = _cc_msg
agent._emit_status(_cc_msg)

Expand Down
15 changes: 11 additions & 4 deletions hermes_cli/cli_agent_setup_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -487,8 +487,13 @@ def _say(plain: str, rich: str) -> None:
self._restore_session_state(session_meta, quiet=_quiet_mode)
else:
_say(
f"Session {self.session_id} found but has no messages. Starting fresh.",
f"[bold {_accent_hex()}]Session {_escape(self.session_id)} found but has no messages. Starting fresh.[/]",
f"Session {self.session_id} exists but has no messages to resume — its transcript is "
f"empty (nothing was committed to it, or the messages were cleared). Starting fresh "
f"with this id; delete the empty one with: hermes sessions delete {self.session_id}",
f"[bold {_accent_hex()}]Session {_escape(self.session_id)} exists but has no messages "
f"to resume — its transcript is empty (nothing was committed to it, or the messages "
f"were cleared). Starting fresh with this id; delete the empty one with: "
f"hermes sessions delete {_escape(self.session_id)}[/]",
)
self._reopen_session()
return True
Expand Down Expand Up @@ -659,8 +664,10 @@ def _preload_resumed_session(self) -> bool:
accent_color = _accent_hex()
if not restored:
self._console_print(
f"[{accent_color}]Session {self.session_id} found but has no "
f"messages. Starting fresh.[/]")
f"[{accent_color}]Session {self.session_id} exists but its transcript is empty "
f"(no messages were ever committed, or they were cleared) — there is nothing to "
f"resume. Pick another session with `hermes sessions list`, or delete this one "
f"with `hermes sessions delete {self.session_id}`.[/]")
return False
restored = [m for m in restored if m.get("role") != "session_meta"]
self.conversation_history = restored
Expand Down
29 changes: 29 additions & 0 deletions plugins/platforms/whatsapp/adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,28 @@ def _wenv(name: str, default: str = "") -> str:
_RUN_TEXT = dict(capture_output=True, text=True, encoding='utf-8', errors='replace', stdin=subprocess.DEVNULL)


def _aiohttp_import_problem() -> Optional[str]:
"""Cause + remediation when ``aiohttp`` cannot serve a request at all, else ``None``.

A half-removed install (a pip reinstall overlapping a running gateway on Windows) leaves an
importable namespace shell: ``import aiohttp`` still succeeds while ``ClientSession`` is gone.
The poll loop's blanket ``except Exception`` swallowed that AttributeError, so every connect
attempt blamed the Node bridge ("Bridge HTTP server did not start in 15s") while the bridge was
healthy and answering /health in milliseconds (#71308).
"""
try:
import aiohttp
except ImportError:
return ("aiohttp is not installed, so the adapter cannot talk to the bridge at all. "
"Install it: pip install aiohttp")
if not hasattr(aiohttp, "ClientSession"):
where = getattr(aiohttp, "__file__", None) or "(namespace package: no __init__.py on disk)"
return (f"aiohttp is installed but unusable: {where} has no ClientSession — a half-removed or "
"partially overwritten install. The bridge was never reached. Reinstall it: "
"pip install --force-reinstall aiohttp")
return None


def _listener_pids_on_port(port: int) -> list:
"""PIDs *listening* on ``port`` (POSIX), never clients — a bare ``lsof -i :PORT`` once killed the user's browser."""
pids: list = []
Expand Down Expand Up @@ -449,6 +471,13 @@ async def _wait_for_bridge(self) -> bool:

def _preflight(self) -> bool:
"""Node + bridge script + creds.json present, else a non-retryable fatal error (an unpaired bridge only prints QR codes; retries would pay 30s each)."""
# Checked first: a broken aiohttp makes every probe fail, and the poll loop would report it
# as a bridge-start timeout — the wrong subsystem entirely (#71308).
aiohttp_problem = _aiohttp_import_problem()
if aiohttp_problem:
logger.warning("[%s] %s", self.name, aiohttp_problem)
self._set_fatal_error("whatsapp_aiohttp_unusable", aiohttp_problem, retryable=False)
return False
bridge_path = Path(self._bridge_script)
creds_path = self._session_path / "creds.json"
checks = (
Expand Down
48 changes: 48 additions & 0 deletions tests/agent/test_compression_count_warning.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,51 @@ def test_no_warning_below_threshold(tmp_path: Path) -> None:
agent._compress_context(messages, "sys", approx_tokens=120_000)

assert not any("compressed" in m.lower() and "times" in m.lower() for m in emitted)


def test_lossless_engine_is_not_told_its_accuracy_degraded(tmp_path: Path) -> None:
"""#53000: an engine that keeps compacted turns retrievable (LCM-style) declares
``lossless_compaction``; the warning must then stop claiming accuracy loss — that
names a cause the engine does not have and sends users to /new for nothing.
"""
from agent.context_engine import ContextEngine

db = SessionDB(db_path=tmp_path / "state.db")
sid = "PARENT_53000"
db.create_session(sid, source="cli")

agent = _build_agent_with_db(db, sid, compression_count=2)

# A real engine object whose CLASS declares itself lossless (how a plugin ships it):
# the production code must read the declaration off the class, not the instance.
class _LosslessEngine:
lossless_compaction = True
compression_count = 2
last_prompt_tokens = 0
last_completion_tokens = 0
_last_summary_error = None
_last_compress_aborted = False
_last_aux_model_failure_model = None
_last_aux_model_failure_error = None

def compress(self, *args, **kwargs):
return [
{"role": "user", "content": "[CONTEXT COMPACTION] summary"},
{"role": "user", "content": "tail"},
]

agent.context_compressor = _LosslessEngine()

emitted: list[str] = []
agent._emit_status = lambda message: emitted.append(message)

messages = [{"role": "user", "content": f"m{i}"} for i in range(20)]
agent._compress_context(messages, "sys", approx_tokens=120_000)

assert any("compacted 2 times" in m.lower() for m in emitted), (
f"lossless engine still got the lossy warning: {emitted}"
)
assert not any("accuracy may degrade" in m.lower() for m in emitted)
assert "compacted 2 times" in (getattr(agent, "_compression_warning", "") or "").lower()
# Default stays lossy: the base engine class declares nothing.
assert ContextEngine.lossless_compaction is False
70 changes: 70 additions & 0 deletions tests/gateway/test_whatsapp_connect.py
Original file line number Diff line number Diff line change
Expand Up @@ -538,3 +538,73 @@ async def test_connect_proceeds_when_creds_present(self, tmp_path):
# but the fatal-error code is NOT the "not paired" one.
assert result is False
assert adapter._fatal_error_code != "whatsapp_not_paired"


# ---------------------------------------------------------------------------
# Pre-flight: an unusable aiohttp names itself, not the Node bridge (#71308)
# ---------------------------------------------------------------------------


class TestAiohttpPreflight:
"""A half-removed ``aiohttp`` stays importable (namespace shell without
``ClientSession``), so every health probe raises AttributeError inside a
blanket ``except`` and the adapter reported "Bridge HTTP server did not
start in 15s" — pointing the operator at the healthy Node bridge instead
of the broken Python dependency.
"""

@staticmethod
def _adapter():
adapter = _make_adapter()
adapter._write_runtime_status_safe = MagicMock()
return adapter

def test_preflight_names_broken_aiohttp_and_the_fix(self, monkeypatch):
import types
import sys

from plugins.platforms.whatsapp.adapter import _aiohttp_import_problem

shell = types.ModuleType("aiohttp") # importable, but no ClientSession
shell.__file__ = None # namespace package: no __init__.py on disk
monkeypatch.setitem(sys.modules, "aiohttp", shell)

problem = _aiohttp_import_problem()
assert problem and "aiohttp" in problem
assert "pip install --force-reinstall aiohttp" in problem

adapter = self._adapter()
assert adapter._preflight() is False
assert adapter._fatal_error_code == "whatsapp_aiohttp_unusable"
assert adapter._fatal_error_retryable is False
assert "Bridge HTTP server did not start" not in adapter._fatal_error_message
assert "namespace package" in adapter._fatal_error_message

def test_preflight_names_missing_aiohttp(self, monkeypatch):
import builtins
import sys

from plugins.platforms.whatsapp.adapter import _aiohttp_import_problem

monkeypatch.delitem(sys.modules, "aiohttp", raising=False)
real_import = builtins.__import__

def _no_aiohttp(name, *args, **kwargs):
if name == "aiohttp" or name.startswith("aiohttp."):
raise ImportError("No module named 'aiohttp'")
return real_import(name, *args, **kwargs)

monkeypatch.setattr(builtins, "__import__", _no_aiohttp)

problem = _aiohttp_import_problem()
assert problem and "not installed" in problem
assert "pip install aiohttp" in problem

adapter = self._adapter()
assert adapter._preflight() is False
assert adapter._fatal_error_code == "whatsapp_aiohttp_unusable"

def test_usable_aiohttp_reports_nothing(self):
from plugins.platforms.whatsapp.adapter import _aiohttp_import_problem

assert _aiohttp_import_problem() is None
4 changes: 4 additions & 0 deletions tests/hermes_cli/test_resume_quiet_stderr.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,8 @@ def test_no_messages_goes_to_stderr_in_quiet_mode(self, capsys):
captured = capsys.readouterr()
assert "has no messages" not in captured.out
assert "has no messages" in captured.err
# The line names the real state (empty transcript) and the way out (#27168):
# it used to say "Starting fresh." while the resume actually aborted.
assert "transcript is empty" in captured.err
assert "hermes sessions delete 20260524_111111_xyz" in captured.err
assert "Starting fresh" in captured.err