core/go

Fork 0

generated from lthn/LEM

audit: review exported API surface and package documentation #6

New issue

Open

opened 2026-02-16 17:01:17 +00:00 by clotho · 1 comment

clotho commented

2026-02-16 17:01:17 +00:00

Member

Review the exported API surface of core/go. For each package:

Check that exported functions/types have doc comments
Verify naming follows Go conventions
Flag any exported symbols that should be unexported
Check for missing error returns or silent failures

Post findings as a comment on this issue. Do not make code changes — this is a review task.

Review the exported API surface of core/go. For each package: 1. Check that exported functions/types have doc comments 2. Verify naming follows Go conventions 3. Flag any exported symbols that should be unexported 4. Check for missing error returns or silent failures Post findings as a comment on this issue. Do not make code changes — this is a review task.

clotho added the

clotho

audit

labels 2026-02-16 17:01:17 +00:00

clotho commented

2026-02-16 17:27:00 +00:00

Author

Member

Exported API Audit Report

This audit reviewed the exported API surface across 14 key packages in the core/go repository.

Executive Summary

The codebase demonstrates strong documentation practices overall, with most exported symbols having clear doc comments. However, several areas require attention regarding naming conventions, missing documentation, and potential API design improvements.

1. Framework Core (pkg/framework/core/)

Package Purpose: Central dependency injection framework managing services, lifecycle, and inter-service communication.

Missing Documentation:

✅ None - excellent documentation coverage

Naming Convention Issues:

⚠️ ACTION, QUERY, QUERYALL, PERFORM - All-caps method names deviate from Go conventions. Should be Action, Query, QueryAll, Perform
These violate Go's MixedCaps convention

Symbols That Should Be Unexported:

✅ None identified - API surface is appropriate

Missing Error Handling:

✅ None identified - proper error returns throughout

Other API Design Concerns:

The all-caps method names create inconsistency with standard Go practice
SetInstance and GetInstance provide global mutable state which could be problematic in testing/concurrent scenarios
Consider deprecating global instance pattern in favor of explicit dependency passing

2. MCP Server (pkg/mcp/)

Package Purpose: Model Context Protocol server providing tools for file operations, RAG, metrics, process management, and browser automation.

Missing Documentation:

Option type (line 35)
WithSubsystem function parameters could use more detail about subsystem lifecycle
webviewInstance global variable (tools_webview.go:16)

Symbols That Should Be Unexported:

webviewInstance (tools_webview.go:16) - Global mutable state, should be unexported
detectLanguageFromPath (mcp.go:440)
parseDuration (tools_metrics.go:181)
convertMetricCounts (tools_metrics.go:161)

Missing Error Handling:

registerProcessTools, registerWSTools, etc. return bool but don't expose why features are unavailable
Tool handler methods silently discard first return value from CallToolRequest

Other API Design Concerns:

Heavy use of global state (webviewInstance) creates concurrency issues
No clear API to check which subsystems are available before calling Run()
Input/Output types for each tool are well-documented ✅

3. WebSocket Hub (pkg/ws/)

Package Purpose: Real-time WebSocket communication hub with channel-based subscriptions.

Missing Documentation:

✅ None - excellent package-level and symbol-level documentation

Symbols That Should Be Unexported:

mustMarshal (line 444)
Client struct fields (hub, conn, send, subscriptions, mu) - should all be unexported

Missing Error Handling:

mustMarshal ignores errors which could lead to silent failures
Broadcast error return behavior not documented when internal channel blocks

Other API Design Concerns:

✅ Excellent API design overall
⚠️ CheckOrigin currently accepts all origins - documented but potentially dangerous default

4. Browser Automation (pkg/webview/)

Package Purpose: Chrome DevTools Protocol client for browser automation.

Missing Documentation:

Action interface (actions.go:10)
ConsoleWatcher struct fields
ExceptionWatcher struct fields
AngularHelper struct fields

Symbols That Should Be Unexported:

cdpMessage, cdpResponse, cdpEvent, cdpError, targetInfo (cdp.go)
Action types (ClickAction, TypeAction, etc.) could be unexported with factory functions
Helper functions in angular.go: getString, formatJSValue, containsString, findString

Missing Error Handling:

Evaluate method allows arbitrary JavaScript execution - security implications noted but could be clearer
Many Action types ignore potential nil pointer issues

Other API Design Concerns:

✅ Excellent documentation in webview.go package comment
✅ ActionSequence provides nice fluent API
Consider adding rate limiting to CDP calls

5. Process Management (pkg/process/)

Package Purpose: External process execution with output capture and lifecycle management.

Missing Documentation:

Process struct fields need internal doc comments for maintainability
RingBuffer type and methods need documentation

Missing Error Handling:

Wait() returns generic exec.ExitError losing specific exit code information
Signal method has incomplete implementation (noted in comments)

Other API Design Concerns:

✅ Good use of context for cancellation
✅ RingBuffer for output capture is well-designed
Consider adding process groups for batch management

6. CLI Framework (pkg/cli/)

Package Purpose: CLI application framework with commands, layouts, and ANSI styling.

Missing Documentation:

AppVersion, BuildCommit, BuildDate, BuildPreRelease (app.go:27-30)
completionCmd (app.go:97)
Various glyph and style types

Symbols That Should Be Unexported:

completionCmd (app.go:97)

Missing Error Handling:

Main() exits with code 1 for all errors - may want more granular exit codes

Other API Design Concerns:

✅ Good separation of ANSI, layout, and command concerns
Heavy use of global state (RootCmd, etc.) makes testing difficult
Consider dependency injection pattern

7. Configuration (pkg/config/)

Package Purpose: Layered configuration management with file, environment, and flag support.

Missing Documentation:

✅ None - excellent documentation

Symbols That Should Be Unexported:

Load function (line 175) is deprecated - should be unexported

Other API Design Concerns:

✅ Excellent use of functional options pattern
✅ Thread-safe with proper mutex usage
✅ Deprecation is well-documented

8. Logging (pkg/log/)

Package Purpose: Structured logging with levels, rotation, and style support.

Missing Documentation:

identity function (log.go:130)

Symbols That Should Be Unexported:

identity function (log.go:130)

Other API Design Concerns:

✅ Good level-based filtering
✅ Rotation support is excellent
Consider adding structured context fields beyond key-value pairs

9. Cryptography (pkg/crypt/)

Package Purpose: Encryption, hashing, HMAC, and key derivation functions.

Missing Documentation:

Package documentation could expand on algorithm choices and security properties
Several constants lack doc comments explaining cryptographic significance

Other API Design Concerns:

✅ Clean API separating different cryptographic primitives
✅ Good use of modern algorithms (ChaCha20-Poly1305, Argon2)
EncryptAES and Encrypt have identical APIs - consider consolidating
Consider adding version prefix to ciphertext for algorithm agility

10. Build System (pkg/build/)

Package Purpose: Project detection and cross-compilation for multiple project types.

Missing Documentation:

Builder interface methods could use more detailed doc comments
Config struct fields need individual doc comments

Other API Design Concerns:

✅ Good abstraction with Builder interface
Consider adding validation methods for Config

11. AI/LLM Client (pkg/agentic/)

Package Purpose: API client for core-agentic service providing task management.

Missing Documentation:

NewClient and NewClientFromConfig could document authentication requirements

Other API Design Concerns:

✅ Clean REST client design
✅ Good use of context throughout
HTTP timeout is hardcoded (30s) - could be configurable
Consider adding retry logic for transient failures

12. Agent CI / Clotho (pkg/agentci/)

Package Purpose: Dual-run verification orchestrator for AI agent CI/CD.

Missing Documentation:

RunMode type and constants could use more explanation
Spinner struct fields need doc comments
Weave method is placeholder but public - should document future plans

Missing Error Handling:

Weave currently just compares strings - weak implementation for public API
No validation of ClothoConfig or AgentConfig inputs

Other API Design Concerns:

✅ Interesting dual-run verification concept
Hard-coded repo criticality logic should be configurable
Consider adding middleware/hooks for custom verification

13. Forgejo Client (pkg/forge/)

Package Purpose: Thin wrapper around Forgejo SDK for repository and issue management.

Missing Documentation:

✅ Package doc is excellent
Individual methods would benefit from examples

Other API Design Concerns:

✅ Very thin wrapper which is appropriate
✅ Config resolution priority is well-documented
Thread safety of client not documented

14. Git Operations (pkg/git/)

Package Purpose: Multi-repository git operations with status checking and sync.

Missing Documentation:

StatusOptions could document behavior when Names map is incomplete

Symbols That Should Be Unexported:

gitCommand (line 226)
gitInteractive (line 166)
getStatus (line 72)
getAheadBehind (line 126)

Other API Design Concerns:

✅ Parallel status checking is excellent for performance
✅ Good use of context throughout
No support for authentication configuration (relies on git config)

Priority Recommendations

1. Critical - Naming Conventions

Rename ACTION, QUERY, QUERYALL, PERFORM in core package to Action, Query, QueryAll, Perform

2. High - Global State

Remove global webviewInstance in pkg/mcp/tools_webview.go
Review all global mutable state for thread-safety issues

3. High - Unexported Symbols

Unexport internal helpers across all packages (35+ symbols identified)
Unexport completionCmd in pkg/cli

4. Medium - Missing Documentation

Add doc comments to pkg/cli exported variables
Document Action interface and related types in pkg/webview
Expand pkg/crypt documentation with security guidance

5. Medium - API Design

Add subsystem availability checking to pkg/mcp
Consider deprecating global instance pattern in core framework
Improve error messages when optional features are unavailable

6. Low - Future Enhancements

Add validation methods to build.Config
Implement full dual-run comparison in agentci.Weave
Add retry logic to agentic.Client

Statistics

Packages Audited: 14
Files Reviewed: 50+
Critical Issues: 1 (naming conventions in core)
High Priority Issues: 3 (global state, unexported symbols)
Medium Priority Issues: 6 (documentation, API design)
Low Priority Issues: 3 (enhancements)

Conclusion

The core/go repository demonstrates strong engineering practices with comprehensive documentation, good error handling, and thoughtful API design. The primary concerns are around naming convention violations in the core framework and excessive use of global state in some packages. Addressing the priority recommendations would significantly improve API consistency and maintainability.

# Exported API Audit Report This audit reviewed the exported API surface across 14 key packages in the core/go repository. ## Executive Summary The codebase demonstrates strong documentation practices overall, with most exported symbols having clear doc comments. However, several areas require attention regarding naming conventions, missing documentation, and potential API design improvements. --- ## 1. Framework Core (pkg/framework/core/) **Package Purpose:** Central dependency injection framework managing services, lifecycle, and inter-service communication. **Missing Documentation:** - ✅ None - excellent documentation coverage **Naming Convention Issues:** - ⚠️ `ACTION`, `QUERY`, `QUERYALL`, `PERFORM` - All-caps method names deviate from Go conventions. Should be `Action`, `Query`, `QueryAll`, `Perform` - These violate Go's MixedCaps convention **Symbols That Should Be Unexported:** - ✅ None identified - API surface is appropriate **Missing Error Handling:** - ✅ None identified - proper error returns throughout **Other API Design Concerns:** - The all-caps method names create inconsistency with standard Go practice - `SetInstance` and `GetInstance` provide global mutable state which could be problematic in testing/concurrent scenarios - Consider deprecating global instance pattern in favor of explicit dependency passing --- ## 2. MCP Server (pkg/mcp/) **Package Purpose:** Model Context Protocol server providing tools for file operations, RAG, metrics, process management, and browser automation. **Missing Documentation:** - `Option` type (line 35) - `WithSubsystem` function parameters could use more detail about subsystem lifecycle - `webviewInstance` global variable (tools_webview.go:16) **Symbols That Should Be Unexported:** - `webviewInstance` (tools_webview.go:16) - Global mutable state, should be unexported - `detectLanguageFromPath` (mcp.go:440) - `parseDuration` (tools_metrics.go:181) - `convertMetricCounts` (tools_metrics.go:161) **Missing Error Handling:** - `registerProcessTools`, `registerWSTools`, etc. return `bool` but don't expose why features are unavailable - Tool handler methods silently discard first return value from `CallToolRequest` **Other API Design Concerns:** - Heavy use of global state (`webviewInstance`) creates concurrency issues - No clear API to check which subsystems are available before calling `Run()` - Input/Output types for each tool are well-documented ✅ --- ## 3. WebSocket Hub (pkg/ws/) **Package Purpose:** Real-time WebSocket communication hub with channel-based subscriptions. **Missing Documentation:** - ✅ None - excellent package-level and symbol-level documentation **Symbols That Should Be Unexported:** - `mustMarshal` (line 444) - `Client` struct fields (hub, conn, send, subscriptions, mu) - should all be unexported **Missing Error Handling:** - `mustMarshal` ignores errors which could lead to silent failures - `Broadcast` error return behavior not documented when internal channel blocks **Other API Design Concerns:** - ✅ Excellent API design overall - ⚠️ `CheckOrigin` currently accepts all origins - documented but potentially dangerous default --- ## 4. Browser Automation (pkg/webview/) **Package Purpose:** Chrome DevTools Protocol client for browser automation. **Missing Documentation:** - `Action` interface (actions.go:10) - `ConsoleWatcher` struct fields - `ExceptionWatcher` struct fields - `AngularHelper` struct fields **Symbols That Should Be Unexported:** - `cdpMessage`, `cdpResponse`, `cdpEvent`, `cdpError`, `targetInfo` (cdp.go) - Action types (`ClickAction`, `TypeAction`, etc.) could be unexported with factory functions - Helper functions in angular.go: `getString`, `formatJSValue`, `containsString`, `findString` **Missing Error Handling:** - `Evaluate` method allows arbitrary JavaScript execution - security implications noted but could be clearer - Many Action types ignore potential nil pointer issues **Other API Design Concerns:** - ✅ Excellent documentation in webview.go package comment - ✅ `ActionSequence` provides nice fluent API - Consider adding rate limiting to CDP calls --- ## 5. Process Management (pkg/process/) **Package Purpose:** External process execution with output capture and lifecycle management. **Missing Documentation:** - `Process` struct fields need internal doc comments for maintainability - `RingBuffer` type and methods need documentation **Missing Error Handling:** - `Wait()` returns generic `exec.ExitError` losing specific exit code information - `Signal` method has incomplete implementation (noted in comments) **Other API Design Concerns:** - ✅ Good use of context for cancellation - ✅ `RingBuffer` for output capture is well-designed - Consider adding process groups for batch management --- ## 6. CLI Framework (pkg/cli/) **Package Purpose:** CLI application framework with commands, layouts, and ANSI styling. **Missing Documentation:** - `AppVersion`, `BuildCommit`, `BuildDate`, `BuildPreRelease` (app.go:27-30) - `completionCmd` (app.go:97) - Various glyph and style types **Symbols That Should Be Unexported:** - `completionCmd` (app.go:97) **Missing Error Handling:** - `Main()` exits with code 1 for all errors - may want more granular exit codes **Other API Design Concerns:** - ✅ Good separation of ANSI, layout, and command concerns - Heavy use of global state (RootCmd, etc.) makes testing difficult - Consider dependency injection pattern --- ## 7. Configuration (pkg/config/) **Package Purpose:** Layered configuration management with file, environment, and flag support. **Missing Documentation:** - ✅ None - excellent documentation **Symbols That Should Be Unexported:** - `Load` function (line 175) is deprecated - should be unexported **Other API Design Concerns:** - ✅ Excellent use of functional options pattern - ✅ Thread-safe with proper mutex usage - ✅ Deprecation is well-documented --- ## 8. Logging (pkg/log/) **Package Purpose:** Structured logging with levels, rotation, and style support. **Missing Documentation:** - `identity` function (log.go:130) **Symbols That Should Be Unexported:** - `identity` function (log.go:130) **Other API Design Concerns:** - ✅ Good level-based filtering - ✅ Rotation support is excellent - Consider adding structured context fields beyond key-value pairs --- ## 9. Cryptography (pkg/crypt/) **Package Purpose:** Encryption, hashing, HMAC, and key derivation functions. **Missing Documentation:** - Package documentation could expand on algorithm choices and security properties - Several constants lack doc comments explaining cryptographic significance **Other API Design Concerns:** - ✅ Clean API separating different cryptographic primitives - ✅ Good use of modern algorithms (ChaCha20-Poly1305, Argon2) - `EncryptAES` and `Encrypt` have identical APIs - consider consolidating - Consider adding version prefix to ciphertext for algorithm agility --- ## 10. Build System (pkg/build/) **Package Purpose:** Project detection and cross-compilation for multiple project types. **Missing Documentation:** - `Builder` interface methods could use more detailed doc comments - `Config` struct fields need individual doc comments **Other API Design Concerns:** - ✅ Good abstraction with `Builder` interface - Consider adding validation methods for Config --- ## 11. AI/LLM Client (pkg/agentic/) **Package Purpose:** API client for core-agentic service providing task management. **Missing Documentation:** - `NewClient` and `NewClientFromConfig` could document authentication requirements **Other API Design Concerns:** - ✅ Clean REST client design - ✅ Good use of context throughout - HTTP timeout is hardcoded (30s) - could be configurable - Consider adding retry logic for transient failures --- ## 12. Agent CI / Clotho (pkg/agentci/) **Package Purpose:** Dual-run verification orchestrator for AI agent CI/CD. **Missing Documentation:** - `RunMode` type and constants could use more explanation - `Spinner` struct fields need doc comments - `Weave` method is placeholder but public - should document future plans **Missing Error Handling:** - `Weave` currently just compares strings - weak implementation for public API - No validation of `ClothoConfig` or `AgentConfig` inputs **Other API Design Concerns:** - ✅ Interesting dual-run verification concept - Hard-coded repo criticality logic should be configurable - Consider adding middleware/hooks for custom verification --- ## 13. Forgejo Client (pkg/forge/) **Package Purpose:** Thin wrapper around Forgejo SDK for repository and issue management. **Missing Documentation:** - ✅ Package doc is excellent - Individual methods would benefit from examples **Other API Design Concerns:** - ✅ Very thin wrapper which is appropriate - ✅ Config resolution priority is well-documented - Thread safety of client not documented --- ## 14. Git Operations (pkg/git/) **Package Purpose:** Multi-repository git operations with status checking and sync. **Missing Documentation:** - `StatusOptions` could document behavior when Names map is incomplete **Symbols That Should Be Unexported:** - `gitCommand` (line 226) - `gitInteractive` (line 166) - `getStatus` (line 72) - `getAheadBehind` (line 126) **Other API Design Concerns:** - ✅ Parallel status checking is excellent for performance - ✅ Good use of context throughout - No support for authentication configuration (relies on git config) --- ## Priority Recommendations ### 1. Critical - Naming Conventions - Rename `ACTION`, `QUERY`, `QUERYALL`, `PERFORM` in core package to `Action`, `Query`, `QueryAll`, `Perform` ### 2. High - Global State - Remove global `webviewInstance` in pkg/mcp/tools_webview.go - Review all global mutable state for thread-safety issues ### 3. High - Unexported Symbols - Unexport internal helpers across all packages (35+ symbols identified) - Unexport `completionCmd` in pkg/cli ### 4. Medium - Missing Documentation - Add doc comments to pkg/cli exported variables - Document Action interface and related types in pkg/webview - Expand pkg/crypt documentation with security guidance ### 5. Medium - API Design - Add subsystem availability checking to pkg/mcp - Consider deprecating global instance pattern in core framework - Improve error messages when optional features are unavailable ### 6. Low - Future Enhancements - Add validation methods to build.Config - Implement full dual-run comparison in agentci.Weave - Add retry logic to agentic.Client --- ## Statistics - **Packages Audited:** 14 - **Files Reviewed:** 50+ - **Critical Issues:** 1 (naming conventions in core) - **High Priority Issues:** 3 (global state, unexported symbols) - **Medium Priority Issues:** 6 (documentation, API design) - **Low Priority Issues:** 3 (enhancements) ## Conclusion The core/go repository demonstrates strong engineering practices with comprehensive documentation, good error handling, and thoughtful API design. The primary concerns are around naming convention violations in the core framework and excessive use of global state in some packages. Addressing the priority recommendations would significantly improve API consistency and maintainability.