Browse Source

fix: add logging to critical empty catch blocks

umi008 3 weeks ago
parent
commit
04d3e0e187

+ 49 - 0
src/companion/manager.test.ts

@@ -620,4 +620,53 @@ describe('CompanionManager', () => {
       size: 'medium',
     });
   });
+
+  it('recovers from corrupt state file gracefully', () => {
+    const statePath = stateFilePath();
+    mkdirSync(path.dirname(statePath), { recursive: true });
+    writeFileSync(statePath, 'not-valid-json');
+
+    const m = make();
+    m.onLoad();
+
+    // Must gracefully degrade to default state
+    const state = readState();
+    expect(state.version).toBe(1);
+    expect(state.sessions).toHaveLength(1);
+  });
+
+  it('handles state write failure during disabled onLoad gracefully', () => {
+    const statePath = stateFilePath();
+    mkdirSync(path.dirname(statePath), { recursive: true });
+    writeFileSync(statePath, JSON.stringify({ version: 1, sessions: [] }));
+    const originalContent = readFileSync(statePath, 'utf8');
+    chmodSync(statePath, 0o444);
+
+    const m = new CompanionManager('test-disabled', '/path', {
+      enabled: false,
+      position: 'bottom-right',
+      size: 'medium',
+    });
+    m.onLoad();
+
+    // State file must be preserved despite write failure (catch swallowed the error)
+    chmodSync(statePath, 0o644);
+    expect(readFileSync(statePath, 'utf8')).toBe(originalContent);
+  });
+
+  it('handles empty state file gracefully', () => {
+    // readState catches JSON parse errors and returns a clean default state
+    const statePath = stateFilePath();
+    mkdirSync(path.dirname(statePath), { recursive: true });
+    writeFileSync(statePath, '');
+
+    const m = make();
+    expect(() => {
+      m.onLoad();
+    }).not.toThrow();
+
+    const state = readState();
+    expect(state.version).toBe(1);
+    expect(state.sessions).toHaveLength(1);
+  });
 });

+ 23 - 8
src/companion/manager.ts

@@ -103,7 +103,9 @@ function acquirePidFileLock(file: string): (() => void) | null {
       return () => {
         try {
           rmSync(lock, { recursive: true, force: true });
-        } catch {}
+        } catch (err) {
+          log('[companion] lock release failed', String(err));
+        }
       };
     } catch (err) {
       const code = (err as NodeJS.ErrnoException).code;
@@ -132,10 +134,13 @@ function pidFileLockHasLiveOwner(lock: string): boolean {
   try {
     const owner = parsePidFile(readFileSync(path.join(lock, 'owner'), 'utf8'));
     if (owner !== null) return isProcessAlive(owner);
-  } catch {
+  } catch (err) {
+    log('[companion] lock owner check failed', String(err));
     try {
       return Date.now() - statSync(lock).mtimeMs < 5000;
-    } catch {}
+    } catch (err) {
+      log('[companion] lock owner check failed', String(err));
+    }
   }
   return false;
 }
@@ -175,7 +180,9 @@ function readState(): CompanionState {
     if (parsed?.version === 1 && Array.isArray(parsed.sessions)) {
       return parsed as CompanionState;
     }
-  } catch {}
+  } catch (err) {
+    log('[companion] state load failed', String(err));
+  }
   return { version: 1, sessions: [] };
 }
 
@@ -206,7 +213,9 @@ function acquireStateLock(file: string): () => void {
       return () => {
         try {
           rmSync(lock, { recursive: true, force: true });
-        } catch {}
+        } catch (err) {
+          log('[companion] lock release failed', String(err));
+        }
       };
     } catch (err) {
       const code = (err as NodeJS.ErrnoException).code;
@@ -252,7 +261,9 @@ export class CompanionManager {
             (s) => s.session_id !== this.id,
           );
         });
-      } catch {}
+      } catch (err) {
+        log('[companion] status update failed', String(err));
+      }
       return;
     }
     this.registerActiveManager();
@@ -345,7 +356,9 @@ export class CompanionManager {
     if (activeManagers.size === 0 && activeExitListener) {
       try {
         process.removeListener('exit', activeExitListener);
-      } catch {}
+      } catch (err) {
+        log('[companion] exit listener removal failed', String(err));
+      }
       activeExitListener = null;
     }
     if (this.config?.enabled !== true) return;
@@ -508,7 +521,9 @@ export class CompanionManager {
       if (spawnedChild && !this.wasSpawner) {
         try {
           spawnedChild.kill();
-        } catch {}
+        } catch (killErr) {
+          log('[companion] spawn failed', String(killErr));
+        }
       }
       log('[companion] spawn guard failed', String(err));
     } finally {

+ 33 - 0
src/companion/updater.test.ts

@@ -286,6 +286,39 @@ describe('companion updater', () => {
       }
     }
   });
+
+  test('returns null on corrupt manifest without throwing', () => {
+    const brokenDir = path.join(TEST_DIR, 'broken-manifest');
+    mkdirSync(path.join(brokenDir, 'src', 'companion'), { recursive: true });
+    writeFileSync(
+      path.join(brokenDir, 'src', 'companion', 'companion-manifest.json'),
+      'not-json',
+    );
+
+    const result = loadCompanionManifestFromPackageRoot(brokenDir);
+
+    // Must gracefully return null instead of throwing
+    expect(result).toBeNull();
+  });
+
+  test('fails gracefully when metadata file is corrupt', () => {
+    const bin = getCompanionBinaryPath();
+    mkdirSync(path.dirname(bin), { recursive: true });
+    writeFileSync(`${bin}.json`, 'not-json');
+
+    const result = ensureCompanionVersion({
+      config: { enabled: true },
+      manifest: {
+        version: '0.2.0',
+        tag: 'companion-v0.2.0',
+        repo: 'owner/repo',
+        checksums: {},
+      },
+      lockTimeoutMs: 1,
+    });
+
+    expect(result).resolves.toMatchObject({ status: 'failed' });
+  });
 });
 
 function archiveName(version: string, target: string): string {

+ 12 - 4
src/companion/updater.ts

@@ -115,7 +115,9 @@ export function loadCompanionManifestFromPackageRoot(
         checksums: parsed.checksums,
       };
     }
-  } catch {}
+  } catch (err) {
+    log('[updater] manifest read failed', String(err));
+  }
   return null;
 }
 
@@ -348,7 +350,9 @@ async function installCompanionArchive(
     if (tempDir) {
       try {
         rmSync(tempDir, { recursive: true, force: true });
-      } catch {}
+      } catch (err) {
+        log('[updater] install cleanup failed', String(err));
+      }
     }
   }
 }
@@ -363,7 +367,9 @@ function readInstallMetadata(
     if (parsed?.version && parsed.tag && parsed.target) {
       return parsed as CompanionInstallMetadata;
     }
-  } catch {}
+  } catch (err) {
+    log('[updater] metadata read failed', String(err));
+  }
   return null;
 }
 
@@ -396,7 +402,9 @@ async function withCompanionInstallLock(
       } finally {
         try {
           rmSync(lock, { recursive: true, force: true });
-        } catch {}
+        } catch (err) {
+          log('[updater] lock release failed', String(err));
+        }
       }
     } catch (err) {
       const code = (err as NodeJS.ErrnoException).code;

+ 80 - 0
src/hooks/image-hook.test.ts

@@ -0,0 +1,80 @@
+import { afterAll, describe, expect, it } from 'bun:test';
+import {
+  chmodSync,
+  mkdirSync,
+  rmSync,
+  utimesSync,
+  writeFileSync,
+} from 'node:fs';
+import * as os from 'node:os';
+import * as path from 'node:path';
+
+import { processImageAttachments } from './image-hook';
+
+const TEST_DIR = path.join(os.tmpdir(), `image-hook-test-${process.pid}`);
+
+function makeTestDir(name: string): { workDir: string; saveDir: string } {
+  const workDir = path.join(TEST_DIR, name);
+  const saveDir = path.join(workDir, '.opencode', 'images');
+  mkdirSync(saveDir, { recursive: true });
+  return { workDir, saveDir };
+}
+
+function makeOldFile(dir: string, name: string): string {
+  const fp = path.join(dir, name);
+  writeFileSync(fp, 'data');
+  const past = new Date(Date.now() - 2 * 60 * 60 * 1000);
+  utimesSync(fp, past, past);
+  return fp;
+}
+
+describe('image-hook catch logging', () => {
+  afterAll(() => {
+    rmSync(TEST_DIR, { recursive: true, force: true });
+  });
+
+  it('survives file cleanup failure without throwing', () => {
+    const { workDir, saveDir } = makeTestDir('cleanup-fail-1');
+
+    makeOldFile(saveDir, 'old-image.png');
+
+    // Make the directory read-only to cause unlinkSync to fail
+    chmodSync(saveDir, 0o555);
+
+    try {
+      // Must not throw despite failed cleanup
+      expect(() => {
+        processImageAttachments({
+          messages: [],
+          workDir,
+          disabledAgents: new Set<string>(),
+          log: () => {},
+        });
+      }).not.toThrow();
+    } finally {
+      chmodSync(saveDir, 0o755);
+    }
+  });
+
+  it('survives subdirectory file cleanup failure without throwing', () => {
+    const { workDir, saveDir } = makeTestDir('cleanup-fail-2');
+    const sessionDir = path.join(saveDir, 'ses-abc');
+    mkdirSync(sessionDir, { recursive: true });
+    makeOldFile(sessionDir, 'img.png');
+
+    chmodSync(sessionDir, 0o555);
+
+    try {
+      expect(() => {
+        processImageAttachments({
+          messages: [],
+          workDir,
+          disabledAgents: new Set<string>(),
+          log: () => {},
+        });
+      }).not.toThrow();
+    } finally {
+      chmodSync(sessionDir, 0o755);
+    }
+  });
+});

+ 15 - 5
src/hooks/image-hook.ts

@@ -9,6 +9,7 @@ import {
   writeFileSync,
 } from 'node:fs';
 import { basename, extname, join } from 'node:path';
+import { log } from '../utils/logger';
 import { isUserMessageWithParts, type MessageWithParts } from './types';
 
 // Debounce: only run cleanup every 10 minutes per directory
@@ -81,10 +82,14 @@ function cleanupAllSessions(saveDir: string): void {
       } else {
         try {
           if (now - statSync(fp).mtimeMs > maxAge) unlinkSync(fp);
-        } catch {}
+        } catch (err) {
+          log('[image-hook] file cleanup failed', String(err));
+        }
       }
     }
-  } catch {}
+  } catch (err) {
+    log('[image-hook] directory scan failed', String(err));
+  }
 
   for (const dir of dirsToScan) {
     try {
@@ -99,7 +104,8 @@ function cleanupAllSessions(saveDir: string): void {
           } else {
             allRemoved = false;
           }
-        } catch {
+        } catch (err) {
+          log('[image-hook] file cleanup failed', String(err));
           allRemoved = false;
         }
       }
@@ -107,9 +113,13 @@ function cleanupAllSessions(saveDir: string): void {
       if (!isEmpty && allRemoved) {
         try {
           rmdirSync(dir);
-        } catch {}
+        } catch (err) {
+          log('[image-hook] directory removal failed', String(err));
+        }
       }
-    } catch {}
+    } catch (err) {
+      log('[image-hook] session cleanup failed', String(err));
+    }
   }
 }