fix: add error logging to all silent catch blocks#185
Merged
Conversation
HIGH (6): Add log.warn to user-visible silent failures: - getModelInfo, readPlan, deletePlan, getAuthStatus in session-manager - Mid-turn and command listModels in index.ts destroySession (8): Create safeDestroySession() helper withMEDIUM log.debug, replacing 8 identical silent catch blocks misc (11): Add log.debug/warn to:MEDIUM - listModels fire-and-forget (context cache, fallback, plan summarization) - getSessionMode fallbacks - Skill toggle RPCs - .env parse (ENOENT suppressed, other errors warned) - Config reload notification, cancelStream, plan surfacing LOW (12): Add log.debug to remaining best-effort catches: - Temp file cleanup, setTyping, addReaction - FS directory walks (MCP plugins, skill discovery) - SKILL.md reads, realpathSync, git diff in tool handlers Zero silent catch blocks remain in session-manager.ts and index.ts. Tests: 3 new tests for parseEnvFile error/ENOENT handling (683 total) Fixes #178 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds structured logging to previously-silent error-handling paths so operational failures are visible instead of being swallowed.
Changes:
- Added
log.debug/log.warnlogging to many previously emptycatch {}and.catch(() => {})call sites insrc/index.tsandsrc/core/session-manager.ts. - Introduced
SessionManager.safeDestroySession()to de-duplicate best-effortdestroySession()cleanup with logging. - Updated
parseEnvFile()to suppress expectedENOENTwhile logging other parse/read failures, plus added focused Vitest coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/index.ts |
Replaces multiple silent best-effort catches with debug/warn logging in runtime paths (message handling, scheduler, cleanup). |
src/core/session-manager.ts |
Adds logging to previously swallowed exceptions, introduces safeDestroySession(), and improves .env parsing error visibility (with ENOENT suppression). |
src/core/error-logging.test.ts |
Adds tests verifying .env parsing logs on non-ENOENT and stays quiet on ENOENT. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add logging to lock chain .catch() patterns (eventLocks, channelLocks) - Add logging to surfacePlanIfExists promise catch - Remove unused afterEach import in tests - Use os.tmpdir() for ENOENT test instead of hardcoded /tmp path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add error logging to all silent catch blocks across the codebase, ensuring no errors are silently swallowed.
What it does
catch {}block insession-manager.tsandindex.tssafeDestroySession()helper to DRY 8 identical destroy-and-ignore patternslog.warnfor user-visible failures,log.debugfor best-effort operationsparseEnvFile(expected when no .env exists) while logging other errorsKey changes
src/core/session-manager.ts: 6 HIGH catches (warn), 8 destroySession catches (safeDestroySession helper), 14 MEDIUM/LOW catches (debug/warn)src/index.ts: 2 HIGH catches (warn), 10 MEDIUM/LOW catches (debug)src/core/error-logging.test.ts: 3 new tests for parseEnvFile error handlingScope
Fixes #178