fix(agent): apply review findings (backend data-integrity + frontend error UX)

Two review agents (backend + frontend) audited the template-agent feature. Real findings fixed (verified the one false-positive — okOrThrow already reads the top-level success because the response interceptor returns response, not response.data): Backend: - buildAgentEnvFromBindings now filters status='active' so a revoked/disabled binding can't still have its KV secret resolved + injected. - same-type binding env collision (e.g. two git repos -> GIT_REPO_URL) now errors instead of silently overwriting. - refreshAgentStatus uses a field-level Updates (status/updated_at) instead of Save(full row) to avoid clobbering a concurrent stop/delete. - tests added: same-type collision rejected, inactive binding rejected. Frontend: - deploy/stop/delete now pass noBusinessError so the global interceptor stops double-toasting the raw English backend message; okOrThrow+onError give one friendly Chinese error. Extracted okOrThrow/noBusinessError into lib/api.ts. - resources page revoke + create + KV-secret-write now check success (okOrThrow) so a failed unbind / credential write no longer falsely reports success. - clipboard copy wrapped in try/catch (no false "copied" on failure). - resources group label shows Chinese resource-type names. Go + frontend builds/tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-04 14:24:25 +08:00
parent 479cd61a07
commit 7a48155568
7 changed files with 128 additions and 38 deletions
@@ -154,8 +154,10 @@ func buildAgentEnvFromBindings(userID int, bindingIDs []int) (map[string]string,

 	for _, id := range bindingIDs {
 		var binding model.ResourceBinding
-		if err := model.DB.Where("id = ? AND user_id = ?", id, userID).First(&binding).Error; err != nil {
-			return nil, fmt.Errorf("resource binding %d not found for user", id)
+		// Only active bindings — a revoked/disabled resource must not be resolved
+		// (its KV secret would otherwise still be read and injected).
+		if err := model.DB.Where("id = ? AND user_id = ? AND status = ?", id, userID, "active").First(&binding).Error; err != nil {
+			return nil, fmt.Errorf("resource binding %d not found or not active", id)
 		}

 		spec := builtinEnvSpec(binding.ResourceType, binding.Provider)
@@ -196,9 +198,15 @@ func buildAgentEnvFromBindings(userID int, bindingIDs []int) (map[string]string,
 			}
 			if value, ok := src[f.Key]; ok {
 				str := fmt.Sprintf("%v", value)
-				if strings.TrimSpace(str) != "" {
-					env[f.Env] = str
+				if strings.TrimSpace(str) == "" {
+					continue
 				}
+				// Two bindings of the same type produce the same env name; refuse
+				// rather than silently let the later one overwrite the earlier.
+				if prev, dup := env[f.Env]; dup && prev != str {
+					return nil, fmt.Errorf("环境变量 %s 冲突：同类型资源（如两个 git/数据库）只能挂一个", f.Env)
+				}
+				env[f.Env] = str
 			}
 		}
 	}
@@ -183,11 +183,17 @@ func refreshAgentStatus(c *gin.Context, row *model.AgentDeployment) {
 	if err != nil || strings.TrimSpace(status) == "" || status == row.Status {
 		return
 	}
+	now := agentNow()
+	nowMs := time.Now().UnixMilli()
 	row.Status = status
-	row.UpdatedAtText = agentNow()
-	row.UpdatedAtMs = time.Now().UnixMilli()
+	row.UpdatedAtText = now
+	row.UpdatedAtMs = nowMs
+	// Field-level update (not Save of the whole row) so a concurrent stop/delete
+	// or other column write isn't clobbered by a stale full-row save.
 	if model.DB != nil {
-		_ = model.DB.Save(row).Error
+		_ = model.DB.Model(&model.AgentDeployment{}).
+			Where("deployment_id = ?", row.DeploymentID).
+			Updates(map[string]any{"status": status, "updated_at_text": now, "updated_at_ms": nowMs}).Error
 	}
 }

@@ -79,6 +79,26 @@ func TestBuildAgentEnvFromBindings_GitNamesProviderAgnostic(t *testing.T) {
 	}
 }

+func TestBuildAgentEnvFromBindings_SameTypeCollisionRejected(t *testing.T) {
+	setupResourceControllerTestDB(t)
+	b1 := model.ResourceBinding{UserId: 7, Name: "g1", ResourceType: "git", Provider: "github", Status: "active", Metadata: `{"repo_url":"https://x/a"}`}
+	b2 := model.ResourceBinding{UserId: 7, Name: "g2", ResourceType: "git", Provider: "gitea", Status: "active", Metadata: `{"repo_url":"https://y/b"}`}
+	require.NoError(t, model.DB.Create(&b1).Error)
+	require.NoError(t, model.DB.Create(&b2).Error)
+
+	_, err := buildAgentEnvFromBindings(7, []int{b1.Id, b2.Id}) // both set GIT_REPO_URL
+	require.Error(t, err)
+}
+
+func TestBuildAgentEnvFromBindings_InactiveRejected(t *testing.T) {
+	setupResourceControllerTestDB(t)
+	b := model.ResourceBinding{UserId: 7, Name: "x", ResourceType: "blob", Provider: "blob", Status: "revoked", Metadata: `{"account":"a"}`}
+	require.NoError(t, model.DB.Create(&b).Error)
+
+	_, err := buildAgentEnvFromBindings(7, []int{b.Id}) // not active
+	require.Error(t, err)
+}
+
 func TestBuildAgentEnvFromBindings_OwnershipEnforced(t *testing.T) {
 	setupResourceControllerTestDB(t)
 	b := model.ResourceBinding{UserId: 7, Name: "x", ResourceType: "blob", Provider: "blob", Metadata: `{}`}
@@ -20,7 +20,7 @@
    <meta property="og:type" content="website" />

    <meta name="theme-color" content="#7B6BE3" />
-  <link rel="icon" href="/favicon.ico"><script defer src="/static/js/vendor-radix.829c7e3fad.js"></script><script defer src="/static/js/vendor-tanstack.632dbe8908.js"></script><script defer src="/static/js/lib-react.5c8909c28c.js"></script><script defer src="/static/js/6374.bc21d8b214.js"></script><script defer src="/static/js/index.deee68f4fa.js"></script><link href="/static/css/index.592272f49e.css" rel="stylesheet"></head>
+  <link rel="icon" href="/favicon.ico"><script defer src="/static/js/vendor-radix.829c7e3fad.js"></script><script defer src="/static/js/vendor-tanstack.632dbe8908.js"></script><script defer src="/static/js/lib-react.5c8909c28c.js"></script><script defer src="/static/js/6374.bc21d8b214.js"></script><script defer src="/static/js/index.1b6c679c84.js"></script><link href="/static/css/index.592272f49e.css" rel="stylesheet"></head>

  <body>
    <div id="root"></div>
@@ -13,7 +13,7 @@ import {
  Trash2,
 } from 'lucide-react'
 import { toast } from 'sonner'
-import { api } from '@/lib/api'
+import { api, noBusinessError, okOrThrow } from '@/lib/api'
 import { cn } from '@/lib/utils'
 import { Button } from '@/components/ui/button'
 import { Skeleton } from '@/components/ui/skeleton'
@@ -87,23 +87,10 @@ async function listAgents(): Promise<AgentItem[]> {
  return res.data?.data?.items ?? []
 }

-// The backend returns HTTP 200 with {success:false, error:{code,message}} for
-// business failures (new-api convention), so axios resolves — we must check the
-// envelope ourselves and throw, otherwise a failed call looks like a success.
-type Envelope = {
-  data?: { success?: boolean; message?: string; error?: { code?: string; message?: string } }
-}
 const FRIENDLY: Record<string, string> = {
  RUNTIME_UNAVAILABLE: '云端服务暂时不可用，请稍后再试',
  RESOURCE_BINDING_INVALID: '所选资源无效，请重新选择',
 }
-function okOrThrow(res: Envelope) {
-  const d = res?.data
-  if (d && d.success === false) {
-    const code = d.error?.code ?? ''
-    throw new Error(FRIENDLY[code] || d.error?.message || d.message || '操作失败')
-  }
-}

 export function DeploySubAgentPage() {
  const qc = useQueryClient()
@@ -129,11 +116,12 @@ export function DeploySubAgentPage() {

  const deploy = useMutation({
    mutationFn: async () => {
-      const res = await api.post('/api/heicode/agents', {
-        template_id: selectedTemplate,
-        binding_ids: selectedBindings,
-      })
-      okOrThrow(res)
+      const res = await api.post(
+        '/api/heicode/agents',
+        { template_id: selectedTemplate, binding_ids: selectedBindings },
+        noBusinessError
+      )
+      okOrThrow(res, FRIENDLY)
    },
    onSuccess: () => {
      toast.success('Agent 已开始部署')
@@ -330,7 +318,13 @@ function AgentRow({

  const stop = useMutation({
    mutationFn: async () => {
-      okOrThrow(await api.post(`/api/heicode/agents/${agent.agent_id}/stop`))
+      okOrThrow(
+        await api.post(
+          `/api/heicode/agents/${agent.agent_id}/stop`,
+          undefined,
+          noBusinessError
+        )
+      )
    },
    onSuccess: () => {
      toast.success('已停止')
@@ -340,7 +334,9 @@ function AgentRow({
  })
  const del = useMutation({
    mutationFn: async () => {
-      okOrThrow(await api.delete(`/api/heicode/agents/${agent.agent_id}`))
+      okOrThrow(
+        await api.delete(`/api/heicode/agents/${agent.agent_id}`, noBusinessError)
+      )
    },
    onSuccess: () => {
      toast.success('已删除')
@@ -349,10 +345,14 @@ function AgentRow({
    onError: (e) => toast.error(e instanceof Error ? e.message : '操作失败'),
  })

-  const copy = () => {
+  const copy = async () => {
    if (!agent.subdomain) return
-    void navigator.clipboard.writeText(agent.subdomain)
-    toast.success('已复制访问地址')
+    try {
+      await navigator.clipboard.writeText(agent.subdomain)
+      toast.success('已复制访问地址')
+    } catch {
+      toast.error('复制失败，请手动复制地址')
+    }
  }

  return (
@@ -374,7 +374,7 @@ function AgentRow({
        </p>
      </div>
      <div className='flex shrink-0 items-center gap-1'>
-        <Button variant='outline' size='sm' className='h-8 gap-1 text-xs' onClick={copy} disabled={!agent.subdomain}>
+        <Button variant='outline' size='sm' className='h-8 gap-1 text-xs' onClick={() => void copy()} disabled={!agent.subdomain}>
          <Copy className='h-3.5 w-3.5' /> 复制地址
        </Button>
        <Button
@@ -11,7 +11,7 @@ import {
 } from 'lucide-react'
 import { useTranslation } from 'react-i18next'
 import { toast } from 'sonner'
-import { api } from '@/lib/api'
+import { api, noBusinessError, okOrThrow } from '@/lib/api'
 import { cn } from '@/lib/utils'
 import { Button } from '@/components/ui/button'
 import { Input } from '@/components/ui/input'
@@ -131,6 +131,13 @@ const KINDS: Kind[] = [
  },
 ]

+const RESOURCE_TYPE_ZH: Record<string, string> = {
+  git: '代码仓库',
+  vm: '虚拟机',
+  database: '数据库',
+  blob: '对象存储',
+}
+
 function maskRef(ref: string): string {
  if (!ref) return '—'
  const i = ref.lastIndexOf('/')
@@ -159,7 +166,9 @@ export function ResourceBindingsPage() {
  })

  const revoke = useMutation({
-    mutationFn: (id: number) => api.delete(`/api/resources/${id}`),
+    mutationFn: async (id: number) => {
+      okOrThrow(await api.delete(`/api/resources/${id}`, noBusinessError))
+    },
    onSuccess: () => {
      toast.success(t('Resource unbound'))
      void qc.invalidateQueries({ queryKey: ['resources'] })
@@ -224,7 +233,7 @@ export function ResourceBindingsPage() {
            {Object.entries(grouped).map(([type, items]) => (
              <div key={type}>
                <p className='text-muted-foreground mb-2 text-[11px] font-semibold tracking-[0.14em] uppercase'>
-                  {type} · {items.length}
+                  {RESOURCE_TYPE_ZH[type] ?? type} · {items.length}
                </p>
                <div className='grid gap-3 sm:grid-cols-2'>
                  {items.map((r) => (
@@ -316,8 +325,10 @@ function BindSheet({
          metadata,
          permission_scope: {},
          constraints: {},
-        }
+        },
+        noBusinessError
      )
+      okOrThrow(created)
      const id = created.data?.data?.id
      if (!id) throw new Error(t('Failed to create binding'))
      // 2) write the credential to Key Vault (only secret fields)
@@ -327,7 +338,15 @@ function BindSheet({
        if (v) secretData[s.k] = v
      }
      if (Object.keys(secretData).length > 0) {
-        await api.post(`/api/resources/${id}/secret`, { data: secretData })
+        // Must verify success — a silent failure here means the credential
+        // never reached Key Vault while the binding row exists (unusable).
+        okOrThrow(
+          await api.post(
+            `/api/resources/${id}/secret`,
+            { data: secretData },
+            noBusinessError
+          )
+        )
      }
    },
    onSuccess: () => {
@@ -158,6 +158,43 @@ api.interceptors.request.use((config) => {
  return config
 })

+// ============================================================================
+// Business-envelope helpers
+// ============================================================================
+
+// Pass as the axios config to suppress the global interceptor's auto error
+// toast (which shows the raw backend message) — the caller handles the error
+// itself (e.g. with a friendly localized message via okOrThrow + onError).
+export const noBusinessError = {
+  skipBusinessError: true,
+  skipErrorHandler: true,
+} as Record<string, unknown>
+
+type BizEnvelope = {
+  data?: {
+    success?: boolean
+    message?: string
+    error?: { code?: string; message?: string }
+  }
+}
+
+// The backend returns HTTP 200 with { success:false, error } for business
+// failures (new-api convention), so axios resolves — callers MUST check the
+// envelope and throw, otherwise a failed call looks like a success. Optionally
+// map an error code to a friendly (localized) message.
+export function okOrThrow(res: BizEnvelope, friendly?: Record<string, string>) {
+  const d = res?.data
+  if (d && d.success === false) {
+    const code = d.error?.code ?? ''
+    throw new Error(
+      (friendly && friendly[code]) ||
+        d.error?.message ||
+        d.message ||
+        '操作失败'
+    )
+  }
+}
+
 // ============================================================================
 // Common API Functions
 // ============================================================================