MAESTRO: Complete refactoring audit Task 17 - git.ts

Audited src/main/ipc/handlers/git.ts covering: - Dead code: None found - all 17 handlers actively used - Deprecated patterns: Runtime require() should be ES import - Duplication: WorktreeConfig interface in 2 locations - Code smells: worktreeSetup handler 107 lines, complex log parsing - Type safety: preload.ts type mismatches (status/diff return types) Key bug discovered: git:status and git:diff preload types declare Promise<string> but handlers return { stdout, stderr }
2026-03-10 08:31:19 +00:00 · 2025-12-19 16:10:59 -06:00
parent 6b35fb41c7
commit c46fefba2f
2 changed files with 226 additions and 4 deletions
--- a/Docs/Refactor-20251219.md
+++ b/Docs/Refactor-20251219.md
@@ -271,7 +271,7 @@
  - Excellent type safety - no `any` types
  - Recommended 7 refactoring actions prioritized by impact

- [ ] **Task 14:** `src/main/ipc/handlers/agents.ts`
+- [x] **Task 14:** `src/main/ipc/handlers/agents.ts`
  Agent management IPC handlers. Examine for:
  - Dead: Unused agent management handlers
  - Deprecated: Legacy agent API patterns
@@ -279,7 +279,16 @@
  - Smells: Agent state management issues
  - Types: Agent type definitions

- [ ] **Task 15:** `src/main/ipc/handlers/autorun.ts`
+  **Completed:** See `Auto Run Docs/Refactor/14.md` for full audit. Key findings:
+  - File is well-structured (382 lines) with all 16 handlers actively used
+  - `AgentConfigsData` interface duplicated in 6 locations across codebase
+  - Config access pattern `agentConfigsStore.get('configs', {})` repeated 13 times
+  - 3 `any` types in `stripAgentFunctions` that need proper typing
+  - `AgentConfig` interface duplicated in 8+ locations (see also Task 4)
+  - No dead code or deprecated patterns found
+  - Recommended 7 refactoring actions prioritized by impact
+
+- [x] **Task 15:** `src/main/ipc/handlers/autorun.ts`
  Auto Run IPC handlers. Examine for:
  - Dead: Unused Auto Run handlers
  - Deprecated: Old file handling patterns
@@ -287,7 +296,18 @@
  - Smells: Missing error handling, path validation
  - Types: File/document type safety

- [ ] **Task 16:** `src/main/ipc/handlers/claude.ts`
+  **Completed:** See `Auto Run Docs/Refactor/15.md` for full audit. Key findings:
+  - File is well-structured (499 lines) with good test coverage and proper path validation security
+  - Debug `console.log` and `[DEBUG]` logging left in production code (lines 171-175)
+  - `TreeNode` interface duplicates `src/shared/treeUtils.ts`, should import from shared
+  - Image extension arrays duplicated twice within file and 2+ other locations across codebase
+  - Path validation logic duplicated inline in `saveImage`/`deleteImage` despite helper existing
+  - Unused `mainWindow` parameter in function signature (only `getMainWindow` used)
+  - Single global debounce timer could cause interference between watched folders
+  - Excellent type safety - no `any` types found
+  - Recommended 7 refactoring actions prioritized by impact
+
+- [x] **Task 16:** `src/main/ipc/handlers/claude.ts`
  Claude-specific handlers. Examine for:
  - Dead: Claude handlers that should use generic agent API
  - Deprecated: Claude-specific patterns that should be generic
@@ -295,7 +315,18 @@
  - Smells: Tight coupling to Claude implementation
  - Types: Claude-specific type leakage

- [ ] **Task 17:** `src/main/ipc/handlers/git.ts`
+  **Completed:** See `Auto Run Docs/Refactor/16.md` for full audit. Key findings:
+  - Entire file (1,566 lines) is deprecated legacy code - should migrate to `agentSessions:*` API
+  - Cost calculation formula duplicated 4 times within file (16 occurrences of `1_000_000` magic number)
+  - Session content parsing logic duplicated 4+ times across codebase
+  - `ClaudeSessionOrigin` types duplicated in 4 separate files
+  - 30 empty/silent catch blocks that mask errors
+  - `claude:getGlobalStats` handler is ~200 lines God method
+  - 6 uses of `unknown` type that should have proper typing
+  - Still actively used by 10+ renderer call sites (migration required)
+  - Recommended 7 refactoring actions prioritized by impact
+
+- [x] **Task 17:** `src/main/ipc/handlers/git.ts`
  Git IPC handlers. Examine for:
  - Dead: Unused git operations
  - Deprecated: Old git command patterns
@@ -303,6 +334,16 @@
  - Smells: Shell injection risks, error handling
  - Types: Git result typing

+  **Completed:** See `Auto Run Docs/Refactor/17.md` for full audit. Key findings:
+  - File is well-structured (579 lines) with good delegation to shared/gitUtils.ts
+  - No dead code - all 17 handlers are exposed via preload.ts and actively used
+  - Runtime `require('child_process')` should be ES import at file top
+  - `WorktreeConfig` interface duplicated in shared/types.ts and renderer/types/index.ts
+  - **Bug: preload.ts type mismatches** - `git:status` and `git:diff` declare `Promise<string>` but handlers return `{ stdout, stderr }`
+  - Log entry result missing `additions`/`deletions` fields in preload.ts type
+  - `git:worktreeSetup` handler is 107 lines - should be extracted to service class
+  - Recommended 7 refactoring actions prioritized by impact
+
 - [ ] **Task 18:** `src/main/ipc/handlers/groupChat.ts`
  Group chat handlers. Examine for:
  - Dead: Unused group chat operations
--- a/Docs/Refactor/17.md
+++ b/Docs/Refactor/17.md
@@ -0,0 +1,181 @@
+# Refactor Task 17: `src/main/ipc/handlers/git.ts`
+
+## Summary
+
+The Git IPC handlers file is well-structured (579 lines) with proper use of shared utilities and helper abstractions. The file demonstrates good separation of concerns by delegating parsing logic to `src/shared/gitUtils.ts`. However, there are some notable issues: a runtime `require()` for `child_process`, return type inconsistencies in the preload API, worktree result type duplication, and complex handler logic in worktree operations. No dead code was found - all handlers are exposed and actively used.
+
+## Dead Code
+
+None found. All 17 git IPC handlers are:
+1. Registered in `registerGitHandlers()`
+2. Exposed via `preload.ts` (lines 284-345)
+3. Called from renderer via `window.maestro.git.*`
+
+All handlers:
+- `git:status`, `git:diff`, `git:isRepo`, `git:numstat` - Basic operations, used by `gitService.ts`
+- `git:branch`, `git:branches`, `git:tags`, `git:remote` - Branch/tag operations, used for autocompletion
+- `git:info`, `git:log`, `git:commitCount`, `git:show`, `git:showFile` - Advanced queries, used by GitLogViewer/MainPanel
+- `git:worktreeInfo`, `git:getRepoRoot`, `git:worktreeSetup`, `git:worktreeCheckout` - Worktree operations, used by useBatchProcessor
+- `git:createPR`, `git:checkGhCli`, `git:getDefaultBranch` - GitHub CLI integration
+
+## Deprecated Patterns
+
+1. **Runtime `require()` for child_process** (line 236-237)
+   ```typescript
+   const { spawnSync } = require('child_process');
+   ```
+   - Should use ES module import at file top
+   - The reason for runtime require is unclear (possibly circular dependency or lazy loading)
+   - Creates TypeScript type inference issues
+
+2. **Inconsistent handler wrapper usage**
+   - Some handlers use `withIpcErrorLogging()` while others use `createIpcHandler()`
+   - Both do similar things but `createIpcHandler` adds more structure
+   - Handlers: `worktreeInfo`, `getRepoRoot`, `getDefaultBranch` use `createIpcHandler`
+   - All others use `withIpcErrorLogging`
+
+## Duplication
+
+1. **`WorktreeConfig` interface duplicated** (2 locations)
+   - `src/shared/types.ts:94` - Primary definition
+   - `src/renderer/types/index.ts:114` - Duplicate definition
+
+2. **Worktree result types defined inline in preload.ts**
+   - `worktreeInfo`, `worktreeSetup`, `worktreeCheckout`, `getRepoRoot`, `getDefaultBranch` each have inline return types in preload.ts (lines 1138-1177)
+   - These should be extracted to shared types and reused
+
+3. **Git existence check pattern repeated** (3 times in git.ts)
+   ```typescript
+   const result = await execFileNoThrow('git', [...], cwd);
+   if (result.exitCode !== 0) { ... }
+   ```
+   - Lines 93-95 (branches), 107-109 (tags), 163-167 (log)
+   - Could be abstracted to a helper
+
+4. **Path existence check pattern** in worktree handlers
+   ```typescript
+   try { await fs.access(path); } catch { return { exists: false, ... }; }
+   ```
+   - Lines 267-272 (worktreeInfo), 359-365 (worktreeSetup)
+
+## Code Smells
+
+1. **`git:worktreeSetup` handler is 107 lines** (lines 337-443)
+   - Complex logic for:
+     - Path resolution and validation
+     - Nested worktree detection
+     - Existing path handling
+     - Branch existence checking
+     - Worktree creation with or without new branch
+   - Should be extracted to a dedicated worktree service class
+
+2. **`git:log` handler has complex inline parsing** (lines 143-201)
+   - 58 lines with inline parsing logic
+   - Parsing logic could be moved to `src/shared/gitUtils.ts` alongside other parsers
+
+3. **Magic strings for git commands scattered throughout**
+   - `--porcelain`, `--abbrev-ref`, `rev-parse`, etc.
+   - While standard git commands, key patterns like commit format strings should be constants:
+     ```typescript
+     '--pretty=format:COMMIT_START%H|%an|%ad|%D|%s'  // line 153
+     ```
+
+4. **Missing proper error handling in git:showFile** (lines 226-259)
+   - Binary file handling uses `spawnSync` which can throw synchronously
+   - No try/catch around the spawnSync call (line 237)
+
+5. **Hard-coded buffer size** (line 240)
+   ```typescript
+   maxBuffer: 50 * 1024 * 1024 // 50MB max
+   ```
+   - Should be a named constant
+
+## Type Safety Issues
+
+1. **Implicit `any` from require()** (line 236)
+   ```typescript
+   const { spawnSync } = require('child_process');
+   ```
+   - TypeScript cannot properly type this at runtime
+   - Should use: `import { spawnSync } from 'child_process';`
+
+2. **Return type mismatch between handler and preload.ts**
+   - `git:status` handler returns `{ stdout, stderr }` (line 41)
+   - But `preload.ts:1111` declares: `status: (cwd: string) => Promise<string>`
+   - This is a bug - the type should be `Promise<{ stdout: string; stderr: string }>`
+
+3. **Similar mismatch for git:diff**
+   - Handler returns `{ stdout, stderr }` (line 51)
+   - Preload declares `Promise<string>` (line 1112)
+
+4. **Complex inline types for worktree results**
+   - `git:worktreeInfo` returns 5 optional properties (lines 315-320)
+   - `git:worktreeSetup` returns 6 optional properties (lines 436-442)
+   - These should be named interfaces in shared types
+
+5. **Missing type for log entry options** (line 145)
+   ```typescript
+   options?: { limit?: number; search?: string }
+   ```
+   - Should be a named interface `GitLogOptions`
+
+6. **Log entry result missing `additions` and `deletions`** in preload.ts
+   - Handler returns `additions` and `deletions` (lines 194-195)
+   - Preload type (lines 1125-1132) does not include these fields
+
+## Recommended Refactoring
+
+1. **High Priority - Fix preload.ts type mismatches**
+   - Update `git:status` return type from `Promise<string>` to `Promise<{ stdout: string; stderr: string }>`
+   - Update `git:diff` return type from `Promise<string>` to `Promise<{ stdout: string; stderr: string }>`
+   - Add `additions` and `deletions` to log entry type
+   - *Impact: Prevents runtime type errors and improves developer experience*
+
+2. **High Priority - Convert require() to ES import**
+   - Move `import { spawnSync } from 'child_process';` to file top
+   - Removes implicit `any` and improves type safety
+   - *Impact: Better type inference, cleaner code*
+
+3. **Medium Priority - Extract worktree logic to service class**
+   - Create `src/main/services/GitWorktreeService.ts`
+   - Move complex worktree logic from handlers
+   - Handlers become thin wrappers calling service methods
+   - *Impact: Improved testability, reduced handler complexity*
+
+4. **Medium Priority - Extract log parsing to shared utility**
+   - Add `parseGitLog()` function to `src/shared/gitUtils.ts`
+   - Consistent with existing pattern (parseGitBranches, parseGitTags, etc.)
+   - *Impact: Better separation of concerns, reusable parsing*
+
+5. **Medium Priority - Define shared worktree result types**
+   - Add to `src/shared/types.ts`:
+     - `WorktreeInfoResult`
+     - `WorktreeSetupResult`
+     - `WorktreeCheckoutResult`
+   - Use in both handler and preload.ts
+   - *Impact: Type consistency, reduced duplication*
+
+6. **Low Priority - Standardize handler wrapper usage**
+   - Choose between `withIpcErrorLogging` and `createIpcHandler` consistently
+   - Or document when to use each
+   - *Impact: Code consistency, easier maintenance*
+
+7. **Low Priority - Extract magic strings to constants**
+   - Create `GIT_COMMANDS` or similar constant object
+   - Extract buffer size to named constant
+   - *Impact: Easier maintenance, clearer intent*
+
+## Estimated Impact
+
+**Risk Level: Low-Medium**
+
+The git handlers are critical for session state and UI updates, but:
+- Changes are mostly type-level and shouldn't affect runtime behavior
+- Worktree operations are used for Auto Run parallelization, requires careful testing
+- Return type fixes in preload.ts may require updates in renderer code (GitStatusContext, etc.)
+
+**Testing Considerations:**
+- Integration tests needed for worktree operations
+- Verify git status/diff return type changes don't break renderer consumers
+- Test git log parsing with various commit formats
+- Test binary file handling (showFile) with large images