Merge pull request #978 from vybestack/issue684

acoliver · web-flow · commit 405eb5e26005 · 2026-01-02T18:44:40.000-03:00
fix: API stats tracking and theme violations in /stats model - fixed #684
diff --git a/packages/cli/src/ui/components/ModelStatsDisplay.test.tsx b/packages/cli/src/ui/components/ModelStatsDisplay.test.tsx
@@ -9,6 +9,7 @@ import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest';
 import { ModelStatsDisplay } from './ModelStatsDisplay.js';
 import * as SessionContext from '../contexts/SessionContext.js';
 import { SessionMetrics } from '../contexts/SessionContext.js';
+import { Colors } from '../colors.js';
 
 // Mock the context to provide controlled data for testing
 vi.mock('../contexts/SessionContext.js', async (importOriginal) => {
@@ -249,4 +250,98 @@ describe('<ModelStatsDisplay />', () => {
     expect(output).not.toContain('gemini-2.5-flash');
     expect(output).toMatchSnapshot();
   });
+
+  /**
+   * Issue #684: Theme violation tests
+   * All text in ModelStatsDisplay should use theme colors, not default white.
+   */
+  describe('theme compliance', () => {
+    it('should use Colors.Foreground for section headers (API, Tokens)', () => {
+      // Verify that Colors.Foreground is defined (theme is available)
+      expect(Colors.Foreground).toBeDefined();
+      expect(typeof Colors.Foreground).toBe('string');
+
+      const { lastFrame } = renderWithMockedStats({
+        models: {
+          'test-model': {
+            api: { totalRequests: 1, totalErrors: 0, totalLatencyMs: 100 },
+            tokens: {
+              prompt: 10,
+              candidates: 20,
+              total: 30,
+              cached: 0,
+              thoughts: 0,
+              tool: 0,
+            },
+          },
+        },
+        tools: {
+          totalCalls: 0,
+          totalSuccess: 0,
+          totalFail: 0,
+          totalDurationMs: 0,
+          totalDecisions: { accept: 0, reject: 0, modify: 0 },
+          byName: {},
+        },
+      });
+
+      const output = lastFrame();
+      // Section headers should be present
+      expect(output).toContain('API');
+      expect(output).toContain('Tokens');
+      // The output includes ANSI codes from ink, verify structure
+      expect(output).toMatchSnapshot();
+    });
+
+    it('should use Colors.Foreground for the empty state message', () => {
+      const { lastFrame } = renderWithMockedStats({
+        models: {},
+        tools: {
+          totalCalls: 0,
+          totalSuccess: 0,
+          totalFail: 0,
+          totalDurationMs: 0,
+          totalDecisions: { accept: 0, reject: 0, modify: 0 },
+          byName: {},
+        },
+      });
+
+      const output = lastFrame();
+      expect(output).toContain('No API calls have been made in this session.');
+      // The text should include color codes from the theme
+      expect(output).toMatchSnapshot();
+    });
+
+    it('should use Colors.Foreground for stat row values', () => {
+      const { lastFrame } = renderWithMockedStats({
+        models: {
+          'test-model': {
+            api: { totalRequests: 5, totalErrors: 0, totalLatencyMs: 500 },
+            tokens: {
+              prompt: 100,
+              candidates: 200,
+              total: 300,
+              cached: 0,
+              thoughts: 0,
+              tool: 0,
+            },
+          },
+        },
+        tools: {
+          totalCalls: 0,
+          totalSuccess: 0,
+          totalFail: 0,
+          totalDurationMs: 0,
+          totalDecisions: { accept: 0, reject: 0, modify: 0 },
+          byName: {},
+        },
+      });
+
+      const output = lastFrame();
+      // Values should be present with theme colors
+      expect(output).toContain('5'); // Requests count
+      expect(output).toContain('100'); // Prompt tokens
+      expect(output).toMatchSnapshot();
+    });
+  });
 });
diff --git a/packages/cli/src/ui/components/ModelStatsDisplay.tsx b/packages/cli/src/ui/components/ModelStatsDisplay.tsx
@@ -25,6 +25,10 @@ interface StatRowProps {
   isSection?: boolean;
 }
 
+/**
+ * StatRow component for displaying a labeled row of values.
+ * Issue #684: Fixed theme violations - all text now uses theme colors.
+ */
 const StatRow: React.FC<StatRowProps> = ({
   title,
   values,
@@ -33,13 +37,16 @@ const StatRow: React.FC<StatRowProps> = ({
 }) => (
   <Box>
     <Box width={METRIC_COL_WIDTH}>
-      <Text bold={isSection} color={isSection ? undefined : Colors.LightBlue}>
+      <Text
+        bold={isSection}
+        color={isSection ? Colors.Foreground : Colors.LightBlue}
+      >
         {isSubtle ? `  ↳ ${title}` : title}
       </Text>
     </Box>
     {values.map((value, index) => (
       <Box width={MODEL_COL_WIDTH} key={index}>
-        <Text>{value}</Text>
+        <Text color={Colors.Foreground}>{value}</Text>
       </Box>
     ))}
   </Box>
diff --git a/packages/core/src/providers/LoggingProviderWrapper.ts b/packages/core/src/providers/LoggingProviderWrapper.ts
@@ -22,12 +22,16 @@ import {
   logConversationResponse,
   logTokenUsage,
   logApiRequest,
+  logApiResponse,
+  logApiError,
 } from '../telemetry/loggers.js';
 import {
   ConversationRequestEvent,
   ConversationResponseEvent,
   TokenUsageEvent,
   ApiRequestEvent,
+  ApiResponseEvent,
+  ApiErrorEvent,
 } from '../telemetry/types.js';
 import { getConversationFileWriter } from '../storage/ConversationFileWriter.js';
 import { ProviderPerformanceTracker } from './logging/ProviderPerformanceTracker.js';
@@ -613,13 +617,25 @@ export class LoggingProviderWrapper implements IProvider {
 
     // Always process stream to extract token metrics
     // If logging not enabled, process for metrics only
+    // Resolve the model name for telemetry - use resolved model, not provider default
+    const resolvedModelName =
+      normalizedOptions.resolved?.model || this.wrapped.getDefaultModel();
     if (!activeConfig?.getConversationLoggingEnabled()) {
-      yield* this.processStreamForMetrics(activeConfig, stream);
+      yield* this.processStreamForMetrics(
+        activeConfig,
+        stream,
+        resolvedModelName,
+      );
       return;
     }
 
     // Log the response stream (which also processes metrics)
-    yield* this.logResponseStream(activeConfig, stream, promptId);
+    yield* this.logResponseStream(
+      activeConfig,
+      stream,
+      promptId,
+      resolvedModelName,
+    );
   }
 
   private async logRequest(
@@ -670,10 +686,12 @@ export class LoggingProviderWrapper implements IProvider {
   /**
    * Process stream to extract token metrics without logging
    * @plan PLAN-20250909-TOKTRACK
+   * @issue #684 - Fixed: Now logs API response telemetry for /stats model
    */
   private async *processStreamForMetrics(
     config: Config | undefined,
     stream: AsyncIterableIterator<IContent>,
+    modelName: string,
   ): AsyncIterableIterator<IContent> {
     const startTime = performance.now();
     let latestTokenUsage: UsageStats | undefined;
@@ -692,11 +710,39 @@ export class LoggingProviderWrapper implements IProvider {
       }
 
       // Process metrics if we have token usage
-      if (latestTokenUsage) {
-        const duration = performance.now() - startTime;
-        const tokenCounts =
-          this.extractTokenCountsFromTokenUsage(latestTokenUsage);
+      const duration = performance.now() - startTime;
+      const tokenCounts = latestTokenUsage
+        ? this.extractTokenCountsFromTokenUsage(latestTokenUsage)
+        : {
+            input_token_count: 0,
+            output_token_count: 0,
+            cached_content_token_count: 0,
+            thoughts_token_count: 0,
+            tool_token_count: 0,
+            cache_read_input_tokens: 0,
+            cache_creation_input_tokens: null,
+          };
+
+      // Issue #684: Log API response telemetry for /stats model tracking
+      if (config) {
+        // Create event and set token counts directly since constructor doesn't support raw counts
+        const event = new ApiResponseEvent(
+          modelName,
+          duration,
+          '', // promptId - not available in metrics-only path
+        );
+        event.input_token_count = tokenCounts.input_token_count;
+        event.output_token_count = tokenCounts.output_token_count;
+        event.cached_content_token_count =
+          tokenCounts.cached_content_token_count;
+        event.thoughts_token_count = tokenCounts.thoughts_token_count;
+        event.tool_token_count = tokenCounts.tool_token_count;
+        event.total_token_count =
+          tokenCounts.input_token_count + tokenCounts.output_token_count;
+        logApiResponse(config, event);
+      }
 
+      if (latestTokenUsage) {
         // Accumulate token usage for session tracking
         this.accumulateTokenUsage(tokenCounts, config);
 
@@ -713,6 +759,24 @@ export class LoggingProviderWrapper implements IProvider {
       // Record error in performance tracker
       const duration = performance.now() - startTime;
       this.performanceTracker.recordError(duration, String(error));
+
+      // Issue #684: Log API error telemetry
+      if (config) {
+        const errorMessage =
+          error instanceof Error ? error.message : String(error);
+        logApiError(
+          config,
+          new ApiErrorEvent(
+            modelName,
+            errorMessage,
+            duration,
+            '', // promptId
+            undefined, // auth_type
+            'stream_error', // error_type
+            undefined, // status_code
+          ),
+        );
+      }
       throw error;
     }
   }
@@ -721,6 +785,7 @@ export class LoggingProviderWrapper implements IProvider {
     config: Config,
     stream: AsyncIterableIterator<IContent>,
     promptId: string,
+    modelName: string,
   ): AsyncIterableIterator<IContent> {
     const startTime = performance.now();
     let responseContent = '';
@@ -756,6 +821,7 @@ export class LoggingProviderWrapper implements IProvider {
         false,
         error,
         latestTokenUsage,
+        modelName,
       );
       throw error;
     }
@@ -770,6 +836,7 @@ export class LoggingProviderWrapper implements IProvider {
         true,
         undefined,
         latestTokenUsage,
+        modelName,
       );
     }
   }
@@ -804,6 +871,7 @@ export class LoggingProviderWrapper implements IProvider {
     success: boolean,
     error?: unknown,
     tokenUsage?: UsageStats,
+    modelName?: string,
   ): Promise<void> {
     try {
       const redactedContent = this.redactor
@@ -845,6 +913,25 @@ export class LoggingProviderWrapper implements IProvider {
         ),
       );
 
+      // Issue #684: Log API response telemetry for /stats model tracking
+      const resolvedModelName = modelName ?? this.wrapped.getDefaultModel();
+      const apiResponseEvent = new ApiResponseEvent(
+        resolvedModelName,
+        duration,
+        promptId,
+      );
+      apiResponseEvent.input_token_count = tokenCounts.input_token_count;
+      apiResponseEvent.output_token_count = tokenCounts.output_token_count;
+      apiResponseEvent.cached_content_token_count =
+        tokenCounts.cached_content_token_count;
+      apiResponseEvent.thoughts_token_count = tokenCounts.thoughts_token_count;
+      apiResponseEvent.tool_token_count = tokenCounts.tool_token_count;
+      apiResponseEvent.total_token_count = totalTokens;
+      if (!success && error) {
+        apiResponseEvent.error = String(error);
+      }
+      logApiResponse(config, apiResponseEvent);
+
       const event = new ConversationResponseEvent(
         this.wrapped.name,
         this.conversationId,
diff --git a/packages/core/src/providers/__tests__/LoadBalancingProvider.circuitbreaker.test.ts b/packages/core/src/providers/__tests__/LoadBalancingProvider.circuitbreaker.test.ts
@@ -14,9 +14,14 @@ import {
   type LoadBalancerSubProfile,
 } from '../LoadBalancingProvider.js';
 import { ProviderManager } from '../ProviderManager.js';
+import { SettingsService } from '../../settings/SettingsService.js';
+import { createRuntimeConfigStub } from '../../test-utils/runtime.js';
+import type { Config } from '../../config/config.js';
 import type { IContent } from '../../services/history/IContent.js';
 
 describe('LoadBalancingProvider Circuit Breaker - Phase 2', () => {
+  let settingsService: SettingsService;
+  let runtimeConfig: Config;
   let providerManager: ProviderManager;
   let config: LoadBalancingProviderConfig;
   const subProfiles: LoadBalancerSubProfile[] = [
@@ -37,7 +42,12 @@ describe('LoadBalancingProvider Circuit Breaker - Phase 2', () => {
   ];
 
   beforeEach(() => {
-    providerManager = new ProviderManager();
+    settingsService = new SettingsService();
+    runtimeConfig = createRuntimeConfigStub(settingsService);
+    providerManager = new ProviderManager({
+      settingsService,
+      config: runtimeConfig,
+    });
     config = {
       profileName: 'test-lb',
       strategy: 'failover',
diff --git a/packages/core/src/providers/__tests__/LoadBalancingProvider.timeout.test.ts b/packages/core/src/providers/__tests__/LoadBalancingProvider.timeout.test.ts
@@ -14,9 +14,14 @@ import {
   type LoadBalancerSubProfile,
 } from '../LoadBalancingProvider.js';
 import { ProviderManager } from '../ProviderManager.js';
+import { SettingsService } from '../../settings/SettingsService.js';
+import { createRuntimeConfigStub } from '../../test-utils/runtime.js';
+import type { Config } from '../../config/config.js';
 import type { IContent } from '../../services/history/IContent.js';
 
 describe('LoadBalancingProvider Timeout Wrapper - Phase 3', () => {
+  let settingsService: SettingsService;
+  let runtimeConfig: Config;
   let providerManager: ProviderManager;
   let config: LoadBalancingProviderConfig;
   const subProfiles: LoadBalancerSubProfile[] = [
@@ -38,7 +43,12 @@ describe('LoadBalancingProvider Timeout Wrapper - Phase 3', () => {
 
   beforeEach(() => {
     vi.useFakeTimers();
-    providerManager = new ProviderManager();
+    settingsService = new SettingsService();
+    runtimeConfig = createRuntimeConfigStub(settingsService);
+    providerManager = new ProviderManager({
+      settingsService,
+      config: runtimeConfig,
+    });
     config = {
       profileName: 'test-lb',
       strategy: 'failover',
diff --git a/packages/core/src/providers/__tests__/LoggingProviderWrapper.apiTelemetry.test.ts b/packages/core/src/providers/__tests__/LoggingProviderWrapper.apiTelemetry.test.ts
