fix: replace bare except with except Exception#172
Conversation
📝 WalkthroughWalkthroughThis pull request refines exception handling specificity across three modules. Tool argument parsing in the chatbot now catches only Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/main_chatbot.py (1)
406-409: Narrow JSON parse fallback tojson.JSONDecodeError(Lines 408, 506).
except Exceptionis broader than needed for this fallback path and can mask unrelated bugs.Proposed refactor
- except Exception: + except json.JSONDecodeError: tool_input = tool_args- except Exception: + except json.JSONDecodeError: tool_input = tool_argsAlso applies to: 504-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/main_chatbot.py` around lines 406 - 409, The current broad except Exception around json.loads should be narrowed to catching json.JSONDecodeError so unrelated errors aren't swallowed; update the try/except that sets tool_input from tool_args (the block referencing tool_input and tool_args) to except json.JSONDecodeError: tool_input = tool_args, and make the same change to the analogous try/except later (the second occurrence around lines 504-507) so only JSON parse failures fall back to the original value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/utils/db/connection_pool.py`:
- Around line 88-89: Replace the silent broad except in the connection rollback
handler (currently "except BaseException: pass" in connection_pool.py) with a
targeted except Exception that logs the failure (use the module's logger via
logger.exception or logger.error with exception info) and include context about
which connection/transaction failed; also make the same change for the analogous
rollback error handler in db_adapters.py—do not catch BaseException so
KeyboardInterrupt/SystemExit aren’t swallowed, and ensure the log message
includes the exception details for observability.
In `@server/utils/db/db_adapters.py`:
- Around line 148-149: Replace the silent catch-all in the two rollback handlers
that currently use "except BaseException: pass" with a proper "except Exception
as e" that logs the error; locate the rollback handler functions in
server/utils/db/db_adapters.py (the handlers catching BaseException during
rollback) and change the handler to catch Exception and call the module/class
logger (e.g., logger.exception(...) or self._logger.exception(...)) with a clear
message including the exception so rollback failures are recorded.
---
Nitpick comments:
In `@server/main_chatbot.py`:
- Around line 406-409: The current broad except Exception around json.loads
should be narrowed to catching json.JSONDecodeError so unrelated errors aren't
swallowed; update the try/except that sets tool_input from tool_args (the block
referencing tool_input and tool_args) to except json.JSONDecodeError: tool_input
= tool_args, and make the same change to the analogous try/except later (the
second occurrence around lines 504-507) so only JSON parse failures fall back to
the original value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8c046929-e45f-4621-9a14-7cd021b37161
📒 Files selected for processing (3)
server/main_chatbot.pyserver/utils/db/connection_pool.pyserver/utils/db/db_adapters.py
| except BaseException: | ||
| pass |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify broad rollback catch blocks in DB utilities
rg -nP 'except\s+BaseException\s*:' server/utils/db/connection_pool.py server/utils/db/db_adapters.pyRepository: Arvo-AI/aurora
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Get full context around lines 88-89 in connection_pool.py
head -100 server/utils/db/connection_pool.py | tail -40Repository: Arvo-AI/aurora
Length of output: 1607
🏁 Script executed:
#!/bin/bash
# Get the full function containing the exception handler at line 88
sed -n '70,110p' server/utils/db/connection_pool.pyRepository: Arvo-AI/aurora
Length of output: 1541
🏁 Script executed:
#!/bin/bash
# Check if there's logging setup in the file
rg -n 'import.*logging|logger\s*=' server/utils/db/connection_pool.pyRepository: Arvo-AI/aurora
Length of output: 114
Add logging for rollback failures in connection error handler (Lines 88-89).
The except BaseException: pass silently swallows rollback errors with no observability. This pattern also exists in server/utils/db/db_adapters.py.
Proposed fix
- except BaseException:
- pass
+ except Exception as rollback_error:
+ logger.warning(
+ "Rollback failed while handling connection error: %s",
+ rollback_error,
+ exc_info=True,
+ )Catching BaseException also swallows KeyboardInterrupt, SystemExit, and GeneratorExit. Per coding guidelines, use try/except with logging in Python error handlers.
🧰 Tools
🪛 Ruff (0.15.6)
[error] 88-89: try-except-pass detected, consider logging the exception
(S110)
[warning] 88-88: Do not catch blind exception: BaseException
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/utils/db/connection_pool.py` around lines 88 - 89, Replace the silent
broad except in the connection rollback handler (currently "except
BaseException: pass" in connection_pool.py) with a targeted except Exception
that logs the failure (use the module's logger via logger.exception or
logger.error with exception info) and include context about which
connection/transaction failed; also make the same change for the analogous
rollback error handler in db_adapters.py—do not catch BaseException so
KeyboardInterrupt/SystemExit aren’t swallowed, and ensure the log message
includes the exception details for observability.
| except BaseException: | ||
| pass |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n server/utils/db/db_adapters.py | head -50Repository: Arvo-AI/aurora
Length of output: 2197
🏁 Script executed:
cat -n server/utils/db/db_adapters.py | sed -n '140,180p'Repository: Arvo-AI/aurora
Length of output: 1243
🏁 Script executed:
wc -l server/utils/db/db_adapters.pyRepository: Arvo-AI/aurora
Length of output: 92
Replace bare except BaseException: pass with except Exception + logging in both rollback handlers (Lines 148–149, 169–170).
Both catch-all handlers silently mask rollback failures without logging. Per coding guidelines, error handling must include logging.
Proposed fixes
try:
connection.rollback()
- except BaseException:
- pass
+ except Exception as rollback_error:
+ logger.warning("Admin rollback failed in legacy context manager: %s", rollback_error, exc_info=True) try:
connection.rollback()
- except BaseException:
- pass
+ except Exception as rollback_error:
+ logger.warning("User rollback failed in legacy context manager: %s", rollback_error, exc_info=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except BaseException: | |
| pass | |
| try: | |
| connection.rollback() | |
| except Exception as rollback_error: | |
| logger.warning("Admin rollback failed in legacy context manager: %s", rollback_error, exc_info=True) |
🧰 Tools
🪛 Ruff (0.15.6)
[error] 148-149: try-except-pass detected, consider logging the exception
(S110)
[warning] 148-148: Do not catch blind exception: BaseException
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/utils/db/db_adapters.py` around lines 148 - 149, Replace the silent
catch-all in the two rollback handlers that currently use "except BaseException:
pass" with a proper "except Exception as e" that logs the error; locate the
rollback handler functions in server/utils/db/db_adapters.py (the handlers
catching BaseException during rollback) and change the handler to catch
Exception and call the module/class logger (e.g., logger.exception(...) or
self._logger.exception(...)) with a clear message including the exception so
rollback failures are recorded.
OlivierTrudeau
left a comment
OlivierTrudeau
left a comment
There was a problem hiding this comment.
Please just change BaseException to Exception instead. BaseException will have the same behaviour as bar except, but it will now be explicit. The except Exception that you used for main_chatbot.py is the right fix. Thank you for bringing this up!
3 participants
Summary
Replace bare
except:clauses withexcept Exception:across 3 files (5 occurrences).Bare
except:catches all exceptions includingSystemExit,KeyboardInterrupt, andGeneratorExit, which should typically propagate. Per PEP 8 (E722).Files changed:
server/utils/db/db_adapters.py(2 occurrences)server/utils/db/connection_pool.py(1 occurrence)server/main_chatbot.py(2 occurrences)Testing
No behavior change — style/correctness fix only.
Summary by CodeRabbit