fix: replace bare except clauses with except Exception (3 files)#3371
fix: replace bare except clauses with except Exception (3 files)#3371harshadkhetpal wants to merge 3 commits into
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
src/autoconfmain.py: Bare src/commonutils/jobs.py: Two bare gen/save_config.py: Top-level bare SummaryThis PR is a code-quality fix implementing PEP 8 compliance for exception handling across three files. No configuration, schema, packaging, or deployment impact. Behavioural change: Non-standard exceptions ( WalkthroughException handling across three modules is narrowed from bare Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common/utils/jobs.py`:
- Around line 263-264: The except blocks in cache_file and del_cache currently
catch broad Exception and return a generic failure; change these to catch the
specific database/ORM exceptions that upsert_job_cache() and delete_job_cache()
can raise (e.g., SQLAlchemyError or the specific error classes used by your DB
client) or remove the broad try/except so errors propagate to the caller; update
the handlers to either catch only those specific exceptions and include the
original exception details in the returned message or allow unexpected
exceptions to bubble up to the job script for proper logging and handling (refer
to functions cache_file, del_cache, upsert_job_cache, delete_job_cache and
remove reliance on a blanket except Exception with format_exc()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 92a62a56-5dcc-46fa-bbd9-608e5c56afaa
📒 Files selected for processing (3)
src/autoconf/main.pysrc/common/gen/save_config.pysrc/common/utils/jobs.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Follow BunkerWeb's Python standards and security posture:
- Use snake_case for functions and variables, PascalCase for classes, and provide concise, accurate docstrings for public classes, functions, and methods.
- Respect Black formatting with a 160-character line limit and the existing pre-commit conventions. Do not insist on adding type annotations to previously untyped code, but accept them when added consistently.
- Catch specific exceptions; never use bare
except:. CatchingExceptionis acceptable only at explicit process boundaries (for example scheduler loops, outer job runners, worker entrypoints, or graceful-shutdown boundaries) when the code logs enough context and either re-raises, returns an error status, or terminates safely.- Never use
os.system,subprocess.*(..., shell=True),eval, orexec. Pass subprocess arguments as a list and prefer explicit binary paths for privileged operations.- Do not use unsafe deserialisers (
pickle,marshal,shelve,jsonpickle,dill) for untrusted data. Useyaml.safe_load()rather than unsafe YAML loading.- Open files with an explicit encoding (normally
utf-8) and usewithstatements for files, sockets, database sessions, and temporary resources.- Use
secretsfor token generation andhmac.compare_digestfor token, HMAC, or signature comparisons.- For HTTP clients (
requests,httpx): always set an explicit timeout, validate destination URLs to block RFC1918/loopback/link-local ranges (SSRF), disable automatic redirects to internal hosts, and be careful with proxy settings.- For filesystem operations: resolve paths with
Path.resolve()and verify they remain under the intended base directory before reading or writing (path traversal).- Use
defusedxmlrather than stdlib XML parsers for untrusted XML.- For SQLAlchemy, use bound parameters and safe query construction. Never call
text()with f-strings or.format(), and flag.execute()calls built...
Files:
src/common/utils/jobs.pysrc/autoconf/main.pysrc/common/gen/save_config.py
src/common/gen/**/*.py
⚙️ CodeRabbit configuration file
src/common/gen/**/*.py:src/common/gen/drives settings validation and config generation:
- Preserve determinism: the same validated settings should render the same output.
- Do not bypass the configurator or validation layer when introducing new settings.
- Any template change that depends on a new setting should come with the corresponding schema or plugin metadata update.
- Be careful with escaping and quoting so user-supplied settings cannot break out of generated directives or file formats.
Files:
src/common/gen/save_config.py
🔇 Additional comments (2)
src/common/gen/save_config.py (1)
251-253: Correct exception narrowing at process boundary.Line 251 is an entrypoint boundary, and this now catches
Exception(notBaseException), logs full context, and terminates safely viasys_exit(1).As per coding guidelines, "Catching
Exceptionis acceptable only at explicit process boundaries ... when the code logs enough context and either re-raises, returns an error status, or terminates safely."src/autoconf/main.py (1)
85-87: Top-level handler is now correctly scoped toException.Line 85 is at the process boundary; this keeps traceback logging and safe termination while no longer intercepting non-
Exceptioncontrol-flow exceptions.As per coding guidelines, "Catching
Exceptionis acceptable only at explicit process boundaries ... when the code logs enough context and either re-raises, returns an error status, or terminates safely."
| except Exception: | ||
| return False, f"exception :\n{format_exc()}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the target file
wc -l src/common/utils/jobs.pyRepository: bunkerity/bunkerweb
Length of output: 91
🏁 Script executed:
# Get the full context around lines 263-264 and 299-300
sed -n '250,280p' src/common/utils/jobs.pyRepository: bunkerity/bunkerweb
Length of output: 1383
🏁 Script executed:
# Also check lines around 299-300
sed -n '285,310p' src/common/utils/jobs.pyRepository: bunkerity/bunkerweb
Length of output: 1067
🏁 Script executed:
# Find all callers of cache_file and del_cache methods
rg 'cache_file|del_cache' --type py -B 2 -A 2Repository: bunkerity/bunkerweb
Length of output: 50375
🏁 Script executed:
sed -n '235,275p' src/common/utils/jobs.pyRepository: bunkerity/bunkerweb
Length of output: 1706
🏁 Script executed:
sed -n '280,310p' src/common/utils/jobs.pyRepository: bunkerity/bunkerweb
Length of output: 1314
Catch specific database exceptions instead of broad Exception in utility methods.
Lines 263–264 (cache_file) and 299–300 (del_cache) catch Exception around database operations. These are helper methods invoked from job scripts (which themselves sit at process boundaries), not process boundaries in their own right. Catching Exception here masks unexpected database failures—connection loss, constraint violations, permission errors—behind a generic (False, "exception") response. Replace with specific exception types (e.g. SQLAlchemy exceptions or whatever upsert_job_cache() and delete_job_cache() actually raise), or let unexpected exceptions propagate to the calling job script where they can be logged with full context at a process boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/utils/jobs.py` around lines 263 - 264, The except blocks in
cache_file and del_cache currently catch broad Exception and return a generic
failure; change these to catch the specific database/ORM exceptions that
upsert_job_cache() and delete_job_cache() can raise (e.g., SQLAlchemyError or
the specific error classes used by your DB client) or remove the broad
try/except so errors propagate to the caller; update the handlers to either
catch only those specific exceptions and include the original exception details
in the returned message or allow unexpected exceptions to bubble up to the job
script for proper logging and handling (refer to functions cache_file,
del_cache, upsert_job_cache, delete_job_cache and remove reliance on a blanket
except Exception with format_exc()).
Summary
Replace bare
except:clauses withexcept Exception:across 3 files (4 occurrences).Per PEP 8, bare
except:clauses should specify an exception type. Since none of these handlers re-raise the caught exception,except Exception:is the appropriate replacement — it catches all standard exceptions while still allowingKeyboardInterruptandSystemExitto propagate normally.Files changed:
src/autoconf/main.py(1 occurrence)src/common/utils/jobs.py(2 occurrences)src/common/gen/save_config.py(1 occurrence)Change pattern:
Testing
No behavior change for standard exceptions —
KeyboardInterruptandSystemExitwill now propagate correctly instead of being silently caught.