chore: simplify running with different thv binary

[kantord]

lets you connect to a custom thv serve using THV_PORT

[kantord]

@claude can you please do a deep review of this pr

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[kantord]

@claude can you please do a deep review of this pr

[claude]

Claude finished @kantord's task —— View job

---

Deep PR Review

I've completed a thorough review of this PR. Here's my detailed feedback:

Tasks

• Check for repository guidelines (CLAUDE.md)
• Read and understand all changed files
• Analyze code quality and design patterns
• Check for potential bugs and security issues
• Review testing and documentation
• Provide comprehensive feedback

---

Summary

This PR adds development-time functionality to switch between different thv binary versions, making it easier for developers to test with custom builds. The implementation is generally well-structured and thoughtful, with good separation of concerns. However, I've identified several areas that need attention before merging.

---

Critical Issues

1. Race Condition in Config File Watching (main/src/main.ts:268-283)

The file watcher uses a debounce without proper cleanup or state tracking:

watch(cfgDir, (eventType, filename) => {
  if (filename === cfgBase) {
    scheduleToolhiveRestart(eventType, cfgBase)
  }
})

Issues:

• No error handling if the directory doesn't exist
• Watcher is never cleaned up (memory leak)
• Race conditions possible if config changes during restart
• Multiple file system events could trigger multiple restarts

Recommendation:

let thvWatcher: fs.FSWatcher | undefined
try {
  const cfgPath = getThvConfigPath()
  const cfgDir = path.dirname(cfgPath)
  const cfgBase = path.basename(cfgPath)
  
  if (existsSync(cfgDir)) {
    thvWatcher = watch(cfgDir, (eventType, filename) => {
      if (filename === cfgBase && !isRestarting) {
        scheduleToolhiveRestart(eventType, cfgBase)
      }
    })
    log.info(`[thv-config] Watching for changes: ${cfgPath}`)
  }
} catch (e) {
  log.warn('[thv-config] Failed to set up watcher for .thv_bin:', e)
}

// Later, on app quit:
if (thvWatcher) {
  thvWatcher.close()
}

2. Type Safety Issues in Config Reading (main/src/toolhive-manager.ts:31-34)

The code has unnecessary type casting that bypasses TypeScript's type system:

const config = readConfig()
const rawMode = (config as { mode?: string }).mode
const mode: 'default' | 'custom' =
  rawMode === 'custom' || rawMode === 'global' ? 'custom' : 'default'

Issues:

• readConfig() returns ThvBinaryConfig which already has a mode property
• Type casting indicates a mismatch between the type definition and actual usage
• The 'global' mode is handled but not defined in the ThvBinaryMode type
• This pattern is repeated in two places (lines 31-34 and 410-412)

Recommendation:
Either update the type definition to include 'global' or clarify why it's treated as 'custom':

type ThvBinaryMode = 'default' | 'custom'

// In scripts/use-thv.ts, add validation:
function readConfig(): ThvBinaryConfig {
  // ... existing code ...
  try {
    const content = readFileSync(configPath, 'utf-8')
    const parsed = JSON.parse(content)
    
    // Normalize legacy 'global' mode to 'custom'
    if (parsed.mode === 'global') {
      parsed.mode = 'custom'
    }
    
    return parsed
  } catch {
    // ... existing code ...
  }
}

3. Process Spawning Without Timeout (main/src/toolhive-manager.ts:100-135)

The getThvBinaryVersion() function spawns a child process with a 3-second timeout, but there's a potential issue:

const timer = setTimeout(() => {
  child.kill('SIGKILL')
  log.warn('thv version timed out')
  resolve(null)
}, 3000)

Issues:

• If the process exits just as the timer fires, both the timer and exit handler will call resolve()
• The close event handler should clear the timer before resolving
• No handling for the case where binPath is invalid but exists

Recommendation:

let resolved = false
const timer = setTimeout(() => {
  if (!resolved) {
    resolved = true
    child.kill('SIGKILL')
    log.warn('thv version timed out')
    resolve(null)
  }
}, 3000)

child.on('close', () => {
  if (!resolved) {
    resolved = true
    clearTimeout(timer)
    const version = parseVersion(combined.trim())
    if (version) cachedBinaryVersion = version
    resolve(version)
  }
})

---

High Priority Issues

4. Missing Error Handling in Script (scripts/use-thv.ts:42-48)

JSON parsing has minimal error handling:

try {
  const content = readFileSync(configPath, 'utf-8')
  return JSON.parse(content)
} catch {
  console.warn('⚠️  Failed to parse .thv_bin, using defaults')
  return { mode: 'default', customPath: '' }
}

Issues:

• Silent failure makes debugging difficult
• Could fail due to file permissions, not just invalid JSON
• No validation of parsed content structure

Recommendation:

try {
  const content = readFileSync(configPath, 'utf-8')
  const parsed = JSON.parse(content)
  
  // Validate structure
  if (typeof parsed.mode !== 'string' || typeof parsed.customPath !== 'string') {
    console.warn('⚠️  Invalid .thv_bin structure, using defaults')
    return { mode: 'default', customPath: '' }
  }
  
  return parsed
} catch (error) {
  console.warn('⚠️  Failed to read/parse .thv_bin:', error.message)
  return { mode: 'default', customPath: '' }
}

5. Version Parsing Could Be More Robust (main/src/toolhive-manager.ts:100-106)

The regex pattern may not catch all valid version formats:

const parseVersion = (text: string): string | null => {
  const current = text.match(/Currently running:\s*(v?[^\s]+)/i)?.[1]
  if (current) return current
  return (
    text.match(/\bv\d+\.\d+(?:\.\d+)?(?:-[0-9A-Za-z\-.]+)?\b/)?.[0] ?? null
  )
}

Issue:

• The fallback regex requires a 'v' prefix, but the first pattern allows it to be optional
• Pre-release versions with + (e.g., v1.0.0+build.123) won't match

Recommendation:

const parseVersion = (text: string): string | null => {
  // Try structured format first
  const current = text.match(/Currently running:\s*(v?[^\s]+)/i)?.[1]
  if (current) return current
  
  // Fallback to semver-like pattern (with optional 'v' prefix)
  return (
    text.match(/\bv?\d+\.\d+(?:\.\d+)?(?:[-+][0-9A-Za-z\-.]+)?\b/)?.[0] ?? null
  )
}

---

Medium Priority Issues

6. React Component Could Handle Loading States Better (renderer/src/common/components/thv-binary-mode-banner.tsx:19-32)

The component fetches data on mount but has no loading state:

useEffect(() => {
  Promise.all([
    window.electronAPI.getThvBinaryMode(),
    window.electronAPI.getToolhiveVersion().catch(() => null),
  ])
    .then(([mode, ver]) => {
      setBinaryMode(mode as ThvBinaryMode)
      setVersion(typeof ver === 'string' ? ver : null)
    })
    .catch((error: unknown) => {
      console.error('Failed to get thv binary info:', error)
    })
}, [])

Issues:

• Component returns null while loading, which could cause layout shift
• Type casting mode as ThvBinaryMode is unnecessary - the API already returns the correct type
• Empty dependency array means this only runs once, won't react to binary changes

Recommendation:

const [isLoading, setIsLoading] = useState(true)

useEffect(() => {
  Promise.all([
    window.electronAPI.getThvBinaryMode(),
    window.electronAPI.getToolhiveVersion().catch(() => null),
  ])
    .then(([mode, ver]) => {
      setBinaryMode(mode)
      setVersion(typeof ver === 'string' ? ver : null)
    })
    .catch((error: unknown) => {
      console.error('Failed to get thv binary info:', error)
    })
    .finally(() => setIsLoading(false))
}, [])

if (isLoading || !binaryMode || binaryMode.isDefault) {
  return null
}

7. Inconsistent Naming in Package Scripts (package.json:23-25)

The naming convention is inconsistent:

"useThv:default": "ts-node ./scripts/use-thv.ts default",
"useThv:custom": "ts-node ./scripts/use-thv.ts custom",
"useThv:show": "ts-node ./scripts/use-thv.ts show"

Issue:

• Camel case with colons is non-standard for npm scripts
• Most npm scripts use kebab-case

Recommendation:

"use-thv:default": "ts-node ./scripts/use-thv.ts default",
"use-thv:custom": "ts-node ./scripts/use-thv.ts custom",
"use-thv:show": "ts-node ./scripts/use-thv.ts show"

8. Missing Test Coverage

No tests were added for the new functionality. Consider adding:

• Unit tests for readConfig() and writeConfig() in scripts/use-thv.ts
• Unit tests for resolveThvBinaryPath() in main/src/toolhive-manager.ts
• Integration tests for the config watcher
• Component tests for ThvBinaryModeBanner

---

Low Priority / Style Issues

9. Magic Numbers (main/src/main.ts:140)

}, 300)  // What does 300ms represent?

Recommendation:

const THV_RESTART_DEBOUNCE_MS = 300
setTimeout(async () => {
  // ...
}, THV_RESTART_DEBOUNCE_MS)

10. Commit Messages

The commit history shows placeholder commit messages (.). While this is acceptable during development, consider squashing or providing more descriptive messages before merging.

11. Documentation in README (README.md:97-114)

The developer notes section is well-written and clear. Good job! Consider adding:

• A note about the .thv_bin file being gitignored
• Example output of pnpm useThv:show
• Troubleshooting tips if the binary path is incorrect

---

Positive Observations

1. Good Separation of Concerns: The config logic is properly separated into scripts/use-thv.ts and imported where needed
2. Production Safety: The feature is correctly disabled in packaged builds (app.isPackaged checks)
3. User Feedback: The banner component provides clear visual feedback about which binary is in use
4. Fallback Behavior: The code gracefully falls back to the embedded binary if the custom path is invalid
5. Documentation: The README is updated with clear usage instructions
6. Accessibility: The Alert component properly uses role="alert" for screen readers

---

Recommendations Summary

Before merging:

1. Fix the race condition in the config file watcher ([Task]: Add router #1)
2. Resolve the type safety issues ([Task]: Add hey-api client, automate fetching the openapi.json #2)
3. Fix the process spawning timeout logic ([Task]: Configure MSW #3)
4. Improve error handling in config reading ([Task]: Configure vitest & react-testing-library #4)

Nice to have:
5. Add test coverage
6. Fix version parsing edge cases (#5)
7. Improve component loading states (#6)
8. Consider squashing commits with better messages

---

Questions for the Author

1. Is the 'global' mode intentionally undocumented, or should it be formalized in the type system?
2. Should the banner auto-dismiss after a certain time, or remain visible for the entire session?
3. Have you tested this with rapid config file changes (e.g., switching back and forth quickly)?
4. Should there be a UI control to switch binaries, or is the CLI script sufficient?

---

View job | Branch: simplify-running-with-local-binary

[kantord]

Hi @claude Thanks! Kept this dev‑only and minimal: .thv_bin is now just a plain path (empty = default), watcher is debounced and cleaned up, and version
reads thv version with a short timeout (no misleading fallback). Skipped extra robustness/tests since this is dev‑only and non‑critical; happy to
revisit if it proves flaky.

Anything else you still think is critical?

[claude]

Claude finished @kantord's task —— View job

---

Review of Updated Changes

Tasks

• Read updated files to understand current implementation
• Check if previous critical issues were addressed
• Identify any remaining critical concerns
• Provide final assessment

---

Assessment

Great work simplifying the implementation! I've reviewed the updated code and here's what I found:

✅ Critical Issues Addressed

1. Type Safety & Config Structure - RESOLVED

• The .thv_bin config is now a simple plain-path file (empty = default)
• No more type casting or confusing mode enums
• Much cleaner: scripts/use-thv.ts:9-20

2. Watcher Cleanup - RESOLVED

• Watcher is now properly stored and cleaned up in will-quit handler: main/src/main.ts:388-395
• Good use of optional chaining for safe cleanup

3. Process Timeout Logic - RESOLVED

• Fixed with a settled flag to prevent double-resolve: main/src/toolhive-manager.ts:121-150
• Proper cleanup with clearTimeout in all paths
• Nice handling of the error event too

4. Debouncing - IMPROVED

• Debounce is properly implemented: main/src/main.ts:125-142
• Constant is well-named: THV_RESTART_DEBOUNCE_MS

---

🟡 Minor Observations (Non-Critical)

These are fine for dev-only code, but noting them for completeness:

1. Watcher Error Handling
The watcher setup could fail silently if existsSync(cfgDir) returns true but the actual watch() call fails (permissions, etc.). The try-catch at main/src/main.ts:271-285 catches this, so it's handled gracefully. ✓

2. Version Parsing
The regex at main/src/toolhive-manager.ts:110 still doesn't handle semver with + (e.g., v1.0.0+build.123), but since this is dev-only and you've explicitly chosen minimal robustness, this is totally fine for the use case.

3. Banner Component
The ThvBinaryModeBanner component will flash briefly on mount before hiding if the mode is default. Not critical since it's dev-only, but if you ever wanted to polish it, you could initialize state to handle loading separately from the actual mode.

---

✅ Final Verdict

Nothing critical remains. The implementation is now:

• ✅ Clean and minimal (plain path file)
• ✅ Properly debounced
• ✅ Watcher cleanup handled
• ✅ Race conditions fixed
• ✅ Appropriate for dev-only usage

The simplifications you made were smart decisions for a development-time feature. The code is solid for the intended purpose.

---

Answer to Your Question

Anything else you still think is critical?

No, nothing critical. All the major issues from the previous review have been addressed:

1. ✅ Race conditions fixed
2. ✅ Type safety improved (simpler approach)
3. ✅ Process spawning timeout fixed
4. ✅ Watcher cleanup added

The remaining observations are minor polish items that aren't necessary for dev-only functionality. You made good pragmatic choices by keeping this minimal and focused.

Ready to merge from a code quality perspective! 🚀

---

View job | Branch: simplify-running-with-local-binary

[Copilot]

Pull Request Overview

This PR adds a developer-only feature to switch between the embedded thv binary and a custom binary during local development. The app reads configuration from a .thv_bin file, watches for changes to automatically restart the ToolHive process, and displays a warning banner in development mode when using a non-default binary.

Key changes:

• Added CLI script (scripts/use-thv.ts) for switching between default and custom thv binaries
• Implemented file watcher in main process to detect .thv_bin changes and restart ToolHive
• Created warning banner component displayed in development mode when using custom binary

Reviewed Changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
scripts/use-thv.ts	New CLI script for managing thv binary configuration
renderer/src/routes/__root.tsx	Added ThvBinaryModeBanner to root component
renderer/src/common/components/ui/alert.tsx	Added warning variant to Alert component
renderer/src/common/components/thv-binary-mode-banner.tsx	New banner component showing custom binary info
preload/src/preload.ts	Exposed new IPC handlers for binary mode and version
package.json	Added npm scripts for binary switching commands
main/src/toolhive-manager.ts	Implemented binary path resolution and version detection
main/src/main.ts	Added file watcher and IPC handlers for binary configuration
docs/README.md	Added documentation for custom binary feature
README.md	Added developer notes about THV binary selection
.gitignore	Added .thv_bin config file to gitignore

[kantord]

use log instead of console.log

[kantord]

remove warning style from here and instead override in the component itself (because the warning is not an official design)