Conversation
WalkthroughCI workflows upgraded to newer actions and in-repo virtualenv caching; HTTP/auth flows now extract and preserve URL query parameters via a new helper Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Auth as ably.rest.auth
participant Helper as ably.util.helper
participant HTTP as ably.http.http
Client->>Auth: request token via auth_url (GET or POST)
Auth->>Helper: extract_url_params(auth_url)
Helper-->>Auth: clean_url + url_params
alt GET
Auth->>HTTP: GET clean_url with params = merge(url_params, auth_params, token_params)
HTTP-->>Auth: response
else POST
Auth->>HTTP: POST clean_url?url_params with body = auth_params + token_params
HTTP-->>Auth: response
end
Auth-->>Client: return token or raise error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the ✨ Finishing Touches🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ably/rest/auth.py (1)
427-429: Add an explicit timeout to the auth_url request.This path bypasses the library’s HTTP wrapper; without a timeout, calls can hang indefinitely.
Consider passing a timeout sourced from options (e.g.,
http_request_timeoutorrealtime_request_timeout) toclient.request(...).
🧹 Nitpick comments (8)
test/ably/realtime/realtimeinit_test.py (1)
36-36: Bounded wait is good; consider centralizing the timeout.To reduce repetition and flakiness spikes, use a shared constant or env-configured timeout instead of hardcoding 5s.
Apply locally here and reuse across tests:
- await asyncio.wait_for(ably.connection.once_async(ConnectionState.CONNECTED), timeout=5) + await asyncio.wait_for(ably.connection.once_async(ConnectionState.CONNECTED), timeout=CONNECTED_TIMEOUT)Define once per module (outside selected lines):
# near imports import os CONNECTED_TIMEOUT = float(os.getenv("ABLY_CONNECTED_TIMEOUT", "10"))test/ably/realtime/realtimechannel_test.py (1)
36-36: Good move adding timeouts; parameterize and dedupe.Multiple identical 5s waits suggest a shared helper/constant to ease tuning in CI.
Minimal change example (apply similarly to all occurrences):
- await asyncio.wait_for(ably.connection.once_async(ConnectionState.CONNECTED), timeout=5) + await asyncio.wait_for(ably.connection.once_async(ConnectionState.CONNECTED), timeout=CONNECTED_TIMEOUT)Add once per module (outside selected lines):
import os CONNECTED_TIMEOUT = float(os.getenv("ABLY_CONNECTED_TIMEOUT", "10")) async def wait_connected(ably, timeout=CONNECTED_TIMEOUT): return await asyncio.wait_for(ably.connection.once_async(ConnectionState.CONNECTED), timeout=timeout)Also applies to: 45-45, 65-65, 94-94, 113-113, 141-141, 168-168, 184-184, 219-219, 253-253, 271-271, 290-290, 310-310
test/ably/realtime/realtimeresume_test.py (1)
32-32: Wrap all unboundedonce_asyncawaits inasyncio.wait_forfor consistency
There are 46 occurrences of unwrappedawait ….once_async(...)across the realtime tests (e.g. eventemitter_test.py:25, realtimeresume_test.py:54, realtimeauth_test.py:21). Wrap each inawait asyncio.wait_for(…, timeout=5)(or another appropriate timeout) to prevent hangs and keep the pattern uniform.test/ably/realtime/realtimeconnection_test.py (1)
46-46: Nice—bounded waits reduce hangs; standardize the timeout.Same suggestion: replace literal 5s with a module-level constant or helper for easier tuning under CI load.
Example:
- await asyncio.wait_for(ably.connection.once_async(ConnectionState.CONNECTED), timeout=5) + await asyncio.wait_for(ably.connection.once_async(ConnectionState.CONNECTED), timeout=CONNECTED_TIMEOUT)Also consider adding timeouts to generic state-change awaits (e.g., Lines 20 and 22) if they’ve been sources of occasional hangs.
Also applies to: 73-73, 126-126, 165-165, 278-278, 287-287, 297-297, 342-342, 368-368, 397-397
.github/workflows/check.yml (2)
34-38: Prefer active Python and in-project venv for clarity.Small tweak to ensure Poetry uses the matrix Python explicitly.
run: | poetry config virtualenvs.create true --local poetry config virtualenvs.in-project true --local + poetry config virtualenvs.prefer-active-python true --local
23-25: Optional: upgrade checkout for security and performance.actions/checkout@v2 is old; move to v4 and consider pinning by commit SHA.
Outside selected lines:
- uses: actions/checkout@v2 + uses: actions/checkout@v4ably/rest/auth.py (1)
404-407: Don’t propagate URL fragments to the outbound request.Fragments (
#...) aren’t sent over HTTP. Keeping them inclean_urlis unnecessary and can confuse logs.Apply this diff:
- clean_url = f"{parsed_url.scheme}://{parsed_url.netloc}{parsed_url.path}" - if parsed_url.fragment: - clean_url += f"#{parsed_url.fragment}" + clean_url = f"{parsed_url.scheme}://{parsed_url.netloc}{parsed_url.path}"test/ably/realtime/realtimeauth_test.py (1)
609-611: Apply the same wait_for timeout to remaining CONNECTED waits for consistency.Keeps the suite uniformly bounded.
Apply this diff:
- await realtime.connection.once_async(ConnectionState.CONNECTED) + await asyncio.wait_for(realtime.connection.once_async(ConnectionState.CONNECTED), timeout=5)Also applies to: 648-649, 667-669
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.github/workflows/check.yml(1 hunks)ably/rest/auth.py(4 hunks)test/ably/realtime/realtimeauth_test.py(16 hunks)test/ably/realtime/realtimechannel_test.py(14 hunks)test/ably/realtime/realtimeconnection_test.py(10 hunks)test/ably/realtime/realtimeinit_test.py(2 hunks)test/ably/realtime/realtimeresume_test.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
test/ably/realtime/realtimeinit_test.py (3)
ably/realtime/realtime.py (1)
connection(132-134)ably/util/eventemitter.py (1)
once_async(168-181)ably/types/connectionstate.py (1)
ConnectionState(8-16)
test/ably/realtime/realtimechannel_test.py (3)
ably/realtime/realtime.py (1)
connection(132-134)ably/util/eventemitter.py (1)
once_async(168-181)ably/types/connectionstate.py (1)
ConnectionState(8-16)
test/ably/realtime/realtimeresume_test.py (6)
ably/realtime/realtime.py (1)
connection(132-134)ably/util/eventemitter.py (1)
once_async(168-181)ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/realtime/connection.py (1)
connection_manager(114-115)ably/transport/websockettransport.py (1)
dispose(187-199)ably/realtime/connectionmanager.py (1)
notify_state(346-384)
test/ably/realtime/realtimeconnection_test.py (2)
ably/util/eventemitter.py (1)
once_async(168-181)ably/types/connectionstate.py (1)
ConnectionState(8-16)
test/ably/realtime/realtimeauth_test.py (5)
ably/realtime/connectionmanager.py (2)
ably(515-516)notify_state(346-384)ably/realtime/realtime.py (1)
connection(132-134)ably/util/eventemitter.py (1)
once_async(168-181)ably/realtime/connection.py (1)
connection_manager(114-115)ably/transport/websockettransport.py (1)
dispose(187-199)
ably/rest/auth.py (3)
ably/rest/rest.py (3)
time(91-95)options(115-116)client_id(98-99)ably/http/http.py (5)
options(275-276)get(246-249)url(67-68)body(75-76)method(63-64)ably/types/tokendetails.py (3)
client_id(43-44)TokenDetails(7-91)to_dict(46-53)
🪛 actionlint (1.7.7)
.github/workflows/check.yml
39-39: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 GitHub Actions: .github/workflows/check.yml
ably/rest/auth.py
[error] 415-415: TypeError: dict() got multiple values for keyword argument 'foo' while constructing token request parameters in token_request_from_auth_url.
🔇 Additional comments (4)
test/ably/realtime/realtimeinit_test.py (1)
1-1: LGTM on import.Required for wait_for; no issues.
.github/workflows/check.yml (1)
27-27: Setup-python v5: LGTM.Covers 3.7–3.13; no issues anticipated.
ably/rest/auth.py (1)
277-278: LGTM on client_id assignment simplification.Clear and equivalent behavior.
test/ably/realtime/realtimeauth_test.py (1)
37-37: Good defensive timeouts around connection waits.This will prevent indefinite hangs and make failures surface quickly.
Also applies to: 56-56, 74-74, 96-96, 110-110, 124-124, 146-150, 162-162, 175-175, 189-189, 313-313, 340-340, 502-502, 517-517, 550-550, 578-578, 583-583
0b69525 to
762dd72
Compare
762dd72 to
3cc9a9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ably/rest/auth.py (2)
434-436: Remove trailing comma; error message is currently a tuple.
This degrades exception messages.- msg = 'auth_url responded with unacceptable content-type ' + content_type + \ - ', should be either text/plain, application/jwt or application/json', + msg = 'auth_url responded with unacceptable content-type ' + content_type + \ + ', should be either text/plain, application/jwt or application/json'
400-404: Normalize auth_params for GET; avoid unpacking non-mapping.
Ifauth_paramsis aTokenDetails,**auth_paramsraises. Normalize like the POST branch.if method == 'GET': - body = {} - # Merge URL params, auth_params, and token_params (later params override earlier ones) - # we do this because httpx version has inconsistency and some versions override query params - # that are specified in url string - params = {**url_params, **auth_params, **token_params} + body = None # do not send a body with GET + # Normalize and merge (later overrides earlier) + if isinstance(auth_params, TokenDetails): + auth_params = auth_params.to_dict() + params = {**url_params, **auth_params, **token_params}
🧹 Nitpick comments (3)
test/ably/realtime/realtimeinit_test.py (1)
36-36: Avoid test flakes: make timeout configurable and slightly higher.
Hard-coding 5s can intermittently fail in CI. Consider using an env-driven or module-level constant (e.g., default 10s) shared across tests.Example:
- await asyncio.wait_for(ably.connection.once_async(ConnectionState.CONNECTED), timeout=5) + # Prefer an env-driven default to reduce CI flakes + timeout = int(os.getenv("ABLY_TEST_CONNECT_TIMEOUT", "10")) + await asyncio.wait_for(ably.connection.once_async(ConnectionState.CONNECTED), timeout=timeout)Additional change outside this hunk:
import os # at the top near other importsably/util/helper.py (2)
42-48: Be explicit about duplicate query keys (current behavior drops all but the last).
Flattening with “last value wins” may hide multi-valued params. If that’s intentional for Ably use-cases, add a short note; otherwise consider preserving all values or raising on duplicates.
49-54: Reconstruct URL with urlunparse and drop fragment for requests.
Fragments aren’t sent over HTTP and can confuse logging/caching. Also preserve path “params” safely.Apply:
-from urllib.parse import urlparse, parse_qs +from urllib.parse import urlparse, parse_qs, urlunparse @@ - # Reconstruct clean URL without query parameters - clean_url = f"{parsed_url.scheme}://{parsed_url.netloc}{parsed_url.path}" - if parsed_url.fragment: - clean_url += f"#{parsed_url.fragment}" + # Reconstruct clean URL without query/fragment + clean_parts = parsed_url._replace(query='', fragment='') + clean_url = urlunparse(clean_parts)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
ably/http/http.py(2 hunks)ably/rest/auth.py(5 hunks)ably/util/helper.py(2 hunks)pyproject.toml(1 hunks)test/ably/realtime/realtimeauth_test.py(16 hunks)test/ably/realtime/realtimechannel_test.py(14 hunks)test/ably/realtime/realtimeconnection_test.py(10 hunks)test/ably/realtime/realtimeinit_test.py(2 hunks)test/ably/realtime/realtimeresume_test.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/ably/realtime/realtimechannel_test.py
- test/ably/realtime/realtimeconnection_test.py
- test/ably/realtime/realtimeresume_test.py
- test/ably/realtime/realtimeauth_test.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T17:21:12.643Z
Learnt from: ttypic
PR: ably/ably-python#621
File: ably/rest/auth.py:407-409
Timestamp: 2025-09-04T17:21:12.643Z
Learning: In Python, dict(mapping, **kwargs) with overlapping keys does NOT raise TypeError - the kwargs override the mapping values. Only dict(**dict1, **dict2) with overlapping keys raises TypeError.
Applied to files:
ably/rest/auth.py
🧬 Code graph analysis (4)
ably/util/helper.py (1)
ably/http/http.py (1)
url(67-68)
test/ably/realtime/realtimeinit_test.py (2)
ably/util/eventemitter.py (1)
once_async(168-181)ably/types/connectionstate.py (1)
ConnectionState(8-16)
ably/http/http.py (2)
ably/util/helper.py (1)
extract_url_params(29-54)ably/rest/rest.py (1)
request(122-142)
ably/rest/auth.py (2)
ably/util/helper.py (1)
extract_url_params(29-54)ably/types/tokendetails.py (2)
client_id(43-44)to_dict(46-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.13)
- GitHub Check: check (3.9)
- GitHub Check: check (3.10)
- GitHub Check: check (3.12)
- GitHub Check: check (3.11)
- GitHub Check: check (3.7)
- GitHub Check: check (3.8)
🔇 Additional comments (5)
test/ably/realtime/realtimeinit_test.py (1)
1-1: LGTM: asyncio import aligns with new timeout usage.pyproject.toml (1)
66-69: respx ≥0.22.0 supports Python 3.13; no extra guard needed
respx 0.22.0 (Dec 19 2024) added Python 3.13 to its test suite and declares support for 3.8+; the existing^0.22.0pin already covers 3.13.ably/http/http.py (1)
14-15: LGTM: shared helper import promotes consistency.ably/rest/auth.py (2)
393-395: LGTM: centralizing URL param extraction here improves parity with http layer.
405-409: POST body merge is fine; mirror normalization done above.
…ime tests - Replaced all `connection.once_async` calls with `asyncio.wait_for` to include a 5-second timeout. - Ensures tests fail gracefully if connection isn't established within the specified timeframe.
3cc9a9a to
9bfa4db
Compare
490767a to
0659f89
Compare
0659f89 to
a1dfdd7
Compare
a1dfdd7 to
05054a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
.github/workflows/check.yml (1)
43-49: Update cache action to v4 and add restore-keys.Prevents failures on ubuntu-latest and improves cache hit rates across lockfile changes.
Apply this diff:
- - uses: actions/cache@v3 + - uses: actions/cache@v4 name: Define a cache for the virtual environment based on the dependencies lock file id: cache with: path: ./.venv key: venv-${{ runner.os }}-${{ matrix.python-version }}-${{ hashFiles('poetry.lock') }} + restore-keys: | + venv-${{ runner.os }}-${{ matrix.python-version }}-
🧹 Nitpick comments (2)
setup.cfg (1)
11-11: Consider excluding common tool caches as well.Add mypy/ruff caches (and optionally .tox) to cut noise locally and in CI.
Apply this diff:
-exclude = .venv,venv,env,.env,.git,__pycache__,.pytest_cache,build,dist,*.egg-info +exclude = .venv,venv,env,.env,.git,__pycache__,.pytest_cache,.mypy_cache,.ruff_cache,.tox,build,dist,*.egg-infotest/ably/realtime/realtimeconnection_test.py (1)
46-46: Bounded waits look good; consider centralizing timeout and applying consistently.Wrapping CONNECTED waits with asyncio.wait_for prevents indefinite hangs. To reduce duplication and allow tuning in CI, add a single helper/constant (e.g., TEST_RT_CONNECT_TIMEOUT=5 from env) and reuse it across tests; also consider applying timeouts to other awaits that can block.
Example helper:
# tests/utils/asyncio_helpers.py import os, asyncio CONNECT_TIMEOUT = int(os.environ.get("TEST_RT_CONNECT_TIMEOUT", "5")) async def wait_connected(conn): return await asyncio.wait_for(conn.once_async(ConnectionState.CONNECTED), timeout=CONNECT_TIMEOUT)Also applies to: 73-73, 126-126, 165-165, 279-279, 287-287, 297-297, 342-342, 368-368, 397-397
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/check.yml(1 hunks).github/workflows/lint.yml(1 hunks)ably/http/http.py(2 hunks)ably/rest/auth.py(5 hunks)ably/util/helper.py(2 hunks)pyproject.toml(1 hunks)setup.cfg(1 hunks)test/ably/realtime/realtimeauth_test.py(16 hunks)test/ably/realtime/realtimechannel_test.py(14 hunks)test/ably/realtime/realtimeconnection_test.py(10 hunks)test/ably/realtime/realtimeinit_test.py(2 hunks)test/ably/realtime/realtimeresume_test.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- test/ably/realtime/realtimeresume_test.py
- test/ably/realtime/realtimeauth_test.py
- test/ably/realtime/realtimeinit_test.py
- ably/http/http.py
- pyproject.toml
- ably/rest/auth.py
- test/ably/realtime/realtimechannel_test.py
🧰 Additional context used
🧬 Code graph analysis (2)
ably/util/helper.py (1)
ably/http/http.py (1)
url(67-68)
test/ably/realtime/realtimeconnection_test.py (1)
ably/util/eventemitter.py (1)
once_async(168-181)
🪛 actionlint (1.7.7)
.github/workflows/lint.yml
36-36: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/check.yml
43-43: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: check (3.10)
- GitHub Check: check (3.13)
- GitHub Check: check (3.11)
- GitHub Check: check (3.9)
- GitHub Check: check (3.12)
🔇 Additional comments (1)
setup.cfg (1)
10-11: Good call adding .venv to flake8 exclude.This avoids linting the cached, in-project venv created by CI.
05054a8 to
23d1036
Compare
23d1036 to
30fdc5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/check.yml (1)
44-50: Add a restore key to improve cache hit rate across minor lockfile changes.This was suggested earlier; re-raising for visibility as it still applies.
- uses: actions/cache@v4 name: Define a cache for the virtual environment based on the dependencies lock file id: cache with: path: ./.venv key: venv-${{ runner.os }}-${{ matrix.python-version }}-${{ hashFiles('poetry.lock') }} + restore-keys: | + venv-${{ runner.os }}-${{ matrix.python-version }}-
🧹 Nitpick comments (3)
.github/workflows/lint.yml (2)
34-40: Consider adding a restore key for better cache reuse.Optional, but can improve hit rate when the lockfile changes slightly; Poetry will reconcile deltas on install.
- uses: actions/cache@v4 name: Define a cache for the virtual environment based on the dependencies lock file id: cache with: path: ./.venv key: venv-${{ runner.os }}-3.9-${{ hashFiles('poetry.lock') }} + restore-keys: | + venv-${{ runner.os }}-3.9-
16-21: Avoid Python version drift between setup and cache key.Extract the version to a single env var so future bumps don’t miss the cache key.
lint: runs-on: ubuntu-latest + env: + PY_VER: '3.9' steps: - - name: Set up Python 3.9 + - name: Set up Python ${{ env.PY_VER }} uses: actions/setup-python@v5 id: setup-python with: - python-version: '3.9' + python-version: ${{ env.PY_VER }} ... - key: venv-${{ runner.os }}-3.9-${{ hashFiles('poetry.lock') }} + key: venv-${{ runner.os }}-${{ env.PY_VER }}-${{ hashFiles('poetry.lock') }}Also applies to: 39-39
.github/workflows/check.yml (1)
56-61: Bound the job itself to prevent hangs despite per-test timeouts.Optional: add a job-level
timeout-minutes(e.g., 45–60) and/or a concurrency group to auto-cancel superseded runs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/check.yml(1 hunks).github/workflows/lint.yml(1 hunks)pyproject.toml(1 hunks)setup.cfg(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- setup.cfg
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.13)
- GitHub Check: check (3.11)
- GitHub Check: check (3.12)
- GitHub Check: check (3.8)
- GitHub Check: check (3.7)
- GitHub Check: check (3.9)
- GitHub Check: check (3.10)
🔇 Additional comments (4)
.github/workflows/lint.yml (3)
22-26: Poetry setup looks good.Using abatilo/actions-poetry@v4 with 2.1.4 aligns with the PR intent and simplifies PATH concerns vs pipx.
27-33: Good switch to in-project venv and explicit interpreter.
poetry env use ${{ steps.setup-python.outputs.python-path }}+ in-project venv makes the cache deterministic.
41-45: Cache health check is solid.Nice safeguard to auto-drop a bad venv and let Poetry recreate it.
.github/workflows/check.yml (1)
37-43: Venv bootstrapping is correct.Explicit interpreter selection + in-project venv works well across the matrix.
There was a problem hiding this comment.
LGTM ( You can check comment at #621 (comment))
Summary by CodeRabbit
Bug Fixes
Tests
Chores