diff --git a/emrg/server/daemon.py b/emrg/server/daemon.py index 526eb20d..ec791868 100644 --- a/emrg/server/daemon.py +++ b/emrg/server/daemon.py @@ -64,6 +64,7 @@ CONTEXT_TOO_LONG, EMPTY_ANSWER_ERROR, LlmClient, + carries_provider_reason, classify_llm_error, is_overlong_error, with_content_risk_hint, @@ -4888,10 +4889,16 @@ async def _run_tool_loop( # host after the wrong thing. The refusal travels as its own # sentence, which names what happened and which retries were # already spent (rant 2026-09-28T16:58:08, requirement D). + # + # A refusal that carries the provider's own sentence is the same + # shape and gets the same treatment: the advice is *one* remedy, + # and it is the right one only while nobody has said what failed. + # A balance refusal printed as "check your config" sends the host + # to edit a file that was never wrong — the account was empty. The + # question is asked of `llm.py` (`carries_provider_reason`), never + # re-derived here from the message's spelling. error_text = str(e) - if CONTENT_FILTER_ERROR not in error_text: - error_text = f"LLM error: {e}. Check config at ~/.emrg/config.toml" - else: + if CONTENT_FILTER_ERROR in error_text: # The ladder being spent is one abort, and *a run* of them is # the fact worth reporting (rant 2026-09-28T15:57:31, # requirement L3): the generic `logger.exception` above names @@ -4908,6 +4915,11 @@ async def _run_tool_loop( round_num, ), ) + elif carries_provider_reason(e): + # The provider said why: that sentence is the whole remedy. + error_text = f"LLM error: {e}" + else: + error_text = f"LLM error: {e}. Check config at ~/.emrg/config.toml" await self._broadcast(session.session_id, { "error": error_text, }) diff --git a/emrg/server/llm.py b/emrg/server/llm.py index 680522af..73885e41 100644 --- a/emrg/server/llm.py +++ b/emrg/server/llm.py @@ -202,6 +202,41 @@ def provider_error_message(body) -> str: return _redact_text(message.strip()) +#: The attribute a refusal carries its provider's sentence on. Set where the +#: sentence is extracted, read by :func:`carries_provider_reason` — one writer, +#: one reader, so a caller never re-derives it from the exception's text. +_PROVIDER_REASON_ATTR = "provider_reason" + + +def _with_provider_reason(exc: BaseException, reason: str) -> BaseException: + """Return ``exc`` marked with the provider's sentence it was built from.""" + setattr(exc, _PROVIDER_REASON_ATTR, reason) + return exc + + +def carries_provider_reason(exc: BaseException) -> bool: + """Whether the provider itself said why it refused — so no generic remedy is owed. + + A refusal that arrives with a sentence is answered: ``{"error": {"message": + "点数余额不足"}}`` tells a host to top up, and "invalid api key" tells it to + replace the key. Appending one generic remedy to every failure is the defect + this answers — a balance refusal was reported as a configuration problem, + sending the host to edit a file that was never wrong (a 402 read as "check + your config", when the config was fine and the account was empty). + + The answer is a *fact about the failure*, not a policy: it says the cause is + already written down. What to print instead is the caller's, and the daemon + prints the provider's sentence alone. + + Read through :data:`_PROVIDER_REASON_ATTR` rather than off the message text, + because the message is the one thing that changes spelling when a provider + rewords itself: the mark is set by the raise site that extracted the + sentence, at both the streaming and the non-streaming path, and an error + raised anywhere else answers ``False`` and keeps the generic remedy. + """ + return bool(getattr(exc, _PROVIDER_REASON_ATTR, "")) + + def is_overlong_error(exc: BaseException) -> bool: """Whether a failure is "the request did not fit" — asked of :func:`classify_llm_error`, never respelled at the call site. @@ -705,10 +740,13 @@ async def chat( # step later. When there is no reason the lines below are reachable # exactly as before, escapes and truncation included. logger.error("LLM error: %s %s", resp.status_code, reason) - raise RuntimeError( - with_content_risk_hint( - f"LLM request failed: {resp.status_code} — {reason}" - ) + raise _with_provider_reason( + RuntimeError( + with_content_risk_hint( + f"LLM request failed: {resp.status_code} — {reason}" + ) + ), + reason, ) hdr = _redact_headers(dict(resp.headers)) text_redacted = _redact_text(text) @@ -843,10 +881,13 @@ async def chat_stream( # The same extraction answers both paths, so they cannot # drift apart. logger.error("LLM stream error: %s %s", resp.status_code, reason) - raise RuntimeError( - with_content_risk_hint( - f"LLM stream request failed: {resp.status_code} — {reason}" - ) + raise _with_provider_reason( + RuntimeError( + with_content_risk_hint( + f"LLM stream request failed: {resp.status_code} — {reason}" + ) + ), + reason, ) logger.error("LLM stream error: %s %s", resp.status_code, _redact_text(text)[:500]) hdr = _redact_headers(dict(resp.headers)) diff --git a/tests/test_daemon.py b/tests/test_daemon.py index 86bac219..8a1a8b23 100644 --- a/tests/test_daemon.py +++ b/tests/test_daemon.py @@ -4286,6 +4286,57 @@ async def stream(messages, tools=None): ) +def test_a_refusal_that_states_its_reason_is_not_sent_to_the_config_file( + tmp_path, monkeypatch +): + """A balance refusal must not carry "check your config". + + The advice is one remedy for every failure, and it is the right one only + while nobody has said what went wrong. A 402 whose body says the account is + empty is answered; printing "Check config at ~/.emrg/config.toml" beside it + sends the host to edit a file that was never wrong. + """ + from emrg.server.llm import _with_provider_reason + + async def stream(messages, tools=None): + raise _with_provider_reason( + RuntimeError("LLM stream request failed: 402 — the account is empty"), + "the account is empty", + ) + yield # pragma: no cover - unreachable; makes this an async generator + + _, frames = _drive_tool_loop_with_stream(tmp_path, monkeypatch, stream) + + errors = [f for f in frames if "error" in f] + assert len(errors) == 1, frames + assert "the account is empty" in errors[0]["error"], errors[0]["error"] + assert "Check config" not in errors[0]["error"], ( + "the host was sent to the config file for a refusal that said why" + ) + + +def test_a_refusal_without_a_reason_keeps_the_config_pointer(tmp_path, monkeypatch): + """The other direction: the advice is not removed wholesale. + + A failure that says nothing about its cause is exactly when the host needs + to be told where the configuration lives, so this arm must stay green while + the one above changes. + """ + async def stream(messages, tools=None): + raise RuntimeError( + "LLM stream request failed: 500 headers={} body=an opaque failure" + ) + yield # pragma: no cover - unreachable; makes this an async generator + + _, frames = _drive_tool_loop_with_stream(tmp_path, monkeypatch, stream) + + errors = [f for f in frames if "error" in f] + assert len(errors) == 1, frames + assert "Check config at ~/.emrg/config.toml" in errors[0]["error"], ( + f"a refusal that named no cause lost its one pointer: {errors[0]['error']!r}" + ) + + def test_an_exhausted_content_filter_ladder_reports_once_with_one_terminal_frame( tmp_path, monkeypatch ): diff --git a/tests/test_llm.py b/tests/test_llm.py index 83fb926b..293c50bb 100644 --- a/tests/test_llm.py +++ b/tests/test_llm.py @@ -25,6 +25,7 @@ CONTEXT_TOO_LONG, OTHER_ERROR, LlmClient, + carries_provider_reason, classify_llm_error, content_filter_retry_messages, content_risk_retry, @@ -1562,8 +1563,8 @@ async def _run(): _REFUSING_HEADERS = {"content-length": "42"} -def _refusal_from(client, path: str, body: bytes, status: int = 402) -> str: - """The message that path raises for a refused response.""" +def _refusal_exception_from(client, path: str, body: bytes, status: int = 402) -> BaseException: + """The exception that path raises for a refused response.""" import asyncio if path == "chat": @@ -1574,7 +1575,12 @@ def _refusal_from(client, path: str, body: bytes, status: int = 402) -> str: client._client = _FakeStreamClient([_RefusingStream(body, status, _REFUSING_HEADERS)]) with pytest.raises(RuntimeError) as caught: asyncio.run(_drain_stream(client, client._client)) - return str(caught.value) + return caught.value + + +def _refusal_from(client, path: str, body: bytes, status: int = 402) -> str: + """The message that path raises for a refused response.""" + return str(_refusal_exception_from(client, path, body, status)) @pytest.mark.parametrize("path", ["chat", "chat_stream"]) @@ -1604,6 +1610,38 @@ def test_a_refusal_reports_the_providers_own_sentence(client, caplog, path): ) +@pytest.mark.parametrize("path", ["chat", "chat_stream"]) +def test_a_sentence_marks_the_refusal_as_self_explaining(client, path): + """The sentence *is* the remedy, so a caller has to be able to tell it exists. + + Both raise sites mark it: the tool loop takes the streaming path while the + summariser takes the other, and the question is asked once for either. + """ + exc = _refusal_exception_from(client, path, _SENTENCE_BODY) + + assert carries_provider_reason(exc) is True, ( + f"{path}: a refusal carrying the provider's own sentence is not marked as " + f"explaining itself, so a generic remedy will be appended to it" + ) + + +@pytest.mark.parametrize("path", ["chat", "chat_stream"]) +def test_a_body_without_a_sentence_is_not_marked(client, path): + """The other direction: nothing was extracted, so the generic pointer is still owed.""" + exc = _refusal_exception_from(client, path, _OPAQUE_BODY) + + assert carries_provider_reason(exc) is False, ( + f"{path}: a refusal that says nothing about its cause was marked as " + f"self-explaining, and the host loses the one pointer it had" + ) + + +def test_an_error_raised_outside_a_refusal_is_not_self_explaining(): + """Anything that did not come from an extracted sentence answers False.""" + assert carries_provider_reason(RuntimeError("boom")) is False + assert carries_provider_reason(ValueError("boom")) is False + + @pytest.mark.parametrize("path", ["chat", "chat_stream"]) def test_a_body_without_a_sentence_keeps_todays_message_exactly(client, caplog, path): """The other half: with nothing to extract, nothing about the output moves.