|
| 1 | +--- |
| 2 | +name: review-rfc |
| 3 | +description: Review RFCs for the ToolHive ecosystem. Use when the user wants to review, critique, or provide feedback on an RFC for toolhive, toolhive-studio, toolhive-registry, toolhive-registry-server, toolhive-cloud-ui, or dockyard projects. |
| 4 | +allowed-tools: Read, Glob, Grep, Bash(git:*), mcp__github__*, mcp__fetch__fetch, WebFetch, Task |
| 5 | +--- |
| 6 | + |
| 7 | +# Review RFC Skill |
| 8 | + |
| 9 | +This skill helps you thoroughly review RFCs for the ToolHive ecosystem, ensuring they meet quality standards, architectural alignment, and security requirements. |
| 10 | + |
| 11 | +## Overview |
| 12 | + |
| 13 | +When reviewing an RFC, you should evaluate it against multiple dimensions: completeness, technical accuracy, architectural alignment, security considerations, and feasibility. |
| 14 | + |
| 15 | +## Review Workflow |
| 16 | + |
| 17 | +### Step 1: Read the RFC |
| 18 | + |
| 19 | +First, read the RFC document completely. If provided a PR number or file path, fetch and read it. |
| 20 | + |
| 21 | +### Step 2: Fetch Architectural Context |
| 22 | + |
| 23 | +Before reviewing, gather context from the ToolHive architecture documentation. Use `mcp__github__get_file_contents` to read relevant docs from `stacklok/toolhive` repo's `docs/arch/` directory: |
| 24 | + |
| 25 | +| Document | When to Read | |
| 26 | +|----------|--------------| |
| 27 | +| `00-overview.md` | Always - understand platform concepts | |
| 28 | +| `01-deployment-modes.md` | RFCs affecting deployment, K8s, or local mode | |
| 29 | +| `02-core-concepts.md` | RFCs introducing new concepts or terminology | |
| 30 | +| `03-transport-architecture.md` | RFCs affecting MCP transports or proxy | |
| 31 | +| `04-secrets-management.md` | RFCs involving secrets or credentials | |
| 32 | +| `05-runconfig-and-permissions.md` | RFCs affecting configuration or permissions | |
| 33 | +| `06-registry-system.md` | RFCs affecting registry functionality | |
| 34 | +| `07-groups.md` | RFCs involving server grouping | |
| 35 | +| `08-workloads-lifecycle.md` | RFCs affecting workload management | |
| 36 | +| `09-operator-architecture.md` | RFCs affecting K8s operator or CRDs | |
| 37 | +| `10-virtual-mcp-architecture.md` | RFCs involving aggregation or virtual MCP | |
| 38 | + |
| 39 | +### Step 3: Check Related Existing RFCs |
| 40 | + |
| 41 | +Search this repository for related RFCs that might: |
| 42 | +- Conflict with the proposal |
| 43 | +- Be superseded by the proposal |
| 44 | +- Provide context or dependencies |
| 45 | + |
| 46 | +### Step 4: Verify Against Target Repository |
| 47 | + |
| 48 | +Search the target repository to verify: |
| 49 | +- Proposed changes align with existing code patterns |
| 50 | +- No conflicts with recent changes |
| 51 | +- API changes are compatible with existing interfaces |
| 52 | + |
| 53 | +## Review Checklist |
| 54 | + |
| 55 | +### A. Structure and Completeness |
| 56 | + |
| 57 | +- [ ] **Metadata present**: Status, Author, Created, Last Updated, Target Repository |
| 58 | +- [ ] **Summary**: Clear 2-3 sentence description |
| 59 | +- [ ] **Problem Statement**: Clearly articulates the problem, who's affected, why it matters |
| 60 | +- [ ] **Goals**: Specific, measurable objectives listed |
| 61 | +- [ ] **Non-Goals**: Explicit scope boundaries defined |
| 62 | +- [ ] **Proposed Solution**: Detailed design with diagrams where appropriate |
| 63 | +- [ ] **Security Considerations**: All required subsections present (see below) |
| 64 | +- [ ] **Alternatives Considered**: At least one alternative evaluated |
| 65 | +- [ ] **Compatibility**: Backward and forward compatibility addressed |
| 66 | +- [ ] **Implementation Plan**: Phased approach with concrete tasks |
| 67 | +- [ ] **Testing Strategy**: Multiple test levels covered |
| 68 | +- [ ] **Open Questions**: Unresolved items listed (if any) |
| 69 | + |
| 70 | +### B. Security Review (CRITICAL) |
| 71 | + |
| 72 | +The Security Considerations section MUST address all of these: |
| 73 | + |
| 74 | +| Section | Questions to Verify | |
| 75 | +|---------|---------------------| |
| 76 | +| **Threat Model** | Are potential threats identified? Are attacker capabilities considered? | |
| 77 | +| **Authentication** | How does this affect auth? Are new auth requirements clear? | |
| 78 | +| **Authorization** | What permission checks are needed? Any new permission models? | |
| 79 | +| **Data Security** | Is sensitive data identified? Is encryption addressed? | |
| 80 | +| **Input Validation** | What user input is accepted? How is it validated? | |
| 81 | +| **Secrets Management** | Are secrets handled properly? Can they be rotated? | |
| 82 | +| **Audit and Logging** | Are security events logged? Compliance considered? | |
| 83 | +| **Mitigations** | Are concrete mitigations proposed for identified threats? | |
| 84 | + |
| 85 | +**Red flags to watch for:** |
| 86 | +- Missing or superficial security section |
| 87 | +- "N/A" without justification for security subsections |
| 88 | +- No threat model for network-exposed features |
| 89 | +- Secrets in configuration examples |
| 90 | +- Missing input validation for user-provided data |
| 91 | +- No audit logging for security-relevant operations |
| 92 | + |
| 93 | +### C. Technical Accuracy |
| 94 | + |
| 95 | +- [ ] **Correct terminology**: Uses ToolHive concepts correctly (Workloads, Transports, Middleware, etc.) |
| 96 | +- [ ] **Architecture alignment**: Follows established patterns and principles |
| 97 | +- [ ] **Code examples**: Syntactically correct, idiomatic for the language |
| 98 | +- [ ] **API design**: Consistent with existing APIs in the target repo |
| 99 | +- [ ] **CRD design**: Follows Kubernetes conventions if applicable |
| 100 | +- [ ] **Configuration format**: Matches existing RunConfig patterns |
| 101 | + |
| 102 | +### D. Diagrams and Examples |
| 103 | + |
| 104 | +- [ ] **Mermaid diagrams**: Complex flows illustrated clearly |
| 105 | +- [ ] **Code examples**: Concrete, not abstract placeholders |
| 106 | +- [ ] **Configuration examples**: Realistic YAML/JSON examples |
| 107 | +- [ ] **Sequence diagrams**: For multi-component interactions |
| 108 | + |
| 109 | +### E. Feasibility and Impact |
| 110 | + |
| 111 | +- [ ] **Implementation complexity**: Is the phased approach realistic? |
| 112 | +- [ ] **Dependencies**: Are external dependencies identified? |
| 113 | +- [ ] **Breaking changes**: Are migration paths provided if needed? |
| 114 | +- [ ] **Performance impact**: Considered where relevant? |
| 115 | +- [ ] **Cross-repo impact**: If `multiple` repos, are all impacts identified? |
| 116 | + |
| 117 | +## ToolHive Ecosystem Context |
| 118 | + |
| 119 | +### Target Repositories |
| 120 | + |
| 121 | +| Repository | Type | Key Considerations | |
| 122 | +|------------|------|-------------------| |
| 123 | +| `toolhive` | Go | Core platform, CLI, operator, proxy, virtual MCP | |
| 124 | +| `toolhive-studio` | TypeScript | Desktop UI, Electron app | |
| 125 | +| `toolhive-registry-server` | Go | Registry API, MCP Registry spec compliance | |
| 126 | +| `toolhive-registry` | Go/JSON | Registry data, server definitions | |
| 127 | +| `toolhive-cloud-ui` | TypeScript/Next.js | Cloud UI, OIDC integration | |
| 128 | +| `dockyard` | Go | Container packaging, security scanning | |
| 129 | + |
| 130 | +### Key Architecture Principles to Verify |
| 131 | + |
| 132 | +1. **Platform, not runner**: Does this enhance the platform abstraction? |
| 133 | +2. **Security by default**: Does this maintain or improve security posture? |
| 134 | +3. **Middleware composability**: Can this be implemented as middleware if appropriate? |
| 135 | +4. **RunConfig portability**: Does this preserve configuration portability? |
| 136 | +5. **Cloud-native**: Is this Kubernetes-friendly where applicable? |
| 137 | + |
| 138 | +### CRD Types (for K8s-related RFCs) |
| 139 | + |
| 140 | +- `MCPServer` - Individual MCP server deployment |
| 141 | +- `MCPRegistry` - Registry configuration |
| 142 | +- `MCPToolConfig` - Tool filtering and configuration |
| 143 | +- `MCPExternalAuthConfig` - External authentication |
| 144 | +- `MCPGroup` - Server grouping |
| 145 | +- `VirtualMCPServer` - Aggregation of multiple servers |
| 146 | + |
| 147 | +## Review Output Format |
| 148 | + |
| 149 | +Structure your review as follows: |
| 150 | + |
| 151 | +```markdown |
| 152 | +## RFC Review: [RFC Title] |
| 153 | + |
| 154 | +### Summary |
| 155 | +[1-2 sentence summary of your overall assessment] |
| 156 | + |
| 157 | +### Strengths |
| 158 | +- [What the RFC does well] |
| 159 | + |
| 160 | +### Areas for Improvement |
| 161 | + |
| 162 | +#### Critical Issues (Must Fix) |
| 163 | +- [Issues that must be addressed before acceptance] |
| 164 | + |
| 165 | +#### Suggestions (Should Consider) |
| 166 | +- [Improvements that would strengthen the RFC] |
| 167 | + |
| 168 | +#### Minor/Nitpicks (Optional) |
| 169 | +- [Small improvements or style suggestions] |
| 170 | + |
| 171 | +### Security Assessment |
| 172 | +[Specific feedback on the security section] |
| 173 | + |
| 174 | +### Architectural Alignment |
| 175 | +[How well does this align with ToolHive architecture?] |
| 176 | + |
| 177 | +### Questions for the Author |
| 178 | +- [Clarifying questions that need answers] |
| 179 | + |
| 180 | +### Recommendation |
| 181 | +[ ] Ready to accept |
| 182 | +[ ] Accept with minor changes |
| 183 | +[ ] Needs revision (address critical issues) |
| 184 | +[ ] Major rework needed |
| 185 | +``` |
| 186 | + |
| 187 | +## Common Issues to Watch For |
| 188 | + |
| 189 | +### Problem Statement Issues |
| 190 | +- Too vague or abstract |
| 191 | +- Doesn't explain who benefits |
| 192 | +- Problem already solved elsewhere |
| 193 | + |
| 194 | +### Design Issues |
| 195 | +- Over-engineered for the problem |
| 196 | +- Missing error handling considerations |
| 197 | +- Doesn't consider edge cases |
| 198 | +- Breaks existing functionality without migration path |
| 199 | + |
| 200 | +### Security Issues |
| 201 | +- Missing threat model |
| 202 | +- Hardcoded credentials in examples |
| 203 | +- No input validation |
| 204 | +- Missing audit logging |
| 205 | +- Overly permissive defaults |
| 206 | + |
| 207 | +### Implementation Issues |
| 208 | +- Unrealistic phasing |
| 209 | +- Missing dependencies |
| 210 | +- No rollback plan |
| 211 | +- Insufficient testing strategy |
| 212 | + |
| 213 | +## Reference Files |
| 214 | + |
| 215 | +- Template: `rfcs/0000-template.md` |
| 216 | +- Contributing guide: `CONTRIBUTING.md` |
| 217 | +- Existing RFCs: `rfcs/THV-*.md` |
0 commit comments