refactor: deduplicate logs-stats/logs-summary pipeline and test scaffolding (#2433)

* Initial plan

* refactor: deduplicate logs-stats/logs-summary pipeline and tests

Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/db2594ab-4341-4cda-9b73-2391401b8c8c

* refactor: tighten LoggingOptions types and document harness spies

Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/db2594ab-4341-4cda-9b73-2391401b8c8c

* fix: exclude test-utils from build, add shouldLog behavior tests

Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/ecadd9eb-35b3-4054-9be8-afffc1aa5c90

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>

Author: Copilot

src/commands/logs-command-helpers.ts

@@ -5,7 +5,7 @@
 import * as fs from 'fs';
 import * as path from 'path';
 import { logger } from '../logger';
-import type { LogSource, PolicyManifest } from '../types';
+import type { LogSource, LogStatsFormat, PolicyManifest } from '../types';
 import {
   discoverLogSources,
   selectMostRecent,
@@ -14,15 +14,16 @@ import {
 import { loadAndAggregate, loadAllLogs } from '../logs/log-aggregator';
 import type { AggregatedStats } from '../logs/log-aggregator';
 import { enrichWithPolicyRules, computeRuleStats } from '../logs/audit-enricher';
+import { formatStats } from '../logs/stats-formatter';
 
 /**
  * Options for determining which logs to show (based on log level)
  */
 export interface LoggingOptions {
   /** The output format being used */
-  format: string;
+  format: LogStatsFormat;
   /** Callback to determine if info logs should be shown */
-  shouldLog: (format: string) => boolean;
+  shouldLog: (format: LogStatsFormat) => boolean;
 }
 
 /**
@@ -144,3 +145,29 @@ export async function loadLogsWithErrorHandling(
     process.exit(1);
   }
 }
+
+/**
+ * Shared output pipeline for `logs stats` and `logs summary`.
+ *
+ * Discovers the log source, loads and aggregates the logs, formats them, and
+ * prints the result. Each command passes only the `shouldLog` predicate that
+ * controls whether informational source-selection messages are emitted.
+ *
+ * @param options - Command options containing `format` and optional `source`
+ * @param shouldLog - Returns true when info-level log messages should be shown
+ */
+export async function runLogsCommand(
+  options: { format: LogStatsFormat; source?: string },
+  shouldLog: (format: LogStatsFormat) => boolean
+): Promise<void> {
+  const source = await discoverAndSelectSource(options.source, {
+    format: options.format,
+    shouldLog,
+  });
+
+  const stats = await loadLogsWithErrorHandling(source);
+
+  const colorize = !!(process.stdout.isTTY && options.format === 'pretty');
+  const output = formatStats(stats, options.format, colorize);
+  console.log(output);
+}

src/commands/logs-stats.test.ts

@@ -3,10 +3,9 @@
  */
 
 import { statsCommand, StatsCommandOptions } from './logs-stats';
-import * as logDiscovery from '../logs/log-discovery';
-import * as logAggregator from '../logs/log-aggregator';
-import * as statsFormatter from '../logs/stats-formatter';
+import { logger } from '../logger';
 import { LogSource } from '../types';
+import { createLogCommandTestHarness } from './test-helpers.test-utils';
 
 // Mock dependencies
 jest.mock('../logs/log-discovery');
@@ -21,26 +20,8 @@ jest.mock('../logger', () => ({
   },
 }));
 
-const mockedDiscovery = logDiscovery as jest.Mocked<typeof logDiscovery>;
-const mockedAggregator = logAggregator as jest.Mocked<typeof logAggregator>;
-const mockedFormatter = statsFormatter as jest.Mocked<typeof statsFormatter>;
-
 describe('logs-stats command', () => {
-  let mockExit: jest.SpyInstance;
-  let mockConsoleLog: jest.SpyInstance;
-
-  beforeEach(() => {
-    jest.clearAllMocks();
-    mockExit = jest.spyOn(process, 'exit').mockImplementation(() => {
-      throw new Error('process.exit called');
-    });
-    mockConsoleLog = jest.spyOn(console, 'log').mockImplementation();
-  });
-
-  afterEach(() => {
-    mockExit.mockRestore();
-    mockConsoleLog.mockRestore();
-  });
+  const harness = createLogCommandTestHarness();
 
   it('should discover and use most recent log source', async () => {
     const mockSource: LogSource = {
@@ -50,29 +31,29 @@ describe('logs-stats command', () => {
       dateStr: new Date().toLocaleString(),
     };
 
-    mockedDiscovery.discoverLogSources.mockResolvedValue([mockSource]);
-    mockedDiscovery.selectMostRecent.mockReturnValue(mockSource);
-    mockedAggregator.loadAndAggregate.mockResolvedValue({
+    harness.mockedDiscovery.discoverLogSources.mockResolvedValue([mockSource]);
+    harness.mockedDiscovery.selectMostRecent.mockReturnValue(mockSource);
+    harness.mockedAggregator.loadAndAggregate.mockResolvedValue({
       totalRequests: 10,
       allowedRequests: 8,
       deniedRequests: 2,
       uniqueDomains: 3,
       byDomain: new Map(),
       timeRange: { start: 1000, end: 2000 },
     });
-    mockedFormatter.formatStats.mockReturnValue('formatted output');
+    harness.mockedFormatter.formatStats.mockReturnValue('formatted output');
 
     const options: StatsCommandOptions = {
       format: 'pretty',
     };
 
     await statsCommand(options);
 
-    expect(mockedDiscovery.discoverLogSources).toHaveBeenCalled();
-    expect(mockedDiscovery.selectMostRecent).toHaveBeenCalled();
-    expect(mockedAggregator.loadAndAggregate).toHaveBeenCalledWith(mockSource);
-    expect(mockedFormatter.formatStats).toHaveBeenCalled();
-    expect(mockConsoleLog).toHaveBeenCalledWith('formatted output');
+    expect(harness.mockedDiscovery.discoverLogSources).toHaveBeenCalled();
+    expect(harness.mockedDiscovery.selectMostRecent).toHaveBeenCalled();
+    expect(harness.mockedAggregator.loadAndAggregate).toHaveBeenCalledWith(mockSource);
+    expect(harness.mockedFormatter.formatStats).toHaveBeenCalled();
+    expect(harness.mockConsoleLog).toHaveBeenCalledWith('formatted output');
   });
 
   it('should use specified source when provided', async () => {
@@ -81,17 +62,17 @@ describe('logs-stats command', () => {
       path: '/custom/path',
     };
 
-    mockedDiscovery.discoverLogSources.mockResolvedValue([]);
-    mockedDiscovery.validateSource.mockResolvedValue(mockSource);
-    mockedAggregator.loadAndAggregate.mockResolvedValue({
+    harness.mockedDiscovery.discoverLogSources.mockResolvedValue([]);
+    harness.mockedDiscovery.validateSource.mockResolvedValue(mockSource);
+    harness.mockedAggregator.loadAndAggregate.mockResolvedValue({
       totalRequests: 5,
       allowedRequests: 5,
       deniedRequests: 0,
       uniqueDomains: 2,
       byDomain: new Map(),
       timeRange: null,
     });
-    mockedFormatter.formatStats.mockReturnValue('formatted');
+    harness.mockedFormatter.formatStats.mockReturnValue('formatted');
 
     const options: StatsCommandOptions = {
       format: 'json',
@@ -100,67 +81,67 @@ describe('logs-stats command', () => {
 
     await statsCommand(options);
 
-    expect(mockedDiscovery.validateSource).toHaveBeenCalledWith('/custom/path');
-    expect(mockedAggregator.loadAndAggregate).toHaveBeenCalledWith(mockSource);
+    expect(harness.mockedDiscovery.validateSource).toHaveBeenCalledWith('/custom/path');
+    expect(harness.mockedAggregator.loadAndAggregate).toHaveBeenCalledWith(mockSource);
   });
 
   it('should exit with error if no sources found', async () => {
-    mockedDiscovery.discoverLogSources.mockResolvedValue([]);
+    harness.mockedDiscovery.discoverLogSources.mockResolvedValue([]);
 
     const options: StatsCommandOptions = {
       format: 'pretty',
     };
 
     await expect(statsCommand(options)).rejects.toThrow('process.exit called');
-    expect(mockExit).toHaveBeenCalledWith(1);
+    expect(harness.mockExit).toHaveBeenCalledWith(1);
   });
 
   it('should exit with error if specified source is invalid', async () => {
-    mockedDiscovery.discoverLogSources.mockResolvedValue([]);
-    mockedDiscovery.validateSource.mockRejectedValue(new Error('Source not found'));
+    harness.mockedDiscovery.discoverLogSources.mockResolvedValue([]);
+    harness.mockedDiscovery.validateSource.mockRejectedValue(new Error('Source not found'));
 
     const options: StatsCommandOptions = {
       format: 'pretty',
       source: '/invalid/path',
     };
 
     await expect(statsCommand(options)).rejects.toThrow('process.exit called');
-    expect(mockExit).toHaveBeenCalledWith(1);
+    expect(harness.mockExit).toHaveBeenCalledWith(1);
   });
 
   it('should pass correct format to formatter', async () => {
     const mockSource: LogSource = { type: 'running', containerName: 'awf-squid' };
 
-    mockedDiscovery.discoverLogSources.mockResolvedValue([mockSource]);
-    mockedDiscovery.selectMostRecent.mockReturnValue(mockSource);
-    mockedAggregator.loadAndAggregate.mockResolvedValue({
+    harness.mockedDiscovery.discoverLogSources.mockResolvedValue([mockSource]);
+    harness.mockedDiscovery.selectMostRecent.mockReturnValue(mockSource);
+    harness.mockedAggregator.loadAndAggregate.mockResolvedValue({
       totalRequests: 0,
       allowedRequests: 0,
       deniedRequests: 0,
       uniqueDomains: 0,
       byDomain: new Map(),
       timeRange: null,
     });
-    mockedFormatter.formatStats.mockReturnValue('{}');
+    harness.mockedFormatter.formatStats.mockReturnValue('{}');
 
     await statsCommand({ format: 'json' });
-    expect(mockedFormatter.formatStats).toHaveBeenCalledWith(
+    expect(harness.mockedFormatter.formatStats).toHaveBeenCalledWith(
       expect.anything(),
       'json',
       expect.any(Boolean)
     );
 
-    mockedFormatter.formatStats.mockClear();
+    harness.mockedFormatter.formatStats.mockClear();
     await statsCommand({ format: 'markdown' });
-    expect(mockedFormatter.formatStats).toHaveBeenCalledWith(
+    expect(harness.mockedFormatter.formatStats).toHaveBeenCalledWith(
       expect.anything(),
       'markdown',
       expect.any(Boolean)
     );
 
-    mockedFormatter.formatStats.mockClear();
+    harness.mockedFormatter.formatStats.mockClear();
     await statsCommand({ format: 'pretty' });
-    expect(mockedFormatter.formatStats).toHaveBeenCalledWith(
+    expect(harness.mockedFormatter.formatStats).toHaveBeenCalledWith(
       expect.anything(),
       'pretty',
       expect.any(Boolean)
@@ -170,15 +151,50 @@ describe('logs-stats command', () => {
   it('should handle aggregation errors gracefully', async () => {
     const mockSource: LogSource = { type: 'running', containerName: 'awf-squid' };
 
-    mockedDiscovery.discoverLogSources.mockResolvedValue([mockSource]);
-    mockedDiscovery.selectMostRecent.mockReturnValue(mockSource);
-    mockedAggregator.loadAndAggregate.mockRejectedValue(new Error('Failed to load'));
+    harness.mockedDiscovery.discoverLogSources.mockResolvedValue([mockSource]);
+    harness.mockedDiscovery.selectMostRecent.mockReturnValue(mockSource);
+    harness.mockedAggregator.loadAndAggregate.mockRejectedValue(new Error('Failed to load'));
 
     const options: StatsCommandOptions = {
       format: 'pretty',
     };
 
     await expect(statsCommand(options)).rejects.toThrow('process.exit called');
-    expect(mockExit).toHaveBeenCalledWith(1);
+    expect(harness.mockExit).toHaveBeenCalledWith(1);
+  });
+
+  it('should emit source-selection info logs for non-JSON formats but suppress them for JSON', async () => {
+    const mockSource: LogSource = {
+      type: 'preserved',
+      path: '/tmp/squid-logs-123',
+      dateStr: 'Mon Jan 01 2024',
+    };
+    const emptyStats = {
+      totalRequests: 0,
+      allowedRequests: 0,
+      deniedRequests: 0,
+      uniqueDomains: 0,
+      byDomain: new Map(),
+      timeRange: null,
+    };
+
+    harness.mockedDiscovery.discoverLogSources.mockResolvedValue([mockSource]);
+    harness.mockedDiscovery.selectMostRecent.mockReturnValue(mockSource);
+    harness.mockedAggregator.loadAndAggregate.mockResolvedValue(emptyStats);
+    harness.mockedFormatter.formatStats.mockReturnValue('');
+
+    // pretty: shouldLog returns true → logger.info should be called
+    await statsCommand({ format: 'pretty' });
+    expect((logger.info as jest.Mock)).toHaveBeenCalled();
+    (logger.info as jest.Mock).mockClear();
+
+    // markdown: shouldLog returns true → logger.info should be called
+    await statsCommand({ format: 'markdown' });
+    expect((logger.info as jest.Mock)).toHaveBeenCalled();
+    (logger.info as jest.Mock).mockClear();
+
+    // json: shouldLog returns false → logger.info should NOT be called
+    await statsCommand({ format: 'json' });
+    expect((logger.info as jest.Mock)).not.toHaveBeenCalled();
   });
 });

src/commands/logs-stats.ts

@@ -3,11 +3,7 @@
  */
 
 import type { LogStatsFormat } from '../types';
-import { formatStats } from '../logs/stats-formatter';
-import {
-  discoverAndSelectSource,
-  loadLogsWithErrorHandling,
-} from './logs-command-helpers';
+import { runLogsCommand } from './logs-command-helpers';
 
 /**
  * Output format type for stats command (alias for shared type)
@@ -33,18 +29,6 @@ export interface StatsCommandOptions {
  * @param options - Command options
  */
 export async function statsCommand(options: StatsCommandOptions): Promise<void> {
-  // Discover and select log source
   // For stats command: show info logs for all non-JSON formats
-  const source = await discoverAndSelectSource(options.source, {
-    format: options.format,
-    shouldLog: (format) => format !== 'json',
-  });
-
-  // Load and aggregate logs
-  const stats = await loadLogsWithErrorHandling(source);
-
-  // Format and output
-  const colorize = !!(process.stdout.isTTY && options.format === 'pretty');
-  const output = formatStats(stats, options.format, colorize);
-  console.log(output);
+  await runLogsCommand(options, (format) => format !== 'json');
 }