fix(desktop): error card honors the classifier's retry verdict + failing-session identity (review feedback)
Addresses @helix4u's review on #91493: - conversation_loop now stamps failure_retryable (the real ClassifiedError verdict) next to failure_reason; error_surface prefers it and only falls back to the reason set for older results. Fallback set corrected to match classify_api_error (auth, format_error, billing_unverified now non-retryable). - The descriptor carries the failing session's provider/model captured at classification time; Copy error details prefers them over the foreground composer atoms. - Open logs is labeled 'Open Desktop logs' on remote/cloud connections — the local folder holds transport logs, not the remote runtime's. - API-exception module allowlist widened to botocore/boto3/google/grpc/ requests/aiohttp so other adapter SDKs don't misclassify as gateway.
This commit is contained in:
@@ -755,6 +755,10 @@ def _billing_failure_result(
|
||||
"failed": True,
|
||||
"error": summary,
|
||||
"failure_reason": classified.reason.value,
|
||||
# The classifier's own retry verdict — carried so UI surfaces
|
||||
# (agent/error_surface.py) show Retry only when a re-run can differ,
|
||||
# instead of re-deriving retryability from a second taxonomy.
|
||||
"failure_retryable": bool(classified.retryable),
|
||||
# The billing verdict may rest on an ambiguous body (#82154) — carry
|
||||
# that through the structured result, not just the prose.
|
||||
"billing_unverified": unverified,
|
||||
@@ -6439,6 +6443,9 @@ def run_conversation(
|
||||
# different exit code. ``rate_limit`` / ``billing`` here
|
||||
# mean "quota wall, not a task error".
|
||||
"failure_reason": classified.reason.value,
|
||||
# The classifier's own retry verdict — UI surfaces use
|
||||
# this instead of re-deriving from the reason string.
|
||||
"failure_retryable": bool(classified.retryable),
|
||||
# True when the billing verdict rests on an ambiguous
|
||||
# body (#82154) — may be a content-filter rejection.
|
||||
"billing_unverified": _billing_unverified,
|
||||
|
||||
+45
-10
@@ -62,13 +62,19 @@ _TRANSPORT_REASONS = {
|
||||
}
|
||||
|
||||
# Reasons that are deterministic for the request — a bare "Retry" repeats the
|
||||
# same failure, so clients shouldn't lead with it.
|
||||
# same failure, so clients shouldn't lead with it. Fallback only: results
|
||||
# from current backends carry the classifier's own verdict in
|
||||
# ``failure_retryable`` and never consult this set. Kept in sync with
|
||||
# ``classify_api_error``'s retryable=False verdicts.
|
||||
_NON_RETRYABLE_REASONS = {
|
||||
"auth",
|
||||
"auth_permanent",
|
||||
"billing",
|
||||
"billing_unverified",
|
||||
"content_policy_blocked",
|
||||
"provider_policy_blocked",
|
||||
"model_not_found",
|
||||
"format_error",
|
||||
"ssl_cert_verification",
|
||||
}
|
||||
|
||||
@@ -99,11 +105,20 @@ _STREAM_DROP_FRAGMENTS = (
|
||||
|
||||
# Exception modules that indicate the failure came from an API/transport call
|
||||
# (vs. a bug in our own dispatcher code, which is a gateway-layer failure).
|
||||
# Covers every SDK family our provider adapters raise from: OpenAI-compatible
|
||||
# (openai/httpx/httpcore), Anthropic, Bedrock (botocore/boto3), Google
|
||||
# (google.*/grpc), plus raw transports (requests/aiohttp/ssl/socket/urllib).
|
||||
_API_EXC_MODULE_PREFIXES = (
|
||||
"openai",
|
||||
"httpx",
|
||||
"httpcore",
|
||||
"anthropic",
|
||||
"botocore",
|
||||
"boto3",
|
||||
"google",
|
||||
"grpc",
|
||||
"requests",
|
||||
"aiohttp",
|
||||
"ssl",
|
||||
"socket",
|
||||
"urllib",
|
||||
@@ -120,8 +135,22 @@ def _looks_like_stream_drop(message: str) -> bool:
|
||||
return any(fragment in msg for fragment in _STREAM_DROP_FRAGMENTS)
|
||||
|
||||
|
||||
def _surface(layer: str, code: str, retryable: bool) -> dict:
|
||||
return {"layer": layer, "code": code, "retryable": bool(retryable)}
|
||||
def _surface(
|
||||
layer: str,
|
||||
code: str,
|
||||
retryable: bool,
|
||||
provider: str = "",
|
||||
model: str = "",
|
||||
) -> dict:
|
||||
out = {"layer": layer, "code": code, "retryable": bool(retryable)}
|
||||
# The failing session's identity, captured at classification time so
|
||||
# clients report the model/provider that actually failed — not whatever
|
||||
# the foreground composer points at when a button is clicked later.
|
||||
if provider:
|
||||
out["provider"] = provider
|
||||
if model:
|
||||
out["model"] = model
|
||||
return out
|
||||
|
||||
|
||||
def build_error_surface_from_result(
|
||||
@@ -147,18 +176,18 @@ def build_error_surface_from_result(
|
||||
from hermes_state import is_disk_full_error
|
||||
|
||||
if error_text and is_disk_full_error(error_text):
|
||||
return _surface(LAYER_DISK, "disk_full", False)
|
||||
return _surface(LAYER_DISK, "disk_full", False, provider, model)
|
||||
except Exception: # pragma: no cover - defensive import guard
|
||||
pass
|
||||
|
||||
if result.get("billing_block") or reason in ("billing", "billing_unverified"):
|
||||
return _surface(LAYER_BILLING, reason or "billing", False)
|
||||
return _surface(LAYER_BILLING, reason or "billing", False, provider, model)
|
||||
|
||||
if not reason:
|
||||
# Failed result without a classified reason (legacy paths).
|
||||
if _looks_like_stream_drop(error_text):
|
||||
return _surface(LAYER_STREAMING, "stream_drop", True)
|
||||
return _surface(LAYER_PROVIDER, "unknown", True)
|
||||
return _surface(LAYER_STREAMING, "stream_drop", True, provider, model)
|
||||
return _surface(LAYER_PROVIDER, "unknown", True, provider, model)
|
||||
|
||||
layer = _REASON_TO_LAYER.get(reason)
|
||||
if layer is None:
|
||||
@@ -168,7 +197,13 @@ def build_error_surface_from_result(
|
||||
layer = LAYER_STREAMING
|
||||
else:
|
||||
layer = LAYER_PROVIDER
|
||||
return _surface(layer, reason, reason not in _NON_RETRYABLE_REASONS)
|
||||
# Prefer the classifier's own retry verdict when the result carries it
|
||||
# (conversation_loop stamps ``failure_retryable`` next to
|
||||
# ``failure_reason``); the reason-set fallback covers older results.
|
||||
retryable = result.get("failure_retryable")
|
||||
if not isinstance(retryable, bool):
|
||||
retryable = reason not in _NON_RETRYABLE_REASONS
|
||||
return _surface(layer, reason, retryable, provider, model)
|
||||
except Exception: # pragma: no cover — never break the error path
|
||||
logger.debug("error_surface: result classification failed", exc_info=True)
|
||||
return None
|
||||
@@ -191,7 +226,7 @@ def build_error_surface_from_exception(
|
||||
from hermes_state import is_disk_full_error
|
||||
|
||||
if is_disk_full_error(exc):
|
||||
return _surface(LAYER_DISK, "disk_full", False)
|
||||
return _surface(LAYER_DISK, "disk_full", False, provider, model)
|
||||
except Exception: # pragma: no cover - defensive import guard
|
||||
pass
|
||||
|
||||
@@ -201,7 +236,7 @@ def build_error_surface_from_exception(
|
||||
)
|
||||
|
||||
if not api_like or not isinstance(exc, Exception):
|
||||
return _surface(LAYER_GATEWAY, type(exc).__name__, True)
|
||||
return _surface(LAYER_GATEWAY, type(exc).__name__, True, provider, model)
|
||||
|
||||
from agent.error_classifier import classify_api_error
|
||||
|
||||
|
||||
@@ -39,7 +39,7 @@ import { useEnterAnimation } from '@/lib/use-enter-animation'
|
||||
import { cn } from '@/lib/utils'
|
||||
import { playSpeechText, stopVoicePlayback } from '@/lib/voice-playback'
|
||||
import { notifyError } from '@/store/notifications'
|
||||
import { $currentModel } from '@/store/session'
|
||||
import { $connection, $currentModel } from '@/store/session'
|
||||
import { $voicePlayback } from '@/store/voice-playback'
|
||||
|
||||
// Stable empty identity for the settled-parts selector — a fresh [] per render
|
||||
@@ -490,6 +490,14 @@ const ErrorRecoveryActions: FC = () => {
|
||||
// child mounts only when one is (see SwitchProviderAction).
|
||||
const inRouter = useInRouterContext()
|
||||
const model = useStore($currentModel)
|
||||
const connection = useStore($connection)
|
||||
|
||||
// Open Logs reveals the LOCAL Electron profile's HERMES_HOME/logs. On a
|
||||
// remote/cloud connection the failed turn's gateway+agent logs live on the
|
||||
// remote box — the local folder only holds Desktop-side transport logs, so
|
||||
// the label says "Open Desktop logs" there instead of implying it opens the
|
||||
// runtime's logs.
|
||||
const remoteConnection = connection?.mode === 'remote'
|
||||
|
||||
// Retry = assistant-ui reload (same wiring as the footer's refresh action):
|
||||
// re-runs the failed turn's prompt in place. Suppressed when the classifier
|
||||
@@ -543,7 +551,7 @@ const ErrorRecoveryActions: FC = () => {
|
||||
{showSwitchProvider && inRouter && <SwitchProviderAction label={copy.errorSwitchProvider} />}
|
||||
{window.hermesDesktop?.logsRoot && (
|
||||
<button className="aui-error-action" onClick={() => void openLogs()} type="button">
|
||||
{copy.errorOpenLogs}
|
||||
{remoteConnection ? copy.errorOpenDesktopLogs : copy.errorOpenLogs}
|
||||
</button>
|
||||
)}
|
||||
<CopyButton appearance="inline" className="aui-error-action" label={copy.errorCopyDiagnostics} text={diagnosticsText} />
|
||||
|
||||
@@ -2449,6 +2449,7 @@ export const ar = defineLocale({
|
||||
errorSwitchProvider: 'تبديل المزوّد',
|
||||
errorOpenLogs: 'فتح السجلات',
|
||||
errorOpenLogsFailed: 'تعذّر فتح مجلد السجلات',
|
||||
errorOpenDesktopLogs: 'فتح سجلات سطح المكتب',
|
||||
errorCopyDiagnostics: 'نسخ تفاصيل الخطأ',
|
||||
filesChanged: count => `${count} ملفات تم تغييرها`,
|
||||
reviewChanges: 'مراجعة',
|
||||
|
||||
@@ -3094,6 +3094,7 @@ export const en: Translations = {
|
||||
errorSwitchProvider: 'Switch provider',
|
||||
errorOpenLogs: 'Open logs',
|
||||
errorOpenLogsFailed: 'Could not open the logs folder',
|
||||
errorOpenDesktopLogs: 'Open Desktop logs',
|
||||
errorCopyDiagnostics: 'Copy error details',
|
||||
filesChanged: count => (count === 1 ? '1 file changed' : `${count} files changed`),
|
||||
reviewChanges: 'Review',
|
||||
|
||||
@@ -2746,6 +2746,7 @@ export const ja = defineLocale({
|
||||
errorSwitchProvider: 'プロバイダーを切り替え',
|
||||
errorOpenLogs: 'ログを開く',
|
||||
errorOpenLogsFailed: 'ログフォルダを開けませんでした',
|
||||
errorOpenDesktopLogs: 'デスクトップのログを開く',
|
||||
errorCopyDiagnostics: 'エラー詳細をコピー',
|
||||
filesChanged: count => `${count} 件のファイルを変更`,
|
||||
reviewChanges: 'レビュー',
|
||||
|
||||
@@ -2665,6 +2665,7 @@ export interface Translations {
|
||||
errorSwitchProvider: string
|
||||
errorOpenLogs: string
|
||||
errorOpenLogsFailed: string
|
||||
errorOpenDesktopLogs: string
|
||||
errorCopyDiagnostics: string
|
||||
filesChanged: (count: number) => string
|
||||
reviewChanges: string
|
||||
|
||||
@@ -2656,6 +2656,7 @@ export const zhHant = defineLocale({
|
||||
errorSwitchProvider: '切換服務商',
|
||||
errorOpenLogs: '開啟日誌',
|
||||
errorOpenLogsFailed: '無法開啟日誌資料夾',
|
||||
errorOpenDesktopLogs: '開啟桌面端日誌',
|
||||
errorCopyDiagnostics: '複製錯誤詳細資訊',
|
||||
filesChanged: count => `${count} 個檔案已變更`,
|
||||
reviewChanges: '檢視',
|
||||
|
||||
@@ -3258,6 +3258,7 @@ export const zh: Translations = {
|
||||
errorSwitchProvider: '切换服务商',
|
||||
errorOpenLogs: '打开日志',
|
||||
errorOpenLogsFailed: '无法打开日志文件夹',
|
||||
errorOpenDesktopLogs: '打开桌面端日志',
|
||||
errorCopyDiagnostics: '复制错误详情',
|
||||
filesChanged: count => `${count} 个文件已更改`,
|
||||
reviewChanges: '查看',
|
||||
|
||||
@@ -32,6 +32,21 @@ describe('parseErrorSurface', () => {
|
||||
it('honors retryable=false', () => {
|
||||
expect(parseErrorSurface({ layer: 'auth', code: 'auth_permanent', retryable: false })?.retryable).toBe(false)
|
||||
})
|
||||
|
||||
it('carries the failing session identity when present', () => {
|
||||
const surface = parseErrorSurface({
|
||||
layer: 'provider',
|
||||
code: 'rate_limit',
|
||||
retryable: true,
|
||||
provider: 'openrouter',
|
||||
model: 'test/m1'
|
||||
})
|
||||
|
||||
expect(surface?.provider).toBe('openrouter')
|
||||
expect(surface?.model).toBe('test/m1')
|
||||
// Absent identity yields no keys, not empty strings.
|
||||
expect(parseErrorSurface({ layer: 'provider', code: 'x', retryable: true })?.provider).toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
||||
describe('formatErrorDiagnostics', () => {
|
||||
@@ -48,6 +63,19 @@ describe('formatErrorDiagnostics', () => {
|
||||
expect(text).toContain('error: boom')
|
||||
})
|
||||
|
||||
it('prefers the descriptor identity over the caller fallback', () => {
|
||||
const text = formatErrorDiagnostics({
|
||||
errorText: 'boom',
|
||||
// Foreground composer atom — potentially stale by click time.
|
||||
model: 'some/other-model',
|
||||
surface: { layer: 'provider', code: 'rate_limit', retryable: true, provider: 'openrouter', model: 'failed/model' }
|
||||
})
|
||||
|
||||
expect(text).toContain('provider: openrouter')
|
||||
expect(text).toContain('model: failed/model')
|
||||
expect(text).not.toContain('some/other-model')
|
||||
})
|
||||
|
||||
it('omits absent fields without leaving blank lines', () => {
|
||||
const text = formatErrorDiagnostics({ errorText: 'boom' })
|
||||
|
||||
|
||||
@@ -25,6 +25,11 @@ export interface ErrorSurface {
|
||||
code: string
|
||||
/** False when retrying unchanged reproduces the same failure. */
|
||||
retryable: boolean
|
||||
/** The failing session's provider/model, captured at classification time —
|
||||
* preferred over the foreground composer's atoms, which can point at a
|
||||
* different model by the time the user clicks an action. */
|
||||
provider?: string
|
||||
model?: string
|
||||
}
|
||||
|
||||
/** Validate a wire payload into an ErrorSurface, or null when absent/garbled. */
|
||||
@@ -33,7 +38,7 @@ export function parseErrorSurface(value: unknown): ErrorSurface | null {
|
||||
return null
|
||||
}
|
||||
|
||||
const raw = value as { code?: unknown; layer?: unknown; retryable?: unknown }
|
||||
const raw = value as { code?: unknown; layer?: unknown; model?: unknown; provider?: unknown; retryable?: unknown }
|
||||
const layer = typeof raw.layer === 'string' ? (raw.layer as ErrorSurfaceLayer) : null
|
||||
|
||||
if (!layer || !ERROR_SURFACE_LAYERS.includes(layer)) {
|
||||
@@ -43,7 +48,9 @@ export function parseErrorSurface(value: unknown): ErrorSurface | null {
|
||||
return {
|
||||
layer,
|
||||
code: typeof raw.code === 'string' && raw.code ? raw.code : 'unknown',
|
||||
retryable: raw.retryable !== false
|
||||
retryable: raw.retryable !== false,
|
||||
...(typeof raw.provider === 'string' && raw.provider ? { provider: raw.provider } : {}),
|
||||
...(typeof raw.model === 'string' && raw.model ? { model: raw.model } : {})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -55,14 +62,19 @@ export function formatErrorDiagnostics(input: {
|
||||
provider?: string
|
||||
surface?: ErrorSurface | null
|
||||
}): string {
|
||||
// The descriptor's identity (captured when the turn failed) beats the
|
||||
// caller-supplied fallback (typically the foreground composer's atoms).
|
||||
const provider = input.surface?.provider || input.provider
|
||||
const model = input.surface?.model || input.model
|
||||
|
||||
const lines = [
|
||||
'── Hermes error diagnostics ──',
|
||||
'── Hermes error details ──',
|
||||
`time: ${new Date().toISOString()}`,
|
||||
input.surface ? `layer: ${input.surface.layer}` : null,
|
||||
input.surface ? `code: ${input.surface.code}` : null,
|
||||
input.surface ? `retryable: ${input.surface.retryable}` : null,
|
||||
input.provider ? `provider: ${input.provider}` : null,
|
||||
input.model ? `model: ${input.model}` : null,
|
||||
provider ? `provider: ${provider}` : null,
|
||||
model ? `model: ${model}` : null,
|
||||
input.appVersion ? `app: ${input.appVersion}` : null,
|
||||
`error: ${input.errorText}`
|
||||
]
|
||||
|
||||
@@ -41,8 +41,10 @@ def test_result_none_for_healthy_result():
|
||||
|
||||
|
||||
def test_result_auth_reasons_map_to_auth_layer():
|
||||
# Both auth reasons are non-retryable, matching classify_api_error's own
|
||||
# verdict (a bare retry replays the same rejected credential).
|
||||
surface = build_error_surface_from_result(_failed_result("auth"))
|
||||
assert surface == {"layer": LAYER_AUTH, "code": "auth", "retryable": True}
|
||||
assert surface == {"layer": LAYER_AUTH, "code": "auth", "retryable": False}
|
||||
|
||||
surface = build_error_surface_from_result(_failed_result("auth_permanent"))
|
||||
assert surface["layer"] == LAYER_AUTH
|
||||
@@ -77,6 +79,8 @@ def test_result_provider_default_for_classified_reasons():
|
||||
|
||||
def test_result_non_retryable_reasons():
|
||||
for reason in (
|
||||
"auth",
|
||||
"format_error",
|
||||
"content_policy_blocked",
|
||||
"model_not_found",
|
||||
"ssl_cert_verification",
|
||||
@@ -85,6 +89,32 @@ def test_result_non_retryable_reasons():
|
||||
assert surface["retryable"] is False, reason
|
||||
|
||||
|
||||
def test_result_prefers_classifier_retry_verdict():
|
||||
"""conversation_loop stamps ``failure_retryable`` from the real
|
||||
ClassifiedError — it must win over the fallback reason set."""
|
||||
surface = build_error_surface_from_result(
|
||||
_failed_result("unknown", failure_retryable=False)
|
||||
)
|
||||
assert surface["retryable"] is False
|
||||
|
||||
surface = build_error_surface_from_result(
|
||||
_failed_result("format_error", failure_retryable=True)
|
||||
)
|
||||
assert surface["retryable"] is True
|
||||
|
||||
|
||||
def test_result_stamps_failing_session_identity():
|
||||
surface = build_error_surface_from_result(
|
||||
_failed_result("rate_limit"), provider="openrouter", model="test/m1"
|
||||
)
|
||||
assert surface["provider"] == "openrouter"
|
||||
assert surface["model"] == "test/m1"
|
||||
|
||||
# Absent identity omits the keys instead of stamping empty strings.
|
||||
surface = build_error_surface_from_result(_failed_result("rate_limit"))
|
||||
assert "provider" not in surface and "model" not in surface
|
||||
|
||||
|
||||
def test_result_timeout_on_custom_endpoint_is_endpoint_layer():
|
||||
surface = build_error_surface_from_result(
|
||||
_failed_result("timeout"), provider="custom"
|
||||
|
||||
@@ -197,6 +197,10 @@ def test_returned_error_result_carries_error_surface(emits, turn_env):
|
||||
"layer": "provider",
|
||||
"code": "rate_limit",
|
||||
"retryable": True,
|
||||
# The failing session's identity rides the descriptor so clients
|
||||
# report the model that actually failed, not the composer's current.
|
||||
"provider": "openrouter",
|
||||
"model": "test/model",
|
||||
}
|
||||
|
||||
snapshot = server._inflight_snapshot(session)
|
||||
|
||||
@@ -420,7 +420,10 @@ generic error toast. The card offers recovery actions matched to the failure:
|
||||
deterministically reproduce the failure, e.g. a content-policy rejection).
|
||||
- **Switch provider** — jumps to Settings → Models for provider, endpoint,
|
||||
auth, and billing failures.
|
||||
- **Open logs** — opens `HERMES_HOME/logs` in your file manager.
|
||||
- **Open logs** — opens `HERMES_HOME/logs` in your file manager. On a remote
|
||||
or Cloud connection the button reads **Open Desktop logs**: it opens the
|
||||
local Desktop-side logs (transport evidence), since the failed turn's
|
||||
gateway/agent logs live on the remote machine.
|
||||
- **Copy error details** — copies a compact plain-text summary (layer, code,
|
||||
provider/model, error message) you can paste into a bug report or Discord.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user