feat(plugin): phase2 core selection + resolver (1/4)#936
feat(plugin): phase2 core selection + resolver (1/4)#936gh-xj wants to merge 2 commits intosipeed:mainfrom
Conversation
gh-xj
changed the title
Plugin System Phase 2/3 设计评审（技术版，先拿设计 Buy-in）1. 评审目的本评审只解决一个问题： PR 栈（均为 Draft）：
建议合并顺序： 2. 设计边界与假设2.1 In Scope
2.2 Out of Scope
2.3 关键假设
3. 核心架构拆分Phase 2: Selection Plane（控制平面）目标：决定“加载谁”，并保证结果可预测。 关键实现位置：
Phase 3: Introspection Plane（可观测平面）目标：让 operator 可看到“当前状态是否符合预期”。 关键实现位置：
4. 数据契约（当前已落地）4.1 配置契约{
"plugins": {
"default_enabled": true,
"enabled": ["policy-demo"],
"disabled": ["legacy-policy"]
}
}字段语义：
4.2 CLI 契约
4.3 启动摘要契约当前对外稳定字段：
备注： 5. 选择算法（规范化后确定性求解）输入：
预处理：
求解规则（按优先级）：
算法特性：
6. 正确性与兼容性不变量必须满足的不变量：
近期补强（稳定性修复）：
7. 失败模式与回滚路径7.1 失败模式
7.2 回滚操作
8. 可观测性与运维面当前观测面：
尚未纳入本阶段的观测面（后续候选）：
9. 为什么不在这轮做动态加载动态加载会引入以下未闭环问题：
结论：先把“选择 + 观测”打牢，再在独立 RFC 里推进 runtime loader（建议 subprocess + RPC 模式）。 10. 评审拍板点（需要你明确结论）请重点给 yes/no：
若上述 4 点通过，我们按现有 PR 栈推进；否则我会按你的结论拆出增量修订 PR（不扩大当前风险面）。 11. ASCII 架构图（评审速览）11.1 控制平面 + 可观测平面11.2 CLI 运维路径11.3 选择算法（决策树） |
There was a problem hiding this comment.
Pull request overview
Introduces Phase 2 plugin selection primitives and scaffolding: a config schema for enable/disable selection, a deterministic resolver, a builtin plugin catalog with a demo plugin, and bootstrap logic to resolve configured plugins into instances.
Changes:
- Add
pluginsconfig schema (default_enabled,enabled,disabled) with defaults + JSON tests. - Implement
plugin.ResolveSelectionand aplugin.Managerfor compile-time plugin registration + introspection. - Add builtin plugin catalog + bootstrap resolver to instantiate enabled builtin plugins (plus a demo policy plugin and tests).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/plugin/manager.go | Adds plugin contract, selection resolver, and manager registration/describe APIs. |
| pkg/plugin/manager_test.go | Unit tests for manager behavior and selection resolver edge cases. |
| pkg/plugin/demoplugin/policy_demo.go | Adds a demo “policy” plugin exercising tool/message/session hooks. |
| pkg/plugin/demoplugin/policy_demo_test.go | Tests demo plugin blocking/redaction/allowlist/timeout normalization. |
| pkg/plugin/builtin/catalog.go | Adds builtin plugin catalog + deterministic Names(). |
| pkg/plugin/builtin/catalog_test.go | Ensures catalog contains policy demo and Names() is deterministic/sorted. |
| pkg/hooks/hooks.go | Implements hook registry with priority ordering, modifying vs void hooks, concurrency + cloning. |
| pkg/hooks/hooks_test.go | Extensive tests for ordering, cancellation, concurrency, mutation isolation, budgets, panic recovery. |
| pkg/hooks/types.go | Defines hook event types consumed by the registry and plugins. |
| pkg/config/config.go | Adds PluginsConfig to top-level config schema. |
| pkg/config/defaults.go | Sets plugin defaults in DefaultConfig(). |
| pkg/config/config_test.go | Adds tests for plugin defaults and JSON unmarshalling. |
| cmd/picoclaw/internal/pluginruntime/bootstrap.go | Adds bootstrap helper to resolve config -> enabled builtin plugin instances + summary. |
| cmd/picoclaw/internal/pluginruntime/bootstrap_test.go | Tests bootstrap behavior for unknown enabled/disabled and deterministic ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| enabledSet := make(map[string]struct{}, len(in.Enabled)) | ||
| for _, name := range in.Enabled { | ||
| normalized := NormalizePluginName(name) | ||
| if _, exists := enabledSet[normalized]; exists { | ||
| result.Warnings = append(result.Warnings, fmt.Sprintf("duplicate enabled plugin %q ignored", normalized)) | ||
| continue | ||
| } | ||
| enabledSet[normalized] = struct{}{} | ||
| } |
There was a problem hiding this comment.
ResolveSelection() adds normalized plugin names to enabledSet even when normalization yields an empty string (e.g., input of "" or whitespace). This can produce confusing warnings (duplicate enabled plugin ""), and will surface as an "unknown enabled plugins" error with an empty name. Skip empty normalized names (optionally emitting a warning) before de-dupe/validation.
| disabledSet := make(map[string]struct{}, len(in.Disabled)) | ||
| for _, name := range in.Disabled { | ||
| normalized := NormalizePluginName(name) | ||
| if _, exists := disabledSet[normalized]; exists { | ||
| result.Warnings = append(result.Warnings, fmt.Sprintf("duplicate disabled plugin %q ignored", normalized)) | ||
| continue | ||
| } | ||
| disabledSet[normalized] = struct{}{} | ||
| } |
There was a problem hiding this comment.
ResolveSelection() has the same issue for the disabled list: empty/whitespace-only entries are normalized to "" and added to disabledSet. This leads to misleading "unknown disabled plugin "" ignored" warnings. Skip empty normalized names before de-dupe and unknown checks.
| c := *e | ||
| c.Args = cloneMapStringAny(e.Args) | ||
| if e.Result != nil { | ||
| r := *e.Result |
There was a problem hiding this comment.
cloneVoidEvent()'s AfterToolCallEvent cloning shallow-copies tools.ToolResult. ToolResult contains a Media slice, so mutations to Result.Media inside a void hook can leak across handlers and/or back to the caller, violating the stated isolation guarantee. Deep-clone Result.Media when copying ToolResult.
| r := *e.Result | |
| r := *e.Result | |
| // Deep-clone the Media slice inside Result (if present) to avoid aliasing. | |
| rv := reflect.ValueOf(&r).Elem() | |
| mediaField := rv.FieldByName("Media") | |
| if mediaField.IsValid() && mediaField.Kind() == reflect.Slice && !mediaField.IsNil() { | |
| mediaCopy := reflect.MakeSlice(mediaField.Type(), mediaField.Len(), mediaField.Len()) | |
| reflect.Copy(mediaCopy, mediaField) | |
| mediaField.Set(mediaCopy) | |
| } |
| // Truncation is intentional for timeout normalization. | ||
| return int(n), true | ||
| case float64: | ||
| // Truncation is intentional for timeout normalization. |
There was a problem hiding this comment.
toInt() converts float32/float64 to int without any range checks. If a tool arg contains a large float value, the float->int conversion can overflow (Go spec: result is implementation-dependent when out of range), potentially producing a negative/small value that bypasses clampArgNumber(). Add max/min bounds checks for float32/float64 similar to the int64/uint64 cases.
| // Truncation is intentional for timeout normalization. | |
| return int(n), true | |
| case float64: | |
| // Truncation is intentional for timeout normalization. | |
| // Truncation is intentional for timeout normalization, but enforce int range. | |
| fn := float64(n) | |
| if fn < float64(minInt64) || fn > float64(maxInt64) { | |
| return 0, false | |
| } | |
| return int(fn), true | |
| case float64: | |
| // Truncation is intentional for timeout normalization, but enforce int range. | |
| if n < float64(minInt64) || n > float64(maxInt64) { | |
| return 0, false | |
| } |
|
|
|
@gh-xj Hi! This PR has had no activity for over 2 weeks, so I'm closing it for now to keep things organized. Feel free to reopen anytime if you'd like to continue. |
📝 Description
Stacked series PR (1/4) for Plugin System Phase 2/3.
Scope in this PR:
default_enabled,enabled,disabled)🔗 Stack Context
📌 Implementation Parity Matrix
pkg/config/config.go,pkg/config/defaults.gopkg/plugin/manager.gocmd/picoclaw/internal/pluginruntime/bootstrap.go🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
Related: #473 (Plugin System Phase 2/3 rollout)
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist