From c7c687aa4bd30ea51d12f3b1a4bc8a65ab43ba1e Mon Sep 17 00:00:00 2001 From: webdevtodayjason Date: Wed, 12 Aug 2026 19:23:19 -0500 Subject: [PATCH] feat(plugins): rename hook to transform_api_error_classification per #64231 verdict Applies the batch-disposition SALVAGE conditions from #64231: the hook id moves to the taxonomy transform-family name, and run-all-then-pick-first dispatch now logs a runtime warning when a valid-but-losing classification is skipped (the #64714 skipped-transform rule). Chaining semantics are stated explicitly at the VALID_HOOKS entry, the dispatch helper docstring, and the hooks.md catalog row and detail section. --- agent/error_classifier.py | 4 +- hermes_cli/plugins.py | 41 ++++++++++++++----- tests/agent/test_shell_hooks.py | 4 +- ...ransform_api_error_classification_hook.py} | 39 ++++++++++++++++-- website/docs/user-guide/features/hooks.md | 8 ++-- 5 files changed, 74 insertions(+), 22 deletions(-) rename tests/{test_classify_api_error_hook.py => test_transform_api_error_classification_hook.py} (86%) diff --git a/agent/error_classifier.py b/agent/error_classifier.py index 63c3a80991..d28ddb0602 100644 --- a/agent/error_classifier.py +++ b/agent/error_classifier.py @@ -648,7 +648,7 @@ def classify_api_error( """Classify an API error into a structured recovery recommendation. Priority-ordered pipeline: - 0. Plugin ``classify_api_error`` hooks (first valid result wins) + 0. Plugin ``transform_api_error_classification`` hooks (first valid result wins) 1. Special-case provider-specific patterns (thinking sigs, tier gates) 2. HTTP status code + message-aware refinement 3. Error code classification (from body) @@ -736,7 +736,7 @@ def classify_api_error( # # Consulted BEFORE the built-in pipeline so a provider plugin can both # add classifications the core patterns miss and correct ones they get - # wrong for its provider (see the ``classify_api_error`` entry in + # wrong for its provider (see the ``transform_api_error_classification`` entry in # hermes_cli.plugins.VALID_HOOKS for the callback contract). Callback # exceptions are isolated inside invoke_hook and malformed returns are # dropped by the helper, so a broken plugin can never break diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index 45c43f322e..00f0f25e7b 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -198,14 +198,16 @@ VALID_HOOKS: Set[str] = { # Dispatch is run-all-then-pick-first: every registered callback runs # with its failures isolated (an early answer never stops later # callbacks), then the first valid result in registration order wins — - # on conflict the first-registered plugin is the tie-break. Invalid - # dicts and unknown reasons are skipped; a broken plugin can never - # break error classification. Cold path: fires only on API failure. + # on conflict the first-registered plugin is the tie-break, and every + # additional valid-but-losing result is reported with a runtime warning + # (the #64714 skipped-transform rule). Invalid dicts and unknown + # reasons are skipped; a broken plugin can never break error + # classification. Cold path: fires only on API failure. # Privacy: error_message/error_body may carry an unredacted provider # error dump. - # Contract: the first-valid-wins mutating shape in + # Contract: the transform-family first-valid-wins shape in # docs/plugins/hook-taxonomy.md. - "classify_api_error", + "transform_api_error_classification", "on_session_start", "on_session_end", "on_session_finalize", @@ -385,7 +387,7 @@ VALID_HOOKS: Set[str] = { # have its output silently ignored — registration is refused loudly instead. # Support for a shell response shape can lift an event out of this set. SHELL_UNSUPPORTED_HOOKS: Set[str] = { - "classify_api_error", + "transform_api_error_classification", } ENTRY_POINTS_GROUP = "hermes_agent.plugins" @@ -5814,7 +5816,7 @@ def get_plugin_error_classification( context_length: int = 0, num_messages: int = 0, ) -> Optional[Dict[str, Any]]: - """Check ``classify_api_error`` hooks for a classification directive. + """Check ``transform_api_error_classification`` hooks for a directive. Consulted by :func:`agent.error_classifier.classify_api_error` BEFORE its built-in pipeline, so a provider plugin can both add classifications @@ -5828,13 +5830,17 @@ def get_plugin_error_classification( wins in registration order — mirroring :func:`get_pre_tool_call_block_message`, invalid or irrelevant returns are silently ignored so a misbehaving plugin degrades to a no-op. + When more than one callback returns a valid classification, the losing + results are skipped with a runtime warning (the #64714 + skipped-transform rule) so conflicting provider plugins are visible in + logs instead of silently shadowed. Privacy: ``error_message`` and ``error_body`` may carry an unredacted provider error dump; callbacks must not log or forward them without redaction. Cold path: fires only on API failure, never on the request hot path. - Contract: the first-valid-wins mutating shape in + Contract: the transform-family first-valid-wins shape in ``docs/plugins/hook-taxonomy.md``. Returns a sanitized dict (``reason`` coerced to ``FailoverReason``, hint @@ -5843,7 +5849,7 @@ def get_plugin_error_classification( from agent.error_classifier import FailoverReason hook_results = invoke_hook( - "classify_api_error", + "transform_api_error_classification", provider=provider, model=model, status_code=status_code, @@ -5857,6 +5863,8 @@ def get_plugin_error_classification( num_messages=num_messages, ) + winner: Optional[Dict[str, Any]] = None + skipped_valid = 0 for result in hook_results: if not isinstance(result, dict): continue @@ -5871,6 +5879,10 @@ def get_plugin_error_classification( else: continue + if winner is not None: + skipped_valid += 1 + continue + out: Dict[str, Any] = {"reason": reason} for key in ( "retryable", @@ -5886,9 +5898,16 @@ def get_plugin_error_classification( error_context = result.get("error_context") if isinstance(error_context, dict): out["error_context"] = error_context - return out + winner = out - return None + if winner is not None and skipped_valid: + logger.warning( + "transform_api_error_classification: skipped %d valid " + "classification(s) after the first result in registration order " + "won (run-all-then-pick-first)", + skipped_valid, + ) + return winner def _ensure_plugins_discovered(force: bool = False) -> PluginManager: diff --git a/tests/agent/test_shell_hooks.py b/tests/agent/test_shell_hooks.py index 9eb1a4fac9..95934ef2e7 100644 --- a/tests/agent/test_shell_hooks.py +++ b/tests/agent/test_shell_hooks.py @@ -271,11 +271,11 @@ class TestParseHooksBlock: def test_python_only_event_refused(self, caplog): - # classify_api_error returns a classification directive that + # transform_api_error_classification returns a classification directive that # _parse_response has no channel for — a shell registration would # be silently ignored, so it must be refused with a warning. specs = shell_hooks._parse_hooks_block({ - "classify_api_error": [ + "transform_api_error_classification": [ {"command": "/tmp/hook.sh"}, ], }) diff --git a/tests/test_classify_api_error_hook.py b/tests/test_transform_api_error_classification_hook.py similarity index 86% rename from tests/test_classify_api_error_hook.py rename to tests/test_transform_api_error_classification_hook.py index 3c410f1b00..09dcde1a15 100644 --- a/tests/test_classify_api_error_hook.py +++ b/tests/test_transform_api_error_classification_hook.py @@ -1,4 +1,4 @@ -"""Tests for the ``classify_api_error`` plugin hook. +"""Tests for the ``transform_api_error_classification`` plugin hook. Covers the seam in ``agent.error_classifier.classify_api_error`` (step 0, consulted before the built-in pipeline) and the sanitization contract of @@ -15,6 +15,7 @@ consuming module, because the import happens at call time. """ import importlib.util +import logging import hermes_cli.plugins as plugins_mod from agent.error_classifier import FailoverReason, classify_api_error @@ -154,6 +155,38 @@ def test_first_valid_result_wins(monkeypatch): assert result.reason == FailoverReason.billing +def test_skipped_valid_results_log_runtime_warning(monkeypatch, caplog): + # The #64714 skipped-transform rule: a valid-but-losing classification + # must surface in logs, never be silently shadowed. Invalid results + # (here "bogus") are not "skipped valid" and must not count. + monkeypatch.setattr( + plugins_mod, "invoke_hook", + lambda name, **kw: [ + {"reason": "bogus"}, + {"reason": "billing"}, + {"reason": "rate_limit"}, + ], + ) + + with caplog.at_level(logging.WARNING, logger=plugins_mod.logger.name): + result = _classify_unclaimed_error() + assert result.reason == FailoverReason.billing + warnings = [r.getMessage() for r in caplog.records if "skipped" in r.getMessage()] + assert len(warnings) == 1 + assert "skipped 1 valid" in warnings[0] + + # A lone winner is not a conflict: no warning. + caplog.clear() + monkeypatch.setattr( + plugins_mod, "invoke_hook", + lambda name, **kw: [{"reason": "billing"}], + ) + with caplog.at_level(logging.WARNING, logger=plugins_mod.logger.name): + result = _classify_unclaimed_error() + assert result.reason == FailoverReason.billing + assert not [r for r in caplog.records if "skipped" in r.getMessage()] + + def test_helper_exception_never_breaks_classification(monkeypatch): def _boom(**kwargs): raise RuntimeError("plugin infrastructure exploded") @@ -179,7 +212,7 @@ def test_hook_receives_parsed_error_context(monkeypatch): _classify_unclaimed_error(approx_tokens=1234, num_messages=7) - assert seen["hook_name"] == "classify_api_error" + assert seen["hook_name"] == "transform_api_error_classification" assert seen["provider"] == "acmecloud" assert seen["model"] == "acme/large-1" assert seen["status_code"] is None @@ -219,7 +252,7 @@ def classify(provider=None, error_message=None, **kwargs): def register(ctx): - ctx.register_hook("classify_api_error", classify) + ctx.register_hook("transform_api_error_classification", classify) ''' diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index ff3a67373c..ecee44c676 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -453,7 +453,7 @@ Payload fields below are the exact event-specific fields supplied by each call s | `on_stream_delta` | Observer | Dispatched per normalized streaming text delta via the bounded observer queue; a stalled callback drops only its own oldest events; return ignored. | `delta`, `kind` (`text` or `reasoning`), `turn_id`, `iteration`, `session_id`, `model`, `provider`, `surface` | Delta text is raw model output; reasoning deltas require the `plugins.stream_reasoning_deltas` opt-in. | | `on_stream_end` | Observer | Dispatched when a streaming response finishes or errors, after the stream closes; return ignored. | `final_text`, `finished`, `error`, `turn_id`, `iteration`, `session_id`, `model`, `provider`, `surface` | Full assembled response text; error text may include provider data. | | `on_interim_message` | Observer | Dispatched when a mid-loop assistant message is surfaced before the final answer (streaming or non-streaming); return ignored. | `text`, `already_streamed`, `turn_id`, `iteration`, `session_id`, `model`, `provider`, `surface` | Full interim assistant text. | -| `classify_api_error` | Directive/control | On each failed provider attempt, at the top of the built-in classifier; all callbacks run, then the first dict with a valid `reason` wins (run-all-then-pick-first). Python plugins only. | `provider`, `model`, `status_code`, `error_type`, `error_code`, `error_message`, `error_body`, `error`, `approx_tokens`, `context_length`, `num_messages` | `error_message` and `error_body` may contain raw provider/user data. | +| `transform_api_error_classification` | Transform | On each failed provider attempt, at the top of the built-in classifier; all callbacks run, then the first dict with a valid `reason` wins (run-all-then-pick-first), and skipped valid results log a runtime warning. Python plugins only. | `provider`, `model`, `status_code`, `error_type`, `error_code`, `error_message`, `error_body`, `error`, `approx_tokens`, `context_length`, `num_messages` | `error_message` and `error_body` may contain raw provider/user data. | | `on_session_start` | Observer | First turn of a new session; return ignored. | `session_id`, `model`, `platform` | Identifiers and routing metadata only. | | `on_session_end` | Observer | Canonically at each turn finalization; CLI/TUI exits have additional reduced legacy shapes. Return ignored. | Canonical: `session_id`, `task_id`, `turn_id`, `completed`, `failed`, `interrupted`, `turn_exit_reason`, `model`, `platform`; exit paths may add `reason`/`api_request_id` and omit fields. | IDs, model/platform, and outcome; canonical payload has no message body. | | `on_session_finalize` | Observer | CLI/TUI/gateway teardown through `finalize_session`; gateway shutdown or expiry may finalize without a reset. Return ignored. | Surface-dependent `session_id`, `platform`, optionally `reason`, `old_session_id`, `new_session_id` | Session and routing identifiers. | @@ -847,11 +847,11 @@ For standing guidance that should shape the built-in missing-evidence nudge, use --- -### `classify_api_error` +### `transform_api_error_classification` Fires **once per failed API call**, at the top of `agent/error_classifier.classify_api_error()` — BEFORE the built-in classification pipeline. Cold path: it never fires on a successful call. Provider plugins use it to own their provider's error quirks (a vendor-specific 404 that should fast-fallback, a misleading status code) without core patches. -This hook is **behavior-changing**: the returned classification drives retry, compression, credential-rotation, and fallback routing for the failed call. +This hook is **behavior-changing** (transform family): the returned classification drives retry, compression, credential-rotation, and fallback routing for the failed call. **Callback signature:** @@ -889,7 +889,7 @@ return { } ``` -Return `None` (or nothing) to decline and defer to the built-in pipeline. Dispatch is **run-all-then-pick-first**: every registered callback runs on each failed call (an earlier answer never stops later callbacks), each callback's failure is isolated, and the first valid result **in registration order** wins — if two plugins can both answer, the first-registered one is the tie-break. Invalid dicts and unknown reasons are skipped, so a broken plugin can never break error classification. +Return `None` (or nothing) to decline and defer to the built-in pipeline. Dispatch is **run-all-then-pick-first**: every registered callback runs on each failed call (an earlier answer never stops later callbacks), each callback's failure is isolated, and the first valid result **in registration order** wins — if two plugins can both answer, the first-registered one is the tie-break, and every valid-but-losing result is reported with a runtime warning so a shadowed provider plugin is visible in logs. Invalid dicts and unknown reasons are skipped, so a broken plugin can never break error classification. **Privacy:** `error_message` and `error_body` may carry an unredacted provider error dump. Do not log or forward them from a callback without redaction.