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.
This commit is contained in:
webdevtodayjason
committed by
Teknium
Teknium
parent
e9a29b9bda
commit
c7c687aa4b
@@ -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
|
||||
|
||||
+30
-11
@@ -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:
|
||||
|
||||
@@ -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"},
|
||||
],
|
||||
})
|
||||
|
||||
+36
-3
@@ -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)
|
||||
'''
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user