[模型] 实现持久化 BYOK 模型目录与动态模型选择 - #155
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a persisted, gateway-sourced model catalog, private AI Gateway validation, catalog-backed model selection across backend and frontend flows, refresh APIs and UI, explicit Workbench model routing, Managed Agent validation, Quickstart model constraints, and related configuration, documentation, and tests. ChangesModel catalog and private gateway
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Postroggy
changed the title
![duckpr[bot]](https://avatars.githubusercontent.com/in/4166808?s=60&v=4){kind=small}
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
Important
整体架构清晰、安全面扎实（重定向阻断、SQL 参数化、authz fail-closed），但有两个建议在合并前处理的问题：失败记录路径使用了无超时限制的 context，可能无限持有刷新锁；以及 /v1/models 能力字段从硬编码改为透传后，需要确认网关确实返回 SDK/Console 客户端依赖的能力键。
Reviewed changes — 本 PR 将硬编码 Claude 模型列表替换为从私有 AI 网关同步的持久化 BYOK 模型目录，覆盖后端目录同步、快照持久化与 stale 语义、前后端动态模型选择、Agent 模型校验，以及网关 URL 安全校验。
- 新增
internal/modelcatalog/包 —Service提供原子刷新（全量分页后才发布）、advisory lock 跨实例去重、stale 快照保留与首次无快照时 503 语义 - 新增
internal/aiupstream/共享包 —Endpoint()校验网关 URL（拒绝anthropic.com、要求绝对 http/https、禁止凭证/query/fragment），NewHTTPClient()全局阻断重定向，统一替换 messages/batches/proxy/workbench 的 HTTP 客户端 - migration
00027+ sqlx 数据访问 —model_catalog_snapshots全局单行快照表，命名参数查询，复用 pgx 连接池；advisory lock 使用 poolAcquire的独立连接 /v1/models与 Console/models统一读取目录 — 删除buildPlatformModels硬编码列表，能力字段改为网关透传；Agent create/update 新增validateModelSelection目录校验- Workbench 新增
POST /models/refresh— admin-only authz，TryRefresh走refreshMu.TryLock+ advisory lock 双层去重 - 前端新增
model-catalogfeature 模块 —ModelCatalogSelect组件、React Query hooks、动态模型枚举注入 Quickstartbuild_agent_config - 删除全部 Claude fallback 路径 —
chatCompletionModel、chatModelFallbacks、miscDefaultChatModel等生产路径硬编码模型均已移除；main.go启动时ValidateDeployment失败则log.Fatalf
⚠️ 启动时 Refresh(ctx) 在 advisory lock 竞争下可阻塞至 RefreshTimeout
main.go 在启动路径同步调用 catalog.Refresh(ctx)。当多实例部署中另一个实例持有 advisory lock 且目录表为空时，refresh() 进入 waitForFirstSharedSnapshot(refreshCtx) 轮询分支，阻塞最多 15 秒（RefreshTimeout 默认值）后返回错误。
该错误只被 logger.Warn 记录，启动会继续，因此不是 fatal 的。但对于冷启动场景，15 秒的同步阻塞发生在 main() 的关键路径上（在 HTTP server ListenAndServe 之前），意味着新实例在这段时间内无法接受任何请求。单实例部署不受影响（advisory lock 总能获取）。
如果这是有意为之的"等待其他实例填充共享快照"行为，建议考虑将初始刷新改为异步，或为启动路径设置更短的超时。
Technical details
# 启动时 advisory lock 阻塞
## Affected sites
- `main.go:81` — `catalog.Refresh(ctx)` 同步调用
- `internal/modelcatalog/service.go:108-111` — `waitForSharedSnapshot && !s.hasSuccessfulSnapshot()` 进入轮询
- `internal/modelcatalog/service.go:152-169` — `waitForFirstSharedSnapshot` 每 100ms 轮询 `store.Load`，直到 `refreshCtx` 到期
## Required outcome
- 启动路径不应被模型目录刷新阻塞超过可接受的延迟阈值
- 或：初始刷新应异步执行，不阻塞 `ListenAndServe`
## Open questions for the human
- 多实例部署是否是当前的目标场景？如果是，15 秒冷启动延迟是否可接受？anthropic/glm-5.2 ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
web/src/features/managed-agents/agentConfig.ts (1)
345-362: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty
availableModelIDsbypasses model validation instead of enforcing it.
availableModelIDs.length && !availableModelIDs.includes(...)short-circuits tofalsewhen the catalog list is empty, so the requested model is trusted verbatim exactly when nothing is confirmed to be available — the opposite of what this guard should do. Every current caller happens to gate on a non-empty catalog before reaching this function, so it's not actively exploitable today, but it's a landmine for any future caller that doesn't pre-check.🐛 Proposed fix
- const requestedModel = quickstartModelInput(rawConfig.model, fallback.model); - const model = - availableModelIDs.length && !availableModelIDs.includes(agentModelName(requestedModel)) - ? fallback.model - : requestedModel; + const requestedModel = quickstartModelInput(rawConfig.model, fallback.model); + const model = availableModelIDs.includes(agentModelName(requestedModel)) ? requestedModel : fallback.model;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/features/managed-agents/agentConfig.ts` around lines 345 - 362, Update the model selection guard in quickstartBuildAgentConfigInput so an empty availableModelIDs catalog does not trust the requested model; treat the model as valid only when availableModelIDs is non-empty and includes agentModelName(requestedModel), otherwise use fallback.model. Preserve the existing requested-model behavior for confirmed available models.web/src/features/managed-agents/agents/detail.tsx (1)
1771-1803: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCmd/Ctrl+S bypasses the model-availability guard enforced on the Save button.
saveDisabled(line 1803) blocks the Save button when!selectedModelAvailable, but the document-level Cmd/Ctrl+S handler callssubmit()directly andsubmithas no equivalent check — unlikecreate-dialog.tsx'shandleCreate, which re-validatesmodelCatalog.modelIDs.includes(agentModelName(parsed.model))right before submitting, and unlike this file's ownselectFormata few lines above.🐛 Proposed fix
const submit = useCallback(async () => { if (submitting) { return; } const parsed = parseCurrentConfig(); if (!parsed) { return; } + if (!modelCatalog.modelIDs.includes(agentModelName(parsed.model))) { + setSaveError(msg('managedAgents.agents.editDialog.selectModel', 'Select an available model first.')); + return; + } setSubmitting(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/features/managed-agents/agents/detail.tsx` around lines 1771 - 1803, Update the submit callback and its Cmd/Ctrl+S path to revalidate model availability before calling updateAgentDetail, using modelCatalog.modelIDs and the parsed agent model as in selectFormat and create-dialog.tsx. Return without submitting when the selected model is unavailable, while preserving the existing button behavior.web/src/features/workbench/drawers.tsx (1)
86-113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSynthetic "current model" entry is mislabeled as "Available model" when it's actually unavailable.
When
draft.model_nameisn't found inmodels,modelOptionsprepends a bare{ model_name: draft.model_name }placeholder. Since it has nodescription/display_name/name,modelDescription(Line 419) falls back to'Available model'— the opposite of what's true for this exact stale/unavailable-model scenario this PR is meant to surface.💡 Proposed fix
const modelOptions = useMemo(() => { if (!draft.model_name || models.some((model) => model.model_name === draft.model_name)) { return models; } - return [{ model_name: draft.model_name }, ...models]; + return [ + { model_name: draft.model_name, description: 'Not available in the current model catalog' }, + ...models, + ]; }, [draft.model_name, models]);Also applies to: 418-420
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/features/workbench/drawers.tsx` around lines 86 - 113, Update the synthetic current-model entry created in the modelOptions useMemo when draft.model_name is absent from models so its metadata produces an unavailable-model label instead of the modelDescription fallback “Available model”; preserve the existing behavior for models returned from the server and ensure filtering still includes the stale model name.web/src/features/workbench/WorkbenchPage.tsx (1)
1033-1041: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
runEvaluateRowssilently no-ops when no catalog model is selected.Unlike
runPrompt/openImprovePrompt/openPromptGenerator, which setrunErrorand open the model drawer whenhasSelectedCatalogModelis false,runEvaluateRows's guard (Line 1038) just returns with no feedback. A user pressing ⌘+Enter in the Evaluate tab with no model selected gets no indication why nothing happened.💡 Proposed fix
- if (!orgUuid || !prompt || !evaluateRows.length || hasUnsavedChanges || !hasSelectedCatalogModel) { + if (!orgUuid || !prompt || !evaluateRows.length || hasUnsavedChanges) { return; } + if (!hasSelectedCatalogModel) { + setRunError('Select an available model before running evaluations.'); + setActiveDrawer('model'); + return; + }Also applies to: 1253-1264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/features/workbench/WorkbenchPage.tsx` around lines 1033 - 1041, Update the guard in runEvaluateRows, and the corresponding Evaluate-tab handler around the referenced secondary range, to handle !hasSelectedCatalogModel like runPrompt, openImprovePrompt, and openPromptGenerator: set runError and open the model drawer before returning. Preserve the existing no-op behavior for the other invalid preconditions.
🧹 Nitpick comments (5)
internal/models/handler.go (1)
42-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd logging on the new catalog-unavailable (503) paths. Both handlers introduce new failure modes (nil catalog /
Snapshot()/ValidateModel()errors) that silently return 503 with no server-side log, unlike the existing DB-error branches inagents/handler.gothat calllog.Printf. Without a log line, diagnosing catalog refresh/connectivity problems in production requires reproducing the client-visible 503.
internal/models/handler.go#L42-L96: add alog.Printf/structured log call in theh.catalog == nilandSnapshot()error branches oflistbefore callingwriteCatalogUnavailable.internal/agents/handler.go#L135-L162: add a log call inwriteStateError'smodelcatalog.IsUnavailable(err)branch (or insidevalidateModelSelectionwhere the underlying error is wrapped) so the root cause isn't lost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/models/handler.go` around lines 42 - 96, The catalog-unavailable paths currently return 503 without server-side diagnostics. In internal/models/handler.go, add logging in Handler.list for both the nil h.catalog branch and the Snapshot error branch before writeCatalogUnavailable, including the relevant error or unavailable context; in internal/agents/handler.go, add logging in writeStateError’s modelcatalog.IsUnavailable(err) branch (or validateModelSelection where the error is wrapped) so the underlying cause is retained. Use the existing logging convention, including log.Printf where appropriate.internal/platformapi/platform_proxy.go (1)
49-49: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBuild the aiupstream client once, not per request.
Every other call site in this PR (
messages/handler.go,batches/upstream.go) constructs theaiupstreamHTTP client once and reuses it; here a new client is allocated inside the per-request closure on every call tohandleProxyMessages.♻️ Proposed fix: hoist client construction out of the request path
+var proxyMessagesClient = aiupstream.NewHTTPClient(nil, 0) + func handleProxyMessages(cfg config.Config) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ... - upstreamRes, err := aiupstream.NewHTTPClient(nil, 0).Do(upstreamReq) + upstreamRes, err := proxyMessagesClient.Do(upstreamReq)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platformapi/platform_proxy.go` at line 49, Hoist aiupstream HTTP client construction out of the per-request closure in handleProxyMessages, creating it once during setup and reusing it for each upstream request. Replace the inline NewHTTPClient call at the upstream request site while preserving the existing request handling behavior.internal/platformapi/model_catalog.go (1)
19-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging the swallowed
Snapshoterror.A failing catalog silently yields an empty
claude_ai_bootstrap_models_config, giving no operational signal for why the console shows no models. A single log line here would aid debugging without changing the graceful-degradation behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platformapi/model_catalog.go` around lines 19 - 22, In the Snapshot error branch of the catalog-loading function, log the captured err with clear context before returning platformModelCatalog{}. Preserve the existing graceful fallback and return behavior.internal/workbench/console_platform_workbench.go (1)
654-663: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftVerify the role gate before allowing refresh.
DB.GetOrganizationUserRolereads theusers.rolecolumn directly, while console member roles are normalized separately (owner,primary_owner,membership_admin→admin). Make sure model-catalog access uses the app-level authorized role orDB.GetOrganizationUserRolereturns the canonical role for these refresh checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/workbench/console_platform_workbench.go` around lines 654 - 663, The model-catalog refresh authorization currently compares the raw role from GetOrganizationUserRole against "admin" and can reject normalized admin roles. Update the role gate around GetOrganizationUserRole to use the app-level canonical authorization role, or ensure GetOrganizationUserRole returns canonical "admin" for owner, primary_owner, and membership_admin before the comparison.web/src/features/model-catalog/api.ts (1)
12-18: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake
csrfTokenrequired onrefreshModelCatalog.This is a mutating (
POST) request, and per project guidelines every cookie-authenticated mutation must sendX-CSRF-Token. MakingcsrfTokenoptional lets a future caller silently omit it.♻️ Proposed fix
-export function refreshModelCatalog(orgUuid: string, csrfToken?: string) { +export function refreshModelCatalog(orgUuid: string, csrfToken: string) {Based on learnings, no direct precedent found here, but as per coding guidelines: "所有基于 cookie 鉴权的变更请求必须发送
X-CSRF-Token".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/features/model-catalog/api.ts` around lines 12 - 18, Make the csrfToken parameter required in refreshModelCatalog, preserving its use in the POST request options so every caller must provide the CSRF token.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/modelcatalog/service.go`:
- Around line 50-58: Update the cold-start snapshot handling after store.Load in
the surrounding snapshot-loading function to retain the normalized models
returned by normalizeModels. Assign the normalized result back to stored.Models
before the snapshot is cached, while preserving the existing error wrapping and
exists checks.
- Around line 224-261: Update Service.fetchAll to reject a completed upstream
response when the accumulated models list is empty, returning the existing
invalid/incomplete response error instead of an empty successful result.
Preserve normal pagination and successful non-empty catalog behavior, and ensure
refresh cannot persist an empty snapshot.
In `@internal/workbench/console_platform_workbench_test.go`:
- Around line 314-351: Reorder the tests so
TestWorkbenchModelCatalogRefreshReportsConcurrentRefresh appears before
TestWorkbenchModelCatalogRefreshReturnsUpdatedCatalog, without changing either
test’s implementation or behavior.
In `@internal/workbench/console_platform_workbench.go`:
- Around line 654-659: Remove principal.UserExternalID from the authorization
failure log in the model catalog refresh handler. Keep the organization ID and
error details in the log.Printf call, while preserving the existing HTTP 500
response and return behavior.
In `@web/src/features/model-catalog/ModelCatalogSelect.tsx`:
- Around line 28-52: The new model-catalog UI strings bypass i18n. In
web/src/features/model-catalog/ModelCatalogSelect.tsx#L28-L52, use useI18n/msg
for the loading, unavailable, select-model, and stale-catalog strings; in
web/src/features/dashboard/home.tsx#L308-L333, localize the context tag with the
existing dashboard.models.tags.context pattern; and in
web/src/features/managed-agents/quickstart/AgentQuickstartPage.tsx#L269-L274,
replace the hardcoded setChatError text with the
managedAgents.quickstart.selectModel message, preserving the existing guard
behavior.
In `@web/src/features/workbench/WorkbenchPage.tsx`:
- Around line 2433-2435: Update the improvePromptLabel/improvePromptActionLabel
derivation and its corresponding button usage so read-only prompts produce the
same non-actionable label state as the disabled condition. Apply the same
adjustment to the additional improve-prompt button instance, while preserving
existing labels for editable prompts and other disabled conditions.
---
Outside diff comments:
In `@web/src/features/managed-agents/agentConfig.ts`:
- Around line 345-362: Update the model selection guard in
quickstartBuildAgentConfigInput so an empty availableModelIDs catalog does not
trust the requested model; treat the model as valid only when availableModelIDs
is non-empty and includes agentModelName(requestedModel), otherwise use
fallback.model. Preserve the existing requested-model behavior for confirmed
available models.
In `@web/src/features/managed-agents/agents/detail.tsx`:
- Around line 1771-1803: Update the submit callback and its Cmd/Ctrl+S path to
revalidate model availability before calling updateAgentDetail, using
modelCatalog.modelIDs and the parsed agent model as in selectFormat and
create-dialog.tsx. Return without submitting when the selected model is
unavailable, while preserving the existing button behavior.
In `@web/src/features/workbench/drawers.tsx`:
- Around line 86-113: Update the synthetic current-model entry created in the
modelOptions useMemo when draft.model_name is absent from models so its metadata
produces an unavailable-model label instead of the modelDescription fallback
“Available model”; preserve the existing behavior for models returned from the
server and ensure filtering still includes the stale model name.
In `@web/src/features/workbench/WorkbenchPage.tsx`:
- Around line 1033-1041: Update the guard in runEvaluateRows, and the
corresponding Evaluate-tab handler around the referenced secondary range, to
handle !hasSelectedCatalogModel like runPrompt, openImprovePrompt, and
openPromptGenerator: set runError and open the model drawer before returning.
Preserve the existing no-op behavior for the other invalid preconditions.
---
Nitpick comments:
In `@internal/models/handler.go`:
- Around line 42-96: The catalog-unavailable paths currently return 503 without
server-side diagnostics. In internal/models/handler.go, add logging in
Handler.list for both the nil h.catalog branch and the Snapshot error branch
before writeCatalogUnavailable, including the relevant error or unavailable
context; in internal/agents/handler.go, add logging in writeStateError’s
modelcatalog.IsUnavailable(err) branch (or validateModelSelection where the
error is wrapped) so the underlying cause is retained. Use the existing logging
convention, including log.Printf where appropriate.
In `@internal/platformapi/model_catalog.go`:
- Around line 19-22: In the Snapshot error branch of the catalog-loading
function, log the captured err with clear context before returning
platformModelCatalog{}. Preserve the existing graceful fallback and return
behavior.
In `@internal/platformapi/platform_proxy.go`:
- Line 49: Hoist aiupstream HTTP client construction out of the per-request
closure in handleProxyMessages, creating it once during setup and reusing it for
each upstream request. Replace the inline NewHTTPClient call at the upstream
request site while preserving the existing request handling behavior.
In `@internal/workbench/console_platform_workbench.go`:
- Around line 654-663: The model-catalog refresh authorization currently
compares the raw role from GetOrganizationUserRole against "admin" and can
reject normalized admin roles. Update the role gate around
GetOrganizationUserRole to use the app-level canonical authorization role, or
ensure GetOrganizationUserRole returns canonical "admin" for owner,
primary_owner, and membership_admin before the comparison.
In `@web/src/features/model-catalog/api.ts`:
- Around line 12-18: Make the csrfToken parameter required in
refreshModelCatalog, preserving its use in the POST request options so every
caller must provide the CSRF token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd051ff3-e356-4d18-b0c6-076e3310fc7a
📒 Files selected for processing (84)
CONTEXT.mdconfig/config.example.yamldeploy/docker-compose/oma-server.yamldocs/configuration-reference.yamldocs/design/be/byok-model-catalog.mddocs/design/be/messages-proxy.mddocs/design/be/runtime-configuration.mddocs/design/docker-compose-deployment.mdinternal/agents/handler.gointernal/agents/model_catalog_test.gointernal/aiupstream/upstream.gointernal/aiupstream/upstream_test.gointernal/api/server.gointernal/batches/upstream.gointernal/config/config.gointernal/config/config_test.gointernal/config/defaults.gointernal/config/reference_test.gointernal/config/types.gointernal/config/yaml_types.gointernal/db/admin.gointernal/db/migrations/00027_add_model_catalog_snapshots.sqlinternal/db/model_catalog.gointernal/db/model_catalog_test.gointernal/messages/handler.gointernal/modelcatalog/postgres_store.gointernal/modelcatalog/service.gointernal/modelcatalog/service_test.gointernal/modelcatalog/types.gointernal/modelcatalog/upstream.gointernal/modelcatalog/upstream_test.gointernal/models/handler.gointernal/models/handler_test.gointernal/platformapi/model_catalog.gointernal/platformapi/platform_auth_routes.gointernal/platformapi/platform_backend_routes.gointernal/platformapi/platform_bootstrap.gointernal/platformapi/platform_bootstrap_builders.gointernal/platformapi/platform_bootstrap_features.gointernal/platformapi/platform_proxy.gointernal/platformapi/support.gointernal/workbench/console_platform_workbench.gointernal/workbench/console_platform_workbench_test.gointernal/workbench/workbench_support.gomain.goscripts/ant-batchestests/files_api_test.gotests/models_api_test.gotests/platform_console_backend_api_test.goweb/src/features/dashboard/DashboardPage.test.tsxweb/src/features/dashboard/feature-pages.tsxweb/src/features/dashboard/home.tsxweb/src/features/dashboard/skills.tsxweb/src/features/managed-agents/ManagedAgentsPage.agents.suite.tsxweb/src/features/managed-agents/ManagedAgentsPage.quickstart.suite.tsxweb/src/features/managed-agents/ManagedAgentsPage.test-utils.tsxweb/src/features/managed-agents/agentConfig.test.tsweb/src/features/managed-agents/agentConfig.tsweb/src/features/managed-agents/agents/create-dialog.tsxweb/src/features/managed-agents/agents/detail.tsxweb/src/features/managed-agents/agents/model.tsxweb/src/features/managed-agents/quickstart/AgentQuickstartPage.tsxweb/src/features/managed-agents/quickstart/components.tsxweb/src/features/managed-agents/quickstart/platformQuickstartRequest.test.tsweb/src/features/managed-agents/quickstart/platformQuickstartRequest.tsweb/src/features/managed-agents/quickstart/quickstartPromptText.tsweb/src/features/managed-agents/resources/ManagedResources.tsxweb/src/features/model-catalog/ModelCatalogSelect.tsxweb/src/features/model-catalog/api.tsweb/src/features/model-catalog/hooks.tsweb/src/features/model-catalog/model.test.tsweb/src/features/model-catalog/model.tsweb/src/features/workbench/WorkbenchPage.test.tsxweb/src/features/workbench/WorkbenchPage.tsxweb/src/features/workbench/api.tsweb/src/features/workbench/dialogs.tsxweb/src/features/workbench/drawers.tsxweb/src/features/workbench/editor.tsxweb/src/features/workbench/evaluate.tsxweb/src/features/workbench/model.test.tsweb/src/features/workbench/model.tsweb/src/shared/i18n/messages/en.jsonweb/src/shared/i18n/messages/zh-CN.jsonweb/src/styles/workbench/shell-editor.css
💤 Files with no reviewable changes (1)
- internal/platformapi/support.go
|
已完成本轮 bot 评审处理，并推送提交 ab82c84。 除逐条回复的 inline comment 外，总评中的跨文件问题也已处理：
本轮验证结果：
go test ./... 的 tests 集成包仍依赖本机 MinIO（127.0.0.1:9000），当前未启动时会在文件 API/E2E 用例初始化阶段失败；同时仍能复现 PR 正文记录的共享数据库顺序基线问题。这些失败不来自本轮修改模块，已在 PR 正文保留说明。 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/features/managed-agents/agents/create-dialog.tsx (1)
447-453: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not discard unsaved config edits when changing the model.
If
parseCurrentConfig()fails, this handler falls back toconfigInputRef.current;hydrateConfig()then rewrites the editor from that stale last-valid config and clears the error. Selecting a model can therefore silently erase the user’s invalid-but-unsaved changes.Return early when parsing fails (or disable the selector while
configErroris set) instead of hydrating from the ref.Suggested fix
onValueChange={(modelID) => { const parsed = parseCurrentConfig(); + if (!parsed) { + return; + } - hydrateConfig({ ...(parsed ?? configInputRef.current), model: modelID }); + hydrateConfig({ ...parsed, model: modelID }); }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/features/managed-agents/agents/create-dialog.tsx` around lines 447 - 453, Update the ModelCatalogSelect onValueChange handler to return immediately when parseCurrentConfig() fails, rather than falling back to configInputRef.current. Only call hydrateConfig with the parsed configuration and selected model when parsing succeeds, preserving invalid unsaved editor changes.internal/workbench/console_platform_workbench.go (1)
2168-2174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep revision
model_nameas the catalog ID.This rewrites API-facing revision state to the mapped gateway ID, while
/modelsreturns the unmapped catalog IDs. With a non-identity mapping, loaded or newly created revisions no longer match an available catalog model. Preserve the catalog ID in revisions; applymodelmapping.Resolveonly inworkbenchAnthropicRequestimmediately before the upstream call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/workbench/console_platform_workbench.go` around lines 2168 - 2174, Update resolveWorkbenchRevisionModel to leave revision["model_name"] unchanged as the catalog ID and remove the mapping mutation. Apply modelmapping.Resolve only within workbenchAnthropicRequest immediately before sending the upstream request, while preserving the catalog ID for loaded and newly created revisions.
🧹 Nitpick comments (1)
internal/modelcatalog/service_test.go (1)
100-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlace failure scenarios before this success scenario.
Move this deduplication success-path test below the refresh-failure cases to preserve the required test ordering.
As per coding guidelines, “Tests should present failure scenarios before success scenarios.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/modelcatalog/service_test.go` around lines 100 - 130, Move TestServiceRefreshDeduplicatesEffectiveModelIDsAcrossPages below the refresh-failure test cases in the test file, preserving its implementation and assertions unchanged while ensuring failure scenarios appear before this success-path test.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/workbench/console_platform_workbench.go`:
- Around line 2168-2174: Update resolveWorkbenchRevisionModel to leave
revision["model_name"] unchanged as the catalog ID and remove the mapping
mutation. Apply modelmapping.Resolve only within workbenchAnthropicRequest
immediately before sending the upstream request, while preserving the catalog ID
for loaded and newly created revisions.
In `@web/src/features/managed-agents/agents/create-dialog.tsx`:
- Around line 447-453: Update the ModelCatalogSelect onValueChange handler to
return immediately when parseCurrentConfig() fails, rather than falling back to
configInputRef.current. Only call hydrateConfig with the parsed configuration
and selected model when parsing succeeds, preserving invalid unsaved editor
changes.
---
Nitpick comments:
In `@internal/modelcatalog/service_test.go`:
- Around line 100-130: Move
TestServiceRefreshDeduplicatesEffectiveModelIDsAcrossPages below the
refresh-failure test cases in the test file, preserving its implementation and
assertions unchanged while ensuring failure scenarios appear before this
success-path test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1eb8745e-9002-4daa-9108-2c84075a44a6
📒 Files selected for processing (36)
docs/configuration-reference.yamldocs/design/be/messages-proxy.mddocs/design/be/runtime-configuration.mdinternal/agents/handler.gointernal/config/config.gointernal/config/config_test.gointernal/config/types.gointernal/modelcatalog/service.gointernal/modelcatalog/service_test.gointernal/modelcatalog/upstream.gointernal/modelcatalog/upstream_test.gointernal/models/handler.gointernal/platformapi/model_catalog.gointernal/platformapi/platform_proxy.gointernal/workbench/console_platform_workbench.gointernal/workbench/console_platform_workbench_test.gointernal/workbench/workbench_support.gomain.goweb/src/features/dashboard/feature-pages.tsxweb/src/features/dashboard/home.tsxweb/src/features/managed-agents/ManagedAgentsPage.agents.suite.tsxweb/src/features/managed-agents/ManagedAgentsPage.test-utils.tsxweb/src/features/managed-agents/agentConfig.test.tsweb/src/features/managed-agents/agentConfig.tsweb/src/features/managed-agents/agents/create-dialog.tsxweb/src/features/managed-agents/agents/detail.tsxweb/src/features/managed-agents/quickstart/AgentQuickstartPage.tsxweb/src/features/managed-agents/quickstart/components.tsxweb/src/features/model-catalog/ModelCatalogSelect.tsxweb/src/features/model-catalog/api.tsweb/src/features/model-catalog/hooks.tsweb/src/features/workbench/WorkbenchPage.test.tsxweb/src/features/workbench/WorkbenchPage.tsxweb/src/features/workbench/drawers.tsxweb/src/shared/i18n/messages/en.jsonweb/src/shared/i18n/messages/zh-CN.json
🚧 Files skipped from review as they are similar to previous changes (29)
- internal/config/config.go
- internal/modelcatalog/upstream_test.go
- internal/config/types.go
- main.go
- web/src/features/model-catalog/api.ts
- internal/platformapi/model_catalog.go
- internal/config/config_test.go
- docs/design/be/messages-proxy.md
- web/src/features/managed-agents/ManagedAgentsPage.agents.suite.tsx
- web/src/features/model-catalog/ModelCatalogSelect.tsx
- docs/configuration-reference.yaml
- web/src/features/model-catalog/hooks.ts
- internal/platformapi/platform_proxy.go
- web/src/features/dashboard/feature-pages.tsx
- web/src/features/dashboard/home.tsx
- web/src/features/managed-agents/agents/detail.tsx
- internal/modelcatalog/upstream.go
- web/src/features/managed-agents/ManagedAgentsPage.test-utils.tsx
- docs/design/be/runtime-configuration.md
- internal/models/handler.go
- internal/agents/handler.go
- internal/modelcatalog/service.go
- internal/workbench/workbench_support.go
- web/src/features/managed-agents/quickstart/AgentQuickstartPage.tsx
- web/src/features/workbench/WorkbenchPage.test.tsx
- web/src/features/workbench/drawers.tsx
- internal/workbench/console_platform_workbench_test.go
- web/src/features/managed-agents/agentConfig.ts
- web/src/features/workbench/WorkbenchPage.tsx
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
Important
增量改动整体质量高：model_mappings 在七个边界（/v1/models、Console /models、目录快照、Workbench 推理、Quickstart、Agent 写入、environment-manager payload）一致解析，链/循环在 config-load 强制拒绝，前次评审的三条反馈（recordFailure 用 refreshCtx、启动刷新异步化、能力字段透传合同）均已修复。有一个建议合并前处理的问题：canRefreshModelCatalog 接受的三个角色在生产 DB 下不可达，且测试用 passthrough mock 掩盖了这一不可达性。
Reviewed changes — 本次评审覆盖 commit ab82c84（自上次 Pullfrog 评审以来的增量），重点为新增 anthropic_upstream.model_mappings 配置与跨边界模型 ID 解析，以及对前次评审反馈的修复。
- 新增
internal/modelmapping包与model_mappings配置 —Resolve/Validate提供 trim、identity、many-to-one 别名；链（A->B->C）与循环在internal/config/config.goparse 时拒绝，而非仅文档约定 - Workbench 请求构造归一化到
newWorkbenchAnthropicRequest— completions/title/prompt/test-case 统一在该边界maps.Clonebody 后解析 top-levelmodel，对 caller 非破坏；fallback 流改用映射后的effectiveModel - 修复前次评审反馈 —
recordFailure两处均改用refreshCtx（共享刷新超时预算，新增failureHadDeadline测试守卫）；main.go初始catalog.Refresh改为 goroutine 不阻塞ListenAndServe，与StartRefreshLoop经refreshMu串行无竞态；加载快照时normalizeModels结果回写stored.Models - 模型目录去重语义调整 — effective ID 重复时由"拒绝并保留 stale 快照"改为"first-slot-wins 去重"，测试更名为
TestServiceRefreshDeduplicatesEffectiveModelIDsAcrossPages - 前端修复前次反馈 — detail.tsx Cmd/Ctrl+S 重新校验模型可用性；drawers.tsx stale 模型项标记 "Not available in current model catalog"；WorkbenchPage
runEvaluateRows缺目录模型时打开 model drawer；model-catalog UI 字符串 i18n（en + zh-CN）；refreshModelCatalog的csrfToken改为必填 - proxy client hoist —
handleProxyMessages的aiupstreamclient 提到闭包外层，一次构造多请求复用，正确阻断重定向
ℹ️ Nitpicks
internal/workbench/console_platform_workbench.go:1388—handleWorkbenchCompletions在调用workbenchAnthropicRequest前显式执行了一次_, err = anthropicMessagesEndpoint(upstreamConfig)，而后者内部已经过aiupstream.ValidateDeployment+anthropicMessagesEndpoint。该冗余调用只能产生一个稍早的 502（endpoint 错误）而非 500（"failed to build Anthropic request"），若该错误响应区分是有意为之可忽略，否则可删除。
anthropic/glm-5.2 ｜ 𝕏
|
本轮 review 已在 8841752 处理：1）创建 Agent 时，JSON 配置无效期间切换模型不再用旧配置覆盖未保存编辑，并新增回归测试；2）目录刷新权限收敛为生产角色词表中唯一可达的 admin，非管理员与不可持久化角色均覆盖 403；3）ModelCatalog 成功去重测试已移到失败场景之后。关于“Revision 应保留映射前 model_name”的建议未采纳： |
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ No new issues found.
Reviewed changes — 本次评审覆盖 commit 8841752（自上次 Pullfrog 评审 ab82c84 以来的增量），全部为对前轮评审反馈的修复，无新问题。
- 收窄
canRefreshModelCatalog到生产词表 — 移除owner/primary_owner/membership_admin三个在users.roleCHECK 约束下不可达的分支；新增测试覆盖user/developer/billing/claude_code_user及三个伪特权角色均返回 403，避免 mock 再次掩盖真实角色合同 - 修复
create-dialog.tsx模型选择丢失未保存编辑 —ModelCatalogSelect的onValueChange在parseCurrentConfig()失败时提前 return，不再回退到configInputRef.current，从而保留用户的无效但未保存编辑 - 测试顺序调整 — 将
TestServiceRefreshDeduplicatesEffectiveModelIDsAcrossPages移到刷新失败用例之后，符合"先失败后成功"的测试组织约定 - 新增 Cmd+S 不可用模型守卫测试 —
ManagedAgentsPage.agents.suite.tsx新增does not save an unavailable model with Cmd+S用例，验证 detail.tsx 的快捷键路径在模型不在目录时阻断提交并提示 - 新增 create-dialog 模型选择交互测试 — 验证在 JSON 无效状态下切换模型不丢弃错误提示且选择器回退到默认模型
｜ View workflow run ｜ Using anthropic/glm-5.2 ｜ 𝕏
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 本次评审覆盖 commit 809d6f7（自上次 Pullfrog 评审 8841752 以来的增量），将未配置有效 default_model_id 时的模型选择行为从"要求用户显式选择"改为"初始选择目录第一项"。
- 调整
resolveCatalogDefaultModelID默认回退 —default_available为 false 时返回catalogModelIDs(...)[0] ?? ''，空目录仍返回空字符串保持未选择状态，所有消费者经useModelCatalog().defaultModelID统一读取 - 同步更新设计文档与配置注释 — CONTEXT.md、byok-model-catalog.md、messages-proxy.md、runtime-configuration.md、config 示例均改为描述"未配置默认值时选择目录第一项"，domain contract 中的 Model Selection 定义同步更新
- 新增前端回归测试 —
model.test.ts更新断言验证首项回退；ManagedAgentsPage.quickstart.suite.tsx新增用例验证 UI 初始选择目录第一项；test-utils扩展modelCatalog参数支持按测试注入自定义目录（全部 139 项 ManagedAgentsPage 测试通过）
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/features/model-catalog/model.ts (1)
29-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize catalog entries during membership checks.
catalogModelIDstrimsmodel_name, butisCatalogModelIDcompares the raw catalog value. A whitespace-padded upstream ID can therefore appear inmodelIDswhile being rejected as unavailable during default resolution.export function isCatalogModelID(modelID: string, models: ModelCatalogModel[]) { const normalizedModelID = modelID.trim(); return ( normalizedModelID.length > 0 && - models.some((model) => model.model_name === normalizedModelID) + models.some((model) => model.model_name.trim() === normalizedModelID) ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/features/model-catalog/model.ts` around lines 29 - 40, Update isCatalogModelID to trim each model.model_name before comparing it with the normalized modelID, matching the normalization performed by catalogModelIDs. Preserve the existing empty-ID guard and membership-check behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/src/features/model-catalog/model.ts`:
- Around line 29-40: Update isCatalogModelID to trim each model.model_name
before comparing it with the normalized modelID, matching the normalization
performed by catalogModelIDs. Preserve the existing empty-ID guard and
membership-check behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b304c9bc-db59-4614-8903-fdebca8ff34d
📒 Files selected for processing (17)
CONTEXT.mdconfig/config.example.yamldeploy/docker-compose/oma-server.yamldocs/configuration-reference.yamldocs/design/be/byok-model-catalog.mddocs/design/be/messages-proxy.mddocs/design/be/runtime-configuration.mddocs/design/docker-compose-deployment.mdinternal/modelcatalog/service_test.gointernal/workbench/console_platform_workbench.gointernal/workbench/console_platform_workbench_test.goweb/src/features/managed-agents/ManagedAgentsPage.agents.suite.tsxweb/src/features/managed-agents/ManagedAgentsPage.quickstart.suite.tsxweb/src/features/managed-agents/ManagedAgentsPage.test-utils.tsxweb/src/features/managed-agents/agents/create-dialog.tsxweb/src/features/model-catalog/model.test.tsweb/src/features/model-catalog/model.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- CONTEXT.md
- web/src/features/model-catalog/model.test.ts
- config/config.example.yaml
- docs/design/docker-compose-deployment.md
- docs/design/be/byok-model-catalog.md
- docs/configuration-reference.yaml
- docs/design/be/runtime-configuration.md
- web/src/features/managed-agents/ManagedAgentsPage.agents.suite.tsx
- deploy/docker-compose/oma-server.yaml
- docs/design/be/messages-proxy.md
- web/src/features/managed-agents/agents/create-dialog.tsx
- web/src/features/managed-agents/ManagedAgentsPage.test-utils.tsx
- internal/workbench/console_platform_workbench_test.go
- internal/workbench/console_platform_workbench.go
- internal/modelcatalog/service_test.go
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 本次评审覆盖 commit da1d687（自上次 Pullfrog 评审 74a86ae 以来的增量），为纯结构性重构，无运行行为变化，无新问题。
- 统一 AI Gateway 包语义 — 将共享 Gateway 安全与地址策略包从
internal/aiupstream重命名为internal/aigateway，全仓 9 处调用方（messages、batches、proxy、workbench、modelcatalog upstream、config、config_test）一致更新，无残留aiupstream引用 - Agent Handler 依赖对象入口 — 将
NewHandler、NewHandlerWithSkillPrewarm、NewHandlerWithModelCatalogAndSkillPrewarm三个按依赖组合命名的构造器合并为单一agents.NewHandler(agents.HandlerDeps{...})，SkillPrewarmEnqueuer接口改为导出；api/server.go与全部agents测试同步迁移 - 网关校验纳入
config.Load()统一生命周期 —aigateway.ValidateConfig与modelmapping.Validate移入config.validate()，main.go不再执行第二次独立校验；Load()已调用validate()，无校验空窗 - 测试辅助拆分 — 新增
loadResolvedConfigTestFile（加载但不校验）与loadValidatedConfigTestFile（加载并校验），使 Docker Compose 模板的"有意缺失网关凭证"场景可被TestDockerComposeTemplateRequiresLocalGatewayCredentials断言为校验失败，而非跳过校验 ServerDeps.ModelCatalog字段 — 目录通过依赖对象注入Server，并向 platform account/email-login/org-root/workbench 路由透传；nil 时回退modelcatalog.UnavailableReader{}，保证未注入目录时 fail-closed 503
go build ./...、go test ./internal/config/... ./internal/agents/... ./internal/aigateway/... 均通过。
｜ View workflow run ｜ Using anthropic/glm-5.2 ｜ 𝕏
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ No new issues found.
Reviewed changes — 本次评审覆盖 commit ca622d5（自上次 Pullfrog 评审以来的增量），该提交是对前次评审"canRefreshModelCatalog 接受生产 DB 不可达角色"反馈的直接修复。
- 删除
(*db.DB).GetOrganizationUserRole包装函数 — 该函数此前吞掉ErrNotFound并返回("", nil)，掩盖了"用户不存在"这一根因；现在调用方直接使用GetAdminUser并显式处理 not-found handleWorkbenchModelCatalogRefresh显式区分db.ErrNotFound— 用户不存在时返回 403（而非 500），语义上更准确地表达"该 principal 无权刷新目录"workbenchModelCatalogRoleStore重命名为workbenchModelCatalogUserStore— 接口方法改为GetAdminUser返回完整db.AdminUser，去掉了原来只返回 raw role 字符串的窄接口；测试 store 现在可注入err字段，使ErrNotFound路径可被测试覆盖- 新增
TestWorkbenchModelCatalogRefreshRejectsMissingOrganizationUser— 验证db.ErrNotFound时返回 403 且不触发刷新；TestWorkbenchModelCatalogRefreshRequiresOrganizationAdmin已覆盖生产users.role词表中的全部非 admin 角色
canRefreshModelCatalog 现在严格限定为 admin，与前次评审中指出的生产 DB CHECK 约束一致；go vet 与 5 个刷新相关测试全部通过。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/config/config_test.go (1)
142-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlace this failure scenario before the success scenario.
Move
TestLoadRejectsInvalidAIGatewayConfigbeforeTestLoadYAMLConfigAndResolvePathsso the file presents invalid configurations before successful loading. As per coding guidelines, “Tests should present failure scenarios before success scenarios.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/config_test.go` around lines 142 - 174, Move the TestLoadRejectsInvalidAIGatewayConfig function before TestLoadYAMLConfigAndResolvePaths in the test file, preserving both test implementations unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/config/config_test.go`:
- Around line 142-174: Move the TestLoadRejectsInvalidAIGatewayConfig function
before TestLoadYAMLConfigAndResolvePaths in the test file, preserving both test
implementations unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b6f37ee1-53a0-455f-a189-face96b13391
📒 Files selected for processing (15)
internal/agents/handler.gointernal/agents/model_catalog_test.gointernal/aigateway/gateway.gointernal/aigateway/gateway_test.gointernal/api/server.gointernal/batches/upstream.gointernal/config/config.gointernal/config/config_test.gointernal/messages/handler.gointernal/modelcatalog/upstream.gointernal/platformapi/platform_proxy.gointernal/workbench/console_platform_workbench.gointernal/workbench/console_platform_workbench_test.gointernal/workbench/workbench_support.gomain.go
💤 Files with no reviewable changes (1)
- main.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/messages/handler.go
- internal/workbench/workbench_support.go
- internal/api/server.go
- internal/agents/model_catalog_test.go
- internal/modelcatalog/upstream.go
- internal/workbench/console_platform_workbench_test.go
- internal/workbench/console_platform_workbench.go
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ No new issues found.
Reviewed changes — 本次评审覆盖 commit a725c4f（自上次 Pullfrog 评审 ab82c84 以来的增量，共 7 个 commit），重点为模型能力（capabilities）从固定类型化字段重构为开放字段集合 + 类型化已知视图，以及对完整 Gateway 能力的透传。
- 重构
Capabilities为开放字段集合 —internal/modelcatalog/types.go将Capabilities从 struct（Thinking/AdaptiveThinking/ToolUse+fields）改为map[string]json.RawMessage，新增KnownCapabilities类型视图与Known()访问器；UnmarshalJSON通过validateKnownCapabilities校验已知键形状，未知键原样保留 - 扩展已知能力词表 —
KnownCapabilities覆盖 batch、citations、code_execution、context_management（含 clear_thinking/clear_tool_uses/compact 子项）、effort（low/medium/high/xhigh/max）、image_input、pdf_input、structured_outputs、thinking（types.enabled/adaptive）、tool_use；三态语义（true/false/nil）一致，未知不推断 - Workbench 完整能力透传 —
workbenchModelFromCatalog同时返回完整capabilities对象与扁平supports_*兼容字段；image_input映射supports_images/supports_vision/supports_image_input，pdf_input映射supports_documents/supports_pdf_input；supported_effort_levels仅在 Gateway 声明 effort 时生成 - Bootstrap thinking 模式与多模态能力 —
bootstrapModels区分AdaptiveThinking（→ adaptive modes）与ThinkingEnabled/Thinking（→ extended modes）；BootstrapModelCapabilities（MMImages/MMPDF）仅在 Gateway 声明 image/pdf 输入时生成 - Dashboard 模型标签与 i18n — 新增 images/PDF 标签，en + zh-CN 消息键同步
- 测试覆盖 — 新增
internal/platformapi/model_catalog_test.go验证 thinking 与多模态能力派生；upstream_test.go与console_platform_workbench_test.go扩展覆盖完整能力 passthrough、effort levels 与 stale 元数据
anthropic/glm-5.2 ｜ 𝕏
|
补充说明：本次提交 c8f6e88 已按 Anthropic 官方合同完善模型边界。Models API 现在使用命名 DTO，支持 limit、after_id、before_id、GET /v1/models/{model_id}，并由官方 Go SDK 覆盖 List/Get 兼容测试；Managed Agents model 对象补齐 effort 字符串及对象形式，并实现同模型对象更新时的 effort 继承语义。设计依据已写入中文设计文档：Models API https://platform.claude.com/docs/en/api/models/list ，Create Agent API https://platform.claude.com/docs/en/api/beta/agents/create ，Agent setup https://platform.claude.com/docs/en/managed-agents/agent-setup 。 |
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
ℹ️ 无严重或必须处理的问题——以下为合并前可处理的轻微建议与一处合同核对。
Reviewed changes — 本次评审覆盖自上次 Pullfrog 评审（ab82c84）以来的三个增量提交（ca622d5、a725c4f、c8f6e88），重点为对齐 Anthropic 官方模型合同与 Agent 模型 effort 语义。
- 新增 Agent model effort 语义 —
internal/agents/handler.go的normalizeModel支持对象模型携带speed与effort（字符串或{type}）；stateFromUpdate在同模型 ID 且对象省略effort时保留既有 effort，切换模型或传字符串模型时回到 provider 默认，与 Create Agent 官方文档一致 /v1/models升级为命名 DTO 与完整分页 —internal/models/handler.go以modelResponse替换map[string]any，新增 Anthropic 游标分页（limit默认 20、范围 1–1000、after_id/before_id互斥）与GET /v1/models/{model_id}retrieve；新增官方 Go SDK 往返测试- 上游元数据校验加严 —
internal/modelcatalog/upstream.go新增validateTokenLimit（拒绝<1）与created_at强制 RFC3339 解析（同时接受 Unix 秒）；非法字段会让整次 refresh 失败并保留 stale 快照 - Quickstart effort schema 注入 —
web/src/features/managed-agents/{types,agentConfig,quickstart/platformQuickstartRequest}.ts新增AGENT_MODEL_EFFORT_LEVELS、AgentModelEffort类型，并在build_agent_config的 object model 分支注入 effort schema - 前次评审反馈已处理 —
canRefreshModelCatalog严格限定admin并改用GetAdminUser（thread 3649711509 已 RESOLVED）；前两次评审的recordFailure超时、启动异步刷新、能力字段透传合同均已修复 - 设计文档对齐官方一手资料 —
docs/design/be/byok-model-catalog.md新增官方合同依据章节，明确/v1/models字段、分页参数、effort/speed 输入形式与三态能力视图
ℹ️ Nitpicks
docs/design/be/byok-model-catalog.md称"AI Gateway 声明created_at时必须符合 RFC 3339"，但 Anthropic 官方 List Models 文档明确写明created_at"May be set to an epoch value if the release date is unknown"。运行时modelCreatedAt实际同时接受 RFC3339 字符串与 Unix 秒整数，行为是兼容的，仅文档表述偏严——建议把这句改为"RFC 3339 字符串或 epoch 整数"以与代码和官方合同一致。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 本次评审覆盖 commit 908cc3e（自上次 Pullfrog 评审 c8f6e88 以来的增量），全部为对前轮评审两条轻微建议的修复，无新问题。
normalizeTokenLimit将0归一化为"未知" —internal/modelcatalog/upstream.go用normalizeTokenLimit替换validateTokenLimit：nil或0返回nil（视为字段缺失），负数仍判定为非法上游响应。新增TestHTTPUpstreamTreatsZeroTokenLimitsAsUnknown回归测试，与 Anthropic 官方 List Models 文档"token 上限无最小值、示例可含 0"的合同对齐- Quickstart 模型不可用提示统一 i18n —
:302改用msg('managedAgents.quickstart.selectModel', ...)，与:632/:762一致；en.json注册Select an available model first.，zh-CN.json注册请先选择一个可用的模型。，三个入口对中文用户不再回落到英文 fallback
前轮两条 thread（3650263725、3650263727）作者已在 GitHub 上 resolve，无需额外处理。
anthropic/glm-5.2 ｜ 𝕏
|
补充处理 DuckPR review 正文中的文档意见： |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/workbench/console_platform_workbench_test.go (1)
540-564: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReorder refresh authorization tests: success test precedes a failure test.
TestWorkbenchModelCatalogRefreshAcceptsOrganizationAdmin(200, success) is placed beforeTestWorkbenchModelCatalogRefreshReportsConcurrentRefresh(409, failure). This is the same failure-before-success pattern already fixed elsewhere in this file for the concurrent-refresh/success pair (per the earlier review thread), but it recurs here for this newer pair.As per coding guidelines: "Tests should present failure scenarios before success scenarios."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/workbench/console_platform_workbench_test.go` around lines 540 - 564, Reorder the refresh authorization tests so TestWorkbenchModelCatalogRefreshReportsConcurrentRefresh, which covers the 409 failure case, appears before TestWorkbenchModelCatalogRefreshAcceptsOrganizationAdmin, which covers the 200 success case. Do not change either test’s implementation.Source: Coding guidelines
internal/agents/handler.go (1)
867-896: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle explicit
model.effort: nullconsistently with omitted effort.
modelObjectOmitsEffort(raw)only checks for the presence of theeffortkey, so{"id":"model-1","effort":null}still carries the currentmodel.Effortin the same-model update path.normalizeModelEffortalso does not acceptnullas “no effort,” so there’s no clear way to override an existing model effort while keeping the same model id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agents/handler.go` around lines 867 - 896, Update the same-model effort handling around normalizeModelEffort and modelObjectOmitsEffort so an explicit model.effort: null is treated the same as omitted effort: clear or omit the existing effort instead of retaining it, and do not return a validation error. Ensure non-null string and object effort values continue through the existing validation paths unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/agents/handler.go`:
- Around line 867-896: Update the same-model effort handling around
normalizeModelEffort and modelObjectOmitsEffort so an explicit model.effort:
null is treated the same as omitted effort: clear or omit the existing effort
instead of retaining it, and do not return a validation error. Ensure non-null
string and object effort values continue through the existing validation paths
unchanged.
In `@internal/workbench/console_platform_workbench_test.go`:
- Around line 540-564: Reorder the refresh authorization tests so
TestWorkbenchModelCatalogRefreshReportsConcurrentRefresh, which covers the 409
failure case, appears before
TestWorkbenchModelCatalogRefreshAcceptsOrganizationAdmin, which covers the 200
success case. Do not change either test’s implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 70a44c04-5b43-4892-a18e-c0d28769e1e9
📒 Files selected for processing (26)
docs/design/be/byok-model-catalog.mdinternal/agents/handler.gointernal/agents/model_test.gointernal/modelcatalog/service.gointernal/modelcatalog/types.gointernal/modelcatalog/upstream.gointernal/modelcatalog/upstream_test.gointernal/models/handler.gointernal/models/handler_test.gointernal/platformapi/model_catalog.gointernal/platformapi/model_catalog_test.gointernal/platformapi/platform_bootstrap_features.gointernal/workbench/console_platform_workbench.gointernal/workbench/console_platform_workbench_test.goweb/src/features/dashboard/DashboardPage.test.tsxweb/src/features/dashboard/home.tsxweb/src/features/managed-agents/agentConfig.test.tsweb/src/features/managed-agents/agentConfig.tsweb/src/features/managed-agents/quickstart/AgentQuickstartPage.tsxweb/src/features/managed-agents/quickstart/platformQuickstartRequest.test.tsweb/src/features/managed-agents/quickstart/platformQuickstartRequest.tsweb/src/features/managed-agents/types.tsweb/src/features/model-catalog/model.tsweb/src/features/workbench/api.tsweb/src/shared/i18n/messages/en.jsonweb/src/shared/i18n/messages/zh-CN.json
🚧 Files skipped from review as they are similar to previous changes (15)
- web/src/features/managed-agents/quickstart/platformQuickstartRequest.test.ts
- web/src/features/model-catalog/model.ts
- web/src/features/dashboard/home.tsx
- docs/design/be/byok-model-catalog.md
- internal/modelcatalog/upstream.go
- web/src/shared/i18n/messages/en.json
- web/src/features/dashboard/DashboardPage.test.tsx
- web/src/features/managed-agents/agentConfig.test.ts
- internal/platformapi/model_catalog.go
- internal/platformapi/platform_bootstrap_features.go
- web/src/features/managed-agents/quickstart/AgentQuickstartPage.tsx
- web/src/features/managed-agents/quickstart/platformQuickstartRequest.ts
- internal/modelcatalog/service.go
- web/src/features/managed-agents/agentConfig.ts
- internal/workbench/console_platform_workbench.go
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
Reviewed changes — 本次评审覆盖 commit 5d00057（自上次 Pullfrog 评审 fe084bc 以来的增量）。该增量为纯粹的 main 合并——main 引入了 internal/db/ 的 UUID 标识符重构（迁移 00027-00030、database-identifier-references.md、全库 UUID typed 化），本分支将 BYOK 代码 rebase 到该基线。BYOK 业务逻辑自 908cc3e 起无变更。
- 迁移重排
00027_add_model_catalog_snapshots.sql → 00031— 因main新增 filestore 相关迁移而顺延，编号无冲突，00031正确落在00030_validate_filestore_file_references与00032_add_filestore_archive_entries之间 internal/db/model_catalog.go适配 main 的 sqlx 约定 — 改用 main 引入的namedGetContext/namedExecContext绑定辅助；CAST(:models AS jsonb)符合"命名参数不写::type"规则；目录为全局单行表，无organization_uuid/workspace_uuid租户列，因此 UUID 标识符重构对 BYOK 目录无租户隔离回归main引入的internal/db/全库重构未触及 BYOK 行为 —model_catalog_snapshots表结构、advisory lock、快照持久化与 stale 语义均与 rebase 前一致go build ./internal/db/... ./internal/modelcatalog/...与go test ./internal/db/ -run 'ModelCatalog'、go test ./internal/modelcatalog/...均通过
前序所有 Pullfrog 评审反馈（recordFailure 共享 refreshCtx、启动异步刷新、能力字段透传、canRefreshModelCatalog 限定 admin、token 限制 0 归一化、Quickstart i18n、created_at 文档对齐等）均已闭环，无未处理线程。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题。前次评审的反馈均已妥善修复，增量改动质量高。
Reviewed changes — 本次评审覆盖 commit 9084e63（自上次 Pullfrog 评审 ab82c84 以来的增量），重点为依赖注入重构、能力字段结构化、advisory lock 复用 sqlx 与模型回退语义。
- 新增
internal/aigateway共享包（原aiupstream重命名） —Endpoint()统一校验网关 URL（拒绝anthropic.com、要求绝对 http/https、禁止凭证/query/fragment），NewHTTPClient()全局阻断重定向并被 messages/batches/proxy/workbench 复用；无残留aiupstream引用 - 能力字段改为结构化
CapabilityValue类型 — 用递归的 typed JSON 值（object/array/string/number/boolean/null）保留 Gateway 扩展，不再把json.RawMessage作为业务模型跨包传递，符合 AGENTS.md 的 JSON 边界规则；已知能力仍提供三态命名视图（KnownCapabilities覆盖 batch/citations/code_execution/context_management/effort/image_input/pdf_input/structured_outputs/thinking/tool_use 及子字段） - advisory lock 提取到
internal/db/advisory_lock.go—TryAcquireAdvisoryLock用 pinned sqlx connection 获取与释放 session-scoped lock，unlock 失败时 close+release 连接避免污染池；modelcatalog/postgres_store.go委托给该共享方法，迁移自 pgx 原生连接 - Workbench 依赖从 request context 迁移到 handler 结构体 —
workbenchHandler持有 store/upstream/catalog/userStore/logger 等稳定依赖，生产RegisterOrgWorkbenchRoutes构造时注入；测试用withWorkbenchDependenciesAndCatalog+newWorkbenchTestHandler经 context key 桥接（test-only shim） - Workbench 生成请求在无默认模型时回退到目录首项 —
resolveWorkbenchModel在DefaultModelID为空且目录非空时使用snapshot.Models[0].ID，与前端初始选择语义一致，新增TestWorkbenchGenerateTestCaseUsesFirstCatalogModelWhenDefaultIsUnset覆盖 - models handler 与 agents handler 503 路径补充结构化日志 —
loadSnapshot的 nil catalog 与Snapshot()错误分支均调用WarnContext/ErrorContext；writeStateError的不可用分支同样记录 - 前端
canRefreshModelCatalog抽取到共享权限模块 —web/src/shared/permissions/model-catalog.ts统一 admin 角色判断，与后端生产users.role词表一致，附单测
前次评审反馈（recordFailure 用 refreshCtx、启动刷新异步化、normalizeModels 回写、csrfToken 必填、stale 模型标记、i18n 等）均已在历次 commit 中修复；canRefreshModelCatalog 角色合同问题已在 8841752 解决（严格限定 admin，伪特权角色返回 403）。go build ./... 与 modelcatalog/aigateway/models/db/agents/workbench/platformapi 包测试均通过。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
Important
增量中有一个必须修复的问题：testModelCatalog.ValidateModel 的语义收紧会破坏 tests/ 集成测试包中 77+ 处使用 claude-opus-4-6 等模型创建 Agent 的成功路径。
Reviewed changes — 本次评审覆盖 commit 0140867 与 89118f1（自上次 Pullfrog 评审 9084e63 以来的增量），重点为失败持久化超时修复、前端 API 层迁移，以及合并 origin/main。
- 修复
recordFailure使用已取消 context 的 bug — 新增recordFailureAfterRefresh包装器，使用context.WithoutCancel(ctx)+ 独立 5 秒超时（failurePersistenceTimeout），确保上游超时后失败记录仍能持久化 - 新增
TestServiceRecordsFailureAfterUpstreamTimeout测试 — 验证超时后RecordFailure收到的 context 未过期，且失败元数据正确持久化；新增timeoutUpstream测试桩 - 修复
testModelCatalog.ValidateModel逻辑 — 从"接受任意非空模型"改为"仅接受目录中的test/model"，但此变更会破坏共享该 catalog 的其他集成测试 - 前端模型目录 API 迁移至
shared/api— 将类型和 API 函数从 feature 层移至web/src/shared/api/model-catalog.ts，feature 层通过 re-export 保持兼容 - 新增 Agent 创建目录外模型拒绝测试 —
tests/agents_api_test.go新增"failure model outside catalog"用例 - 合并
origin/main— 引入db.Open的 logger 参数签名变更和文档微调
anthropic/glm-5.2 ｜ 𝕏
| if strings.TrimSpace(modelID) == "test/model" { | ||
| return nil | ||
| } | ||
| return modelcatalog.ErrUnknownModel |
There was a problem hiding this comment.
ValidateModel 收紧到仅接受 test/model 会破坏 tests/ 包中所有使用 claude-opus-4-6（及其他非 test/model 模型）创建 Agent 的集成测试。
testModelCatalog 被 newTestAppWithStoreAndLogger 注入到共享的 testApp 中（line 1064），而 validateModelSelection（internal/agents/handler.go:505）在 Agent 创建路径上调用 ValidateModel。tests/agents_api_test.go、tests/sessions_api_test.go、tests/deployments_api_test.go 等文件中共有 77+ 处 createAgent 调用使用 claude-opus-4-6，这些调用会因 ErrUnknownModel 返回 400 并触发 createAgent 的 t.Fatalf。
Technical details
# `testModelCatalog.ValidateModel` 收紧破坏共享集成测试
## Affected sites
- `tests/files_api_test.go:1100-1105` — `ValidateModel` 从接受任意非空模型改为仅接受 `test/model`
- `tests/agents_api_test.go:135` — `createAgent(t, app, `{"model":"claude-opus-4-6",...}`)` 等 77+ 处
- `internal/agents/handler.go:505` — `validateModelSelection` 在 `stateFromCreate` 中调用 `ValidateModel`
## Required outcome
- `testModelCatalog.ValidateModel` 需要接受集成测试中实际使用的模型 ID（`claude-opus-4-6`、`claude-sonnet-4-6`、`claude-opus-4-8`），同时仍能拒绝 `not-in-catalog` 以满足新增的 `failure model outside catalog` 用例
- 或者将 `testModelCatalog.Snapshot` 的模型列表扩展为包含测试中使用的全部模型 ID
## Suggested approach
将 `ValidateModel` 改为接受一个已知测试模型集合（包括 `test/model`、`claude-opus-4-6`、`claude-sonnet-4-6`、`claude-opus-4-8`），拒绝其他所有 ID：
```go
var testModelIDs = map[string]bool{
"test/model": true,
"claude-opus-4-6": true,
"claude-sonnet-4-6": true,
"claude-opus-4-8": true,
}
func (testModelCatalog) ValidateModel(_ context.Context, modelID string) error {
if testModelIDs[strings.TrimSpace(modelID)] {
return nil
}
return modelcatalog.ErrUnknownModel
}
```2 participants
总览
Refs #62
本 PR 将模型发现、模型选择、Agent 配置校验与实际请求路由统一到同一个 AI Gateway 配置，完成可持久化的 BYOK 模型目录。部署者只需配置 Gateway 地址与密钥，后端便会动态获取模型，前端也会使用同一份目录展示和选择模型，不再依赖生产代码中的 Claude 模型硬编码。
本 PR 覆盖 #62 中的 BYOK 模型目录与动态选择部分；Request ID、Usage、Cost 等可观测性能力不在本次范围内，因此使用
Refs #62，不直接关闭该 Issue。提供的功能与改动
1. 持久化模型目录
/v1/models分页拉取完整模型列表，并对模型 ID 做校验与去重。sqlx，并复用现有pgxpool，不创建额外连接池。model_catalog.refresh_interval周期刷新；管理员也可以手动触发刷新。2. 明确的失败与默认模型语义
model_catalog.default_model_id；未配置默认模型时，前端默认选择目录中的第一个模型；目录为空时保持未选择状态。3. 前后端统一消费同一目录
/v1/models、Console/models、平台 Bootstrap、Dashboard、Workbench、Agent Quickstart 和 Agent 创建/编辑流程统一读取模型目录。4. 完善 Models API 与模型能力合同
/v1/models支持limit、after_id、before_id，并新增GET /v1/models/{model_id}。{ "type": ... }形式，并保留 speed 等扩展字段。5. 统一 Gateway 配置、路由与安全策略
anthropic_upstream.base_url、api_key和model_mappings，确保“前端看到的模型”和“实际执行的模型”来自同一路由规则。internal/aigateway统一处理配置校验、Endpoint 构造和重定向凭据策略。关键设计与原因
配置关系
anthropic_upstream决定从哪里发现模型以及向哪里发送模型请求；model_catalog只控制目录刷新策略和默认选择，不维护另一份静态模型清单。验证与范围
已完成以下验证：
本 PR 不包含多 Gateway 路由、模型定价维护、模型 CRUD，以及 Request ID、Usage、Cost 采集与展示。
参考资料
总结
完成本 PR 后，模型目录从部署者配置的 AI Gateway 动态获取并持久化，前后端、Agent 配置和运行请求共享同一份模型事实来源。它移除了生产路径对 Claude 型号的硬编码，同时保留清晰的失败语义、历史兼容性和后续扩展模型能力字段的空间。
Summary by CodeRabbit
New Features
Bug Fixes