From 7365ab4f0adf69ff3464adce9220d8e9edb08b7b Mon Sep 17 00:00:00 2001 From: finn763 <165816600+finn763@users.noreply.github.com> Date: Tue, 15 Sep 2026 14:49:59 +0800 Subject: [PATCH] fix: name the real cause in three misleading errors + CONTRIBUTING convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave #111128 (error messages name the proximate symptom instead of the cause). Three user-facing messages reported the wrong subsystem at the failure site: - whatsapp: a half-removed aiohttp stays importable (namespace shell, no ClientSession), so every health probe raised AttributeError inside a blanket except and connect() blamed the Node bridge with "Bridge HTTP server did not start in 15s" forever. Preflight now names the broken dependency and the fix (pip install --force-reinstall aiohttp) (#71308). - agent: the repeated-compaction warning claimed "accuracy may degrade" even for engines whose compaction keeps turns retrievable (LCM-style). Engines can declare ContextEngine.lossless_compaction = True and the message then says what actually happened (#53000). - cli: resuming a session with an empty transcript printed "found but has no messages. Starting fresh." — on the preload path it actually aborted with exit 1. Both paths now name the empty transcript and the way out (hermes sessions delete ) (#27168). CONTRIBUTING.md gains the convention: every user-facing error names the actual cause plus the remediation step, never the proximate symptom; regression test on the touched path. Tests: assertions added/updated on all three paths. --- CONTRIBUTING.md | 17 +++++ agent/context_engine.py | 4 ++ agent/conversation_compression.py | 16 ++++- hermes_cli/cli_agent_setup_mixin.py | 15 ++-- plugins/platforms/whatsapp/adapter.py | 29 ++++++++ tests/agent/test_compression_count_warning.py | 48 +++++++++++++ tests/gateway/test_whatsapp_connect.py | 70 +++++++++++++++++++ tests/hermes_cli/test_resume_quiet_stderr.py | 4 ++ 8 files changed, 196 insertions(+), 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0e6761ee18a4..ce38b25b411a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -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 diff --git a/agent/context_engine.py b/agent/context_engine.py index a052021b2783..40096b890a83 100644 --- a/agent/context_engine.py +++ b/agent/context_engine.py @@ -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: diff --git a/agent/conversation_compression.py b/agent/conversation_compression.py index 2cf2eaec0a7a..3ae26d875a21 100644 --- a/agent/conversation_compression.py +++ b/agent/conversation_compression.py @@ -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) diff --git a/hermes_cli/cli_agent_setup_mixin.py b/hermes_cli/cli_agent_setup_mixin.py index 94adf5d4c4e0..9578b882b109 100644 --- a/hermes_cli/cli_agent_setup_mixin.py +++ b/hermes_cli/cli_agent_setup_mixin.py @@ -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 @@ -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 diff --git a/plugins/platforms/whatsapp/adapter.py b/plugins/platforms/whatsapp/adapter.py index 21d97ed9612e..c18c72cf15ae 100644 --- a/plugins/platforms/whatsapp/adapter.py +++ b/plugins/platforms/whatsapp/adapter.py @@ -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 = [] @@ -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 = ( diff --git a/tests/agent/test_compression_count_warning.py b/tests/agent/test_compression_count_warning.py index dc8ebc93a9fe..e93f26c3ecb0 100644 --- a/tests/agent/test_compression_count_warning.py +++ b/tests/agent/test_compression_count_warning.py @@ -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 diff --git a/tests/gateway/test_whatsapp_connect.py b/tests/gateway/test_whatsapp_connect.py index 0e3bfe28e042..49e5772b17ec 100644 --- a/tests/gateway/test_whatsapp_connect.py +++ b/tests/gateway/test_whatsapp_connect.py @@ -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 diff --git a/tests/hermes_cli/test_resume_quiet_stderr.py b/tests/hermes_cli/test_resume_quiet_stderr.py index df82ce4dda95..3ac9333f9303 100644 --- a/tests/hermes_cli/test_resume_quiet_stderr.py +++ b/tests/hermes_cli/test_resume_quiet_stderr.py @@ -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