Skip to content

Commit d68b5ff

Browse files
JAORMXclaude
andcommitted
Update RFC based on codebase analysis
- Remove logger, healthcheck from Tier 1 (have ToolHive coupling) - Add oauth, permissions to Tier 1 (verified zero coupling) - Add Tier 2 with refactoring requirements (logger needs Viper decoupling) - Add "Not Recommended" section for deeply coupled packages - Remove verbose API signatures - Remove Alternatives Considered section Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 28367fb commit d68b5ff

1 file changed

Lines changed: 62 additions & 164 deletions

File tree

rfcs/THV-0032-toolhive-core-shared-library.md

Lines changed: 62 additions & 164 deletions
Original file line numberDiff line numberDiff line change
@@ -195,16 +195,16 @@ Based on analysis of the `toolhive` codebase:
195195

196196
#### Tier 1: Immediate Graduation Candidates
197197

198-
These packages meet all graduation criteria and should be included in the initial `toolhive-core` release:
198+
These packages meet all graduation criteria (zero ToolHive-specific coupling, well-tested, minimal dependencies) and should be included in the initial `toolhive-core` release:
199199

200200
| Package | Current Location | Stability | Rationale |
201201
|---------|------------------|-----------|-----------|
202-
| **logger** | `pkg/logger/` | Stable (v1) | 185 imports, foundational utility |
203-
| **errors** | `pkg/errors/` | Stable (v1) | HTTP-aware error handling, minimal deps |
204-
| **validation** | `pkg/validation/` | Stable (v1) | Security-focused input validation |
205-
| **healthcheck** | `pkg/healthcheck/` | Stable (v1) | Standardized health responses |
206-
| **versions** | `pkg/versions/` | Stable (v1) | Build metadata, User-Agent |
207-
| **env** | `pkg/env/` | Stable (v1) | Testable environment access |
202+
| **errors** | `pkg/errors/` | Stable (v1) | HTTP-aware error handling, zero deps, 10mo stable |
203+
| **oauth** | `pkg/oauth/` | Stable (v1) | RFC-compliant OAuth/OIDC types, fosite dep only |
204+
| **env** | `pkg/env/` | Stable (v1) | Testable environment access, zero deps |
205+
| **permissions** | `pkg/permissions/` | Stable (v1) | Container permission profiles, stdlib-only, security validations |
206+
| **validation** | `pkg/validation/` | Stable (v1) | RFC 7230 HTTP header validation, security-focused |
207+
| **versions** | `pkg/versions/` | Stable (v1) | Build metadata, User-Agent generation |
208208

209209
**Proposed `toolhive-core` structure:**
210210

@@ -218,49 +218,62 @@ toolhive-core/
218218
│ ├── errors.go # CodedError, WithCode, Code()
219219
│ ├── errors_test.go
220220
│ └── doc.go
221-
├── logger/
222-
│ ├── logger.go # Initialize, logging functions
223-
│ ├── logger_test.go
221+
├── oauth/
222+
│ ├── oauth.go # AuthorizationServerMetadata, OIDCDiscoveryDocument
223+
│ ├── oauth_test.go
224+
│ └── doc.go
225+
├── env/
226+
│ ├── env.go # Reader interface, OSReader
227+
│ ├── env_test.go
228+
│ └── doc.go
229+
├── permissions/
230+
│ ├── profile.go # Profile, NetworkPermissions, MountDeclaration
231+
│ ├── profile_test.go
224232
│ └── doc.go
225233
├── validation/
226234
│ ├── validation.go # HTTP header, URI, group name validators
227235
│ ├── validation_test.go
228236
│ └── doc.go
229-
├── healthcheck/
230-
│ ├── healthcheck.go # HealthResponse, MCPStatus types
231-
│ ├── healthcheck_test.go
232-
│ └── doc.go
233-
├── versions/
234-
│ ├── version.go # VersionInfo, GetUserAgent
235-
│ ├── version_test.go
236-
│ └── doc.go
237-
└── env/
238-
├── env.go # Reader interface, OSReader
239-
├── env_test.go
237+
└── versions/
238+
├── version.go # VersionInfo, GetUserAgent
239+
├── version_test.go
240240
└── doc.go
241241
```
242242

243-
#### Tier 2: Secondary Candidates (Future Releases)
243+
#### Tier 2: Minor Refactoring Required
244244

245-
These packages are stable but require additional review before graduation:
245+
These packages have solid implementations but need minor changes to remove ToolHive-specific coupling:
246246

247-
| Package | Current Location | Status | Graduation Considerations |
248-
|---------|------------------|--------|---------------------------|
249-
| **secrets** | `pkg/secrets/` | Beta | Multiple providers (1Password, keyring), needs interface review |
250-
| **auth/oauth** | `pkg/auth/oauth/` | Beta | PKCE support, token exchange - needs decoupling from transport |
251-
| **transport/types** | `pkg/transport/types/` | Beta | Middleware abstraction, widely used |
252-
| **mcp** | `pkg/mcp/` | Beta | MCP primitives, protocol-specific |
247+
| Package | Current Location | Status | Required Changes |
248+
|---------|------------------|--------|------------------|
249+
| **healthcheck** | `pkg/healthcheck/` | Beta | Remove unused logger import |
250+
| **logger** | `pkg/logger/` | Beta | Remove `viper.GetBool("debug")` coupling; accept config via parameters |
251+
| **auth/tokenexchange** | `pkg/auth/tokenexchange/` | Beta | Split core RFC 8693 logic from ToolHive middleware |
252+
| **networking** | `pkg/networking/` | Beta | Replace ToolHive logger with slog or interface |
253+
| **lockfile** | `pkg/lockfile/` | Beta | Accept logger interface instead of importing pkg/logger |
253254

254-
#### Tier 3: Future Candidates (Requires Evolution)
255+
#### Tier 3: Partial Extraction or Future Work
255256

256-
These packages need further stabilization before graduation consideration:
257+
These packages have useful components but require significant refactoring or are too coupled:
257258

258259
| Package | Current Location | Status | Notes |
259260
|---------|------------------|--------|-------|
260-
| **authz** | `pkg/authz/` | Alpha | Cedar policy support still evolving |
261-
| **authserver** | `pkg/authserver/` | Beta | Complex, may warrant own module |
262-
| **vmcp** | `pkg/vmcp/` | Stable | Domain-specific, may stay in toolhive |
263-
| **config** | `pkg/config/` | Beta | Build-specific logic, needs refactoring |
261+
| **labels** | `pkg/labels/` | Beta | Extract K8s validation funcs only; keep ToolHive constants |
262+
| **secrets** | `pkg/secrets/` | Beta | Multiple providers, needs interface review |
263+
| **transport/types** | `pkg/transport/types/` | Beta | Middleware abstraction, widely used but coupled |
264+
| **mcp** | `pkg/mcp/` | Beta | MCP primitives, protocol-specific |
265+
266+
#### Not Recommended for Extraction
267+
268+
These packages are too coupled to ToolHive internals:
269+
270+
| Package | Reason |
271+
|---------|--------|
272+
| **auth** (heavy pieces) | Product-level wiring: token.go, oauth/*, remote/*, secrets/* |
273+
| **runner**, **workloads** | Deep orchestration coupling, RunConfig migrations |
274+
| **config** | Viper global state, ToolHive-specific env vars |
275+
| **ignore** | Hardcoded `.thvignore`, ToolHive paths |
276+
| **process** | Hardcoded paths, imports pkg/container/runtime |
264277

265278
### Detailed Design
266279

@@ -283,103 +296,18 @@ github.com/stacklok/toolhive-core
283296
└── [packages]/
284297
```
285298

286-
#### API Design: Logger Package
287-
288-
```go
289-
// Package logger provides structured logging for ToolHive services.
290-
//
291-
// Stability: Stable (v1)
292-
// Since: v0.1.0
293-
package logger
294-
295-
import (
296-
"github.com/go-logr/logr"
297-
)
298-
299-
// Logger is the interface for structured logging.
300-
type Logger interface {
301-
logr.Logger
302-
}
303-
304-
// Config holds logger configuration.
305-
type Config struct {
306-
// Level is the minimum log level (debug, info, warn, error).
307-
Level string
308-
// Format is the output format (json, text).
309-
Format string
310-
// Development enables development-friendly output.
311-
Development bool
312-
}
313-
314-
// Initialize creates and configures the global logger.
315-
// This should be called once at application startup.
316-
func Initialize(cfg Config) error
317-
318-
// Get returns the configured logger instance.
319-
// If Initialize has not been called, returns a no-op logger.
320-
func Get() Logger
321-
322-
// WithName returns a logger with the specified name added.
323-
func WithName(name string) Logger
324-
325-
// WithValues returns a logger with additional key-value pairs.
326-
func WithValues(keysAndValues ...interface{}) Logger
327-
```
328-
329-
#### API Design: Errors Package
330-
331-
```go
332-
// Package errors provides HTTP-aware error handling.
333-
//
334-
// Stability: Stable (v1)
335-
// Since: v0.1.0
336-
package errors
299+
#### Package Descriptions
337300

338-
// CodedError wraps an error with an HTTP status code.
339-
type CodedError struct {
340-
err error
341-
code int
342-
}
301+
| Package | Purpose | Key Exports |
302+
|---------|---------|-------------|
303+
| **errors** | HTTP-aware error wrapping | `CodedError`, `WithCode()`, `Code()` |
304+
| **oauth** | RFC 8414/OIDC discovery types | `AuthorizationServerMetadata`, `OIDCDiscoveryDocument`, redirect validation |
305+
| **env** | Testable environment access | `Reader` interface, `OSReader` |
306+
| **permissions** | Container security profiles | `Profile`, `NetworkPermissions`, `MountDeclaration`, security validations |
307+
| **validation** | RFC 7230 HTTP validation | Header name/value validators, group name, resource URI |
308+
| **versions** | Build metadata | `VersionInfo`, `GetUserAgent()` |
343309

344-
// WithCode wraps an error with an HTTP status code.
345-
func WithCode(err error, code int) *CodedError
346-
347-
// Code extracts the HTTP status code from an error.
348-
// Returns 500 if the error does not have a code.
349-
func Code(err error) int
350-
351-
// Error implements the error interface.
352-
func (e *CodedError) Error() string
353-
354-
// Unwrap returns the underlying error.
355-
func (e *CodedError) Unwrap() error
356-
```
357-
358-
#### API Design: Validation Package
359-
360-
```go
361-
// Package validation provides security-focused input validation.
362-
//
363-
// Stability: Stable (v1)
364-
// Since: v0.1.0
365-
package validation
366-
367-
// ValidateHTTPHeaderName validates an HTTP header name per RFC 7230.
368-
func ValidateHTTPHeaderName(name string) error
369-
370-
// ValidateHTTPHeaderValue validates an HTTP header value per RFC 7230.
371-
func ValidateHTTPHeaderValue(value string) error
372-
373-
// ValidateGroupName validates a ToolHive group name.
374-
// Group names must be lowercase, cannot contain consecutive spaces,
375-
// and cannot contain null bytes.
376-
func ValidateGroupName(name string) error
377-
378-
// ValidateResourceURI validates a resource URI per RFC 8707.
379-
func ValidateResourceURI(uri string) error
380-
```
381-
382-
#### Configuration Changes
310+
#### Migration
383311

384312
Consuming projects update their `go.mod`:
385313

@@ -449,36 +377,6 @@ import "github.com/stacklok/toolhive-core/logger"
449377
| Credential exposure | Redaction functions, sensitive field markers |
450378
| Breaking changes | Semver, deprecation warnings, migration guides |
451379

452-
## Alternatives Considered
453-
454-
### Alternative 1: Go Workspace / Monorepo
455-
456-
- **Description**: Use Go workspaces to share packages within a monorepo
457-
- **Pros**: Single repo, easier refactoring, atomic changes
458-
- **Cons**: Couples release cycles, complicates CI/CD, doesn't help external consumers
459-
- **Why not chosen**: ToolHive ecosystem already uses multiple repos; changing to monorepo is disruptive
460-
461-
### Alternative 2: Git Submodules
462-
463-
- **Description**: Share packages via git submodules
464-
- **Pros**: Direct source access, version pinning
465-
- **Cons**: Complex workflow, poor Go tooling support, merge conflicts
466-
- **Why not chosen**: Go modules provide better dependency management
467-
468-
### Alternative 3: Internal Packages with Replace Directives
469-
470-
- **Description**: Keep packages internal, use `replace` directives for sharing
471-
- **Pros**: No new repo, immediate availability
472-
- **Cons**: No stability guarantees, manual coordination, breaks `go get`
473-
- **Why not chosen**: Doesn't solve the core problems of stability and discoverability
474-
475-
### Alternative 4: Copy-Paste with Attribution
476-
477-
- **Description**: Manually copy code between projects
478-
- **Pros**: Full control, no external dependencies
479-
- **Cons**: Divergence over time, duplicated bug fixes, inconsistency
480-
- **Why not chosen**: Already causing problems; doesn't scale
481-
482380
## Compatibility
483381

484382
### Backward Compatibility
@@ -542,7 +440,7 @@ import "github.com/stacklok/toolhive-core/logger"
542440

543441
### Integration Tests
544442

545-
- Cross-package integration tests (e.g., logger + errors)
443+
- Cross-package integration tests (e.g., validation + errors)
546444
- Import cycle detection in CI
547445

548446
### Compatibility Tests
@@ -552,7 +450,7 @@ import "github.com/stacklok/toolhive-core/logger"
552450

553451
### Performance Tests
554452

555-
- Benchmark logger throughput
453+
- Benchmark validation functions
556454
- Memory allocation tests for hot paths
557455

558456
### Security Tests
@@ -572,8 +470,8 @@ import "github.com/stacklok/toolhive-core/logger"
572470
## Open Questions
573471

574472
1. **Release cadence**: Monthly minor releases, or release when ready?
575-
2. **Tier 2 timeline**: When should we start evaluating secrets/auth for graduation?
576-
3. **Logger choice**: Should toolhive-core use zap (current toolhive approach) or slog (stdlib, used by registry-server)? Or provide adapters for both?
473+
2. **Tier 2 timeline**: When should we start refactoring logger/healthcheck/tokenexchange for graduation?
474+
3. **Logger refactoring approach**: When decoupling logger from Viper, should we use zap with config injection, switch to slog, or provide an interface that supports both?
577475

578476
## References
579477

0 commit comments

Comments
 (0)