Skip to content

fix: add error logging to all silent catch blocks #185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ChrisRomp merged 2 commits into from
Apr 2, 2026
Merged

fix: add error logging to all silent catch blocks #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9b8edc
fix: add error logging to all silent catch blocks (#178)
ChrisRomp Apr 2, 2026
b6f2ec1
fix: address CCR feedback on error logging PR
ChrisRomp Apr 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions src/core/error-logging.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
import path from 'node:path';
import os from 'node:os';
import fs from 'node:fs';

// Capture the logger mock so we can assert on calls
const mockLog = { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() };
vi.mock('../logger.js', () => ({
createLogger: () => mockLog,
setLogLevel: vi.fn(),
initLogFile: vi.fn(),
}));

// Must import after mocking
const { parseEnvFile } = await import('./session-manager.js');

describe('error logging', () => {
beforeEach(() => {
vi.clearAllMocks();
});

describe('parseEnvFile', () => {
it('does not log on ENOENT (missing file is expected)', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'env-test-'));
const missingPath = path.join(tmpDir, 'nonexistent.env');
const result = parseEnvFile(missingPath);
expect(result).toEqual({});
expect(mockLog.warn).not.toHaveBeenCalled();
fs.rmSync(tmpDir, { recursive: true, force: true });
});

it('logs warn on non-ENOENT errors', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'env-test-'));
const dirPath = path.join(tmpDir, 'a-directory');
fs.mkdirSync(dirPath);

// Reading a directory as a file throws EISDIR, not ENOENT
const result = parseEnvFile(dirPath);
expect(result).toEqual({});
expect(mockLog.warn).toHaveBeenCalledTimes(1);
expect(mockLog.warn).toHaveBeenCalledWith(
expect.stringContaining('Failed to parse .env file'),
expect.anything(),
);

fs.rmSync(tmpDir, { recursive: true, force: true });
});

it('parses valid .env file without warnings', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'env-test-'));
const envPath = path.join(tmpDir, '.env');
fs.writeFileSync(envPath, 'FOO=bar\nBAZ="quoted"\n# comment\n');

const result = parseEnvFile(envPath);
expect(result).toEqual({ FOO: 'bar', BAZ: 'quoted' });
expect(mockLog.warn).not.toHaveBeenCalled();

fs.rmSync(tmpDir, { recursive: true, force: true });
});
});
});
75 changes: 47 additions & 28 deletions src/core/session-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@ export function parseEnvFile(filePath: string): Record<string, string> {
if (key) vars[key] = value;
}
return vars;
} catch {
} catch (err: any) {
if (err?.code !== 'ENOENT') {
log.warn(`Failed to parse .env file ${filePath}:`, err);
}
return {};
}
}
Expand Down Expand Up @@ -158,7 +161,7 @@ function loadMcpServers(): Record<string, any> {
walk(full);
}
}
} catch { /* permission errors etc */ }
} catch (err) { log.debug('FS walk permission error:', err); }
};
walk(pluginsDir);
}
Expand Down Expand Up @@ -345,7 +348,7 @@ function discoverSkillDirectories(workingDirectory: string): string[] {
walk(full, depth + 1);
}
}
} catch { /* permission errors etc */ }
} catch (err) { log.debug('Skill dir walk permission error:', err); }
};
walk(pluginsDir, 0);
}
Expand All @@ -360,7 +363,7 @@ function discoverSkillDirectories(workingDirectory: string): string[] {
dirs.push(path.join(skillsRoot, entry.name));
}
}
} catch { /* permission errors etc */ }
} catch (err) { log.debug('Skill root readdir failed:', err); }
}

if (dirs.length > 0) {
Expand Down Expand Up @@ -440,6 +443,15 @@ export class SessionManager {
ensureWorkspacesDir();
}

/** Best-effort session destroy with logging. */
private async safeDestroySession(sessionId: string): Promise<void> {
try {
await this.bridge.destroySession(sessionId);
} catch (err) {
log.debug(`destroySession(${sessionId.slice(0, 8)}) failed (best-effort):`, err);
}
}

/** Resolve hooks for a workspace, caching the result. */
private async resolveHooks(workingDirectory: string): Promise<SessionHooks | undefined> {
const cached = this.workspaceHooks.get(workingDirectory);
Expand Down Expand Up @@ -594,7 +606,7 @@ export class SessionManager {
const content = fs.readFileSync(skillFile, 'utf8');
const descMatch = content.match(/^description:\s*["']?(.+?)["']?\s*$/m);
if (descMatch) description = descMatch[1];
} catch { /* skip */ }
} catch (err) { log.debug(`Failed to read SKILL.md for ${name}:`, err); }
}

skills.push({ name, description, source, pending: sessionDirs ? !sessionDirs.has(dir) : undefined, disabled: disabledSet.has(name) });
Expand Down Expand Up @@ -657,7 +669,7 @@ export class SessionManager {
if (unsub) { unsub(); this.sessionUnsubscribes.delete(existingId); }
try {
await this.bridge.destroySession(existingId);
} catch { /* best-effort */ }
} catch (err) { log.debug(`destroySession(${existingId.slice(0, 8)}) failed (best-effort):`, err); }
this.channelSessions.delete(channelId);
this.sessionChannels.delete(existingId);
this.contextUsage.delete(channelId);
Expand Down Expand Up @@ -686,7 +698,7 @@ export class SessionManager {
// in-memory state (including MCP connections), allowing a clean re-init.
const unsub = this.sessionUnsubscribes.get(existingId);
if (unsub) { unsub(); this.sessionUnsubscribes.delete(existingId); }
try { await this.bridge.destroySession(existingId); } catch { /* best-effort */ }
await this.safeDestroySession(existingId);

// Re-read global MCP servers so /reload picks up user-level config changes
this.mcpServers = loadMcpServers();
Expand Down Expand Up @@ -733,7 +745,7 @@ export class SessionManager {
if (existingId) {
const unsub = this.sessionUnsubscribes.get(existingId);
if (unsub) { unsub(); this.sessionUnsubscribes.delete(existingId); }
try { await this.bridge.destroySession(existingId); } catch { /* best-effort */ }
await this.safeDestroySession(existingId);
this.channelSessions.delete(channelId);
this.sessionChannels.delete(existingId);
this.contextUsage.delete(channelId);
Expand All @@ -751,7 +763,7 @@ export class SessionManager {
if (otherChannel) {
const unsub = this.sessionUnsubscribes.get(targetSessionId);
if (unsub) { unsub(); this.sessionUnsubscribes.delete(targetSessionId); }
try { await this.bridge.destroySession(targetSessionId); } catch { /* best-effort */ }
await this.safeDestroySession(targetSessionId);
this.channelSessions.delete(otherChannel);
this.sessionChannels.delete(targetSessionId);
this.contextUsage.delete(otherChannel);
Expand Down Expand Up @@ -805,7 +817,7 @@ export class SessionManager {
try {
const models = await this.bridge.listModels(getConfig().providers);
availableModels = models.map(m => m.id);
} catch { /* best-effort */ }
} catch (err) { log.debug('listModels for fallback chain failed:', err); }

const byokPrefixes = Object.keys(getConfig().providers ?? {});
const chain = buildFallbackChain(prefs.model, availableModels, configFallbacks, byokPrefixes);
Expand Down Expand Up @@ -845,7 +857,7 @@ export class SessionManager {
log.info(`Attempting to re-attach session ${sessionId}...`);
const unsub = this.sessionUnsubscribes.get(sessionId);
if (unsub) { unsub(); this.sessionUnsubscribes.delete(sessionId); }
try { await this.bridge.destroySession(sessionId); } catch { /* best-effort */ }
await this.safeDestroySession(sessionId);
await this.attachSession(channelId, sessionId);
const reconnected = this.bridge.getSession(sessionId);
if (reconnected) {
Expand Down Expand Up @@ -918,7 +930,7 @@ export class SessionManager {
try {
const unsub = this.sessionUnsubscribes.get(sessionId);
if (unsub) { unsub(); this.sessionUnsubscribes.delete(sessionId); }
try { await this.bridge.destroySession(sessionId); } catch { /* best-effort */ }
await this.safeDestroySession(sessionId);
await this.attachSession(channelId, sessionId);
} catch (attachErr: any) {
log.warn(`Re-attach failed for ${sessionId}:`, attachErr?.message ?? attachErr);
Expand Down Expand Up @@ -1060,7 +1072,7 @@ export class SessionManager {
if (prefs.model === model) {
await this.cacheContextWindowTokens(channelId, model, models);
}
}).catch(() => { /* best-effort */ });
}).catch((err) => { log.debug("listModels (context cache) failed:", err); });
}

/** Check if two models on the same provider have different wireApi settings. */
Expand Down Expand Up @@ -1125,7 +1137,8 @@ export class SessionManager {
try {
const models = await this.bridge.listModels(getConfig().providers);
return models.find(m => m.id === modelId) ?? null;
} catch {
} catch (err) {
log.warn('getModelInfo: listModels failed:', err);
return null;
}
}
Expand All @@ -1142,7 +1155,7 @@ export class SessionManager {
try {
const result = await this.withSessionRetry(channelId, (sid) => this.bridge.getSessionMode(sid), false);
return result.mode;
} catch { /* fall through to prefs */ }
} catch (err) { log.debug(`getSessionMode(${channelId.slice(0, 8)}) failed, falling through to prefs:`, err); }
}
const prefs = await getChannelPrefs(channelId);
return prefs?.sessionMode ?? 'interactive';
Expand All @@ -1163,7 +1176,8 @@ export class SessionManager {
try {
const result = await this.withSessionRetry(channelId, (sid) => this.bridge.readPlan(sid), false);
return { exists: result.exists, content: result.content };
} catch {
} catch (err) {
log.warn(`readPlan failed for ${channelId.slice(0, 8)}:`, err);
return { exists: false, content: null };
}
}
Expand All @@ -1175,7 +1189,8 @@ export class SessionManager {
try {
await this.withSessionRetry(channelId, (sid) => this.bridge.deletePlan(sid), false);
return true;
} catch {
} catch (err) {
log.warn(`deletePlan failed for ${channelId.slice(0, 8)}:`, err);
return false;
}
}
Expand Down Expand Up @@ -1217,7 +1232,7 @@ export class SessionManager {
try {
const models = await this.bridge.listModels(getConfig().providers);
availableIds = models.map(m => m.id);
} catch { /* best-effort */ }
} catch (err) { log.debug('listModels for plan summarization failed:', err); }

let model: string | undefined;
for (const candidate of SessionManager.SUMMARIZER_MODELS) {
Expand All @@ -1244,7 +1259,7 @@ export class SessionManager {
return null;
} finally {
if (session) {
try { await this.bridge.destroySession(session.sessionId); } catch { /* best-effort */ }
await this.safeDestroySession(session.sessionId);
}
}
}
Expand All @@ -1262,7 +1277,7 @@ export class SessionManager {
try {
const mode = await this.getSessionMode(channelId);
inPlanMode = mode === 'plan';
} catch { /* best-effort */ }
} catch (err) { log.debug(`getSessionMode for plan surfacing (${channelId.slice(0, 8)}) failed:`, err); }

return { exists: true, summary, inPlanMode };
}
Expand All @@ -1271,7 +1286,8 @@ export class SessionManager {
async getAuthStatus(): Promise<{ isAuthenticated: boolean; statusMessage?: string; login?: string }> {
try {
return await this.bridge.getAuthStatus();
} catch {
} catch (err) {
log.warn('getAuthStatus failed:', err);
return { isAuthenticated: false, statusMessage: 'Unable to check auth status' };
}
}
Expand Down Expand Up @@ -1571,8 +1587,8 @@ export class SessionManager {
try {
modelList = await this.bridge.listModels(getConfig().providers);
availableModels = modelList.map(m => m.id);
} catch {
log.warn('Failed to fetch model list for fallback resolution');
} catch (err) {
log.warn('Failed to fetch model list for fallback resolution:', err);
}

const resolvedMcpServers = this.resolveMcpServers(workingDirectory);
Expand Down Expand Up @@ -1725,7 +1741,7 @@ export class SessionManager {
if (currentPrefs.model === resumeModel) {
await this.cacheContextWindowTokens(channelId, resumeModel, models);
}
}).catch(() => { /* best-effort */ });
}).catch((err) => { log.debug("listModels (context cache) failed:", err); });
}

/**
Expand Down Expand Up @@ -1850,7 +1866,7 @@ export class SessionManager {

} finally {
if (session) {
try { await this.bridge.destroySession(session.sessionId); } catch { /* best-effort */ }
await this.safeDestroySession(session.sessionId);
}
}
}
Expand Down Expand Up @@ -2115,7 +2131,8 @@ export class SessionManager {
let realPath: string;
try {
realPath = fs.realpathSync(resolved);
} catch {
} catch (err) {
log.debug(`send_file: realpathSync failed for "${resolved}":`, err);
return { content: 'File not found.' };
}
// Validate the real file path is within workspace or allowed paths
Expand Down Expand Up @@ -2164,7 +2181,8 @@ export class SessionManager {
let realPath: string;
try {
realPath = fs.realpathSync(resolved);
} catch {
} catch (err) {
log.debug(`show_file: realpathSync failed for "${resolved}":`, err);
return { content: 'File not found.' };
}
const allowed = [showWorkDir, ...showAllowPaths];
Expand All @@ -2187,7 +2205,8 @@ export class SessionManager {
try {
content = execFileSync('git', ['diff', '--', realPath], { cwd: dir, encoding: 'utf-8', timeout: 5000 });
if (!content.trim()) content = '(no pending changes)';
} catch {
} catch (err) {
log.debug(`show_file: git diff failed for "${realPath}":`, err);
content = '(not a git repository or git diff failed)';
}
await adapter.sendMessage(channelId, `**${fileName}** (diff)\n\`\`\`\`diff\n${content}\n\`\`\`\``);
Expand Down
Loading
Loading