Cleanup worker-service.ts: remove dead code and fallback concept (#706)

* refactor(worker): remove dead code from worker-service.ts

Remove ~216 lines of unreachable code:
- Delete `runInteractiveSetup` function (defined but never called)
- Remove unused imports: fs namespace, spawn, homedir, readline,
  existsSync/writeFileSync/readFileSync/mkdirSync
- Clean up CursorHooksInstaller imports (keep only used exports)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix(worker): only enable SDK fallback when Claude is configured

Add isConfigured() method to SDKAgent that checks for ANTHROPIC_API_KEY
or claude CLI availability. Worker now only sets SDK agent as fallback
for third-party providers when credentials exist, preventing cascading
failures for users who intentionally use Gemini/OpenRouter without Claude.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* refactor(worker): remove misleading re-export indirection

Remove unnecessary re-export of updateCursorContextForProject from
worker-service.ts. ResponseProcessor now imports directly from
CursorHooksInstaller.ts where the function is defined. This eliminates
misleading indirection that suggested a circular dependency existed.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* refactor(mcp): use build-time injected version instead of hardcoded strings

Replace hardcoded '1.0.0' version strings with __DEFAULT_PACKAGE_VERSION__
constant that esbuild replaces at build time. This ensures MCP server and
client versions stay synchronized with package.json.

- worker-service.ts: MCP client version now uses packageVersion
- ChromaSync.ts: MCP client version now uses packageVersion
- mcp-server.ts: MCP server version now uses packageVersion
- Added clarifying comments for empty MCP capabilities objects

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* feat: Implement cleanup and validation plans for worker-service.ts

- Added a comprehensive cleanup plan addressing 23 identified issues in worker-service.ts, focusing on safe deletions, low-risk simplifications, and medium-risk improvements.
- Created an execution plan for validating intentional patterns in worker-service.ts, detailing necessary actions and priorities.
- Generated a report on unjustified logic in worker-service.ts, categorizing issues by severity and providing recommendations for immediate and short-term actions.
- Introduced documentation for recent activity in the mem-search plugin, enhancing traceability and context for changes.

* fix(sdk): remove dangerous ANTHROPIC_API_KEY check from isConfigured

Claude Code uses CLI authentication, not direct API calls. Checking for
ANTHROPIC_API_KEY could accidentally use a user's API key (from other
projects) which costs 20x more than Claude Code's pricing.

Now only checks for claude CLI availability.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix(worker): remove fallback agent concept entirely

Users who choose Gemini/OpenRouter want those providers, not secret
fallback behavior. Removed setFallbackAgent calls and the unused
isConfigured() method.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Alex Newman
2026-01-13 23:30:13 -05:00
committed by GitHub
parent c314946204
commit 05323c9db5
18 changed files with 1836 additions and 882 deletions

View File

@@ -0,0 +1,314 @@
# Plan: Cleanup worker-service.ts Unjustified Logic
**Created:** 2026-01-13
**Source:** `docs/reports/nonsense-logic.md`
**Target:** `src/services/worker-service.ts` (813 lines)
**Goal:** Address 23 identified issues, prioritizing safe deletions first
---
## Phase 0: Documentation Discovery (COMPLETED)
### Evidence Gathered
**Exit Code Strategy (CLAUDE.md:44-54):**
```
- Exit 0: Success or graceful shutdown (Windows Terminal closes tabs)
- Exit 1: Non-blocking error
- Exit 2: Blocking error
Philosophy: Exit 0 prevents Windows Terminal tab accumulation
```
**Signal Handler Pattern (ProcessManager.ts:294-317):**
- Uses mutable reference object `isShuttingDownRef`
- Factory function `createSignalHandler()` returns handler with embedded state
- Current implementation has 3-hop indirection
**MCP Client Pattern (worker-service.ts:157-160, ChromaSync.ts:124-136):**
```typescript
this.mcpClient = new Client({
name: 'worker-search-proxy',
version: '1.0.0'
}, { capabilities: {} });
```
**Verification Results:**
- `runInteractiveSetup` (lines 439-639): **NEVER CALLED** - grep shows only definition
- `import * as fs from 'fs'` (line 13): **UNUSED** - no `fs.` usage found
- `import { spawn } from 'child_process'` (line 14): **UNUSED** - no `spawn(` calls
- `homedir` (line 15): Only used in `runInteractiveSetup` (dead code)
- `processPendingQueues` default `= 10`: Never used, all callers pass explicit args
---
## Phase 1: Safe Deletions (Dead Code & Unused Imports)
### 1.1 Delete `runInteractiveSetup` Function
**What:** Delete lines 435-639 (~201 lines)
**Why:** Function is defined but never called. Setup happens via `handleCursorCommand()`.
**Evidence:** `grep -n "runInteractiveSetup" src/services/worker-service.ts` returns only definition
**Pattern to follow:** N/A - straight deletion
**Steps:**
1. Read worker-service.ts lines 435-650
2. Delete the entire function including section comment (lines 435-639)
3. Run `npm run build` to verify no compile errors
**Verification:**
- `grep "runInteractiveSetup" src/` returns nothing
- Build succeeds
### 1.2 Remove Unused Imports
**What:** Delete lines 13, 14, 17
**Current (delete these):**
```typescript
import * as fs from 'fs'; // Line 13 - UNUSED
import { spawn } from 'child_process'; // Line 14 - UNUSED
import * as readline from 'readline'; // Line 17 - Only in dead code
```
**Keep:**
```typescript
import { homedir } from 'os'; // Line 15 - DELETE (only in dead code)
import { existsSync, writeFileSync, readFileSync, mkdirSync } from 'fs'; // Line 16 - CHECK USAGE
```
**Steps:**
1. After deleting `runInteractiveSetup`, grep for remaining usages:
- `grep "homedir" src/services/worker-service.ts`
- `grep "readline" src/services/worker-service.ts`
- `grep "detectClaudeCode\|findCursorHooksDir\|installCursorHooks\|configureCursorMcp" src/services/worker-service.ts`
2. Delete imports with zero usages
3. Run `npm run build`
**Verification:**
- No TypeScript unused import warnings
- Build succeeds
### 1.3 Clean Up Cursor Integration Imports
After deleting `runInteractiveSetup`, some CursorHooksInstaller imports become unused:
- `detectClaudeCode` - only in runInteractiveSetup
- `findCursorHooksDir` - only in runInteractiveSetup
- `installCursorHooks` - only in runInteractiveSetup
- `configureCursorMcp` - only in runInteractiveSetup
**Steps:**
1. Grep each import after dead code removal
2. Remove any that are now unused
3. Keep `updateCursorContextForProject` (re-exported) and `handleCursorCommand` (used in main)
**Verification:**
- `grep "detectClaudeCode\|findCursorHooksDir\|installCursorHooks\|configureCursorMcp" src/services/worker-service.ts` returns nothing
- Build succeeds
---
## Phase 2: Low-Risk Simplifications
### 2.1 Remove Unused Default Parameter
**What:** Line 350 - `async processPendingQueues(sessionLimit: number = 10)`
**Why:** Default never used. All callers pass explicit args (50 in startup, dynamic in HTTP)
**Change from:**
```typescript
async processPendingQueues(sessionLimit: number = 10): Promise<{...}>
```
**Change to:**
```typescript
async processPendingQueues(sessionLimit: number): Promise<{...}>
```
**Verification:**
- Build succeeds
- All call sites provide explicit values
### 2.2 Simplify onRestart Callback
**Location:** Lines 395-396 (approximate, find exact)
**Issue:** `onShutdown` and `onRestart` both call `this.shutdown()`
**Find pattern:**
```typescript
onShutdown: () => this.shutdown(),
onRestart: () => this.shutdown()
```
**Options:**
1. **Keep as-is** if restart semantically differs from shutdown (future-proofing)
2. **Add comment** explaining intentional parity
3. **Remove onRestart** if never used differently
**Investigation needed:** Grep for `onRestart` usage in Server.ts to understand contract
**Steps:**
1. Grep `onRestart` in `src/services/server/`
2. If Server.ts treats them identically, add clarifying comment
3. If different, document why both map to shutdown
### 2.3 Fix Over-Commented Lines (Sample Only)
**Strategy:** Do NOT strip all comments. Only remove comments that describe obvious code.
**Anti-pattern (remove):**
```typescript
// WHAT: Imports centralized logging utility with structured output
// WHY: All worker logs go through this for consistent formatting
import { logger } from '../utils/logger.js';
```
**Pattern to follow:** Remove WHAT/WHY on simple imports. Keep architectural comments.
**Scope:** Sample 5-10 obvious comment removals to demonstrate approach, not exhaustive
---
## Phase 3: Medium-Risk Improvements
### 3.1 Simplify Signal Handler Pattern
**Current (worker-service.ts:180-192 + ProcessManager.ts:294-317):**
```typescript
// 3-hop indirection with mutable reference
const shutdownRef = { value: this.isShuttingDown };
const handler = createSignalHandler(() => this.shutdown(), shutdownRef);
process.on('SIGTERM', () => {
this.isShuttingDown = shutdownRef.value; // Sync back
handler('SIGTERM');
});
```
**Simplified approach:**
```typescript
private registerSignalHandlers(): void {
const handler = async (signal: string) => {
if (this.isShuttingDown) {
logger.warn('SYSTEM', `Received ${signal} but shutdown already in progress`);
return;
}
this.isShuttingDown = true;
logger.info('SYSTEM', `Received ${signal}, shutting down...`);
try {
await this.shutdown();
process.exit(0);
} catch (error) {
logger.error('SYSTEM', 'Error during shutdown', {}, error as Error);
process.exit(0);
}
};
process.on('SIGTERM', () => handler('SIGTERM'));
process.on('SIGINT', () => handler('SIGINT'));
}
```
**Decision needed:** Does `createSignalHandler` serve other callers? If yes, keep factory but simplify worker usage.
**Steps:**
1. Grep `createSignalHandler` usage across codebase
2. If only worker-service uses it, inline and simplify
3. If shared, simplify worker's usage while keeping factory
### 3.2 Unify Dual Initialization Tracking
**Current (lines 111, 129-130):**
```typescript
private initializationCompleteFlag: boolean = false;
private initializationComplete: Promise<void>;
```
**Recommendation:** Keep both but add clarifying comments:
- Promise: For async waiters (HTTP handlers)
- Flag: For sync checks (status endpoints)
**Alternative:** Use Promise with inspection pattern:
```typescript
private initializationComplete = false;
private initializationPromise: Promise<void>;
// Flag derived from promise state via finally() callback
```
**Steps:**
1. Add documentation comment explaining dual tracking purpose
2. Consider if flag can be derived from promise state instead
### 3.3 Reduce 5-Minute Timeout
**Location:** Lines 464-478 (approximate)
**Current:** `const timeoutMs = 300000; // 5 minutes`
**Recommendation:** Reduce to 30-60 seconds for HTTP handler, keep 5min for MCP init
**Caution:** MCP initialization can legitimately be slow (ChromaDB, model loading). May need different timeouts per use case.
**Steps:**
1. Find exact line for context inject timeout
2. Verify this is separate from MCP init timeout
3. Reduce HTTP handler timeout to 30-60 seconds
4. Keep MCP init timeout at 5 minutes
---
## Phase 4: Deferred / Low Priority
These items are noted but NOT part of this cleanup:
| Issue | Reason to Defer |
|-------|-----------------|
| Exit code 0 always | Documented Windows Terminal workaround - intentional |
| Re-export for circular import | Works correctly, architectural fix is separate work |
| Fallback agent verification | Behavioral change, needs feature design |
| MCP version hardcoding | Low impact, separate version management issue |
| Empty capabilities | Documentation issue only |
| Unsafe `as Error` casts | Common TS pattern, low risk |
---
## Phase 5: Verification
### 5.1 Build Verification
```bash
npm run build
```
Expected: No errors
### 5.2 Test Suite
```bash
npm test
```
Expected: All tests pass
### 5.3 Grep for Anti-patterns
```bash
# Verify dead code removed
grep -r "runInteractiveSetup" src/
# Verify unused imports removed
grep "import \* as fs from 'fs'" src/services/worker-service.ts
grep "import { spawn }" src/services/worker-service.ts
```
Expected: No matches
### 5.4 Runtime Check
```bash
npm run build-and-sync
# Start worker and verify basic operation
```
---
## Summary
| Phase | Items | Estimated Reduction |
|-------|-------|---------------------|
| Phase 1 | Dead code + unused imports | ~210 lines |
| Phase 2 | Low-risk simplifications | ~5 lines + clarity |
| Phase 3 | Medium-risk improvements | ~30 lines |
| Total | | ~245 lines (~30% reduction) |
**Execution Order:** Phase 1 → Phase 2 → Phase 3 → Phase 5 (verification after each)

View File

@@ -0,0 +1,356 @@
# Execution Plan: Intentional Patterns Validation Actions
**Created:** 2026-01-13
**Source:** `docs/reports/intentional-patterns-validation.md`
**Target:** `src/services/worker-service.ts` and related files
---
## Phase 0: Documentation Discovery (COMPLETED)
### Evidence Gathered
**Files Analyzed:**
- `docs/reports/intentional-patterns-validation.md` - Pattern verdicts and recommendations
- `docs/reports/nonsense-logic.md` - Original 23 issues identified
- `.claude/plans/cleanup-worker-service-nonsense-logic.md` - Existing cleanup plan
- `src/services/worker-service.ts` (813 lines) - Current state
**Current State:**
- File has been reduced from 1445 lines to 813 lines in prior refactoring
- `runInteractiveSetup` still exists at line 439 (~200 lines of dead code)
- Re-export at line 78: `export { updateCursorContextForProject };`
- MCP version hardcoded "1.0.0" at line 159
- Fallback agents set at lines 144-146 without verification
- Unused imports: `fs`, `spawn`, `homedir`, `readline` at lines 13-17
**Allowed APIs (from validation report):**
- Exit code 0 pattern: **KEEP** (documented Windows Terminal workaround)
- `as Error` casts: **KEEP** (documented project policy)
- Dual init tracking: **KEEP** (serves async + sync callers)
- Signal handler ref pattern: **KEEP** (standard JS mutable state sharing)
- Empty MCP capabilities: **KEEP** (correct per MCP spec)
**Actions Required:**
| Pattern | Action | Priority |
|---------|--------|----------|
| Re-export for circular import | Remove (no actual circular dep) | LOW |
| Fallback agent without check | Add availability verification | HIGH |
| MCP version hardcoded | Update to use package.json | LOW |
| Dead code `runInteractiveSetup` | Delete (~200 lines) | HIGH |
| Unused imports | Delete | LOW |
---
## Phase 1: Delete Dead Code (HIGH PRIORITY)
### 1.1 Delete `runInteractiveSetup` Function
**What:** Delete lines 435-639 (approximately 200 lines)
**File:** `src/services/worker-service.ts`
**Location confirmed:** Line 439 starts `async function runInteractiveSetup(): Promise<number>`
**Steps:**
1. Read worker-service.ts lines 435-650 to find exact boundaries
2. Delete the section comment and entire function
3. Run build to verify no compile errors
**Verification:**
```bash
grep -n "runInteractiveSetup" src/services/worker-service.ts
# Expected: No output (function deleted)
npm run build
# Expected: No errors
```
### 1.2 Remove Unused Imports
**What:** Delete imports only used by dead code
**Lines to delete:** 13-17 (check each)
**Current imports to remove:**
```typescript
import * as fs from 'fs'; // Line 13 - UNUSED (namespace never accessed)
import { spawn } from 'child_process'; // Line 14 - UNUSED (MCP uses StdioClientTransport)
import { homedir } from 'os'; // Line 15 - Only in dead code
import * as readline from 'readline'; // Line 17 - Only in dead code
```
**Keep:**
```typescript
import { existsSync, writeFileSync, readFileSync, mkdirSync } from 'fs'; // Line 16 - CHECK
```
**Steps:**
1. After deleting `runInteractiveSetup`, grep each import
2. Delete any with zero usages
3. Run build to verify
**Verification:**
```bash
grep -n "^import \* as fs" src/services/worker-service.ts
grep -n "import { spawn }" src/services/worker-service.ts
# Expected: No output
npm run build
```
### 1.3 Remove Unused CursorHooksInstaller Imports
**After deleting dead code, check:**
```typescript
import {
updateCursorContextForProject, // KEEP (re-exported)
handleCursorCommand, // KEEP (used in main)
detectClaudeCode, // DELETE (only in dead code)
findCursorHooksDir, // DELETE (only in dead code)
installCursorHooks, // DELETE (only in dead code)
configureCursorMcp // DELETE (only in dead code)
} from './integrations/CursorHooksInstaller.js';
```
**Verification:**
```bash
grep "detectClaudeCode\|findCursorHooksDir\|installCursorHooks\|configureCursorMcp" src/services/worker-service.ts
# Expected: Only import line (which gets trimmed)
```
---
## Phase 2: Fix Fallback Agent Oversight (HIGH PRIORITY)
### 2.1 Add SDKAgent Availability Check
**Problem:** Lines 144-146 set Claude SDK as fallback without verifying it's configured
```typescript
this.geminiAgent.setFallbackAgent(this.sdkAgent);
this.openRouterAgent.setFallbackAgent(this.sdkAgent);
```
**Risk:** User chooses Gemini because they lack Claude credentials → transient Gemini error → fallback to Claude SDK → cascading failure
**Solution Options:**
**Option A: Add isConfigured() method to SDKAgent**
1. Add method to SDKAgent that checks for valid Claude SDK credentials
2. Only set fallback if `sdkAgent.isConfigured()` returns true
3. Log warning when fallback unavailable
**Pattern to follow (from SDKAgent.ts constructor):**
```typescript
// Check if Claude SDK can be initialized
public isConfigured(): boolean {
// Claude SDK uses subprocess, check if claude command exists
try {
// Check for ANTHROPIC_API_KEY or claude CLI availability
return !!process.env.ANTHROPIC_API_KEY || this.checkClaudeCliAvailable();
} catch {
return false;
}
}
```
**Option B: Document limitation (minimal fix)**
Add comment explaining the risk:
```typescript
// NOTE: Fallback to Claude SDK may fail if user lacks Claude credentials
// Consider adding availability check in future (Issue #XXX)
this.geminiAgent.setFallbackAgent(this.sdkAgent);
```
**Recommended: Option A**
**Steps:**
1. Read SDKAgent.ts to understand initialization pattern
2. Add `isConfigured()` method that checks Claude CLI/credentials
3. Update worker-service.ts to conditionally set fallback
4. Add warning log when fallback unavailable
5. Run tests
**Verification:**
```bash
grep -n "isConfigured" src/services/worker/SDKAgent.ts
# Expected: Method definition
grep -n "setFallbackAgent" src/services/worker-service.ts
# Expected: Conditional calls with isConfigured check
npm test
```
---
## Phase 3: Remove Unnecessary Re-Export (LOW PRIORITY)
### 3.1 Fix Misleading Re-Export
**Current (worker-service.ts:77-78):**
```typescript
// Re-export updateCursorContextForProject for SDK agents
export { updateCursorContextForProject };
```
**Issue:** Comment implies avoiding circular import, but investigation found NO circular dependency exists.
**Import chain:**
```
CursorHooksInstaller.ts (defines) → worker-service.ts (imports, re-exports) → ResponseProcessor.ts (imports)
```
**ResponseProcessor.ts could import directly from CursorHooksInstaller.ts**
**Options:**
1. **Remove re-export entirely** - Update ResponseProcessor.ts to import from CursorHooksInstaller directly
2. **Fix comment** - Update to reflect actual reason (API surface simplification)
**Recommended: Option 1 (cleaner)**
**Steps:**
1. Update `src/services/worker/agents/ResponseProcessor.ts`:
- Change: `import { updateCursorContextForProject } from '../../worker-service.js';`
- To: `import { updateCursorContextForProject } from '../../integrations/CursorHooksInstaller.js';`
2. Delete re-export from worker-service.ts (lines 77-78)
3. Run build to verify
**Verification:**
```bash
grep -n "export { updateCursorContextForProject" src/services/worker-service.ts
# Expected: No output
grep -n "updateCursorContextForProject" src/services/worker/agents/ResponseProcessor.ts
# Expected: Import from CursorHooksInstaller
npm run build
```
---
## Phase 4: Update MCP Version (LOW PRIORITY)
### 4.1 Use Package Version for MCP Client
**Current (worker-service.ts:157-160):**
```typescript
this.mcpClient = new Client({
name: 'worker-search-proxy',
version: '1.0.0' // Hardcoded, should match package.json (9.0.4)
}, { capabilities: {} });
```
**Also affects (from report):**
- `src/services/sync/ChromaSync.ts:126-131`
- MCP server (separate file)
**Pattern to follow:**
```typescript
import { version } from '../../package.json' assert { type: 'json' };
this.mcpClient = new Client({
name: 'worker-search-proxy',
version: version
}, { capabilities: {} });
```
**Alternative (if JSON import not supported):**
```typescript
import { readFileSync } from 'fs';
const pkg = JSON.parse(readFileSync(new URL('../../package.json', import.meta.url), 'utf-8'));
this.mcpClient = new Client({
name: 'worker-search-proxy',
version: pkg.version
}, { capabilities: {} });
```
**Steps:**
1. Check if JSON import assertion works in project
2. Update worker-service.ts MCP client initialization
3. Update ChromaSync.ts similarly
4. Run build to verify
**Verification:**
```bash
grep -n "version: '1.0.0'" src/services/worker-service.ts src/services/sync/ChromaSync.ts
# Expected: No output
npm run build
```
### 4.2 Add MCP Capabilities Comment
**Current:**
```typescript
}, { capabilities: {} });
```
**Add clarifying comment:**
```typescript
}, {
// MCP spec: Clients accept all server capabilities; no declaration needed
capabilities: {}
});
```
---
## Phase 5: Verification
### 5.1 Build Check
```bash
npm run build
```
**Expected:** No TypeScript errors
### 5.2 Test Suite
```bash
npm test
```
**Expected:** All tests pass
### 5.3 Grep for Anti-Patterns
```bash
# Verify dead code removed
grep -r "runInteractiveSetup" src/
# Expected: No matches
# Verify unused imports removed
grep "import \* as fs from 'fs'" src/services/worker-service.ts
# Expected: No match
# Verify re-export removed
grep "export { updateCursorContextForProject" src/services/worker-service.ts
# Expected: No match
# Verify fallback has check
grep -A2 "setFallbackAgent" src/services/worker-service.ts
# Expected: Conditional with isConfigured check
```
### 5.4 Runtime Check
```bash
npm run build-and-sync
# Manually verify worker starts and basic operations work
```
---
## Summary
| Phase | Description | Lines Changed | Priority |
|-------|-------------|---------------|----------|
| Phase 1 | Delete dead code + imports | ~200 deleted | HIGH |
| Phase 2 | Add fallback verification | ~10 added | HIGH |
| Phase 3 | Remove re-export | ~5 changed | LOW |
| Phase 4 | Update MCP version | ~3 changed | LOW |
| Phase 5 | Verification | N/A | N/A |
**Execution Order:** Phase 1 → Phase 2 → Phase 3 → Phase 4 → Phase 5
**Note:** Each phase should be followed by verification (build + test) before proceeding.
---
## Patterns Confirmed KEEP (No Action)
These patterns were validated as intentional:
1. **Exit code 0 always** - Windows Terminal tab accumulation workaround (commit 222a73da)
2. **`as Error` casts** - Documented project policy with anti-pattern detection
3. **Dual init tracking** - Promise for async, flag for sync callers
4. **Signal handler ref pattern** - Standard JS mutable state sharing
5. **Empty MCP capabilities** - Correct per MCP client spec