feat(plugins): transform_api_error_classification hook so provider plugins own their error quirks

[webdevtodayjason]

What

Responding to the lead dev's call for plugin-interface expansion ideas — this is the first concrete seam, shipped as code instead of a suggestion: a transform_api_error_classification plugin hook, consulted at the top of classify_api_error() before the built-in pipeline, so model-provider plugins can own their provider's error-classification quirks without core patches.

• New "transform_api_error_classification" entry in VALID_HOOKS (contract documented inline, same style as pre_verify / pre_gateway_dispatch). Callbacks receive the parsed error context (provider, model, status_code, error_type, error_code, error_message, error_body, error, approx_tokens, context_length, num_messages), self-scope on provider, and return None to pass or {"reason": "<FailoverReason name>", ...optional recovery-hint overrides}.
• get_plugin_error_classification() mirrors get_pre_tool_call_block_message(): first valid result wins, invalid dicts/unknown reasons are skipped, exceptions are isolated — a broken plugin can never break classification.
• Zero behavior change when no plugin claims the error. All 179 existing tests in tests/agent/test_error_classifier.py pass untouched.

Why this seam

A large slice of the open-PR backlog is single-provider error-classification patches: #58451, #58355, and the tool-use-404 piece of #58502, plus the class they represent. Each one is an if provider == X-shaped edit to agent/error_classifier.py waiting on review. (Two earlier citations were struck after review feedback: #58366 is chain-build-time model resolution and #58474 is credential-pool resolution; different layers, not absorbed by this hook.) With this hook, that entire contribution class becomes plugin territory — publishable today, no merge required.

Review updates (July 15)

Per the sweeper review: rebased onto current main, where the OpenRouter tool-use 404 is now handled natively, so the bundled reference plugin (whose whole point was that gap) is removed per the standalone-repo policy for vendor integrations. All hook tests now use a synthetic unclaimed error (fake provider, neutral message, no status code) that no present or future built-in rule can claim, so the fixtures cannot go stale the way the OpenRouter one did.

The hook is also now explicitly Python-plugin-only: VALID_HOOKS doubles as the shell-hook allow-list, and the shell response parser has no channel for the classification directive, so a shell registration is refused at config parse with a warning (new SHELL_UNSUPPORTED_HOOKS set) instead of being silently ignored. The hook is documented in the hooks reference as the third behavior-changing hook with its full kwargs contract.

Review updates (August 12)

Per the #64231 batch-disposition verdict (SALVAGE): the hook id is renamed to transform_api_error_classification, the taxonomy transform-family name, across VALID_HOOKS, the dispatch helper, the shell-hook exclusion, tests, and the hooks.md catalog. Chaining semantics are now explicit per the #64714 rule: run-all-then-pick-first, first valid result in registration order wins, and every valid-but-losing result logs a runtime warning instead of being silently shadowed. Rebased onto current main (715d26c).

Testing

• 14 tests in tests/test_transform_api_error_classification_hook.py: baseline fall-through, plugin-wins, override-of-builtin, enum/string reasons, dataclass-default hints, invalid-reason/non-dict skipping, first-valid-wins, skipped-result runtime warning, helper-exception isolation, hook kwargs contract, message/error_context sanitization, and a synthetic plugin self-scoping and running end-to-end through the real invoke_hook path (no mocks).
• tests/agent/test_error_classifier.py: 75 passed, untouched by this branch.
• Manual end-to-end with a real PluginManager discovery + plugins.enabled config: plugin loads, hook registers, the 404 classifies as model_not_found, and unclaimed errors (e.g. 429 → rate_limit) are untouched.
• ruff check clean on all touched files.

Taxonomy classification (for the #64231 batch review)

• Classification: transform family, confirmed by the chore(plugins): lifecycle-event catalog, hook taxonomy, and batch disposition of pending hook PRs #64231 verdict. Callbacks return a classification and the first valid result overrides the built-in pipeline, so this carries the mutating contract rather than the observer bar.
• Privacy note: the payload's error_body/error_message fields can carry user content or raw provider error dumps (the Delivered-text persistence surfaces bypass PII/redaction hooks (rich-sent index, delivery mirror, API error dumps) #58248 class). Plugins should self-scope on provider and treat these fields as sensitive.
• Cost: the call site only fires on API failures, off the hot path by construction.
• Payload: keyword-only today; schema-versioning to align with feat(plugins): plugin API versioning, stability contract, and compat test suite #64179 once it lands.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FWMcB7RPSYUpsXDfBgwjzM

[Kolomaster68]

Nice seam — provider plugins owning their own error quirks makes a lot of sense, and it should genuinely shrink that backlog class. 👍

One small correction to the list though: #58366 (mine) isn't an error-classification patch — it doesn't touch agent/error_classifier.py. It adds a recommended:free sentinel to fallback_providers so the chain tracks the Portal's rotating free model (model resolution at chain-build time, not error handling at request time). Different code path, so it wouldn't be absorbed by this hook — just flagging so it doesn't get swept up in a batch cleanup if this lands.

[webdevtodayjason]

Good flag, you're right. #58366 is chain-build-time model resolution, not request-time error classification. Different layer, wouldn't be absorbed by this hook. Pulled it from the PR description so it doesn't end up in anyone's batch-cleanup list.

Took a second look at the rest of the citations while I was in there and struck #58474 too. That one is credential-pool resolution, also not classifier territory. The solid citations for this class are #58451, #58355, and the tool-use-404 piece of #58502. Description is updated.

[teknium1]

Thanks for proposing a concrete plugin seam and isolating malformed callback results.

Problems

• The bundled OpenRouter example is now stale. Current main already matches the exact phrase in agent/error_classifier.py:295 and returns model_not_found, retryable=False, and should_fallback=True in the 404 path at agent/error_classifier.py:929-934 (commit 2bb11adb4976169f2434ce72a6185cb189d83079). Consequently, the new baseline assertion in tests/test_classify_api_error_hook.py:48 is no longer true. Please remove or replace that provider-specific demo; third-party integrations also belong in standalone plugin repos under the repository policy.
• VALID_HOOKS is also the shell-hook allow-list (agent/shell_hooks.py:311-336). But _parse_response() at agent/shell_hooks.py:566-620 does not preserve {\"reason\": ...} directives for this event, so a configured shell hook can register successfully while its classification is ignored. Support the shell response shape with tests, or make this Python-plugin-only.
• Please document this behavior-changing hook in website/docs/user-guide/features/hooks.md; its current reference says only pre_tool_call and pre_llm_call affect behavior (:374-397).

Suggested changes

• Retain generic hook tests using a synthetic unclaimed provider error, and add the shell-path or explicit exclusion plus public contract documentation.

Automated hermes-sweeper review.

[teknium1]

This baseline is stale on current main: commit 2bb11adb4976169f2434ce72a6185cb189d83079 added this exact phrase to _MODEL_NOT_FOUND_PATTERNS, and the 404 path now returns model_not_found with retryable=False and fallback. Replace the demo premise with an unclaimed synthetic provider error.

[teknium1]

Adding this to VALID_HOOKS also enables hooks.classify_api_error shell hooks, but agent/shell_hooks.py:_parse_response() only preserves pre-tool, pre-verify, or context responses. A shell hook returning the documented {reason: ...} directive is silently ignored; either implement that response path with tests or keep this event out of the shell-hook allow-list.

[webdevtodayjason]

All three addressed in 5a2a0e759 (rebased onto current main).

1. Stale demo: you're right, main now claims that exact 404 natively, which is the best possible outcome for the original bug. The bundled plugin is removed per the standalone-repo policy, and every hook test now uses a synthetic unclaimed error (fake provider, neutral message, no status code). I verified empirically that a status-less neutral message is the only shape guaranteed to reach the unknown/retryable fall-through on current main (any 4xx is claimed as format_error), so these fixtures cannot go stale the way the OpenRouter one did.

2. Shell-hook gap: real catch. Added SHELL_UNSUPPORTED_HOOKS in hermes_cli/plugins.py containing classify_api_error, checked in _parse_hooks_block before the VALID_HOOKS gate: a shell registration for this event is now refused at config parse with a warning naming it Python-plugin-only, instead of registering and having its directive silently dropped by _parse_response. Regression test included; a future shell response shape can lift the event out of the set.

3. Docs: classify_api_error is documented in website/docs/user-guide/features/hooks.md as the third behavior-changing hook, with the full kwargs contract, return shape, first-valid-wins semantics, and the Python-only note; the general-rules line and quick-reference table are updated to match.

Suites: tests/test_classify_api_error_hook.py 15 passed, tests/agent/test_shell_hooks.py and tests/agent/test_error_classifier.py green, 263 total across the three touched files.

[edwardgem]

The dispatch wording landed in #75861 (docs/plugins/hook-taxonomy.md) with your correction folded in — first-valid-wins, run-all dispatch, isolated failures, classify_api_error noted explicitly as run-all-then-pick-first.

Since we don't have push access to this branch, want to grab the conforming pass yourself now that you can see the exact wording, or would you rather I open a PR against your fork's branch (or drop a suggested diff here) for you to pull in? Either works — just don't want to duplicate effort if you're already planning to touch it.

[webdevtodayjason]

Grabbed it myself, thanks for the pointer. 774329fac aligns all three spots (the VALID_HOOKS comment, the helper docstring, and the hooks.md section) to run-all-then-pick-first, adds the Privacy flag on error_message and error_body, and states the cold-path trigger explicitly. Wording only, no behavior change, 33/33 tests still green. One note: I didn't hard-link docs/plugins/hook-taxonomy.md since #75861 hasn't merged yet. Happy to add the link once it lands.

[webdevtodayjason]

I'll grab the conforming pass myself, thanks for checking rather than duplicating. Since the code already does run-all-then-pick-first, this should mostly be docstring work: the Privacy: line on error_body and error_message, the cold-path note, and pointing the hook's VALID_HOOKS entry at hook-taxonomy.md. I'll flag it on #75861 if any of the wording doesn't survive contact with the code.

[webdevtodayjason]

Applied the #64231 verdict conditions in 8a24479a4. The hook id is now transform_api_error_classification everywhere: VALID_HOOKS, the dispatch helper, the shell-hook exclusion, tests, and both hooks.md spots. Dispatch stays run-all-then-pick-first with the first valid result in registration order winning, and per the #64714 skipped-transform rule a valid-but-losing classification now logs a runtime warning instead of being silently shadowed, with a test covering both the warning and the no-conflict quiet path. Rebased onto current main (715d26c). 14/14 on the hook suite, 134/134 across the hook, shell-hook, and error-classifier suites, ruff clean. If the #64714 chaining resolution lands with different transform semantics I'll conform to it.

[teknium1]

Merged via #85231 (rebase-merge, your commits preserved as author) with a trim commit on top per review: hook test suite reduced to core coverage and hooks.md section condensed. All #64231 verdict conditions retained. Thanks @webdevtodayjason!