Problem
The cli package has inconsistent constructor conventions for non-command types. NewFileTracker returns (*FileTracker, error) because it validates external state at construction time (git root must exist). However, other non-cobra constructors do not follow this pattern:
| Constructor |
Return type |
Does external I/O? |
NewFileTracker() |
(*FileTracker, error) |
Yes — runs gitutil.FindGitRoot() |
NewMCPRegistryClient(url) |
*MCPRegistryClient |
Creates http.Client (no error possible) |
NewDependencyGraph(dir) |
*DependencyGraph |
Stores path, no validation |
NewToolGraph() |
*ToolGraph |
Pure memory allocation |
NewCopilotCodingAgentDetector(...) |
*CopilotCodingAgentDetector |
Stores path, no validation |
The root cause: NewFileTracker eagerly validates its precondition (requires a git repo) while all others defer validation to method calls. This creates a surprising inconsistency — callers must handle an error from NewFileTracker but never from similar constructors.
This extends a pattern previously identified in agentdrain (Run 2, 2026-05-07), where all 4 constructors consistently return errors — unlike cli which is mixed.
Location
pkg/cli/file_tracker.go:27 — NewFileTrackerpkg/cli/mcp_registry.go:39 — NewMCPRegistryClientpkg/cli/dependency_graph.go:32 — NewDependencyGraphpkg/cli/tool_graph.go:30 — NewToolGraph
Impact
- Severity: Medium
- Affected code:
cli package constructor API
- Risk: Callers are uncertain when to expect errors from constructors. A future constructor that validates may be written without an error return, masking failures until method calls.
Recommendation
Pick one of two consistent options and apply it across the cli non-command constructors:
Option A (Lazy validation — preferred for CLI):
Remove eager validation from NewFileTracker. Move git root lookup to the first method that actually needs it (e.g., TrackCreated or Commit). All non-command constructors then return only *T.
// Before
func NewFileTracker() (*FileTracker, error) {
gitRoot, err := gitutil.FindGitRoot()
if err != nil { return nil, err }
return &FileTracker{OriginalContent: make(map[string][]byte), gitRoot: gitRoot}, nil
}
// After
func NewFileTracker() *FileTracker {
return &FileTracker{OriginalContent: make(map[string][]byte)}
}
// gitRoot resolved lazily in the method that needs itOption B (Eager validation):
Add validation to other constructors that have meaningful preconditions (e.g., validate workflowsDir exists in NewDependencyGraph).
Option A is recommended since CLI commands often construct objects before confirming they're in a git repo, and lazy errors produce clearer messages at use-time.
Validation
Estimated Effort: Small
Generated by Sergo (Run 3, 2026-05-08) — Run ID: §25537174291
Generated by Sergo - Serena Go Expert · ● 506.4K · ◷
Problem
The
clipackage has inconsistent constructor conventions for non-command types.NewFileTrackerreturns(*FileTracker, error)because it validates external state at construction time (git root must exist). However, other non-cobra constructors do not follow this pattern:NewFileTracker()(*FileTracker, error)gitutil.FindGitRoot()NewMCPRegistryClient(url)*MCPRegistryClienthttp.Client(no error possible)NewDependencyGraph(dir)*DependencyGraphNewToolGraph()*ToolGraphNewCopilotCodingAgentDetector(...)*CopilotCodingAgentDetectorThe root cause:
NewFileTrackereagerly validates its precondition (requires a git repo) while all others defer validation to method calls. This creates a surprising inconsistency — callers must handle an error fromNewFileTrackerbut never from similar constructors.This extends a pattern previously identified in
agentdrain(Run 2, 2026-05-07), where all 4 constructors consistently return errors — unlikecliwhich is mixed.Location
pkg/cli/file_tracker.go:27—NewFileTrackerpkg/cli/mcp_registry.go:39—NewMCPRegistryClientpkg/cli/dependency_graph.go:32—NewDependencyGraphpkg/cli/tool_graph.go:30—NewToolGraphImpact
clipackage constructor APIRecommendation
Pick one of two consistent options and apply it across the
clinon-command constructors:Option A (Lazy validation — preferred for CLI):
Remove eager validation from
NewFileTracker. Move git root lookup to the first method that actually needs it (e.g.,TrackCreatedorCommit). All non-command constructors then return only*T.Option B (Eager validation):
Add validation to other constructors that have meaningful preconditions (e.g., validate
workflowsDirexists inNewDependencyGraph).Option A is recommended since CLI commands often construct objects before confirming they're in a git repo, and lazy errors produce clearer messages at use-time.
Validation
NewFileTrackerto remove error handling (or handle at method call)gitand non-git contextsEstimated Effort: Small
Generated by Sergo (Run 3, 2026-05-08) — Run ID: §25537174291