feat(drivers): refactor hooks/ to drivers/ with class-based driver pattern - #1131
Draft
ocervell wants to merge 2 commits into
Draft
feat(drivers): refactor hooks/ to drivers/ with class-based driver pattern#1131ocervell wants to merge 2 commits into
ocervell wants to merge 2 commits into
Conversation
…ttern Refactors the existing flat-function hook system into a proper class-based driver system, aligning implementation with documentation terminology. Key changes: - New `secator/drivers/` package with `Driver` base class and concrete implementations: `GCSDriver`, `MongoDBDriver`, `ApiDriver`, `DiscordDriver` - Each driver exposes a `hooks` property returning runner-class-keyed hook dicts, a `check()` method, and named handler methods as instance methods - `Runner.__init__` now accepts `drivers=[]` parameter; hooks are extracted from driver instances and merged at construction time - CLI (`cli_helper.py`) now instantiates driver classes (via `get_driver_instance` in loader.py) instead of importing raw `HOOKS` dicts - `secator/hooks/*.py` files become thin backward-compatibility shims that import from `secator/drivers/` and re-export `HOOKS` for external callers - `loader.py` gains `get_driver_instance(name)` with lazy imports to avoid loading optional deps (pymongo, google-cloud-storage) unless the driver is used - Celery `start_runner` task accepts `drivers=[]` for forward serialization; `Runner.delay()` passes drivers through; bound methods on driver instances are picklable in Python 3, so remote workers correctly reconstruct them Serialization: driver `__init__` args are primitives; module-level DB/GCS clients are singletons not stored on the instance; Python 3 bound methods carry the class + instance state through pickle, so Celery workers reconstruct them. Co-authored-by: Olivier Cervello <ocervell@users.noreply.github.com>
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR refactors the driver architecture from hook-based delegation to explicit Driver classes. It introduces a ChangesDriver Infrastructure and Implementation
Sequence DiagramsequenceDiagram
participant User as CLI User
participant CLI as CLI Handler
participant Loader as Loader.get_driver_instance()
participant Registry as DRIVER_REGISTRY
participant DriverClass as Driver Class
participant Runner as Runner Instance
participant DriverHooks as Driver.hooks
User->>CLI: secator x httpx --driver gcs <target>
CLI->>Loader: get_driver_instance('gcs')
Loader->>Registry: lookup 'gcs'
Registry-->>Loader: (module_path, GCSDriver)
Loader->>DriverClass: GCSDriver(bucket_name=...)
DriverClass-->>Loader: driver instance
Loader-->>CLI: driver instance
CLI->>Runner: Runner(..., drivers=[gcs_driver])
Runner->>DriverHooks: extract driver.hooks
DriverHooks-->>Runner: {Task: {on_item: [...]}}
Runner->>Runner: merge hooks into runner.hooks
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ocervell
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@secator/celery.py`:
- Line 147: The start_runner function uses mutable default arguments (results,
run_opts, hooks, drivers, validators, context) which leak state across calls;
change their defaults to None in the signature and inside start_runner
initialize them to fresh empty lists/dicts (e.g. if results is None: results =
[]; if run_opts is None: run_opts = {}; etc.), ensuring callers still get the
same behavior but each invocation gets its own independent collections;
reference the start_runner method to make this change and update any internal
assumptions that these variables may be shared.
In `@secator/cli_helper.py`:
- Around line 280-283: The code in secator/cli_helper.py that builds
driver_names assumes CONFIG.drivers.defaults exists; update the driver list
construction in the function where driver_instances/driver_names are built to
defensively read defaults (e.g. use a safe accessor or fallback) so you
concatenate CONFIG.drivers.defaults or [] with the provided driver list;
specifically reference CONFIG, its drivers.defaults attribute and the
driver_names variable and ensure you coerce a missing or None defaults into an
empty list before list(dict.fromkeys(...)) is applied.
In `@secator/drivers/api.py`:
- Around line 155-157: The code repeatedly calls get_workspace_name(...) on hot
paths; change the logic to check runner.context for an existing workspace_name
before calling get_workspace_name and only call and store the result once (e.g.,
if 'workspace_name' not in runner.context: runner.context['workspace_name'] =
get_workspace_name(...)), then reuse runner.context['workspace_name'] on
subsequent hooks; apply the same change to the other occurrence(s) (the block
around the code that currently calls get_workspace_name at lines ~207-209) to
ensure a single lookup per runner context.
- Around line 44-49: Wrap the call to response.json() in a safe parse: check for
empty body or status 204 and use a try/except (catch
json.JSONDecodeError/ValueError) so non-JSON/empty responses don't raise before
response.raise_for_status(); if parsing fails set result to an empty dict or
None and still call response.raise_for_status() to surface HTTP errors, then run
debug/console.print logic against the guarded result. Also add simple caching
for get_workspace_name (e.g., memoize or an LRU cache) so repeated calls per
event return the cached workspace name instead of performing repeated lookups.
In `@secator/drivers/gcs.py`:
- Around line 58-63: The code currently sets gs:// paths before uploads finish
and uses item._uuid (which may be unset) to form blob names, risking broken
links and collisions; fix by generating a collision-safe blob name (use
item._uuid if present else uuid.uuid4().hex or append a random/timestamp suffix)
and only set the item field after a successful upload by wrapping upload_blob in
a helper (e.g., upload_and_set_field) that calls upload_blob(bucket_name, data,
blob_name) and on success does setattr(item, k,
f'gs://{self.bucket_name}/{blob_name}'); start that helper in the Thread and
append it to runner.threads as before so fields are updated only after confirmed
upload.
In `@secator/drivers/mongodb.py`:
- Around line 17-29: get_mongodb_client() currently always uses
CONFIG.addons.mongodb.url so any MongoDBDriver(url=...) instance is ignored;
change get_mongodb_client to accept an optional url parameter (e.g.,
get_mongodb_client(url=None)) and resolve url = url or
CONFIG.addons.mongodb.url, and replace the single-module _mongodb_client with a
small cache keyed by the resolved url (e.g., _mongodb_clients[url]) so each
distinct URL gets its own MongoClient; update MongoDBDriver.__init__ (and other
callers in this file) to pass self.url into get_mongodb_client(self.url) and
keep the module-level singleton behavior per-URL and the original non-pickling
semantics.
In `@secator/hooks/gcs.py`:
- Around line 7-14: The module-level compatibility wrappers download_blob and
upload_blob discard and return None instead of returning the underlying
GCSDriver results; modify download_blob and upload_blob to return the value from
_driver.download_blob(...) and _driver.upload_blob(...) respectively so existing
callers receive the driver’s return (e.g., return
_driver.download_blob(bucket_name, source_blob_name, destination_file_name) and
return _driver.upload_blob(bucket_name, source_file_name,
destination_blob_name)).
In `@secator/loader.py`:
- Around line 256-268: The legacy shim class _ExternalDriver must fully
implement the Driver contract: add a stable identity/name and a check()
implementation that delegates to the HOOKS if present (or raises a clear
exception if the hook is missing) so registry-backed code can substitute it;
specifically, inside the _ExternalDriver class (which already subclasses Driver
and exposes hooks) add a name or __repr__/id property set from driver_name (e.g.
self.name or class attribute) and implement def check(self, *args, **kwargs):
return hooks.get('check', lambda *a, **k: (_ for _ in
()).throw(NotImplementedError("check hook not provided")))(*args, **kwargs) (or
equivalent delegation) to ensure callers expecting Driver.check() behave
consistently.
In `@secator/runners/_base.py`:
- Line 93: The __init__ of Runner uses mutable default args (inputs, results,
run_opts, hooks, validators, context, drivers) causing state leakage; change the
signature of Runner.__init__ to accept None for those parameters (e.g.,
inputs=None, context=None, etc.) and inside the constructor initialize them to
fresh objects (self.inputs = [] if inputs is None else list(inputs),
self.context = {} if context is None else dict(context), etc.) so each Runner
instance gets its own lists/dicts and mutations to self.context or other
attributes do not affect other instances; update any callers that relied on
falsy defaults if necessary.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8368c52-f0e8-477c-a00f-fb28b974dd1d
📒 Files selected for processing (14)
secator/celery.pysecator/cli_helper.pysecator/drivers/__init__.pysecator/drivers/_base.pysecator/drivers/api.pysecator/drivers/discord.pysecator/drivers/gcs.pysecator/drivers/mongodb.pysecator/hooks/api.pysecator/hooks/discord.pysecator/hooks/gcs.pysecator/hooks/mongodb.pysecator/loader.pysecator/runners/_base.py
|
|
||
| @app.task(bind=True) | ||
| def start_runner(self, config, targets, results=[], run_opts={}, hooks={}, validators={}, context={}): | ||
| def start_runner(self, config, targets, results=[], run_opts={}, hooks={}, drivers=[], validators={}, context={}): |
Contributor
There was a problem hiding this comment.
Remove shared mutable task defaults.
This task mutates run_opts and context on every call, so the default dict/list objects will leak state across Celery jobs handled by the same worker process.
Suggested fix
-@app.task(bind=True)
-def start_runner(self, config, targets, results=[], run_opts={}, hooks={}, drivers=[], validators={}, context={}):
+@app.task(bind=True)
+def start_runner(self, config, targets, results=None, run_opts=None, hooks=None, drivers=None, validators=None, context=None):
+ results = [] if results is None else results
+ run_opts = {} if run_opts is None else run_opts
+ hooks = {} if hooks is None else hooks
+ drivers = [] if drivers is None else drivers
+ validators = {} if validators is None else validators
+ context = {} if context is None else context
context = context or {}🧰 Tools
🪛 Ruff (0.15.15)
[warning] 147-147: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 147-147: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 147-147: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 147-147: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 147-147: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 147-147: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/celery.py` at line 147, The start_runner function uses mutable
default arguments (results, run_opts, hooks, drivers, validators, context) which
leak state across calls; change their defaults to None in the signature and
inside start_runner initialize them to fresh empty lists/dicts (e.g. if results
is None: results = []; if run_opts is None: run_opts = {}; etc.), ensuring
callers still get the same behavior but each invocation gets its own independent
collections; reference the start_runner method to make this change and update
any internal assumptions that these variables may be shared.
Comment on lines
+280
to
+283
| # Build driver instances from driver names | ||
| driver_instances = [] | ||
| driver_names = driver.split(',') if driver else [] | ||
| driver_names = list(dict.fromkeys(CONFIG.drivers.defaults + driver_names)) |
Contributor
There was a problem hiding this comment.
Guard against CONFIG.drivers.defaults being unset.
Line 283 assumes CONFIG.drivers.defaults is always a list. If the config leaves it unset, this concatenation raises before any driver validation or addon checks run.
Suggested fix
# Build driver instances from driver names
driver_instances = []
driver_names = driver.split(',') if driver else []
- driver_names = list(dict.fromkeys(CONFIG.drivers.defaults + driver_names))
+ default_drivers = CONFIG.drivers.defaults or []
+ driver_names = list(dict.fromkeys(default_drivers + driver_names))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/cli_helper.py` around lines 280 - 283, The code in
secator/cli_helper.py that builds driver_names assumes CONFIG.drivers.defaults
exists; update the driver list construction in the function where
driver_instances/driver_names are built to defensively read defaults (e.g. use a
safe accessor or fallback) so you concatenate CONFIG.drivers.defaults or [] with
the provided driver list; specifically reference CONFIG, its drivers.defaults
attribute and the driver_names variable and ensure you coerce a missing or None
defaults into an empty list before list(dict.fromkeys(...)) is applied.
Comment on lines
+44
to
+49
| result = response.json() | ||
| debug('API response', sub='drivers.api', verbose=True, obj=result) | ||
| if not response.ok and result.get('detail'): | ||
| console.print(Error(message=f'API error: {result["detail"]}')) | ||
| response.raise_for_status() | ||
| return result |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python requests, does Response.json() raise an exception on HTTP 204 or other empty/non-JSON response bodies?
💡 Result:
Yes, in the Python requests library, the Response.json method raises an exception when the response body is empty or contains invalid JSON [1][2][3][4]. Specifically, it raises a requests.exceptions.JSONDecodeError (which wraps the underlying json.decoder.JSONDecodeError from the Python standard library) [5][3][4]. Because a HTTP 204 No Content response typically contains no body, calling.json on such a response will result in this exception [2][6]. To handle this safely, you should either check the status code or verify that the response body is not empty before attempting to decode it [2][6]: if response.status_code == 204 or not response.content: # Handle empty response (e.g., return None or an empty dict) data = {} else: data = response.json Alternatively, you can catch the exception directly [2][5][6]: try: data = response.json except requests.exceptions.JSONDecodeError: # Handle case where content is not valid JSON or is empty data = None
Citations:
- 1: Request with Empty Content Throws Errors When
.json()is Called psf/requests#4186 - 2: https://thelinuxcode.com/responsejson-in-python-requests-reliable-json-parsing-in-2026/
- 3: https://docs.python-requests.org/en/latest/api/?highlight=json
- 4: https://requests.readthedocs.io/en/latest/api/?highlight=exception
- 5: From
Expecting value: line 1 column 1 (char 0)toResponse content not in json formatpsf/requests#4908 - 6: https://stackoverflow.com/questions/37605278/how-to-determine-if-my-python-requests-call-to-api-returns-no-data
Guard response.json() against empty/non-JSON responses in secator/drivers/api.py.
response.json() will raise on HTTP 204 (empty body) or invalid/non-JSON error bodies, which can break hook execution before you reach raise_for_status(). Also consider caching get_workspace_name results to avoid repeated lookups per event.
💡 Proposed fix
- result = response.json()
- debug('API response', sub='drivers.api', verbose=True, obj=result)
- if not response.ok and result.get('detail'):
- console.print(Error(message=f'API error: {result["detail"]}'))
- response.raise_for_status()
- return result
+ result = {}
+ if response.content:
+ try:
+ result = response.json()
+ except ValueError:
+ result = {}
+ debug('API response', sub='drivers.api', verbose=True, obj=result)
+ if not response.ok and result.get('detail'):
+ console.print(Error(message=f'API error: {result["detail"]}'))
+ response.raise_for_status()
+ return result📝 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.
Suggested change
| result = response.json() | |
| debug('API response', sub='drivers.api', verbose=True, obj=result) | |
| if not response.ok and result.get('detail'): | |
| console.print(Error(message=f'API error: {result["detail"]}')) | |
| response.raise_for_status() | |
| return result | |
| result = {} | |
| if response.content: | |
| try: | |
| result = response.json() | |
| except ValueError: | |
| result = {} | |
| debug('API response', sub='drivers.api', verbose=True, obj=result) | |
| if not response.ok and result.get('detail'): | |
| console.print(Error(message=f'API error: {result["detail"]}')) | |
| response.raise_for_status() | |
| return result |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/drivers/api.py` around lines 44 - 49, Wrap the call to
response.json() in a safe parse: check for empty body or status 204 and use a
try/except (catch json.JSONDecodeError/ValueError) so non-JSON/empty responses
don't raise before response.raise_for_status(); if parsing fails set result to
an empty dict or None and still call response.raise_for_status() to surface HTTP
errors, then run debug/console.print logic against the guarded result. Also add
simple caching for get_workspace_name (e.g., memoize or an LRU cache) so
repeated calls per event return the cached workspace name instead of performing
repeated lookups.
Comment on lines
+155
to
+157
| workspace_name = self.get_workspace_name(runner.context.get('workspace_id')) | ||
| if workspace_name: | ||
| runner.context['workspace_name'] = workspace_name |
Contributor
There was a problem hiding this comment.
Avoid repeated workspace API lookups on every runner/finding event.
get_workspace_name(...) is called repeatedly in hot paths, causing avoidable remote calls per hook execution. Cache once per runner context and reuse.
💡 Proposed fix
- workspace_name = self.get_workspace_name(runner.context.get('workspace_id'))
+ workspace_name = runner.context.get('workspace_name')
+ if workspace_name is None:
+ workspace_name = self.get_workspace_name(runner.context.get('workspace_id'))
if workspace_name:
runner.context['workspace_name'] = workspace_name- workspace_name = self.get_workspace_name(runner.context.get('workspace_id'))
+ workspace_name = runner.context.get('workspace_name')
+ if workspace_name is None:
+ workspace_name = self.get_workspace_name(runner.context.get('workspace_id'))
if workspace_name:
runner.context['workspace_name'] = workspace_nameAlso applies to: 207-209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/drivers/api.py` around lines 155 - 157, The code repeatedly calls
get_workspace_name(...) on hot paths; change the logic to check runner.context
for an existing workspace_name before calling get_workspace_name and only call
and store the result once (e.g., if 'workspace_name' not in runner.context:
runner.context['workspace_name'] = get_workspace_name(...)), then reuse
runner.context['workspace_name'] on subsequent hooks; apply the same change to
the other occurrence(s) (the block around the code that currently calls
get_workspace_name at lines ~207-209) to ensure a single lookup per runner
context.
Comment on lines
+58
to
+63
| blob_name = f'{item._uuid}_{k}{ext}' | ||
| t = Thread(target=self.upload_blob, args=(self.bucket_name, v, blob_name)) | ||
| t.start() | ||
| runner.threads.append(t) | ||
| setattr(item, k, f'gs://{self.bucket_name}/{blob_name}') | ||
| return item |
Contributor
There was a problem hiding this comment.
Do not publish gs:// paths before upload success, and avoid _uuid-based collisions.
Current flow rewrites item fields before upload completion and derives blob name from item._uuid (which can be unset). This can persist broken links or collide object names.
💡 Direction for fix
+from uuid import uuid4
@@
- blob_name = f'{item._uuid}_{k}{ext}'
- t = Thread(target=self.upload_blob, args=(self.bucket_name, v, blob_name))
+ item_id = getattr(item, '_uuid', None) or str(uuid4())
+ blob_name = f'{item_id}_{k}{ext}'
+ t = Thread(target=self._upload_and_set_field, args=(item, k, v, blob_name))
t.start()
runner.threads.append(t)
- setattr(item, k, f'gs://{self.bucket_name}/{blob_name}')
+ # only set field after successful upload🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/drivers/gcs.py` around lines 58 - 63, The code currently sets gs://
paths before uploads finish and uses item._uuid (which may be unset) to form
blob names, risking broken links and collisions; fix by generating a
collision-safe blob name (use item._uuid if present else uuid.uuid4().hex or
append a random/timestamp suffix) and only set the item field after a successful
upload by wrapping upload_blob in a helper (e.g., upload_and_set_field) that
calls upload_blob(bucket_name, data, blob_name) and on success does
setattr(item, k, f'gs://{self.bucket_name}/{blob_name}'); start that helper in
the Thread and append it to runner.threads as before so fields are updated only
after confirmed upload.
Comment on lines
+17
to
+29
| def get_mongodb_client(): | ||
| """Get or create MongoDB client (module-level singleton, not pickled with driver instances).""" | ||
| global _mongodb_client | ||
| if _mongodb_client is None: | ||
| import pymongo | ||
| _mongodb_client = pymongo.MongoClient( | ||
| escape_mongodb_url(CONFIG.addons.mongodb.url), | ||
| maxPoolSize=CONFIG.addons.mongodb.max_pool_size, | ||
| serverSelectionTimeoutMS=CONFIG.addons.mongodb.server_selection_timeout_ms, | ||
| connect=False, | ||
| tz_aware=True | ||
| ) | ||
| return _mongodb_client |
Contributor
There was a problem hiding this comment.
MongoDBDriver(url=...) is effectively ignored.
get_mongodb_client() is hardwired to CONFIG.addons.mongodb.url, so instance-provided url never affects connections. This breaks the driver’s constructor contract and can route writes to the wrong database.
💡 Proposed fix
-_mongodb_client = None
+_mongodb_clients = {}
-def get_mongodb_client():
+def get_mongodb_client(url=None):
"""Get or create MongoDB client (module-level singleton, not pickled with driver instances)."""
- global _mongodb_client
- if _mongodb_client is None:
+ global _mongodb_clients
+ resolved_url = escape_mongodb_url(url or CONFIG.addons.mongodb.url)
+ if resolved_url not in _mongodb_clients:
import pymongo
- _mongodb_client = pymongo.MongoClient(
- escape_mongodb_url(CONFIG.addons.mongodb.url),
+ _mongodb_clients[resolved_url] = pymongo.MongoClient(
+ resolved_url,
maxPoolSize=CONFIG.addons.mongodb.max_pool_size,
serverSelectionTimeoutMS=CONFIG.addons.mongodb.server_selection_timeout_ms,
connect=False,
tz_aware=True
)
- return _mongodb_client
+ return _mongodb_clients[resolved_url]- client = get_mongodb_client()
+ client = get_mongodb_client(self.url)
@@
- client = get_mongodb_client()
+ client = get_mongodb_client(self.url)Also applies to: 212-213, 247-248, 276-277
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/drivers/mongodb.py` around lines 17 - 29, get_mongodb_client()
currently always uses CONFIG.addons.mongodb.url so any MongoDBDriver(url=...)
instance is ignored; change get_mongodb_client to accept an optional url
parameter (e.g., get_mongodb_client(url=None)) and resolve url = url or
CONFIG.addons.mongodb.url, and replace the single-module _mongodb_client with a
small cache keyed by the resolved url (e.g., _mongodb_clients[url]) so each
distinct URL gets its own MongoClient; update MongoDBDriver.__init__ (and other
callers in this file) to pass self.url into get_mongodb_client(self.url) and
keep the module-level singleton behavior per-URL and the original non-pickling
semantics.
Comment on lines
+7
to
+14
| def download_blob(bucket_name, source_blob_name, destination_file_name): | ||
| """Downloads a file from the bucket.""" | ||
| start_time = time() | ||
| storage_client = get_gcs_client() | ||
| bucket = storage_client.bucket(bucket_name) | ||
| blob = bucket.blob(source_blob_name) | ||
| blob.download_to_filename(destination_file_name) | ||
| end_time = time() | ||
| elapsed = end_time - start_time | ||
| debug(f'in {elapsed:.4f}s', obj={'blob': 'DOWNLOADED', 'blob_name': source_blob_name, 'bucket': bucket_name}, obj_after=False, sub='hooks.gcs') # noqa: E501 | ||
| """Module-level wrapper for GCSDriver.download_blob for backward compatibility.""" | ||
| _driver.download_blob(bucket_name, source_blob_name, destination_file_name) | ||
|
|
||
|
|
||
| HOOKS = { | ||
| Task: {'on_item': [process_item]} | ||
| } | ||
| def upload_blob(bucket_name, source_file_name, destination_blob_name): | ||
| """Module-level wrapper for GCSDriver.upload_blob for backward compatibility.""" | ||
| _driver.upload_blob(bucket_name, source_file_name, destination_blob_name) |
Contributor
There was a problem hiding this comment.
Return the wrapped GCS helper results.
Both compatibility wrappers currently discard the driver's return value, so any existing caller of download_blob() or upload_blob() now gets None even if GCSDriver returns a path/object/blob handle.
Suggested fix
def download_blob(bucket_name, source_blob_name, destination_file_name):
"""Module-level wrapper for GCSDriver.download_blob for backward compatibility."""
- _driver.download_blob(bucket_name, source_blob_name, destination_file_name)
+ return _driver.download_blob(bucket_name, source_blob_name, destination_file_name)
@@
def upload_blob(bucket_name, source_file_name, destination_blob_name):
"""Module-level wrapper for GCSDriver.upload_blob for backward compatibility."""
- _driver.upload_blob(bucket_name, source_file_name, destination_blob_name)
+ return _driver.upload_blob(bucket_name, source_file_name, destination_blob_name)📝 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.
Suggested change
| def download_blob(bucket_name, source_blob_name, destination_file_name): | |
| """Downloads a file from the bucket.""" | |
| start_time = time() | |
| storage_client = get_gcs_client() | |
| bucket = storage_client.bucket(bucket_name) | |
| blob = bucket.blob(source_blob_name) | |
| blob.download_to_filename(destination_file_name) | |
| end_time = time() | |
| elapsed = end_time - start_time | |
| debug(f'in {elapsed:.4f}s', obj={'blob': 'DOWNLOADED', 'blob_name': source_blob_name, 'bucket': bucket_name}, obj_after=False, sub='hooks.gcs') # noqa: E501 | |
| """Module-level wrapper for GCSDriver.download_blob for backward compatibility.""" | |
| _driver.download_blob(bucket_name, source_blob_name, destination_file_name) | |
| HOOKS = { | |
| Task: {'on_item': [process_item]} | |
| } | |
| def upload_blob(bucket_name, source_file_name, destination_blob_name): | |
| """Module-level wrapper for GCSDriver.upload_blob for backward compatibility.""" | |
| _driver.upload_blob(bucket_name, source_file_name, destination_blob_name) | |
| def download_blob(bucket_name, source_blob_name, destination_file_name): | |
| """Module-level wrapper for GCSDriver.download_blob for backward compatibility.""" | |
| return _driver.download_blob(bucket_name, source_blob_name, destination_file_name) | |
| def upload_blob(bucket_name, source_file_name, destination_blob_name): | |
| """Module-level wrapper for GCSDriver.upload_blob for backward compatibility.""" | |
| return _driver.upload_blob(bucket_name, source_file_name, destination_blob_name) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/hooks/gcs.py` around lines 7 - 14, The module-level compatibility
wrappers download_blob and upload_blob discard and return None instead of
returning the underlying GCSDriver results; modify download_blob and upload_blob
to return the value from _driver.download_blob(...) and _driver.upload_blob(...)
respectively so existing callers receive the driver’s return (e.g., return
_driver.download_blob(bucket_name, source_blob_name, destination_file_name) and
return _driver.upload_blob(bucket_name, source_file_name,
destination_blob_name)).
Comment on lines
+256
to
+268
| # Fall back to external driver (HOOKS-based): wrap in a compatible shim | ||
| from secator.utils import import_dynamic | ||
| hooks = import_dynamic(f'secator.hooks.{driver_name}', 'HOOKS') | ||
| if hooks is not None: | ||
| from secator.drivers._base import Driver | ||
|
|
||
| class _ExternalDriver(Driver): | ||
| @property | ||
| def hooks(self): | ||
| return hooks | ||
|
|
||
| _ExternalDriver.__name__ = f'{driver_name.capitalize()}Driver' | ||
| return _ExternalDriver() |
Contributor
There was a problem hiding this comment.
Make the legacy shim satisfy the driver contract.
The fallback _ExternalDriver only exposes hooks, so HOOKS-based external drivers are no longer substitutable with registry-backed drivers under the new class-based API. Any shared code that expects every driver to have a stable identity and a check() method will break on this path.
Suggested fix
class _ExternalDriver(Driver):
+ `@property`
+ def name(self):
+ return driver_name
+
`@property`
def hooks(self):
return hooks
+
+ def check(self):
+ return True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/loader.py` around lines 256 - 268, The legacy shim class
_ExternalDriver must fully implement the Driver contract: add a stable
identity/name and a check() implementation that delegates to the HOOKS if
present (or raises a clear exception if the hook is missing) so registry-backed
code can substitute it; specifically, inside the _ExternalDriver class (which
already subclasses Driver and exposes hooks) add a name or __repr__/id property
set from driver_name (e.g. self.name or class attribute) and implement def
check(self, *args, **kwargs): return hooks.get('check', lambda *a, **k: (_ for _
in ()).throw(NotImplementedError("check hook not provided")))(*args, **kwargs)
(or equivalent delegation) to ensure callers expecting Driver.check() behave
consistently.
| enable_duplicate_check = True | ||
|
|
||
| def __init__(self, config, inputs=[], results=[], run_opts={}, hooks={}, validators={}, context={}): | ||
| def __init__(self, config, inputs=[], results=[], run_opts={}, hooks={}, validators={}, context={}, drivers=[]): |
Contributor
There was a problem hiding this comment.
Avoid shared mutable defaults in Runner.__init__.
Line 93 reuses the same list/dict objects across runner instances. context is mutated later in the class, so state from one run can bleed into the next in a long-lived process.
Suggested fix
- def __init__(self, config, inputs=[], results=[], run_opts={}, hooks={}, validators={}, context={}, drivers=[]):
+ def __init__(self, config, inputs=None, results=None, run_opts=None, hooks=None, validators=None, context=None, drivers=None):
+ inputs = [] if inputs is None else inputs
+ results = [] if results is None else results
+ run_opts = {} if run_opts is None else run_opts
+ hooks = {} if hooks is None else hooks
+ validators = {} if validators is None else validators
+ context = {} if context is None else context
+ drivers = [] if drivers is None else drivers📝 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.
Suggested change
| def __init__(self, config, inputs=[], results=[], run_opts={}, hooks={}, validators={}, context={}, drivers=[]): | |
| def __init__(self, config, inputs=None, results=None, run_opts=None, hooks=None, validators=None, context=None, drivers=None): | |
| inputs = [] if inputs is None else inputs | |
| results = [] if results is None else results | |
| run_opts = {} if run_opts is None else run_opts | |
| hooks = {} if hooks is None else hooks | |
| validators = {} if validators is None else validators | |
| context = {} if context is None else context | |
| drivers = [] if drivers is None else drivers |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 93-93: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 93-93: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 93-93: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 93-93: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 93-93: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 93-93: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 93-93: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/runners/_base.py` at line 93, The __init__ of Runner uses mutable
default args (inputs, results, run_opts, hooks, validators, context, drivers)
causing state leakage; change the signature of Runner.__init__ to accept None
for those parameters (e.g., inputs=None, context=None, etc.) and inside the
constructor initialize them to fresh objects (self.inputs = [] if inputs is None
else list(inputs), self.context = {} if context is None else dict(context),
etc.) so each Runner instance gets its own lists/dicts and mutations to
self.context or other attributes do not affect other instances; update any
callers that relied on falsy defaults if necessary.
Contributor
Author
|
Claude finished @ocervell's task in 6m 12s —— View job Changes implementedTasks:
Summary of changesCodeRabbit issues fixed (8 files, 95 additions / 27 deletions):
Dynamic hooks compatibility (#1116/#1126): External drivers can now use either pattern:
|
…ternal driver support - Fix mutable default args in Runner.__init__ and start_runner (B006) - Guard CONFIG.drivers.defaults against None in cli_helper - Guard response.json() against empty/non-JSON responses in ApiDriver - Cache workspace_name in runner.context to avoid repeated API lookups - Fix GCS uuid collision (use uuid4() fallback) and set gs:// field only after successful upload - Fix MongoDBDriver ignoring url parameter; use per-URL client cache - Fix missing return values in GCS backward-compat wrappers - Complete _ExternalDriver shim with name property and check() method - Add _file_has_driver_class() helper to detect class-based external drivers - Update discover_external_drivers() to discover both HOOKS-based and class-based Driver subclasses - Update get_driver_instance() to instantiate class-based external drivers directly - Skip class-based driver files in discover_external_tasks() to avoid misclassification Co-authored-by: Olivier Cervello <ocervell@users.noreply.github.com>
ocervell
added
status-ready
status-needs-testing
status-needs-human
ocervell
marked this pull request as draft
June 10, 2026 09:12
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
1 participant
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.
Closes #1127
Refactors the existing flat-function hook system into a proper class-based driver system, aligning implementation with documentation terminology.
Changes
secator/drivers/package withDriverbase class andGCSDriver,MongoDBDriver,ApiDriver,DiscordDriverhooksproperty,check()method, and handler methods as instance methodsRunner.__init__acceptsdrivers=[]; hooks extracted & merged at construction timeget_driver_instance()in loader.pysecator/hooks/*.pybecome backward-compat shimsstart_runneracceptsdrivers=[]for forward serializationGenerated with Claude Code
Summary by CodeRabbit
Release Notes