Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,79 @@ awmg/
- Add Godoc comments for all exported functions, types, and packages
- Mock external dependencies (Docker, network) in tests

### Constructor Naming Conventions

The codebase uses three distinct constructor patterns. Follow these conventions consistently:

#### 1. `New*(args) *Type` - Standard Constructors

Use for simple object creation without error handling or complex initialization.

```go
// Creates a new instance of the type directly
func NewConnection(ctx context.Context) *Connection { ... }
func NewRegistry() *Registry { ... }
func NewSession(sessionID, token string) *Session { ... }
```

**When to use:**
- Object creation is always successful (no errors to return)
- Direct instantiation of struct with provided parameters
- Most common pattern in the codebase (35+ usages)
Comment on lines +253 to +267
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The New* examples/signature here don’t match the codebase: internal/mcp/connection.go defines NewConnection(...) (*Connection, error) (and NewHTTPConnection(...) (*Connection, error)), so this section’s contract “no errors to return” and the example func NewConnection(ctx context.Context) *Connection are inaccurate. Please update the documentation to either (a) use examples that truly match the New*(...) *Type pattern (e.g., NewRegistry, NewSession) and state that constructors returning errors should be Create*, or (b) adjust the stated pattern to reflect actual usage (but then the criteria bullets should change accordingly).

This issue also appears on line 305 of the same file.

Copilot uses AI. Check for mistakes.

#### 2. `Create*(args) (Type, error)` - Factory Patterns

Use for factory functions that perform registry lookups or complex configuration-based initialization.

```go
// Looks up a guard type from registry and creates it
func CreateGuard(name string) (Guard, error) { ... }

// Complex initialization with potential failures
func CreateHTTPServerForMCP(cfg *Config) (*http.Server, error) { ... }
Comment on lines +269 to +278
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Create* example for CreateHTTPServerForMCP is incorrect and conflicts with the pattern described here: in internal/server/transport.go, CreateHTTPServerForMCP(addr string, unifiedServer *UnifiedServer, apiKey string) *http.Server does not take *Config and does not return an error. Either pick a different Create* example that actually matches (T, error), or adjust this section to explicitly allow Create* factories that return only *T when creation can’t fail.

Suggested change
#### 2. `Create*(args) (Type, error)` - Factory Patterns
Use for factory functions that perform registry lookups or complex configuration-based initialization.
```go
// Looks up a guard type from registry and creates it
func CreateGuard(name string) (Guard, error) { ... }
// Complex initialization with potential failures
func CreateHTTPServerForMCP(cfg *Config) (*http.Server, error) { ... }
#### 2. `Create*(args) (Type, error)` / `Create*(args) *Type` - Factory Patterns
Use for factory functions that perform registry lookups or complex configuration-based initialization. Prefer returning `(Type, error)` when creation can fail; return only `*Type` when creation is guaranteed to succeed.
```go
// Looks up a guard type from registry and creates it
func CreateGuard(name string) (Guard, error) { ... }
// HTTP server setup that cannot fail at creation time
func CreateHTTPServerForMCP(addr string, unifiedServer *UnifiedServer, apiKey string) *http.Server { ... }

Copilot uses AI. Check for mistakes.
```

**When to use:**
- Registry-based object creation (looking up registered types)
- Complex configuration that might fail
- Need to validate parameters and return errors
- Factory pattern with type selection logic

#### 3. `Init*(args) error` - Global State Initialization

Use for initializing global singletons, loggers, or package-level state.

```go
// Initializes global file logger singleton
func InitFileLogger(dir string) error { ... }

// Initializes global JSON logger singleton
func InitJSONLLogger(dir string) error { ... }
Comment on lines +293 to +296
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Init* examples/signatures are inconsistent with the actual logger APIs: InitFileLogger and InitJSONLLogger both take (logDir, fileName string) (not just dir string). Please update the example signatures here so contributors copy/paste the correct call pattern.

Suggested change
func InitFileLogger(dir string) error { ... }
// Initializes global JSON logger singleton
func InitJSONLLogger(dir string) error { ... }
func InitFileLogger(logDir, fileName string) error { ... }
// Initializes global JSON logger singleton
func InitJSONLLogger(logDir, fileName string) error { ... }

Copilot uses AI. Check for mistakes.
```

**When to use:**
- Initializing global variables or package-level state
- Singleton initialization that should only happen once
- Setting up loggers, configuration, or other shared resources
- Typically returns an error if initialization fails

#### Examples from Codebase

**Standard Constructors (`New*`):**
- `NewConnection`, `NewHTTPConnection` (mcp package)
- `NewUnified`, `NewSession` (server package)
- `NewRegistry`, `NewNoopGuard` (guard package)
- `NewLabel`, `NewAgentLabels` (difc package)

**Factory Patterns (`Create*`):**
- `CreateGuard` (guard package) - registry lookup
- `CreateHTTPServerForMCP` (server package) - complex config-based creation

**Global Initialization (`Init*`):**
- `InitFileLogger`, `InitJSONLLogger`, `InitMarkdownLogger`, `InitServerFileLogger` (logger package)

**When in doubt:** Use `New*` for most constructors. Only use `Create*` when implementing factory patterns with type selection, and `Init*` for global state initialization.

### Debug Logging

Use the logger package for debug logging:
Expand Down