From 2ace78d1e50ab772a765ef5691b995e9362e5014 Mon Sep 17 00:00:00 2001 From: Xinmin Zeng <135568692+fancyboi999@users.noreply.github.com> Date: Thu, 28 May 2026 18:38:45 +0800 Subject: [PATCH] fix(frontend): surface backend detail when agent name check fails (#3048) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(frontend): surface backend detail when agent name check fails The new-agent page caught AgentNameCheckError but only branched on reason === "backend_unreachable". Everything else (notably the 422 "Invalid agent name '...'. Must match ^[A-Za-z0-9-]+$" response from GET /api/agents/check when the user submits a name with disallowed characters — trailing space, dot, Chinese, invisible whitespace from copy-paste) fell through to the generic fallback "Could not verify name availability — please try again", swallowing the detail that already told the user exactly what to fix. Add a request_failed branch that surfaces err.message (which checkAgentName already populates from the backend's detail at core/agents/api.ts). The disabled / backend_unreachable / unknown- error paths are unchanged. Pin the contract with unit tests covering: 200 success, fetch rejection, 502/503/504 network errors, agents_api disabled detail, 422 validation detail carried verbatim, statusText fallback when detail is absent, and a regression guard against misclassifying a 422 as agents_api disabled. Closes #3041 * fix(frontend): localise the error prefix when surfacing backend detail The previous commit surfaced the backend's raw `err.message` on the new-agent page when the name check failed. The detail itself is English (backend's `_validate_agent_name` text, any 5xx business message, etc.) and dropping it bare into a zh-CN page produced a jarring English-among-Chinese line that didn't match neighbouring strings like "已存在同名智能体" / "无法验证名称可用性". Add `nameStepCheckErrorWithDetail` as a templated string ("Name check failed: {detail}" / "名称校验失败：{detail}"), mirroring the existing `nameStepBootstrapMessage` `{name}` template pattern. The page wraps `err.message` in it when present and falls back to the plain `nameStepCheckError` when the detail is empty. Rendered output (verified locally with a Console fetch mock that returns 500 + detail): zh-CN: 名称校验失败：Database connection lost: SQLAlchemy connection pool exhausted (max 5 connections, all in use) en-US: Name check failed: Database connection lost: SQLAlchemy connection pool exhausted (max 5 connections, all in use) The localised prefix tells the user *what operation* failed; the raw detail tells them *why*. Translating the detail itself would be lossy (any unbounded backend string would need a translation table) and would break the debuggability the previous commit delivered. Refs #3041 * fix(frontend): distinguish backend detail from generated fallback in AgentNameCheckError Addresses Copilot's review on #3048: the previous commits keyed off `err.message`, but `checkAgentName` substitutes a generated fallback string ("Failed to check agent name: ${statusText}") when the backend sent no detail. That guaranteed `err.message` was always truthy, made the `nameStepCheckError` fallback branch unreachable in practice, and could surface awkward strings like "名称校验失败：Failed to check agent name: Bad Gateway" in the UI. Add an explicit `detail: string | null` field to AgentNameCheckError. `checkAgentName` populates it only when the backend response actually carried a string `detail` (defensive guard against the dict-shaped detail that other deer-flow endpoints use for typed error codes). The new-agent page now selects on `err.detail` instead of `err.message` so the localised fallback wins when no real detail exists. Also fix the prettier formatting that broke lint-frontend CI on the previous push. Test changes: - The 422 carry-through test now asserts both `detail` and `message` hold the backend string verbatim. - A new "falls back to statusText in message but leaves detail null" test pins the contract that no real detail ⇒ no UI surface leak. - A new "treats non-string detail as null" test guards against future backend schema drift toward dict-shaped detail. Refs #3041 #3048 --- .../src/app/workspace/agents/new/page.tsx | 22 +++ frontend/src/core/agents/api.ts | 13 +- frontend/src/core/i18n/locales/en-US.ts | 1 + frontend/src/core/i18n/locales/types.ts | 1 + frontend/src/core/i18n/locales/zh-CN.ts | 1 + frontend/tests/unit/core/agents/api.test.ts | 149 ++++++++++++++++++ 6 files changed, 186 insertions(+), 1 deletion(-) create mode 100644 frontend/tests/unit/core/agents/api.test.ts diff --git a/frontend/src/app/workspace/agents/new/page.tsx b/frontend/src/app/workspace/agents/new/page.tsx index a0337cf64..e7553d9d4 100644 --- a/frontend/src/app/workspace/agents/new/page.tsx +++ b/frontend/src/app/workspace/agents/new/page.tsx @@ -146,6 +146,27 @@ export default function NewAgentPage() { err.reason === "backend_unreachable" ) { setNameError(t.agents.nameStepNetworkError); + } else if ( + err instanceof AgentNameCheckError && + err.reason === "request_failed" + ) { + // Surface the backend-provided detail (e.g. validation error) when + // one is present, wrapped in a localised prefix so zh-CN users + // don't see a bare English string next to the surrounding Chinese + // UI. Falls back to the generic localised fallback when the backend + // sent no detail — `err.message` is unreliable for this branch + // because `checkAgentName` substitutes a generated fallback string + // ("Failed to check agent name: ${statusText}") when `detail` is + // missing, so testing `err.message` would always be truthy and the + // generated fallback would leak through. + setNameError( + err.detail + ? t.agents.nameStepCheckErrorWithDetail.replace( + "{detail}", + err.detail, + ) + : t.agents.nameStepCheckError, + ); } else { setNameError(t.agents.nameStepCheckError); } @@ -172,6 +193,7 @@ export default function NewAgentPage() { t.agents.nameStepNetworkError, t.agents.nameStepBootstrapMessage, t.agents.nameStepCheckError, + t.agents.nameStepCheckErrorWithDetail, t.agents.nameStepInvalidError, threadId, ]); diff --git a/frontend/src/core/agents/api.ts b/frontend/src/core/agents/api.ts index 5cfc40466..680d4c432 100644 --- a/frontend/src/core/agents/api.ts +++ b/frontend/src/core/agents/api.ts @@ -9,6 +9,15 @@ export class AgentNameCheckError extends Error { constructor( message: string, public readonly reason: "backend_unreachable" | "request_failed", + /** + * Raw backend `detail` string when the failure came from a backend + * response carrying one. `null` when no detail was provided (e.g. + * network-layer failure, empty response body, unparseable body) — in + * which case `message` is a generated fallback like "Failed to check + * agent name: Bad Gateway" and the UI should prefer its own localized + * fallback instead of surfacing the generated string. + */ + public readonly detail: string | null = null, ) { super(message); this.name = "AgentNameCheckError"; @@ -104,9 +113,11 @@ export async function checkAgentName( "backend_unreachable", ); } + const backendDetail = typeof err.detail === "string" ? err.detail : null; throw new AgentNameCheckError( - err.detail ?? `Failed to check agent name: ${res.statusText}`, + backendDetail ?? `Failed to check agent name: ${res.statusText}`, "request_failed", + backendDetail, ); } return res.json() as Promise<{ available: boolean; name: string }>; diff --git a/frontend/src/core/i18n/locales/en-US.ts b/frontend/src/core/i18n/locales/en-US.ts index b6ce0c76a..9d83c77a4 100644 --- a/frontend/src/core/i18n/locales/en-US.ts +++ b/frontend/src/core/i18n/locales/en-US.ts @@ -204,6 +204,7 @@ export const enUS: Translations = { nameStepNetworkError: "Network request failed — check your network or backend connection", nameStepCheckError: "Could not verify name availability — please try again", + nameStepCheckErrorWithDetail: "Name check failed: {detail}", nameStepApiDisabledError: "Custom agent management is not enabled on this server. Please contact your administrator.", nameStepBootstrapMessage: diff --git a/frontend/src/core/i18n/locales/types.ts b/frontend/src/core/i18n/locales/types.ts index 79e279192..3213f34b1 100644 --- a/frontend/src/core/i18n/locales/types.ts +++ b/frontend/src/core/i18n/locales/types.ts @@ -141,6 +141,7 @@ export interface Translations { nameStepAlreadyExistsError: string; nameStepNetworkError: string; nameStepCheckError: string; + nameStepCheckErrorWithDetail: string; nameStepApiDisabledError: string; nameStepBootstrapMessage: string; save: string; diff --git a/frontend/src/core/i18n/locales/zh-CN.ts b/frontend/src/core/i18n/locales/zh-CN.ts index 105aca551..bcbacd268 100644 --- a/frontend/src/core/i18n/locales/zh-CN.ts +++ b/frontend/src/core/i18n/locales/zh-CN.ts @@ -192,6 +192,7 @@ export const zhCN: Translations = { nameStepAlreadyExistsError: "已存在同名智能体", nameStepNetworkError: "网络请求失败，请检查网络或后端连接", nameStepCheckError: "无法验证名称可用性，请稍后重试", + nameStepCheckErrorWithDetail: "名称校验失败：{detail}", nameStepApiDisabledError: "服务器未开启自定义智能体管理功能，请联系管理员。", nameStepBootstrapMessage: diff --git a/frontend/tests/unit/core/agents/api.test.ts b/frontend/tests/unit/core/agents/api.test.ts new file mode 100644 index 000000000..9ea61b5e7 --- /dev/null +++ b/frontend/tests/unit/core/agents/api.test.ts @@ -0,0 +1,149 @@ +/** + * Tests for the error-classification behaviour of `checkAgentName`. + * + * Issue #3041: when the backend returns a non-200 response (e.g. a 500 with + * a database error, a 422 from misbehaving routing, or any other 4xx/5xx + * not in the 502/503/504 set), the UI used to swallow the backend detail + * into a generic "Could not verify name availability" fallback because the + * page-level catch block only handled `reason === "backend_unreachable"`. + * + * The fix carries the raw backend detail as `AgentNameCheckError.detail` + * (distinct from `message`, which always has a non-empty value because + * `checkAgentName` substitutes a generated fallback when the backend sent + * no detail). The UI uses `detail` to decide whether to surface a real + * backend string or fall back to the localised "could not verify" copy. + * + * These tests pin both halves of the contract so a future refactor doesn't + * silently drop the detail or leak the generated fallback into the UI. + */ +import { beforeEach, describe, expect, test, vi } from "vitest"; + +vi.mock("@/core/api/fetcher", () => ({ + fetch: vi.fn(), +})); + +vi.mock("@/core/config", () => ({ + getBackendBaseURL: () => "", +})); + +import { AgentsApiDisabledError, checkAgentName } from "@/core/agents/api"; +import { fetch as fetcher } from "@/core/api/fetcher"; + +const mockedFetch = vi.mocked(fetcher); + +function jsonResponse(status: number, body: unknown): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }); +} + +beforeEach(() => { + mockedFetch.mockReset(); +}); + +describe("checkAgentName", () => { + test("returns availability payload on 200", async () => { + mockedFetch.mockResolvedValueOnce( + jsonResponse(200, { available: true, name: "dealagent" }), + ); + const result = await checkAgentName("dealagent"); + expect(result).toEqual({ available: true, name: "dealagent" }); + }); + + test("treats network-layer fetch rejection as backend_unreachable", async () => { + mockedFetch.mockRejectedValueOnce(new TypeError("Failed to fetch")); + await expect(checkAgentName("dealagent")).rejects.toMatchObject({ + name: "AgentNameCheckError", + reason: "backend_unreachable", + }); + }); + + test.each([502, 503, 504])( + "treats HTTP %i as backend_unreachable", + async (status) => { + mockedFetch.mockResolvedValueOnce( + jsonResponse(status, { detail: "Bad Gateway" }), + ); + await expect(checkAgentName("dealagent")).rejects.toMatchObject({ + name: "AgentNameCheckError", + reason: "backend_unreachable", + }); + }, + ); + + test("recognises agents_api disabled detail and throws AgentsApiDisabledError", async () => { + const detail = + "Custom-agent management API is disabled. Set agents_api.enabled=true to expose agent and user-profile routes over HTTP."; + mockedFetch.mockResolvedValueOnce(jsonResponse(403, { detail })); + await expect(checkAgentName("dealagent")).rejects.toBeInstanceOf( + AgentsApiDisabledError, + ); + }); + + test("carries backend 422 detail through AgentNameCheckError.detail (issue #3041)", async () => { + // This is the exact response shape produced by `_validate_agent_name` + // when the user submits a name with disallowed characters — e.g. a + // trailing space, a dot, a Chinese character, or invisible whitespace + // pasted in from another window. + const detail = + "Invalid agent name 'deal agent'. Must match ^[A-Za-z0-9-]+$ (letters, digits, and hyphens only)."; + mockedFetch.mockResolvedValueOnce(jsonResponse(422, { detail })); + + await expect(checkAgentName("deal agent")).rejects.toMatchObject({ + name: "AgentNameCheckError", + reason: "request_failed", + // The full detail is preserved on both `detail` (for the UI to + // recognise "real backend detail vs generated fallback") and + // `message` (for stack traces / logs). + detail, + message: detail, + }); + }); + + test("falls back to statusText in message but leaves detail null when backend returns no detail", async () => { + // The fallback message must NOT mask the absence of a real backend + // detail — the page-level catch relies on `detail === null` to choose + // the localised generic fallback rather than rendering the bare + // "Failed to check agent name: Internal Server Error" string. + mockedFetch.mockResolvedValueOnce( + new Response("", { status: 500, statusText: "Internal Server Error" }), + ); + await expect(checkAgentName("dealagent")).rejects.toMatchObject({ + name: "AgentNameCheckError", + reason: "request_failed", + detail: null, + message: expect.stringContaining("Internal Server Error"), + }); + }); + + test("treats non-string detail as null (defence against future schema drift)", async () => { + // If the backend ever returns `{detail: {code, message}}` (the shape + // used by auth errors today) on this endpoint, we must not surface a + // `[object Object]` string. `detail` should fall back to null so the + // page uses its localised fallback. + mockedFetch.mockResolvedValueOnce( + jsonResponse(500, { detail: { code: "x", message: "y" } }), + ); + await expect(checkAgentName("dealagent")).rejects.toMatchObject({ + name: "AgentNameCheckError", + reason: "request_failed", + detail: null, + }); + }); + + test("does not misclassify a 422 with unrelated detail as agents_api disabled", async () => { + // Defence-in-depth: the disabled detector matches on the substring + // "agents_api.enabled", so a 422 whose detail accidentally contains + // the same substring would be misclassified. The validation detail + // produced by `_validate_agent_name` never contains it; this test + // simply asserts that "Invalid agent name ..." stays in the + // request_failed branch, which is where the page now surfaces it. + const detail = + "Invalid agent name 'deal.agent'. Must match ^[A-Za-z0-9-]+$ (letters, digits, and hyphens only)."; + mockedFetch.mockResolvedValueOnce(jsonResponse(422, { detail })); + await expect(checkAgentName("deal.agent")).rejects.not.toBeInstanceOf( + AgentsApiDisabledError, + ); + }); +});