Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/aiogram_webhook/engines/multi.py (1)
29-29: 💤 Low valueRedundant assignment before super().init.
self.routeis set here, butsuper().__init__also setsself.route = route. The assignment on this line can be removed.♻️ Proposed fix
def __init__( self, dispatcher, /, *, web: WebAdapter[AppT, RawRequestT, FrameworkResponseT], route: Route, security: Security | None = None, handle_in_background: bool = True, ) -> None: - self.route = route self._task_trackers: dict[int, TaskTracker] = {} self._bots: dict[int, Bot] = {}🤖 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 `@src/aiogram_webhook/engines/multi.py` at line 29, Remove the redundant assignment to self.route prior to calling super().__init__ in the constructor of the class in multi.py: the constructor currently sets self.route = route and then calls super().__init__(...) which also sets self.route, so delete the earlier standalone assignment (the symbol to locate is the self.route assignment in the class __init__ and the super().__init__ call). Ensure only the super().__init__ is responsible for initializing self.route to avoid duplicate work.src/aiogram_webhook/tasks.py (1)
46-52: ⚡ Quick winAvoid unbounded wait after cancellation in
close().After timeout-triggered cancellation, awaiting
gather(*pending)can still hang forever if a task ignores/swallows cancellation. Add a bounded second wait and log stubborn tasks instead of blocking shutdown indefinitely.Proposed change
if pending: logger.warning("Timeout reached. Cancelling %s pending tasks.", len(pending)) for task in pending: task.cancel() - # Wait for cancellations to process - await asyncio.gather(*pending, return_exceptions=True) + # Wait for cancellations to process, but do not block forever. + _, still_pending = await asyncio.wait( + pending, + timeout=timeout, + return_when=asyncio.ALL_COMPLETED, + ) + if still_pending: + logger.error("Failed to cancel %s background tasks within timeout.", len(still_pending))🤖 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 `@src/aiogram_webhook/tasks.py` around lines 46 - 52, In close() after cancelling pending tasks (the block that logs "Timeout reached. Cancelling %s pending tasks." and calls task.cancel()), add a bounded second wait instead of a raw await asyncio.gather(*pending, return_exceptions=True): call asyncio.wait or asyncio.wait_for with a short timeout (e.g., a few seconds) to avoid hanging if a task swallows CancelledError, then log any tasks that did not finish as "stubborn" including their repr/len(pending) (use the same pending variable and task objects) and optionally cancel again or set task.cancel() + create_task to ensure shutdown proceeds; ensure you still use return_exceptions=True when awaiting to avoid raising exceptions during the bounded wait and preserve existing logging context from logger.src/aiogram_webhook/web/aiohttp.py (1)
4-4: ⚡ Quick winAvoid exposing aiohttp private
_SENTINELin adapter typing.Using
_SENTINELties this module to aiohttp internals and makes upgrades brittle.Suggested fix
-from aiohttp.helpers import _SENTINEL, sentinel +from aiohttp.helpers import sentinel @@ - def json_response( - self, status_code: int, data: Mapping[str, str] | _SENTINEL = sentinel, headers: Mapping[str, str] | None = None - ) -> Response: + def json_response( + self, status_code: int, data: Any = sentinel, headers: Mapping[str, str] | None = None + ) -> Response: return json_response(status=status_code, data=data, headers=headers)Also applies to: 79-80
🤖 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 `@src/aiogram_webhook/web/aiohttp.py` at line 4, The import exposes aiohttp's private _SENTINEL which couples this adapter to aiohttp internals; remove the private import and stop referencing _SENTINEL in the module (including usages around the current sentinel handling at the lines noted ~79-80). Replace uses of _SENTINEL with the public aiohttp.helpers.sentinel (already imported as sentinel) or with a local sentinel/typing.Any/Optional sentinel placeholder in type hints so the adapter relies only on public API; update any function/type annotations or default parameter checks that reference _SENTINEL to use the public sentinel or a local equivalent (e.g., named constant) instead.
🤖 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 `@src/aiogram_webhook/__init__.py`:
- Around line 10-15: The optional import block around FastAPIAdapter currently
catches all ImportError and can hide real bugs inside
aiogram_webhook.web.fastapi; change the except clause to catch
ModuleNotFoundError only so only missing optional dependency errors are
suppressed (leave the import and the __all__.insert("FastAPIAdapter") logic
as-is and do not swallow other exceptions raised from the module).
In `@src/aiogram_webhook/configs/webhook.py`:
- Around line 16-17: Add bounds validation for the max_connections field in the
webhook config dataclass by implementing a __post_init__ on the dataclass (or
extending the existing __post_init__) that checks if max_connections is not None
and then ensures 1 <= max_connections <= 100, raising a ValueError with a clear
message if the check fails; reference the max_connections attribute and the
dataclass's __post_init__ to locate where to insert the guard so invalid values
are rejected at construction rather than later when calling the Telegram API.
In `@src/aiogram_webhook/route/query.py`:
- Around line 76-82: The loop that builds values currently treats any
non-str/int item as a QueryValue by casting (values, items, QueryValue, Const),
which hides bad input and causes AttributeError later; change the normalization
in the items loop to explicitly accept only str, int or actual QueryValue
instances (use isinstance(item, QueryValue) or equivalent) and immediately raise
a clear TypeError/ValueError with a descriptive message identifying the
offending item when encountering any other type, so invalid config is rejected
early.
In `@src/aiogram_webhook/route/route.py`:
- Around line 59-63: The code computes self._path_params using
self._path_param_names and self._params before calling self._validate_config(),
which can cause a raw KeyError instead of raising
MissingRouteParamDeclarationError; fix by validating configuration first (call
self._validate_config() before building self._path_params and self._query_items)
or alternatively guard the lookup in the tuple comprehension to detect missing
names and raise MissingRouteParamDeclarationError explicitly (refer to symbols:
self._validate_config, self._path_params, self._path_param_names, self._params,
MissingRouteParamDeclarationError).
In `@src/aiogram_webhook/web/aiohttp.py`:
- Around line 35-38: In client_ip, guard against request.transport being None
and against get_extra_info returning a non-tuple/None: first check
self._request.transport is not None (cast to Transport only after the
None-check), call get_extra_info("peername") only if transport exists, verify
peername is a sequence/tuple with at least one element and that the first
element is a string (or convertible) before returning peername[0]; otherwise
return None. Update references to Transport and self._request.transport in
client_ip to implement these defensive checks.
In `@src/aiogram_webhook/web/fastapi.py`:
- Around line 61-67: FastAPIAdapter.register currently ignores the on_startup
and on_shutdown parameters (they are marked unused), so lifecycle callbacks
never run; update the register method to register these with the FastAPI app by
calling app.add_event_handler("startup", on_startup) and
app.add_event_handler("shutdown", on_shutdown) after creating/initializing app
and before adding the route, mirroring AiohttpAdapter behavior; locate the
register method in FastAPIAdapter and add those two calls using the on_startup
and on_shutdown symbols so engine lifecycle logic is executed.
---
Nitpick comments:
In `@src/aiogram_webhook/engines/multi.py`:
- Line 29: Remove the redundant assignment to self.route prior to calling
super().__init__ in the constructor of the class in multi.py: the constructor
currently sets self.route = route and then calls super().__init__(...) which
also sets self.route, so delete the earlier standalone assignment (the symbol to
locate is the self.route assignment in the class __init__ and the
super().__init__ call). Ensure only the super().__init__ is responsible for
initializing self.route to avoid duplicate work.
In `@src/aiogram_webhook/tasks.py`:
- Around line 46-52: In close() after cancelling pending tasks (the block that
logs "Timeout reached. Cancelling %s pending tasks." and calls task.cancel()),
add a bounded second wait instead of a raw await asyncio.gather(*pending,
return_exceptions=True): call asyncio.wait or asyncio.wait_for with a short
timeout (e.g., a few seconds) to avoid hanging if a task swallows
CancelledError, then log any tasks that did not finish as "stubborn" including
their repr/len(pending) (use the same pending variable and task objects) and
optionally cancel again or set task.cancel() + create_task to ensure shutdown
proceeds; ensure you still use return_exceptions=True when awaiting to avoid
raising exceptions during the bounded wait and preserve existing logging context
from logger.
In `@src/aiogram_webhook/web/aiohttp.py`:
- Line 4: The import exposes aiohttp's private _SENTINEL which couples this
adapter to aiohttp internals; remove the private import and stop referencing
_SENTINEL in the module (including usages around the current sentinel handling
at the lines noted ~79-80). Replace uses of _SENTINEL with the public
aiohttp.helpers.sentinel (already imported as sentinel) or with a local
sentinel/typing.Any/Optional sentinel placeholder in type hints so the adapter
relies only on public API; update any function/type annotations or default
parameter checks that reference _SENTINEL to use the public sentinel or a local
equivalent (e.g., named constant) instead.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1ca9eca-c119-4594-9628-f3a902a84af9
📒 Files selected for processing (57)
.github/workflows/tests.ymlCONTRIBUTING.mdREADME.mdpyproject.tomlsrc/aiogram_webhook/__init__.pysrc/aiogram_webhook/adapters/aiohttp/adapter.pysrc/aiogram_webhook/adapters/aiohttp/mapping.pysrc/aiogram_webhook/adapters/base_adapter.pysrc/aiogram_webhook/adapters/base_mapping.pysrc/aiogram_webhook/adapters/fastapi/adapter.pysrc/aiogram_webhook/adapters/fastapi/mapping.pysrc/aiogram_webhook/config/__init__.pysrc/aiogram_webhook/configs/__init__.pysrc/aiogram_webhook/configs/bot.pysrc/aiogram_webhook/configs/webhook.pysrc/aiogram_webhook/engines/__init__.pysrc/aiogram_webhook/engines/base.pysrc/aiogram_webhook/engines/errors.pysrc/aiogram_webhook/engines/multi.pysrc/aiogram_webhook/engines/simple.pysrc/aiogram_webhook/engines/single.pysrc/aiogram_webhook/engines/target.pysrc/aiogram_webhook/engines/token.pysrc/aiogram_webhook/errors.pysrc/aiogram_webhook/logs.pysrc/aiogram_webhook/py.typedsrc/aiogram_webhook/route/__init__.pysrc/aiogram_webhook/route/errors.pysrc/aiogram_webhook/route/params.pysrc/aiogram_webhook/route/query.pysrc/aiogram_webhook/route/route.pysrc/aiogram_webhook/routing/__init__.pysrc/aiogram_webhook/routing/base.pysrc/aiogram_webhook/routing/path.pysrc/aiogram_webhook/routing/query.pysrc/aiogram_webhook/routing/static.pysrc/aiogram_webhook/security/checks/check.pysrc/aiogram_webhook/security/checks/ip.pysrc/aiogram_webhook/security/errors.pysrc/aiogram_webhook/security/secret_token.pysrc/aiogram_webhook/security/security.pysrc/aiogram_webhook/tasks.pysrc/aiogram_webhook/utils/__init__.pysrc/aiogram_webhook/utils/config.pysrc/aiogram_webhook/utils/webhook_payload.pysrc/aiogram_webhook/web/__init__.pysrc/aiogram_webhook/web/aiohttp.pysrc/aiogram_webhook/web/base.pysrc/aiogram_webhook/web/fastapi.pytests/fixtures/__init__.pytests/fixtures/fixtures_bound_request.pytests/fixtures/fixtures_checks.pytests/test_ip_check.pytests/test_route.pytests/test_routing.pytests/test_secret_token.pytests/test_security.py
💤 Files with no reviewable changes (15)
- tests/fixtures/fixtures_bound_request.py
- src/aiogram_webhook/routing/base.py
- src/aiogram_webhook/adapters/base_adapter.py
- src/aiogram_webhook/adapters/fastapi/adapter.py
- src/aiogram_webhook/engines/simple.py
- src/aiogram_webhook/adapters/base_mapping.py
- src/aiogram_webhook/engines/init.py
- tests/test_routing.py
- src/aiogram_webhook/routing/init.py
- src/aiogram_webhook/adapters/fastapi/mapping.py
- src/aiogram_webhook/adapters/aiohttp/adapter.py
- src/aiogram_webhook/routing/static.py
- src/aiogram_webhook/routing/query.py
- src/aiogram_webhook/routing/path.py
- src/aiogram_webhook/adapters/aiohttp/mapping.py
Comment on lines
+76
to
+82
| values: list[QueryValue] = [] | ||
| for item in items: | ||
| if isinstance(item, str | int): | ||
| values.append(Const(item)) | ||
| else: | ||
| values.append(cast("QueryValue", item)) | ||
|
|
There was a problem hiding this comment.
Reject unsupported query item types during normalization.
Line 81 blindly casts unknown objects to QueryValue; invalid inputs then fail later with AttributeError instead of a config error.
Suggested fix
values: list[QueryValue] = []
for item in items:
- if isinstance(item, str | int):
+ if isinstance(item, str | int):
values.append(Const(item))
+ elif isinstance(item, QueryValue):
+ values.append(item)
else:
- values.append(cast("QueryValue", item))
+ raise RouteConfigError(
+ f"Invalid Route config: unsupported query value type for param {name!r}: {type(item).__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
| values: list[QueryValue] = [] | |
| for item in items: | |
| if isinstance(item, str | int): | |
| values.append(Const(item)) | |
| else: | |
| values.append(cast("QueryValue", item)) | |
| values: list[QueryValue] = [] | |
| for item in items: | |
| if isinstance(item, str | int): | |
| values.append(Const(item)) | |
| elif isinstance(item, QueryValue): | |
| values.append(item) | |
| else: | |
| raise RouteConfigError( | |
| f"Invalid Route config: unsupported query value type for param {name!r}: {type(item).__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 `@src/aiogram_webhook/route/query.py` around lines 76 - 82, The loop that
builds values currently treats any non-str/int item as a QueryValue by casting
(values, items, QueryValue, Const), which hides bad input and causes
AttributeError later; change the normalization in the items loop to explicitly
accept only str, int or actual QueryValue instances (use isinstance(item,
QueryValue) or equivalent) and immediately raise a clear TypeError/ValueError
with a descriptive message identifying the offending item when encountering any
other type, so invalid config is rejected early.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 57 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/aiogram_webhook/configs/webhook.py:16
- The previous model enforced Telegram's
max_connectionsrange, but the dataclass now accepts any integer even though the field documentation still says 1-100. Reintroduce validation (for example in__post_init__or beforeset_webhook) so invalid configuration fails locally instead of being sent to Telegram.
| overrides["allowed_updates"] = allowed_updates | ||
| return self.webhook_config.model_copy(update=overrides) if overrides else self.webhook_config | ||
| def _build_lifecycle_data(self, *, app: AppT, **kwargs) -> dict[str, Any]: | ||
| return {"app": app, "dispatcher": self.dispatcher, **kwargs} |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/aiogram_webhook/route/query.py (1)
70-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on unsupported query input types.
Line 70-85 still accepts invalid query input shapes/types: non-iterable unsupported values can throw raw
TypeError, and unsupported iterable items get coerced viaConst(item)instead of raising a route config error.Suggested fix
- if not value: + if not isinstance(value, (list, tuple)): + raise RouteConfigError( + f"Invalid Route config: unsupported query input type for param {name!r}: {type(value).__name__}." + ) + + if not value: raise RouteConfigError( f"Invalid Route config: query param must contain at least one value. Param: {name!r}." ) @@ values: list[QueryValue] = [] for item in value: if isinstance(item, QueryValue): values.append(item) + elif isinstance(item, (str, int)): + values.append(Const(item)) else: - values.append(Const(item)) + raise RouteConfigError( + f"Invalid Route config: unsupported query value type for param {name!r}: {type(item).__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 `@src/aiogram_webhook/route/query.py` around lines 70 - 85, The code accepts unsupported query input shapes and silently coerces unexpected iterable items; update the block that handles `value` so it fail-fast: if `value` is a str or int keep current `normalized[name] = (Const(value),)`, otherwise if `value` is not a proper iterable (e.g., not list/tuple/set) raise `RouteConfigError` with a clear message, and when iterating `for item in value` only accept `QueryValue` or supported primitive types (str/int) — if an item is unsupported, raise `RouteConfigError` instead of wrapping it with `Const(item)`. Use the existing symbols `RouteConfigError`, `QueryValue`, `Const`, and `normalized[name]` to locate and change the logic.
🧹 Nitpick comments (1)
src/aiogram_webhook/engines/single.py (1)
17-28: ⚡ Quick winMake bot session ownership explicit before closing it on shutdown.
SingleBotEnginereceives an externalBotinstance but unconditionally closes its session during shutdown. This contradictsTokenBotEngine, which checks session ownership before closing. If the bot's session is managed externally, this will unexpectedly tear down a shared resource.Add a
close_bot_sessionparameter (defaultFalse) to control whether the session is closed during shutdown, matching the ownership-aware pattern already established inTokenBotEngine.Suggested patch
class SingleBotEngine( BaseWebhookEngine[AppT, RawRequestT, FrameworkResponseT], Generic[AppT, RawRequestT, FrameworkResponseT] ): def __init__( self, dispatcher, bot: Bot, /, *, web: WebAdapter[AppT, RawRequestT, FrameworkResponseT], route: Route, security=None, webhook_config=None, handle_in_background: bool = True, + close_bot_session: bool = False, ) -> None: self.bot = bot self._task_tracker = TaskTracker() + self._close_bot_session = close_bot_session super().__init__( dispatcher, web=web, route=route, @@ async def on_shutdown(self, app: AppT, *args, **kwargs) -> None: # noqa: ARG002 logger.info("Stopping single-bot webhook engine for bot %s", self.bot.id) await self._task_tracker.close() @@ - if self.bot.session is not None: + if self._close_bot_session and self.bot.session is not None: await self.bot.session.close()🤖 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 `@src/aiogram_webhook/engines/single.py` around lines 17 - 28, SingleBotEngine's __init__ currently takes an external Bot but always closes its session on shutdown; add a close_bot_session: bool = False parameter to __init__ (store it as self._close_bot_session) so ownership is explicit. Update the shutdown method in SingleBotEngine to only await self._bot.session.close() when self._close_bot_session is True (mirroring TokenBotEngine's ownership check) and ensure any existing logic that references closing the bot session uses this flag instead.
🤖 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.
Duplicate comments:
In `@src/aiogram_webhook/route/query.py`:
- Around line 70-85: The code accepts unsupported query input shapes and
silently coerces unexpected iterable items; update the block that handles
`value` so it fail-fast: if `value` is a str or int keep current
`normalized[name] = (Const(value),)`, otherwise if `value` is not a proper
iterable (e.g., not list/tuple/set) raise `RouteConfigError` with a clear
message, and when iterating `for item in value` only accept `QueryValue` or
supported primitive types (str/int) — if an item is unsupported, raise
`RouteConfigError` instead of wrapping it with `Const(item)`. Use the existing
symbols `RouteConfigError`, `QueryValue`, `Const`, and `normalized[name]` to
locate and change the logic.
---
Nitpick comments:
In `@src/aiogram_webhook/engines/single.py`:
- Around line 17-28: SingleBotEngine's __init__ currently takes an external Bot
but always closes its session on shutdown; add a close_bot_session: bool = False
parameter to __init__ (store it as self._close_bot_session) so ownership is
explicit. Update the shutdown method in SingleBotEngine to only await
self._bot.session.close() when self._close_bot_session is True (mirroring
TokenBotEngine's ownership check) and ensure any existing logic that references
closing the bot session uses this flag instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87a7f539-48c6-4d3c-9279-0d4fea776235
📒 Files selected for processing (7)
src/aiogram_webhook/configs/webhook.pysrc/aiogram_webhook/engines/base.pysrc/aiogram_webhook/engines/single.pysrc/aiogram_webhook/engines/token.pysrc/aiogram_webhook/route/query.pysrc/aiogram_webhook/route/route.pysrc/aiogram_webhook/web/aiohttp.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/aiogram_webhook/configs/webhook.py
- src/aiogram_webhook/engines/token.py
- src/aiogram_webhook/engines/base.py
- src/aiogram_webhook/web/aiohttp.py
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 by CodeRabbit
New Features
Routeclass for flexible webhook URL configuration with path parameters and query supportSingleBotEngineand enhancedTokenEnginefor improved bot managementDocumentation
Refactor
routing→route,adapters→webChores