feat(pkg/tool): add GlobalToolFactoryRegistry for external tool regis…#1110
feat(pkg/tool): add GlobalToolFactoryRegistry for external tool regis…#1110249313652 wants to merge 1 commit into
Conversation
…tration This PR introduces a factory pattern-based registry that allows external projects to register Built-in Tools without modifying goClaw source code. Key features: - Zero intrusion: External projects register tools via init() - Auto-registration: goClaw automatically loads tools on startup - Factory pattern: Lazy initialization, on-demand creation - Type safety: Native Go interfaces, compile-time checking - High performance: In-process calls, microsecond latency Files added: - pkg/tool/types.go: Public Tool interface and Result struct - pkg/tool/factory.go: GlobalToolFactoryRegistry implementation Files modified: - cmd/gateway_setup.go: Add autoRegisterExternalTools() Backward compatible: Yes Breaking changes: None Test coverage: Compilation + functional tests
clark-cant
left a comment
There was a problem hiding this comment.
Review: feat(pkg/tool): add GlobalToolFactoryRegistry for external tool registration (#1110)
Summary: Introduces a public pkg/tool package with GlobalToolFactoryRegistry allowing external Go projects to register built-in tools via init(), plus an adapter in cmd/gateway_setup.go to bridge public and internal tool interfaces.
Risk level: Medium — new public API surface and extension mechanism with security implications.
Mandatory gates
- Duplicate / prior implementation: clear — no duplicate PR or issue found.
- Project standards: issue found — see findings below.
- Strategic necessity: questionable — see findings below.
Findings
Important:
-
Wrong base branch — This PR targets
main, but per CONTRIBUTING.md all PRs must targetdev.mainis frozen for stable releases; only hotfixes target it. Please rebase ontodev. -
Branch name mismatch — Branch is named
feature/pr-documentationbut the PR adds a tool factory registry, not documentation. This suggests the branch was repurposed without renaming, which makes git history confusing. -
Merge conflicts — PR is in CONFLICTING state and cannot be merged. Needs rebase.
-
No tests — The new
pkg/toolpackage has zero test coverage. At minimum: registry Register/Create/List/Count, duplicate registration panic, adapter bridging, and concurrent registration safety. This is a public API that external projects will depend on. -
Registerpanics on duplicate —panic(fmt.Sprintf(...))in a library function is hostile. External code callingRegisterfrominit()will crash the entire process with no recovery path. Return an error instead, or uselog.Printf+ skip like the rest of the codebase. -
internalToolAdapterdropsMetadata— The adapter mapspublicResultto internaltools.Resultbut silently discardsMetadata map[string]any. Either propagate it or remove the field from the publicResultstruct. -
No CI checks — No CI has run on this branch. Cannot verify build, vet, or test status.
Suggestion:
-
Strategic fit — GoClaw already offers three extension mechanisms: Custom Tools (shell), MCP Servers (external process), and Skills (workflows). Adding a fourth Go-level
init()registration path means external projects must compile against GoClaw as a Go library. This is a fundamentally different integration model. Consider whether this belongs in core or as a separate GoClaw SDK/fork pattern. The PR body'sgoclaw-quantexample suggests a fork-based workflow, which may not need in-core support. -
PR body is excessively long — The PR description is ~400 lines of marketing-style content (benefits tables, timelines, migration guides, best practices). A concise summary of changes + test plan would be more appropriate for code review.
-
log.PrintfinRegister— The codebase usesslogfor structured logging. The[ToolFactory]prefix inlog.Printfis inconsistent.
Verdict: Comment — needs rebase to dev, conflict resolution, tests, and design discussion before merge.
Posted by /ck:review-pr at 2026-06-26T06:50:00Z
[PR] feat(pkg/tool): Add GlobalToolFactoryRegistry for External Tool Registration
📋 Overview
This PR introduces
GlobalToolFactoryRegistryto goClaw, enabling external projects to register Built-in Tools through a factory pattern with zero source code modification.Core Value Proposition
🎯 Problem Statement
Current Pain Points
External projects cannot register Go Built-in Tools with goClaw. Current options:
Proposed Solution
Provide a standardized extension point via
GlobalToolFactoryRegistry:📁 Changes
New Files
1.
pkg/tool/types.go(71 lines)Public Tool interface and Result struct:
2.
pkg/tool/factory.go(89 lines)Global factory registry implementation:
Modified Files
3.
cmd/gateway_setup.go(+20 lines)Added auto-registration logic in
setupToolRegistry():Also added adapter type to bridge public and internal tool interfaces:
🧪 Testing
Compilation Test
Functional Test
Verified using goClaw Quant project:
Expected Output:
💡 Usage Examples
Example 1: Quantitative Trading Tools
Example 2: Writing Assistant Tools
📊 Benefits
For goClaw Ecosystem
Completes Extension Matrix
Lowers Integration Barrier
Enhances Platform Competitiveness
For Developers
🔍 Design Decisions
1. Why Factory Pattern?
Option A: Direct Registration
Option B: Factory Registration (✅ Adopted)
Reasons for choosing Option B:
2. Why Public Interface?
internal/tools.Toolis inaccessible to external projectspkg/tool.Toolas a public interface3. Why sync.RWMutex?
Register()uses Write LockCreate()andList()use Read Lock✅ Backward Compatibility
📝 Migration Guide
Migrating from Source Modification
Before (modifying goClaw source):
After (using factory pattern):
🎓 Best Practices
1. Tool Naming
2. Parameter Definition
Use JSON Schema format:
3. Error Handling
🤝 Contributors
📅 Timeline
PR Type: ✨ Feature
Scope:
pkg/tool/,cmd/gateway_setup.goBackward Compatible: ✅ Yes
Breaking Changes: ❌ None
Test Coverage: ✅ Compilation + Functional Tests
Documentation: ✅ Complete
🙏 Thank you for reviewing! Looking forward to your feedback and suggestions!