feat(logging-p0): add screenshot capture and browser console diagnostics for ERP errors

Enhance ERP automation error diagnostics by capturing PNG screenshots
on every error and forwarding browser console warnings/errors to the
structured logger. Includes automatic cleanup of old screenshots
aligned with the configured log retention period.

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

src/main/services/erp/erp-auth.ts

@@ -2,6 +2,7 @@ import { chromium } from 'playwright'
 import type { ErpConfig, ErpSession } from '../../types/erp.types'
 import { createLogger } from '../logger'
 import { capturePageContext } from './erp-error-context'
+import { attachPageDiagnostics, attachContextDiagnostics } from './page-diagnostics'
 
 const log = createLogger('ErpAuthService')
 
@@ -55,6 +56,8 @@ export class ErpAuthService {
     })
 
     const page = await context.newPage()
+    attachPageDiagnostics(page)
+    attachContextDiagnostics(context)
 
     // Navigate to login page (use actual login URL from Python code)
     const loginUrl = `${this.config.url}/yonbip/resources/uap/rbac/login/main/index.html`

src/main/services/erp/erp-error-context.ts

@@ -6,12 +6,61 @@
  */
 
 import type { Page } from 'playwright'
+import fs from 'fs'
+import path from 'path'
+import { getLogDir } from '../logger/shared'
 
 export interface ErpErrorContext {
   pageUrl?: string
   frameHierarchy?: Array<{ name: string; url: string }>
   targetSelector?: string
   step?: string
+  screenshotPath?: string
+}
+
+/**
+ * Sanitize a step name for use as a filename component.
+ * Replaces non-alphanumeric characters with underscores and truncates.
+ */
+function sanitizeForFilename(step: string | undefined): string {
+  if (!step) return 'unknown'
+  return step.replace(/[^a-zA-Z0-9_-]/g, '_').slice(0, 40)
+}
+
+/**
+ * Capture a screenshot of the page for error diagnostics.
+ * Stored as PNG under <logDir>/screenshots/.
+ * Defensive: never throws.
+ */
+async function captureScreenshot(page: Page, step?: string): Promise<string | undefined> {
+  try {
+    if (page.isClosed()) return undefined
+
+    const screenshotDir = path.join(getLogDir(), 'screenshots')
+    fs.mkdirSync(screenshotDir, { recursive: true })
+
+    const now = new Date()
+    const timestamp = [
+      now.getFullYear(),
+      String(now.getMonth() + 1).padStart(2, '0'),
+      String(now.getDate()).padStart(2, '0'),
+      '_',
+      String(now.getHours()).padStart(2, '0'),
+      String(now.getMinutes()).padStart(2, '0'),
+      String(now.getSeconds()).padStart(2, '0')
+    ].join('')
+
+    const filename = `err_${timestamp}_${sanitizeForFilename(step)}.png`
+    const filePath = path.join(screenshotDir, filename)
+
+    const buffer = await page.screenshot({ type: 'png', timeout: 5000 })
+    fs.writeFileSync(filePath, buffer)
+
+    return filePath
+  } catch {
+    // screenshot failure must not propagate
+    return undefined
+  }
 }
 
 /**
@@ -49,5 +98,7 @@ export async function capturePageContext(
     ctx.step = step
   }
 
+  ctx.screenshotPath = await captureScreenshot(page, step)
+
   return ctx
 }

src/main/services/erp/page-diagnostics.ts

@@ -0,0 +1,50 @@
+/**
+ * Page Diagnostics
+ *
+ * Attaches browser console and error listeners to Playwright pages
+ * so that ERP-side JS errors are visible in the application log.
+ *
+ * Only warning and error level console messages are captured —
+ * ERP (YonBIP) outputs large volumes of info-level messages that
+ * would drown the log.
+ */
+
+import type { Page, BrowserContext } from 'playwright'
+import { createLogger } from '../logger'
+
+const log = createLogger('BrowserDiagnostics')
+
+/**
+ * Attach console and error listeners to a single page.
+ */
+export function attachPageDiagnostics(page: Page): void {
+  page.on('console', (msg) => {
+    const type = msg.type()
+    if (type !== 'warning' && type !== 'error') return
+
+    const location = msg.location()
+    log.error(`[Browser ${type}] ${msg.text()}`, {
+      pageUrl: page.url(),
+      consoleType: type,
+      location: location ? `${location.url}:${location.lineNumber}` : undefined
+    })
+  })
+
+  page.on('pageerror', (error) => {
+    log.error(`[Browser pageerror] ${error.message}`, {
+      pageUrl: page.url(),
+      error: error.message,
+      stack: error.stack
+    })
+  })
+}
+
+/**
+ * Attach diagnostics to all current and future pages in a browser context.
+ * Covers popups and pages opened by ERP automation.
+ */
+export function attachContextDiagnostics(context: BrowserContext): void {
+  context.on('page', (page) => {
+    attachPageDiagnostics(page)
+  })
+}

src/main/services/logger/index.ts

@@ -13,7 +13,7 @@ import DailyRotateFile from 'winston-daily-rotate-file'
 import path from 'path'
 import { BrowserWindow } from 'electron'
 import { serializeError, sanitizeError } from './error-utils'
-import { getLogDir, isProduction } from './shared'
+import { getLogDir, isProduction, cleanupOldScreenshots } from './shared'
 import { IPC_CHANNELS } from '../../../shared/ipc-channels'
 import { getContext, run } from './request-context'
 
@@ -212,6 +212,9 @@ export function applyLoggingConfig(config: { level: string; appRetention: number
       })
     )
   }
+
+  // Clean up old screenshot files beyond the retention window
+  cleanupOldScreenshots(config.appRetention)
 }
 
 /**

src/main/services/logger/shared.ts

@@ -58,3 +58,35 @@ export const LOG_LEVEL_PRIORITY: Record<string, number> = {
 export function isLoggable(level: string, threshold: string): boolean {
   return (LOG_LEVEL_PRIORITY[level] ?? 0) >= (LOG_LEVEL_PRIORITY[threshold] ?? 2)
 }
+
+/**
+ * Delete screenshot files older than the given retention period.
+ * Scans <logDir>/screenshots/ and removes .png files whose mtime exceeds
+ * the retention window. Defensive: never throws.
+ *
+ * @param retentionDays - Number of days to keep screenshots
+ */
+export function cleanupOldScreenshots(retentionDays: number): void {
+  try {
+    const screenshotDir = path.join(getLogDir(), 'screenshots')
+    if (!fs.existsSync(screenshotDir)) return
+
+    const files = fs.readdirSync(screenshotDir)
+    const cutoff = Date.now() - retentionDays * 24 * 60 * 60 * 1000
+
+    for (const file of files) {
+      if (!file.endsWith('.png')) continue
+      const filePath = path.join(screenshotDir, file)
+      try {
+        const stat = fs.statSync(filePath)
+        if (stat.mtimeMs < cutoff) {
+          fs.unlinkSync(filePath)
+        }
+      } catch {
+        // individual file deletion failure should not stop the loop
+      }
+    }
+  } catch {
+    // entire cleanup is best-effort
+  }
+}

tests/unit/logger-integration.test.ts

@@ -3,7 +3,10 @@
  * Verifies RequestContext is properly integrated with Logger
  */
 
-import { describe, it, expect } from 'vitest'
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
+import fs from 'fs'
+import path from 'path'
+import { getLogDir } from '../../src/main/services/logger/shared'
 
 describe('Logger RequestContext Integration', () => {
   it('should export run from request-context', async () => {
@@ -52,3 +55,81 @@ describe('Logger RequestContext Integration', () => {
     expect(loggerModule.createLogger).toBeDefined()
   })
 })
+
+describe('cleanupOldScreenshots', () => {
+  let unlinkSyncSpy: ReturnType<typeof vi.spyOn>
+
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  afterEach(() => {
+    vi.restoreAllMocks()
+  })
+
+  it('should delete PNG files older than retention period', async () => {
+    const { cleanupOldScreenshots } = await import('../../src/main/services/logger/shared')
+    const screenshotDir = path.join(getLogDir(), 'screenshots')
+
+    const now = Date.now()
+    const oldTime = now - 20 * 24 * 60 * 60 * 1000 // 20 days ago
+    const recentTime = now - 5 * 24 * 60 * 60 * 1000 // 5 days ago
+
+    vi.spyOn(fs, 'existsSync').mockImplementation((p) => {
+      if (typeof p === 'string' && p.includes('screenshots')) return true
+      return false
+    })
+
+    vi.spyOn(fs, 'readdirSync').mockReturnValue(['err_old.png', 'err_recent.png'] as any)
+
+    vi.spyOn(fs, 'statSync').mockImplementation((p) => {
+      if (typeof p === 'string' && p.includes('err_old')) {
+        return { mtimeMs: oldTime } as any
+      }
+      return { mtimeMs: recentTime } as any
+    })
+
+    unlinkSyncSpy = vi.spyOn(fs, 'unlinkSync').mockReturnValue(undefined)
+
+    cleanupOldScreenshots(14)
+
+    // Only the old file should be deleted
+    expect(unlinkSyncSpy).toHaveBeenCalledTimes(1)
+    expect(unlinkSyncSpy).toHaveBeenCalledWith(path.join(screenshotDir, 'err_old.png'))
+  })
+
+  it('should not delete anything if screenshots directory does not exist', async () => {
+    const { cleanupOldScreenshots } = await import('../../src/main/services/logger/shared')
+
+    vi.spyOn(fs, 'existsSync').mockReturnValue(false)
+    const readdirSyncSpy = vi.spyOn(fs, 'readdirSync')
+
+    cleanupOldScreenshots(14)
+
+    expect(readdirSyncSpy).not.toHaveBeenCalled()
+  })
+
+  it('should skip non-PNG files', async () => {
+    const { cleanupOldScreenshots } = await import('../../src/main/services/logger/shared')
+    const screenshotDir = path.join(getLogDir(), 'screenshots')
+
+    const now = Date.now()
+    const oldTime = now - 20 * 24 * 60 * 60 * 1000
+
+    vi.spyOn(fs, 'existsSync').mockImplementation((p) => {
+      if (typeof p === 'string' && p.includes('screenshots')) return true
+      return false
+    })
+
+    vi.spyOn(fs, 'readdirSync').mockReturnValue(['notes.txt', 'data.json', 'err_old.png'] as any)
+
+    vi.spyOn(fs, 'statSync').mockReturnValue({ mtimeMs: oldTime } as any)
+    unlinkSyncSpy = vi.spyOn(fs, 'unlinkSync').mockReturnValue(undefined)
+
+    cleanupOldScreenshots(14)
+
+    // Only the .png file should be deleted
+    expect(unlinkSyncSpy).toHaveBeenCalledTimes(1)
+    expect(unlinkSyncSpy).toHaveBeenCalledWith(path.join(screenshotDir, 'err_old.png'))
+  })
+})

tests/unit/services/erp/erp-error-context.test.ts

@@ -0,0 +1,133 @@
+/**
+ * Tests for ERP Error Context Capture with screenshot support
+ */
+
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
+import fs from 'fs'
+import path from 'path'
+
+// Mock logger/shared before importing the module under test
+vi.mock('../../../../src/main/services/logger/shared', () => ({
+  getLogDir: vi.fn(() => '/tmp/test-logs')
+}))
+
+// Mock fs
+vi.mock('fs', () => ({
+  default: {
+    mkdirSync: vi.fn(),
+    writeFileSync: vi.fn(),
+    existsSync: vi.fn(() => true),
+    readdirSync: vi.fn(() => []),
+    statSync: vi.fn()
+  }
+}))
+
+import { capturePageContext } from '../../../../src/main/services/erp/erp-error-context'
+
+function createMockPage(overrides: Record<string, unknown> = {}) {
+  return {
+    url: vi.fn(() => 'https://erp.example.com/page'),
+    frames: vi.fn(() => [
+      { name: () => 'main', url: () => 'https://erp.example.com/main' },
+      { name: () => 'forwardFrame', url: () => 'https://erp.example.com/frame' }
+    ]),
+    isClosed: vi.fn(() => false),
+    screenshot: vi.fn(() => Buffer.from('fake-png-data')),
+    ...overrides
+  } as any
+}
+
+describe('capturePageContext', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    vi.mocked(fs.mkdirSync).mockReturnValue(undefined)
+    vi.mocked(fs.writeFileSync).mockReturnValue(undefined)
+  })
+
+  afterEach(() => {
+    vi.restoreAllMocks()
+  })
+
+  it('should capture screenshot and return screenshotPath on success', async () => {
+    const page = createMockPage()
+
+    const ctx = await capturePageContext(page, '#selector', 'login.username')
+
+    expect(ctx.screenshotPath).toBeDefined()
+    expect(ctx.screenshotPath).toContain('err_')
+    expect(ctx.screenshotPath).toContain('.png')
+    expect(ctx.screenshotPath).toContain('login_username')
+    expect(fs.writeFileSync).toHaveBeenCalledWith(
+      expect.stringContaining('.png'),
+      expect.any(Buffer)
+    )
+  })
+
+  it('should return screenshotPath undefined when page is closed', async () => {
+    const page = createMockPage({ isClosed: vi.fn(() => true) })
+
+    const ctx = await capturePageContext(page)
+
+    expect(ctx.screenshotPath).toBeUndefined()
+    expect(fs.writeFileSync).not.toHaveBeenCalled()
+  })
+
+  it('should still return other fields when screenshot fails', async () => {
+    const page = createMockPage({
+      screenshot: vi.fn(() => {
+        throw new Error('screenshot timeout')
+      })
+    })
+
+    const ctx = await capturePageContext(page, '#target', 'test-step')
+
+    // Other fields should still be populated
+    expect(ctx.pageUrl).toBe('https://erp.example.com/page')
+    expect(ctx.frameHierarchy).toHaveLength(2)
+    expect(ctx.targetSelector).toBe('#target')
+    expect(ctx.step).toBe('test-step')
+    // Screenshot should be undefined due to failure
+    expect(ctx.screenshotPath).toBeUndefined()
+  })
+
+  it('should sanitize special characters in step name for filename', async () => {
+    const page = createMockPage()
+
+    const ctx = await capturePageContext(page, undefined, 'login/user:test*step')
+
+    expect(ctx.screenshotPath).toBeDefined()
+    expect(ctx.screenshotPath).toContain('login_user_test_step')
+  })
+
+  it('should use "unknown" in filename when step is not provided', async () => {
+    const page = createMockPage()
+
+    const ctx = await capturePageContext(page)
+
+    expect(ctx.screenshotPath).toBeDefined()
+    expect(ctx.screenshotPath).toContain('unknown')
+  })
+
+  it('should truncate long step names to 40 characters', async () => {
+    const page = createMockPage()
+    const longStep = 'a'.repeat(80)
+
+    const ctx = await capturePageContext(page, undefined, longStep)
+
+    expect(ctx.screenshotPath).toBeDefined()
+    const filename = path.basename(ctx.screenshotPath!)
+    // Extract step portion: err_YYYYMMDD_HHmmss_<step>.png
+    const stepPart = filename.replace(/^err_\d{8}_\d{6}_/, '').replace(/\.png$/, '')
+    expect(stepPart.length).toBe(40)
+  })
+
+  it('should create screenshots directory if it does not exist', async () => {
+    const page = createMockPage()
+
+    await capturePageContext(page)
+
+    expect(fs.mkdirSync).toHaveBeenCalledWith(expect.stringContaining('screenshots'), {
+      recursive: true
+    })
+  })
+})

tests/unit/services/erp/page-diagnostics.test.ts

@@ -0,0 +1,184 @@
+/**
+ * Tests for Page Diagnostics module
+ */
+
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+
+// vi.hoisted() runs before vi.mock() factories, even though both are hoisted.
+const mockLog = vi.hoisted(() => ({
+  error: vi.fn(),
+  warn: vi.fn(),
+  info: vi.fn(),
+  debug: vi.fn()
+}))
+
+vi.mock('../../../../src/main/services/logger', () => ({
+  createLogger: () => mockLog
+}))
+
+import {
+  attachPageDiagnostics,
+  attachContextDiagnostics
+} from '../../../../src/main/services/erp/page-diagnostics'
+
+describe('page-diagnostics', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  describe('attachPageDiagnostics', () => {
+    it('should log console warning messages', () => {
+      const listeners: Record<string, ((...args: any[]) => any)[]> = {}
+      const page = {
+        on: vi.fn((event: string, handler: (...args: any[]) => any) => {
+          if (!listeners[event]) listeners[event] = []
+          listeners[event].push(handler)
+        }),
+        url: vi.fn(() => 'https://erp.example.com/page')
+      } as any
+
+      attachPageDiagnostics(page)
+
+      const consoleHandlers = listeners['console'] ?? []
+      expect(consoleHandlers).toHaveLength(1)
+
+      consoleHandlers[0]({
+        type: () => 'warning',
+        text: () => 'Something suspicious',
+        location: () => ({ url: 'app.js', lineNumber: 42 })
+      })
+
+      expect(mockLog.error).toHaveBeenCalledWith(
+        expect.stringContaining('Browser warning'),
+        expect.objectContaining({
+          pageUrl: 'https://erp.example.com/page',
+          consoleType: 'warning'
+        })
+      )
+    })
+
+    it('should log console error messages', () => {
+      const listeners: Record<string, ((...args: any[]) => any)[]> = {}
+      const page = {
+        on: vi.fn((event: string, handler: (...args: any[]) => any) => {
+          if (!listeners[event]) listeners[event] = []
+          listeners[event].push(handler)
+        }),
+        url: vi.fn(() => 'https://erp.example.com/page')
+      } as any
+
+      attachPageDiagnostics(page)
+
+      const consoleHandlers = listeners['console'] ?? []
+      consoleHandlers[0]({
+        type: () => 'error',
+        text: () => 'Uncaught TypeError',
+        location: () => ({ url: 'vendor.js', lineNumber: 100 })
+      })
+
+      expect(mockLog.error).toHaveBeenCalledWith(
+        expect.stringContaining('Browser error'),
+        expect.objectContaining({
+          consoleType: 'error'
+        })
+      )
+    })
+
+    it('should NOT log console info messages', () => {
+      const listeners: Record<string, ((...args: any[]) => any)[]> = {}
+      const page = {
+        on: vi.fn((event: string, handler: (...args: any[]) => any) => {
+          if (!listeners[event]) listeners[event] = []
+          listeners[event].push(handler)
+        }),
+        url: vi.fn(() => 'https://erp.example.com/page')
+      } as any
+
+      attachPageDiagnostics(page)
+
+      const consoleHandlers = listeners['console'] ?? []
+      consoleHandlers[0]({
+        type: () => 'info',
+        text: () => 'XHR loaded',
+        location: () => ({ url: 'app.js', lineNumber: 10 })
+      })
+
+      expect(mockLog.error).not.toHaveBeenCalled()
+    })
+
+    it('should NOT log console log messages', () => {
+      const listeners: Record<string, ((...args: any[]) => any)[]> = {}
+      const page = {
+        on: vi.fn((event: string, handler: (...args: any[]) => any) => {
+          if (!listeners[event]) listeners[event] = []
+          listeners[event].push(handler)
+        }),
+        url: vi.fn(() => 'https://erp.example.com/page')
+      } as any
+
+      attachPageDiagnostics(page)
+
+      const consoleHandlers = listeners['console'] ?? []
+      consoleHandlers[0]({
+        type: () => 'log',
+        text: () => 'debug output',
+        location: () => null
+      })
+
+      expect(mockLog.error).not.toHaveBeenCalled()
+    })
+
+    it('should log pageerror events', () => {
+      const listeners: Record<string, ((...args: any[]) => any)[]> = {}
+      const page = {
+        on: vi.fn((event: string, handler: (...args: any[]) => any) => {
+          if (!listeners[event]) listeners[event] = []
+          listeners[event].push(handler)
+        }),
+        url: vi.fn(() => 'https://erp.example.com/page')
+      } as any
+
+      attachPageDiagnostics(page)
+
+      const pageErrorHandlers = listeners['pageerror'] ?? []
+      expect(pageErrorHandlers).toHaveLength(1)
+
+      pageErrorHandlers[0](new Error('Script error on page'))
+
+      expect(mockLog.error).toHaveBeenCalledWith(
+        expect.stringContaining('Browser pageerror'),
+        expect.objectContaining({
+          pageUrl: 'https://erp.example.com/page',
+          error: 'Script error on page'
+        })
+      )
+    })
+  })
+
+  describe('attachContextDiagnostics', () => {
+    it('should auto-attach diagnostics to new pages in the context', () => {
+      const contextListeners: Record<string, ((...args: any[]) => any)[]> = {}
+      const context = {
+        on: vi.fn((event: string, handler: (...args: any[]) => any) => {
+          if (!contextListeners[event]) contextListeners[event] = []
+          contextListeners[event].push(handler)
+        })
+      } as any
+
+      attachContextDiagnostics(context)
+
+      const pageHandlers = contextListeners['page'] ?? []
+      expect(pageHandlers).toHaveLength(1)
+
+      const newPage = {
+        on: vi.fn(),
+        url: vi.fn(() => 'https://erp.example.com/popup')
+      } as any
+
+      pageHandlers[0](newPage)
+
+      expect(newPage.on).toHaveBeenCalledWith('console', expect.any(Function))
+      expect(newPage.on).toHaveBeenCalledWith('pageerror', expect.any(Function))
+    })
+  })
+})