fix(models): refresh provider model listings

[lavaman131]

Refresh Claude and Copilot model discovery so repeated /model runs fetch current SDK data instead of stale cached results. Invalidate the UI model cache on each /model invocation and prefer an installed copilot binary on PATH when resolving the SDK CLI entry point.

[claude]

PR Review: fix(models): refresh provider model listings

Summary

This PR addresses model list caching issues by adding cache invalidation for Claude and Copilot model discovery, ensuring repeated /model runs fetch current SDK data instead of stale cached results.

---

✅ Strengths

1. Well-structured cache invalidation pattern: The invalidateModelCache() method in UnifiedModelOperations is clean and follows a standard cache invalidation pattern.

2. Good test coverage: The PR includes comprehensive tests for:

   • Cache invalidation behavior (refetches models after cache invalidation)
   • Copilot model listing (external server, fresh RPC, temporary SDK client)
   • Claude fresh model fetching logic
   • Built-in command cache invalidation

3. Dependency injection for testability: getBundledCopilotCliPath() now accepts a CopilotCliPathResolverDependencies interface, making it testable with mocked file system and which implementations.

4. Proper cleanup: The stop() call in listSdkModelsFromFreshClient() is wrapped in try/finally with a catch for best-effort cleanup.

---

⚠️ Issues & Suggestions

1. Type safety concern in listModelsForCopilot (Medium)

// src/models/model-operations.ts:286-287
const modelInfos = await this.sdkListCopilotModels();
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return modelInfos.map((m: any) => fromCopilotModelInfo(m));

The CopilotSdkListModelsFn returns Promise<unknown[]> and then casts to any. Consider defining a proper interface for the Copilot model info shape instead of relying on any.

Recommendation: Use the CopilotSdkModelRecord type from copilot.ts or define a shared type.

2. Optional chaining on non-null assertion (Low)

// src/models/model-operations.ts:433-435
const found = this.cachedModels?.some(
  m => m.id === model || m.modelID === model
) ?? false;

This change is correct but inconsistent with the caching logic above. If listAvailableModels() succeeds, cachedModels should always be set. Consider documenting why this defensive check is necessary.

3. Session-based read counter in Claude client (Low - Design Question)

// src/sdk/clients/claude.ts:362
private modelListReadsBySession = new Map<string, number>();

The first call uses the session query, subsequent calls use fetchFreshSupportedModels(). This works but the rationale could be clearer. Consider:

• Adding a comment explaining why the first call can use cached session data
• Consider whether invalidating on each /model call (already done in builtin-commands.ts) makes this session-level tracking redundant

4. PATH resolution complexity (Low)

The findExternalCopilotBinaryOnPath function duplicates some logic with isNodeModulesBinPath. The checks for node_modules/.bin are repeated with multiple path separator styles:

return path.includes(`${sep}node_modules${sep}.bin${sep}`) ||
  path.endsWith(`${sep}node_modules${sep}.bin`) ||
  path.includes("/node_modules/.bin/") ||
  // ...

This is defensive coding but could be simplified using a single regex or normalized path comparison.

5. Version mismatch in lockfile (Info)

Package.json specifies ^0.2.69 for claude-agent-sdk but bun.lock resolves to 0.2.70. The caret allows this, just noting the difference.

---

🔒 Security

No security concerns identified. The PR doesn't introduce user input handling, external network calls to untrusted sources, or sensitive data handling.

---

📊 Performance

• Positive: Cache invalidation avoids stale data issues without adding overhead
• Consideration: Creating a fresh SDK client in listSdkModelsFromFreshClient() has startup cost, but this is only called on explicit /model invocations, not during normal operation

---

🧪 Test Coverage

Good coverage with 65+ lines of new tests in model-operations.test.ts, 49+ in claude.test.ts, and 174+ in copilot.test.ts. Tests cover:

• Injected model listers
• Cache invalidation behavior
• Fresh vs cached model fetching
• PATH resolution edge cases (node_modules exclusion, external binary preference)

---

Final Verdict

Approve with minor suggestions. The PR achieves its goal of refreshing model listings on each /model invocation. The implementation is clean, well-tested, and handles edge cases appropriately. The suggestions above are minor improvements that could be addressed in follow-up work.