refactor: consolidate agents module and reorganize test structure

[reachraza]

Refactor Agent Module Architecture and Test Organization

Summary

This PR restructures the agent-related code into a well-organized module with proper separation of concerns and ensures the test directory mirrors the source structure with comprehensive coverage.

Changes

1. Agent Module Decomposition (e95ef0c3)

• Extracted agent-detector.ts into focused, single-responsibility modules:
  • definitions.ts - Agent configuration definitions and registry
  • capabilities.ts - Agent capability flags and queries
  • path-prober.ts - Binary detection and path probing utilities
  • detector.ts - High-level agent detection orchestration

2. Module Consolidation & Test Reorganization (454cdefd)

• Created src/main/agents/index.ts barrel file for clean imports
• Moved inline tests to centralized test directory:
  • process-listeners/__tests__/ → src/__tests__/main/process-listeners/
  • debug-package/__tests__/ → src/__tests__/main/debug-package/
• Added new test files for previously uncovered modules:
  • claude-session-storage.test.ts (32 tests)
  • CallbackRegistry.test.ts (29 tests)
  • LiveSessionManager.test.ts (39 tests)

Test Coverage

Metric	Before	After	Delta
Total Tests	16,201	16,301	+100

Benefits

• Better discoverability - All agent code in one place with barrel exports
• Cleaner imports - import { AgentDetector, getAgentCapabilities } from './agents'
• Improved testability - Smaller, focused modules are easier to unit test
• Consistent test structure - Tests mirror source directory layout
• Increased coverage - New tests for storage and web-server managers

Testing

npm run test        # 16,301 tests passing
npm run lint        # TypeScript checks pass
npm run lint:eslint # ESLint checks pass

[claude]

Code Review: PR #247 - Refactor Agent Module Architecture

Thanks for this well-executed refactoring! The decomposition of agent-detector.ts into focused modules is a clear improvement. Here's my detailed review:

---

🟢 Overall Assessment: STRONG APPROVAL with minor recommendations

Summary:

• ✅ Clean separation of concerns
• ✅ Maintains backward compatibility
• ✅ Comprehensive test coverage (+100 tests)
• ✅ Follows project conventions
• ✅ Excellent error handling and logging

Critical Issues: None
High Priority: None
Medium Priority: 3 items
Low Priority: 5 items

---

📋 Medium Priority Issues

1. Type Safety for AgentConfigOption.default (definitions.ts:20)

Issue: Using any defeats TypeScript's type safety:

default: any; // Current implementation

Recommendation: Use discriminated union:

export type AgentConfigOption =
  | { type: 'checkbox'; key: string; label: string; description: string; default: boolean; argBuilder?: (value: boolean) => string[] }
  | { type: 'text'; key: string; label: string; description: string; default: string; argBuilder?: (value: string) => string[] }
  | { type: 'number'; key: string; label: string; description: string; default: number; argBuilder?: (value: number) => string[] }
  | { type: 'select'; key: string; label: string; description: string; default: string; options: string[]; argBuilder?: (value: string) => string[] };

This provides full type inference and eliminates any.

---

2. Error Handling in Model Discovery (detector.ts:227-263)

Issue: runModelDiscovery doesn't catch exceptions from execFileNoThrow, allowing errors to bubble up uncaught.

Recommendation:

private async runModelDiscovery(agentId: string, agent: AgentConfig): Promise<string[]> {
    const env = getExpandedEnv();
    const command = agent.path || agent.command;

    try {
        switch (agentId) {
            case 'opencode': {
                const result = await execFileNoThrow(command, ['models'], undefined, env);
                // ... rest of logic
            }
            // ... other cases
        }
    } catch (error) {
        logger.error(`Model discovery threw exception for ${agentId}`, LOG_CONTEXT, { error });
        return [];
    }
}

---

3. Sequential Path Probing Performance (path-prober.ts:264-278)

Issue: Paths are checked sequentially, which could be slow when checking many locations (20+ paths for some binaries).

Current implementation:

for (const probePath of pathsToCheck) {
    try {
        await fs.promises.access(probePath, fs.constants.F_OK);
        return probePath;
    } catch {
        // Continue to next
    }
}

Recommendation: Check paths in parallel:

export async function probeWindowsPaths(binaryName: string): Promise<string | null> {
    const pathsToCheck = getWindowsKnownPaths(binaryName);

    const results = await Promise.allSettled(
        pathsToCheck.map(async (probePath) => {
            await fs.promises.access(probePath, fs.constants.F_OK);
            return probePath;
        })
    );

    for (const result of results) {
        if (result.status === 'fulfilled') {
            logger.debug(`Direct probe found ${binaryName}`, LOG_CONTEXT, { path: result.value });
            return result.value;
        }
    }

    return null;
}

This could significantly reduce detection time, especially on slower file systems.

---

📝 Low Priority Issues

4. Path Pattern Duplication (path-prober.ts:199-353)

Issue: Many path patterns are repeated across binaries in getWindowsKnownPaths() and getUnixKnownPaths():

• npm global paths
• WinGet paths
• Homebrew paths
• Local bin paths

Recommendation: Extract common patterns:

const commonWindowsPaths = {
    npmGlobal: [path.join(appData, 'npm'), path.join(localAppData, 'npm')],
    wingetLinks: [
        path.join(localAppData, 'Microsoft', 'WinGet', 'Links'),
        path.join(programFiles, 'WinGet', 'Links')
    ],
    // ...
};

This reduces maintenance burden when adding new binaries.

---

5. Hardcoded Model Cache TTL (detector.ts:28)

Issue: 5-minute cache is reasonable but not configurable. For development/testing, different TTLs might be useful.

Recommendation: Make configurable via constructor:

constructor(private readonly modelCacheTtlMs: number = 5 * 60 * 1000) {}

---

6. Silent Error Swallowing (path-prober.ts:189-191)

Issue: The outer catch block swallows all errors silently:

} catch {
    return { exists: false };
}

Recommendation: Log for debugging:

} catch (error) {
    logger.debug(`Error checking custom path ${customPath}`, LOG_CONTEXT, { error });
    return { exists: false };
}

---

7. Missing Test: Model Cache Expiration (detector.test.ts)

Issue: No test verifies that cache is invalidated after TTL expires.

Recommendation: Add test with fake timers:

it('should invalidate model cache after TTL expires', async () => {
    vi.useFakeTimers();
    
    await detector.discoverModels('opencode');
    
    vi.advanceTimersByTime(6 * 60 * 1000); // 6 minutes
    
    mockExecFileNoThrow.mockClear();
    await detector.discoverModels('opencode');
    
    expect(mockExecFileNoThrow).toHaveBeenCalledWith(/* ... */);
    
    vi.useRealTimers();
});

---

8. Documentation Enhancements

Model Discovery Section (detector.ts:9):

/**
 * Agent Detection and Configuration Manager
 * 
 * Responsibilities:
 * - Detects installed agents via file system probing and PATH resolution
 * - Manages agent configuration and capability metadata
 * - Caches detection results for performance
 * - Discovers available models for agents that support model selection
 * 
 * Model Discovery:
 * - Model lists are cached for 5 minutes to balance freshness and performance
 * - Each agent implements its own model discovery command
 * - Cache can be manually cleared or bypassed with forceRefresh flag
 */

Path Probing Strategy (path-prober.ts:7):

/**
 * Binary Path Detection Utilities
 * 
 * Packaged Electron apps don't inherit shell environment, so we need to
 * probe known installation paths directly.
 * 
 * Detection Strategy:
 * 1. Direct file system probing of known installation paths (fastest, most reliable)
 * 2. Fall back to which/where command with expanded PATH
 * 
 * This two-tier approach ensures we find binaries even when:
 * - PATH is not inherited correctly
 * - Binaries are in non-standard locations
 * - Shell initialization files (.bashrc, .zshrc) aren't sourced
 */

---

🎯 What's Great About This PR

1. Test Coverage Excellence - 100 new tests with comprehensive edge case coverage
2. Clean Module Boundaries - Each module has clear, single responsibility
3. Backward Compatible - No breaking changes, clean migration path via barrel exports
4. Type Safety - Good use of TypeScript features throughout (except the one any noted above)
5. Consistent Logging - Appropriate log levels with context
6. Error Resilience - Graceful degradation when detection fails

---

🚀 Recommendation

Approve and merge with follow-up issues for the suggested improvements. The code is production-ready as-is—all issues identified are enhancements rather than blockers. The refactoring successfully achieves its goals of:

• ✅ Better discoverability and maintainability
• ✅ Improved testability with focused modules
• ✅ Consistent test structure mirroring source layout
• ✅ Increased test coverage

Consider creating follow-up issues for the type safety improvement (#1) and parallel path probing (#3) as they provide the most value.

Great work on this refactoring! 🎉

[claude]

Code Review: PR #247 - Agent Module Refactoring

Executive Summary

Verdict: ✅ APPROVE - Excellent refactoring with high-quality implementation

This PR successfully decomposes the monolithic agent-detector.ts (865 lines) into a well-organized module structure with clear separation of concerns. The refactoring maintains full backward compatibility, adds 100 comprehensive tests, and demonstrates excellent software engineering practices.

---

Strengths

1. Architecture & Design (⭐⭐⭐⭐⭐)

Clean Module Boundaries:

src/main/agents/
├── index.ts           # Barrel exports (clean API surface)
├── detector.ts        # Agent detection orchestration
├── definitions.ts     # Static agent configurations
├── capabilities.ts    # Feature capability matrix
├── path-prober.ts    # Platform-specific binary detection
└── session-storage.ts # Storage interface registry

Each module has a single, focused responsibility with zero circular dependencies.

Type Safety:

• Discriminated unions for AgentConfigOption provide full type inference
• Proper type exports via barrel pattern
• stripAgentFunctions() safely handles IPC serialization

Design Patterns:

• Registry pattern for session storage (extensible, type-safe)
• Promise deduplication prevents parallel detection races
• Cache-first approach with TTL-based invalidation
• Conservative defaults with PLACEHOLDER markers for future agents

2. Test Coverage (⭐⭐⭐⭐⭐)

+100 tests across 5 comprehensive suites:

Test Suite	Lines	Coverage
detector.test.ts	43KB	Detection lifecycle, model discovery, caching, platform-specific
definitions.test.ts	8.5KB	All 7 agents, configuration options, helper functions
capabilities.test.ts	11KB	Feature matrix, capability queries, per-agent verification
path-prober.test.ts	14KB	Windows/Unix paths, extensions, executability, fallbacks
session-storage.test.ts	15KB	Registry operations, interface compliance

Test Quality Highlights:

• ✅ Cross-platform testing (Windows, macOS, Linux)
• ✅ Error scenarios and graceful degradation
• ✅ Mock implementations validate interface contracts
• ✅ Proper mocking with vi.spyOn and cleanup

3. Code Quality (⭐⭐⭐⭐⭐)

Promise Deduplication (detector.ts:65-77):

if (this.detectionInProgress) return this.detectionInProgress;
this.detectionInProgress = this.doDetectAgents();
try {
  return await this.detectionInProgress;
} finally {
  this.detectionInProgress = null;
}

Prevents race conditions during parallel detection calls.

Capability Feature Matrix (capabilities.ts):

• 19 capability flags with clear semantics
• Per-agent verification comments
• Conservative defaults for unknown agents

Path Probing Strategy (path-prober.ts):

• Direct file system probing (fastest)
• Fallback to which/where commands
• Proper tilde expansion and extension handling (.exe, .cmd on Windows)
• Executability checks on Unix platforms

4. Migration Completeness (⭐⭐⭐⭐⭐)

All 29 files updated with correct imports:

// Old (scattered imports)
import { AgentDetector } from './agent-detector';
import { getAgentCapabilities } from './agent-capabilities';

// New (barrel export)
import { AgentDetector, getAgentCapabilities } from './agents';

✅ No stale imports detected

• All IPC handlers updated
• All storage layer files updated
• All tests updated
• All utilities updated

5. Documentation (⭐⭐⭐⭐)

• Inline docstrings for all modules
• PLACEHOLDER comments for future implementation
• Verification comments in capabilities.ts
• Clear section comments in barrel exports

---

Areas for Enhancement

1. Parallel Path Probing (Performance Optimization)

Current: Sequential path checking in probeWindowsPaths() and probeUnixPaths()

Recommendation: Use Promise.allSettled() for parallel filesystem access:

export async function probeWindowsPaths(binaryName: string): Promise<string | null> {
  const pathsToCheck = getWindowsKnownPaths(binaryName);
  
  const results = await Promise.allSettled(
    pathsToCheck.map(async (probePath) => {
      await fs.promises.access(probePath, fs.constants.F_OK);
      return probePath;
    })
  );
  
  const found = results.find(r => r.status === 'fulfilled');
  return found?.status === 'fulfilled' ? found.value : null;
}

Impact: Could reduce detection time by 50-80% when checking 20+ paths on slow filesystems.

---

2. Enhanced Error Handling in Model Discovery

Current: runModelDiscovery() doesn't catch exceptions from execFileNoThrow

File: src/main/agents/detector.ts:227-263

Recommendation: Wrap discovery logic in try-catch:

private async runModelDiscovery(agentId: string, agent: AgentConfig): Promise<string[]> {
  try {
    const env = getExpandedEnv();
    const command = agent.path || agent.command;

    switch (agentId) {
      case 'opencode': {
        const result = await execFileNoThrow(command, ['models'], undefined, env);
        // ... parsing logic
      }
    }
  } catch (error) {
    logger.error(`Model discovery threw exception for ${agentId}`, LOG_CONTEXT, { error });
    return [];
  }
}

Impact: Prevents uncaught promise rejections if execFileNoThrow throws.

---

3. Reduce Path Pattern Duplication

Current: Many path patterns repeated across binaries in getWindowsKnownPaths() and getUnixKnownPaths()

File: src/main/agents/path-prober.ts:199-353

Recommendation: Extract common path builders:

const commonPatterns = {
  npmGlobal: (binary: string) => [
    path.join(appData, 'npm', binary),
    path.join(localAppData, 'npm', binary)
  ],
  homebrewBin: (binary: string) => [
    `/opt/homebrew/bin/${binary}`,
    `/usr/local/bin/${binary}`
  ],
  wingetLinks: (binary: string) => [
    path.join(localAppData, 'Microsoft', 'WinGet', 'Links', binary),
    path.join(programFiles, 'WinGet', 'Links', binary)
  ]
};

Impact: Reduces maintenance burden when adding new agents.

---

4. Test Gap: Model Cache Expiration

Missing: No test verifies cache is invalidated after TTL expires

Recommendation: Add test with fake timers:

it('should invalidate model cache after TTL expires', async () => {
  vi.useFakeTimers();
  
  await detector.discoverModels('opencode');
  vi.advanceTimersByTime(6 * 60 * 1000); // 6 minutes
  
  mockExecFileNoThrow.mockClear();
  await detector.discoverModels('opencode');
  
  expect(mockExecFileNoThrow).toHaveBeenCalled();
  vi.useRealTimers();
});

---

5. Improved Error Logging

Current: Silent error swallowing in checkCustomPath() catch block

File: src/main/agents/path-prober.ts:189-191

Recommendation:

} catch (error) {
  logger.debug(`Error checking custom path ${customPath}`, LOG_CONTEXT, { error });
  return { exists: false };
}

Impact: Aids debugging when custom paths fail validation.

---

Security Assessment

Aspect	Status	Notes
Binary validation	✅ Secure	Checks existence + executability before use
Environment handling	✅ Secure	Safe PATH expansion without injection risks
Custom path validation	✅ Secure	Tilde expansion, extension handling, permissions
Function serialization	✅ Handled	stripAgentFunctions() removes non-serializable properties
Error boundaries	✅ Complete	Graceful degradation with appropriate logging

No security concerns identified.

---

Performance Assessment

Feature	Pattern	Performance
Agent detection	Cached + deduplication	✅ O(1) on cache hits; no parallel races
Model discovery	5-minute TTL cache	✅ Balances freshness and performance
Path probing	Sequential (current)	⚠️ Could be faster with parallel checks
Capability lookup	O(1) object access	✅ Direct dictionary lookup
Session storage	Registry pattern	✅ O(1) lookup with lazy init

No performance regressions. One optimization opportunity identified above.

---

Checklist

• Code quality and best practices - Excellent
• Potential bugs or issues - None blocking (minor enhancements noted)
• Performance considerations - No regressions; one optimization opportunity
• Security concerns - None identified
• Test coverage - Comprehensive (+100 tests)
• Breaking changes - None (full backward compatibility)
• Documentation - Good inline documentation
• Follows CLAUDE.md conventions - Yes (proper module structure, testing patterns)

---

Recommendation

✅ APPROVE AND MERGE

This refactoring is production-ready. All identified issues are enhancements rather than blockers. The code successfully achieves:

• ✅ Better discoverability via barrel exports
• ✅ Improved testability with focused modules
• ✅ Consistent test structure mirroring source layout
• ✅ Increased test coverage (+100 tests)
• ✅ Clean separation of concerns
• ✅ Zero breaking changes

Follow-up Issues (Optional):

1. Implement parallel path probing for performance (Internal Logging #1 priority)
2. Add model cache TTL expiration test
3. Extract common path patterns to reduce duplication

Excellent work! 🎉

---

Reviewed by: Claude Sonnet 4.5 (AI Code Review)
Date: 2026-01-29

[claude]

Code Review: Agent Module Refactoring

Overall Assessment

Verdict: Strong refactoring with excellent architectural improvements ✅

This PR demonstrates thoughtful architectural restructuring with clear separation of concerns. The refactor significantly improves code maintainability, testability, and discoverability.

---

Strengths 💪

1. Excellent Module Organization

• Clean separation of concerns across focused modules
• Barrel export pattern provides clean API surface
• 67% reduction in main detector file complexity (865 to 283 lines)

2. Type Safety Improvements

• Excellent use of discriminated unions for AgentConfigOption
• Eliminates previous any types and provides full type inference

3. Performance Optimizations

• Parallel path probing using Promise.allSettled reduces sequential I/O bottlenecks
• Model cache TTL now configurable via constructor (better testability)

4. Error Handling

• Good defensive programming with try/catch in runModelDiscovery
• Graceful degradation when model discovery fails

5. Test Quality

• 100 new tests added with excellent coverage
• Tests mirror source structure
• Good use of mocking for external dependencies

---

Issues & Recommendations 🔍

Critical Issues

None identified - No blocking issues found.

Medium Priority

1. Remaining any Type in agent-args.ts (Line 102) - Consider using discriminated union for better type inference
2. Type Assertion at agent-args.ts:123 - Consider using type guard instead

Low Priority

3. Model Cache Expiration Test - Add test using fake timers (may already exist per commit 7a93437)
4. Path Prober Error Handling - Distinguish between file not found vs permission denied
5. Windows Extension Handling - Extract into helper function for readability
6. Node Version Manager Path Detection - Cache results in getExpandedEnv()

---

Security Considerations 🔒

No Security Issues Found ✅

1. Path Traversal: Properly sanitized
2. Command Injection: Not applicable - paths validated before use
3. Environment Variables: Properly scoped with explicit precedence
4. Untrusted Input: Custom paths validated with fs.promises.access()

---

Test Coverage 📊

Excellent Coverage

• +100 tests (16,201 to 16,301)
• Tests mirror source structure
• Good mock usage

Suggestions:

1. Add cache expiration test (if not present)
2. Add integration test for full detection flow
3. Consider property-based tests for path validation

---

Architecture & Design 🏗️

Excellent Patterns:

1. Separation of Concerns
2. Dependency Injection
3. Promise Deduplication
4. Caching Strategy
5. Platform Abstraction

Consistency:

• ✅ Follows CLAUDE.md conventions
• ✅ Uses existing logger infrastructure
• ✅ Matches established patterns

---

Performance Impact 📈

Positive Improvements:

1. Parallel path probing: significant on slow filesystems
2. Model cache: reduces redundant CLI invocations
3. No regressions - detection logic unchanged

---

Recommendations Summary

Must Address Before Merge: None - PR is merge-ready

Should Address (Nice to Have):

1. Remove any type in agent-args.ts:102
2. Replace type assertion with type guard
3. Add cache expiration test if not present

Optional Future Improvements:

1. Extract Windows extension resolution helper
2. Cache version manager paths
3. Add explicit error logging for permission issues

---

Conclusion

This is excellent refactoring work demonstrating strong architectural vision, attention to type safety, thoughtful performance optimization, and comprehensive test coverage.

Recommendation: APPROVE ✅

Great work @reachraza! This refactor significantly improves the maintainability and testability of the agent detection system.

---

Testing Checklist

Before merging:

• npm run test passes (16,301 tests)
• npm run lint passes
• npm run lint:eslint passes
• Verify agent detection works for all supported agents
• Verify custom paths work correctly
• Verify model discovery works for OpenCode
• Manual testing on Windows, macOS, and Linux

---

Review conducted by Claude Code in accordance with CLAUDE.md guidelines