From 2142de92f25ed9c1556e403010ed64bb96165118 Mon Sep 17 00:00:00 2001 From: AskaEth Date: Sat, 24 Oct 2026 19:53:38 +0800 Subject: [PATCH] =?UTF-8?q?fix(plugin):=20=E6=8B=92=E6=94=B6=20env/args=20?= =?UTF-8?q?=E7=9A=84=E9=9D=9E=E5=AD=97=E7=AC=A6=E4=B8=B2=E5=80=BC=20+=20?= =?UTF-8?q?=E4=BF=9D=E7=95=99=E9=94=AE=E6=94=B6=E6=88=90=E5=86=85=E7=BD=AE?= =?UTF-8?q?=20id=20=E9=9B=86=E5=90=88=20+=20=E5=90=AF=E7=94=A8=E8=90=BD?= =?UTF-8?q?=E7=9B=98=E5=A4=B1=E8=B4=A5=E6=96=87=E6=A1=88?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit r1 复审 P2-1 + 两条顺手件。 P2-1（必修）：`actionStringMap` / `actionStringList` 之前对非字符串值 `continue`，于是 前端 kv 控件里最自然的写法 `{"PORT":8080}` 会被**静默丢掉**、安装仍报 `installed:true`。 现在非字符串值、空键、非数组一律 400 invalid_config，文案只点名**键与类型**（不回显值）； 盘上零变更、注册表零工具、快照零行（测试同时断言）。 顺手件 1：`isReservedPanelKey` 从「只挡 mcp」改成内置提供者闭集（mcp / web-search / browser，写成 switch，新增内置页时漏补一眼可见）。之前一台名为 web-search 的 server 被 卸载时，`forgetPanels → MarkUninstalled` 会把内置「搜索提供商」页在重启前整页摘掉。 顺手件 2：启用「已连上但启用态未落盘」的文案改成事实（原来那句「它仍处于停用状态」与 事实相反）。为此新增 `ErrPluginStatePersist` 哨兵，把「磁盘没记住」与「连不上」分开； `state=connected` 的派生口径不动。停用失败文案同时补上「server 仍在运行」。 测试：三条新用例，均已验「修前红 → 修后绿」（见 review-request 的取证）。 --- internal/capability/plugin/enabled.go | 11 +- internal/capability/plugin/mcppanel.go | 18 +- .../capability/plugin/mcppanel_actions.go | 158 +++++++++++++----- internal/capability/plugin/mcppanel_test.go | 156 +++++++++++++++++ 4 files changed, 297 insertions(+), 46 deletions(-) diff --git a/internal/capability/plugin/enabled.go b/internal/capability/plugin/enabled.go index 6a67bb7..b7f0b2d 100644 --- a/internal/capability/plugin/enabled.go +++ b/internal/capability/plugin/enabled.go @@ -25,6 +25,13 @@ import ( // ErrPluginNotFound 报告指定的插件既没活着也不在 plugins.json 里。 var ErrPluginNotFound = errors.New("plugin: not found") +// ErrPluginStatePersist 报告「启用态没写成」：内存/连接态可能已经变了，但 plugins.json +// 还是旧值 —— 因此**重启后会退回旧状态**。 +// +// 它与「连不上」是两件相反的事，调用方必须能分开（文案说反就是骗人）：连不上 ⇒ server +// 现在不可用；落盘失败 ⇒ server 可能正连着、工具可用，只是这个事实活不过一次重启。 +var ErrPluginStatePersist = errors.New("plugin: enabled state not persisted") + // notFound 构造一条带名字的「找不到」。 func notFound(name string) error { if name == "" { @@ -131,9 +138,11 @@ func (m *Manager) enable(ctx context.Context, name string) error { // persistEnabled 把启用态写回 plugins.json（upsert by name，其余字段原样保留）。 // // 它**不**碰内存里的活 server：调用方按自己的顺序决定何时连线/断线。 +// 失败用 ErrPluginStatePersist 包起来：调用方要能把它与「连不上」区分开 —— 这两个失败 +// 的事实相反（一个没连上、一个可能已经连上而磁盘没记住）。 func (m *Manager) persistEnabled(cfg runtimeconfig.PluginConfig, on bool) error { if err := m.persistUpsert(cfg.WithEnabled(on)); err != nil { - return fmt.Errorf("插件 %q 的启用态未落盘: %w", cfg.Name, err) + return fmt.Errorf("%w: 插件 %q 的启用态未落盘: %w", ErrPluginStatePersist, cfg.Name, err) } return nil } diff --git a/internal/capability/plugin/mcppanel.go b/internal/capability/plugin/mcppanel.go index b5a3c4e..854d3e6 100644 --- a/internal/capability/plugin/mcppanel.go +++ b/internal/capability/plugin/mcppanel.go @@ -186,6 +186,18 @@ func (m *Manager) AttachMCPPanel() error { // isReservedPanelKey 报告该目录键属于**宿主内置面板提供者**（不是热装插件的命名空间）。 // // 护栏的方向是单向的：热装插件的安装/停用/卸载绝不许改动内置提供者的目录条目。理由很 -// 实在 —— 内置提供者的 id 是给人看的短名字（mcp），而 server 的名字由主人随手起；一台 -// 恰好叫 mcp 的 server 在卸载时若照常 MarkUninstalled("mcp")，整页「MCP 服务器」就消失了。 -func isReservedPanelKey(key string) bool { return key == MCPPanelPluginID } +// 实在 —— 内置提供者的 id 是给人看的短名字（mcp / web-search / browser），而 server 的 +// 名字由主人随手起；一台恰好同名的 server 在卸载时若照常 MarkUninstalled，那整页内置页 +// 就在**重启前**消失了（`forgetPanels → MarkUninstalled` 连声明与动作索引一起清）。 +// +// 名单写死成 switch 而不是包级 map：新增内置页时没补这一行，review 时一眼可见（与 +// paneldecl.shapeOf 同一取舍）。当前三个内置提供者：本包的 mcp 页（MCP 服务器）、 +// `plugins/tools/io/search` 的 web-search 页、`plugins/tools/browser` 的 browser 页。 +func isReservedPanelKey(key string) bool { + switch key { + case MCPPanelPluginID, "web-search", "browser": + return true + default: + return false + } +} diff --git a/internal/capability/plugin/mcppanel_actions.go b/internal/capability/plugin/mcppanel_actions.go index 599e92f..d73973e 100644 --- a/internal/capability/plugin/mcppanel_actions.go +++ b/internal/capability/plugin/mcppanel_actions.go @@ -87,16 +87,10 @@ func (m *Manager) actInstallServer(ctx context.Context, req ActionRequest) (any, if err != nil { return nil, err } - cfg := runtimeconfig.PluginConfig{ - Name: actionText(values, FieldServerName), - Kind: runtimeconfig.PluginKindMCP, - Transport: actionText(values, FieldTransport), - Endpoint: firstNonEmpty(actionText(values, FieldURL), actionText(values, "endpoint")), - Command: actionText(values, FieldCommand), - Args: actionStringList(values[FieldArgs]), - APIKey: actionText(values, FieldAPIKey), - Env: actionStringMap(values[FieldEnv]), - }.Normalize() + cfg, err := installConfigOf(values) + if err != nil { + return nil, err + } if err := cfg.Validate(); err != nil { return nil, NewActionError(mcpCodeInvalidConfig, fmt.Sprintf("配置不完整：%v", err), 400, err) } @@ -114,6 +108,40 @@ func (m *Manager) actInstallServer(ctx context.Context, req ActionRequest) (any, }, nil } +// installConfigOf 把表单值翻译成插件配置。 +// +// `args` / `env` 的形状错**必须在这里变成 400**：前端的 `kv` 控件是自由 JSON textarea， +// 主人写 `{"PORT":8080}` 是最自然的写法，若后端把非字符串值静默丢掉，结果就是「装完提示 +// 成功、server 也连上了、env 却没生效」—— 正是任务书点名要防的「看着配好了、其实没生效」。 +// 同理 `args` 里的非字符串元素也不许悄悄消失。 +func installConfigOf(values map[string]any) (runtimeconfig.PluginConfig, error) { + args, err := actionStringList(values[FieldArgs]) + if err != nil { + return runtimeconfig.PluginConfig{}, invalidConfigError(err) + } + env, err := actionStringMap(values[FieldEnv]) + if err != nil { + return runtimeconfig.PluginConfig{}, invalidConfigError(err) + } + return runtimeconfig.PluginConfig{ + Name: actionText(values, FieldServerName), + Kind: runtimeconfig.PluginKindMCP, + Transport: actionText(values, FieldTransport), + Endpoint: firstNonEmpty(actionText(values, FieldURL), actionText(values, "endpoint")), + Command: actionText(values, FieldCommand), + Args: args, + APIKey: actionText(values, FieldAPIKey), + Env: env, + }.Normalize(), nil +} + +// invalidConfigError 把一条「配置没写对」包成结构化动作失败（400）。 +// +// 文案里只出现**键名与类型**，绝不回显值：env 的值可能就是凭据。 +func invalidConfigError(err error) error { + return NewActionError(mcpCodeInvalidConfig, fmt.Sprintf("配置没写对：%v", err), 400, err) +} + // actReconnectServer 断开并重新连接一台已启用的 server（重新发现工具）。 func (m *Manager) actReconnectServer(ctx context.Context, req ActionRequest) (any, error) { name, err := actionRowName(req.Payload) @@ -146,6 +174,12 @@ func (m *Manager) actEnableServer(ctx context.Context, req ActionRequest) (any, return nil, NewActionError(mcpCodeServerNotFound, fmt.Sprintf("没有这台 server：%s", name), 404, err) } m.log.Warn("mcp enable failed", "name", name, "error", err) + // 两种失败的事实不同，文案也必须不同：连不上 ⇒ 它仍是停用的；**已经连上但启用态 + // 没落盘** ⇒ 现在能用、重启后会回到停用（说成「仍处于停用状态」就是与事实相反）。 + if errors.Is(err, ErrPluginStatePersist) { + return nil, NewActionError(mcpCodeEnableFailed, + fmt.Sprintf("%q 已连上，但启用态未落盘：重启后会回到停用，详细原因见后端日志", name), 502, err) + } return nil, NewActionError(mcpCodeEnableFailed, fmt.Sprintf("启用 %q 失败：它仍处于停用状态，详细原因见后端日志", name), 502, err) } @@ -163,8 +197,9 @@ func (m *Manager) actDisableServer(ctx context.Context, req ActionRequest) (any, return nil, NewActionError(mcpCodeServerNotFound, fmt.Sprintf("没有这台 server：%s", name), 404, err) } m.log.Warn("mcp disable failed", "name", name, "error", err) + // 停用的落盘在**前面**（见 enabled.go）：写不进去时 server 还在跑、工具还在。 return nil, NewActionError(mcpCodeDisableFailed, - fmt.Sprintf("停用 %q 失败：启用态未能落盘，详细原因见后端日志", name), 500, err) + fmt.Sprintf("停用 %q 失败：启用态未能落盘，server 仍在运行（可再试一次），详细原因见后端日志", name), 500, err) } return map[string]any{"name": name, "enabled": false, "connected": false}, nil } @@ -259,51 +294,90 @@ func actionText(values map[string]any, key string) string { } // actionStringList 取一个字符串列表（`multiSelect` 提交的是 `[]string`，JSON 解码后是 -// `[]any`；元素非字符串即忽略，空串不保留）。空列表返回 nil（配置里就不写这个键）。 -func actionStringList(raw any) []string { - items, ok := raw.([]any) - if !ok { - if strs, okStr := raw.([]string); okStr { - return compactStrings(strs) +// `[]any`）。 +// +// 形状错一律报错：非数组、元素非字符串都不许**静默丢弃**（少一个参数、或多出一个空参数， +// 都会让 server 起来后行为不对，而面板还报「装好了」）。空项按前端 `splitListValue` 的同一 +// 口径去掉（那只来自多余的分隔符，不是用户意图）。 +func actionStringList(raw any) ([]string, error) { + switch items := raw.(type) { + case nil: + return nil, nil + case []string: + return compactStrings(items), nil + case []any: + out := make([]string, 0, len(items)) + for i, item := range items { + s, ok := item.(string) + if !ok { + return nil, fmt.Errorf("args 的第 %d 项不是字符串（收到 %s）；每个参数都必须是字符串", i+1, jsonKindOf(item)) + } + out = append(out, s) } - return nil + return compactStrings(out), nil + default: + return nil, fmt.Errorf("args 必须是字符串数组（收到 %s）", jsonKindOf(raw)) } - out := make([]string, 0, len(items)) - for _, item := range items { - s, ok := item.(string) - if !ok { - continue - } - out = append(out, s) - } - return compactStrings(out) } // actionStringMap 取一个键值对（`kv` 提交的是 JSON 对象）。 // -// 值只接受字符串：`{"PORT":8080}`（数字）这类会被**拒之门外而不是静默转成 "8080"** —— -// 静默转换正是「看着配好了、其实没生效」的来源。 -func actionStringMap(raw any) map[string]string { - obj, ok := raw.(map[string]any) - if !ok { - return nil - } - out := make(map[string]string, len(obj)) - for k, v := range obj { - s, ok := v.(string) - if !ok { - continue +// 值**只接受字符串**，其它类型一律报错而不是丢掉那个键：`{"PORT":8080}` 是最自然的写法， +// 静默丢掉它等于「装完提示成功、env 却没生效」（前端 `KvControl` 只校验「是个对象」， +// 所以这条路 UI 天然可达）。空键同理 —— 丢一个键与拒一个键必须选后者。 +func actionStringMap(raw any) (map[string]string, error) { + var pairs map[string]any + switch obj := raw.(type) { + case nil: + return nil, nil + case map[string]any: + pairs = obj + case map[string]string: + pairs = make(map[string]any, len(obj)) + for k, v := range obj { + pairs[k] = v } + default: + return nil, fmt.Errorf("env 必须是键值对对象（收到 %s）", jsonKindOf(raw)) + } + out := make(map[string]string, len(pairs)) + for k, v := range pairs { key := strings.TrimSpace(k) if key == "" { - continue + return nil, fmt.Errorf("env 里有空键；键就是环境变量名，不能为空") + } + s, ok := v.(string) + if !ok { + return nil, fmt.Errorf("env.%s 的值不是字符串（收到 %s）；要写数字/布尔请加引号，如 \"8080\"", key, jsonKindOf(v)) } out[key] = s } if len(out) == 0 { - return nil + return nil, nil + } + return out, nil +} + +// jsonKindOf 给出一个 JSON 值的中文类型名（只为错误文案：说清「收到的是什么」）。 +// +// 它刻意不打印值本身：env 的值可能就是凭据。 +func jsonKindOf(v any) string { + switch v.(type) { + case nil: + return "null" + case string: + return "字符串" + case bool: + return "布尔" + case float64, float32, int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64: + return "数字" + case []any: + return "数组" + case map[string]any: + return "对象" + default: + return fmt.Sprintf("%T", v) } - return out } // compactStrings 去掉两端空白与空项；结果为空时返回 nil。 diff --git a/internal/capability/plugin/mcppanel_test.go b/internal/capability/plugin/mcppanel_test.go index 7393dfd..5bd045d 100644 --- a/internal/capability/plugin/mcppanel_test.go +++ b/internal/capability/plugin/mcppanel_test.go @@ -10,11 +10,15 @@ import ( "context" "net/http" "net/http/httptest" + "os" + "runtime" + "strings" "testing" "git.yeij.top/AskaEth/Cyrene_v2/internal/auth" "git.yeij.top/AskaEth/Cyrene_v2/internal/capability/paneldecl" "git.yeij.top/AskaEth/Cyrene_v2/internal/config/runtimeconfig" + "git.yeij.top/AskaEth/Cyrene_v2/pkg/plugins/sdk" ) // TestMCPPanelDeclarationIsValid 锁定 ①：整份声明过 paneldecl 的形状校验。 @@ -272,3 +276,155 @@ func containsString(items []string, want string) bool { } return false } + +// TestMCPPanelInstallRejectsNonStringEnv 锁定 P2-1：`env` / `args` 里的非字符串值必须 +// 变成 400，**不许静默丢键**（`{"PORT":8080}` 是前端 kv 控件最自然的写法：装完提示成功、 +// server 连上了、env 却没生效 —— 比直接失败更坏）。 +func TestMCPPanelInstallRejectsNonStringEnv(t *testing.T) { + t.Parallel() + srv := newFakeMCP(t, "", "add") + mgr, hub, _ := newPanelManager(t) + defer mgr.Close(context.Background()) + if err := mgr.AttachMCPPanel(); err != nil { + t.Fatalf("AttachMCPPanel: %v", err) + } + ctx := context.Background() + + cases := []struct { + name string + mutate func(values map[string]any) + wantKey string + }{ + {"env 值是数字", func(v map[string]any) { v["env"] = map[string]any{"PORT": 9182.0} }, "env.PORT"}, + {"env 值是布尔", func(v map[string]any) { v["env"] = map[string]any{"DEBUG": true} }, "env.DEBUG"}, + {"args 元素是数字", func(v map[string]any) { v["args"] = []any{"-y", 42.0} }, "args"}, + } + for _, tc := range cases { + values := map[string]any{ + "name": "calc", "transport": "http", "url": srv.URL, + "env": map[string]any{"TOKEN": "abc"}, + "args": []any{"--flag"}, + } + tc.mutate(values) + _, err := hub.Dispatch(ctx, ActionRequest{ + PluginID: MCPPanelPluginID, Action: ActionInstallMCPServer, + Payload: map[string]any{"values": values}, + }) + ae, ok := AsActionError(err) + if !ok || ae.Code != mcpCodeInvalidConfig || ae.Status != http.StatusBadRequest { + t.Fatalf("%s：err = %v，期望 %s/400", tc.name, err, mcpCodeInvalidConfig) + } + if !strings.Contains(ae.Message, tc.wantKey) { + t.Fatalf("%s：文案没点名出错的键（%q 不含 %q）", tc.name, ae.Message, tc.wantKey) + } + // 文案里不许回显**收到的值**（env 的值可能就是凭据）：9182 是本用例特意挑的 + // 独特数字，文案里的 "8080" 只是「要加引号」的示例，不是回显。 + if strings.Contains(ae.Message, "9182") { + t.Fatalf("%s：文案回显了值：%q", tc.name, ae.Message) + } + } + + // 三次都失败 ⇒ 盘上零变更、注册表零工具、列表零行。 + if cfgs, err := runtimeconfig.LoadPlugins(mgr.path); err != nil || len(cfgs) != 0 { + t.Fatalf("盘上 = %+v（err=%v），期望空", cfgs, err) + } + if names := toolNames(mgr.reg); len(names) != 0 { + t.Fatalf("注册表里有工具 = %v", names) + } + if rows := mustRows(t, hub, ctx); len(rows) != 0 { + t.Fatalf("快照里有行 = %+v", rows) + } +} + +// TestReservedPanelKeysProtectBuiltinProviders 锁定顺手件 1：三台内置提供者的命名空间 +// 都不许被一台同名 server 的安装/停用/启用/卸载改写（否则内置页在重启前就消失）。 +func TestReservedPanelKeysProtectBuiltinProviders(t *testing.T) { + t.Parallel() + for _, id := range []string{MCPPanelPluginID, "web-search", "browser"} { + id := id + t.Run(id, func(t *testing.T) { + t.Parallel() + srv := newFakeMCP(t, "", "add") + mgr, hub, _ := newPanelManager(t) + defer mgr.Close(context.Background()) + ctx := context.Background() + // 模拟该内置提供者在装配期声明的那一页。 + if _, err := hub.Declare(id, []sdk.Panel{samplePanel()}); err != nil { + t.Fatalf("Declare(%s): %v", id, err) + } + installFake(t, mgr, id, srv.URL) + assertBuiltinPageAlive(t, hub, id, "安装") + if err := mgr.SetEnabled(ctx, id, false); err != nil { + t.Fatalf("SetEnabled(false): %v", err) + } + assertBuiltinPageAlive(t, hub, id, "停用") + if err := mgr.SetEnabled(ctx, id, true); err != nil { + t.Fatalf("SetEnabled(true): %v", err) + } + if _, err := mgr.Uninstall(ctx, id); err != nil { + t.Fatalf("Uninstall: %v", err) + } + assertBuiltinPageAlive(t, hub, id, "卸载") + }) + } +} + +// assertBuiltinPageAlive 断言该内置提供者的页仍在目录里。 +func assertBuiltinPageAlive(t *testing.T, hub *Panels, id, step string) { + t.Helper() + entries, err := hub.List(auth.RoleOwner) + if err != nil { + t.Fatalf("List: %v", err) + } + for _, e := range entries { + if e.PluginID == id { + return + } + } + t.Fatalf("%s一台同名 server 之后，内置页 %q 不见了：%+v", step, id, entries) +} + +// TestMCPPanelEnablePersistFailureReportsAccurately 锁定顺手件 2：server **已连上**但 +// 启用态没落盘时，文案必须说的是这件事（「仍处于停用状态」是相反的陈述）。 +// +// 只在本平台跑：靠「把 plugins.json 置成只读」让 rename 失败（Unix 下 rename 由目录权限 +// 决定，同法不成立）。 +func TestMCPPanelEnablePersistFailureReportsAccurately(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("只读位只在 windows 的 rename 上生效") + } + t.Parallel() + srv := newFakeMCP(t, "", "add") + mgr, hub, _ := newPanelManager(t) + defer mgr.Close(context.Background()) + if err := mgr.AttachMCPPanel(); err != nil { + t.Fatalf("AttachMCPPanel: %v", err) + } + ctx := context.Background() + installViaAction(t, hub, ctx, "calc", srv.URL) + if _, err := dispatchRow(t, hub, ActionDisableMCPServer, "calc"); err != nil { + t.Fatalf("disable: %v", err) + } + if err := os.Chmod(mgr.path, 0o444); err != nil { + t.Fatalf("chmod: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(mgr.path, 0o600) }) + + _, err := dispatchRow(t, hub, ActionEnableMCPServer, "calc") + ae, ok := AsActionError(err) + if !ok || ae.Code != mcpCodeEnableFailed || ae.Status != http.StatusBadGateway { + t.Fatalf("启用（落盘失败）= %v，期望 %s/502", err, mcpCodeEnableFailed) + } + // 断言的是**文案与事实一致**（下面两条），不是某个内部哨兵：这句话是给主人看的。 + + if !strings.Contains(ae.Message, "未落盘") || strings.Contains(ae.Message, "仍处于停用状态") { + t.Fatalf("文案与事实不符：%q", ae.Message) + } + // 事实是「已经连上、工具可用」，快照也这么说（state 派生口径不变）。 + if names := toolNames(mgr.reg); !names["add"] { + t.Fatalf("启用后工具 = %v，期望已连上并注册", names) + } + if row := mustRows(t, hub, ctx)[0]; row.State != ServerStateConnected || row.Enabled { + t.Fatalf("快照 = %+v，期望 state=connected 且 enabled=false（盘上仍是停用）", row) + } +}