refactor(testing): modernize testing infrastructure (ENG-34)#16
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 Plus 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 |
![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
🤖 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 `@tests/authentication/test_auth.py`:
- Around line 277-284: Update the reuse-detection flow in the authentication
test to restore `token_a` in `client.cookies` after the successful rotation
request and before the second `POST /auth/refresh`; ensure the request
explicitly presents the revoked original token rather than the rotated cookie.
In `@tests/authentication/test_google_oauth2.py`:
- Around line 25-35: Extend the OAuth state flow tested by
test_google_login_redirect to persist the initiating client’s one-time nonce,
require the callback client to match that binding, and consume the nonce after
successful validation. Add tests proving a state issued for client A is rejected
by client B and that a successfully accepted state cannot be replayed, while
preserving the existing signature and age checks.
- Around line 197-251: Extend test_google_callback_inactive_user to query the
inactive user after the rejected callback and assert its google_id remains
unset. Update login_with_google to validate is_active before persisting the
Google account link, ensuring the 403 path does not mutate the user.
- Around line 18-22: Update the setup_oauth_config fixture to isolate its Google
OAuth overrides by using monkeypatch or explicitly saving and restoring the
original settings values after each test. Ensure the cached Settings singleton
is returned to its pre-test state for both google_client_id and
google_client_secret.
In `@tests/authentication/test_password_reset.py`:
- Around line 327-353: Update the reset email assertion in the password-reset
test to use mock_send.assert_awaited_once() instead of
mock_send.assert_called_once(), preserving the existing call_kwargs and
email-content assertions.
In `@tests/fixtures/client.py`:
- Around line 32-40: Preserve shared state during fixture teardown: in
tests/fixtures/client.py lines 32-40, capture the previous get_db entry in
app.dependency_overrides and restore it afterward, removing only that entry when
no prior override existed; in tests/fixtures/mocks.py lines 18-25, capture the
existing limiter.enabled value before changing it and restore that exact value
during teardown.
🪄 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 Plus
Run ID: 1394aef7-4b6c-4c69-a54f-c4d246a42e34
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
src/ai_trading_discipline_copilot/routers/auth.pytests/README.mdtests/api/test_rate_limiting.pytests/authentication/test_auth.pytests/authentication/test_email_verification.pytests/authentication/test_google_oauth2.pytests/authentication/test_password_reset.pytests/conftest.pytests/factories/__init__.pytests/factories/user_factory.pytests/fixtures/__init__.pytests/fixtures/auth.pytests/fixtures/client.pytests/fixtures/database.pytests/fixtures/mocks.pytests/fixtures/tokens.pytests/fixtures/users.pytests/helpers/__init__.pytests/helpers/assertions.pytests/helpers/database.pytests/helpers/jwt.pytests/mocks/__init__.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🤖 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 `@tests/authentication/test_auth.py`:
- Around line 277-284: Update the reuse-detection flow in the authentication
test to restore `token_a` in `client.cookies` after the successful rotation
request and before the second `POST /auth/refresh`; ensure the request
explicitly presents the revoked original token rather than the rotated cookie.
In `@tests/authentication/test_google_oauth2.py`:
- Around line 25-35: Extend the OAuth state flow tested by
test_google_login_redirect to persist the initiating client’s one-time nonce,
require the callback client to match that binding, and consume the nonce after
successful validation. Add tests proving a state issued for client A is rejected
by client B and that a successfully accepted state cannot be replayed, while
preserving the existing signature and age checks.
- Around line 197-251: Extend test_google_callback_inactive_user to query the
inactive user after the rejected callback and assert its google_id remains
unset. Update login_with_google to validate is_active before persisting the
Google account link, ensuring the 403 path does not mutate the user.
- Around line 18-22: Update the setup_oauth_config fixture to isolate its Google
OAuth overrides by using monkeypatch or explicitly saving and restoring the
original settings values after each test. Ensure the cached Settings singleton
is returned to its pre-test state for both google_client_id and
google_client_secret.
In `@tests/authentication/test_password_reset.py`:
- Around line 327-353: Update the reset email assertion in the password-reset
test to use mock_send.assert_awaited_once() instead of
mock_send.assert_called_once(), preserving the existing call_kwargs and
email-content assertions.
In `@tests/fixtures/client.py`:
- Around line 32-40: Preserve shared state during fixture teardown: in
tests/fixtures/client.py lines 32-40, capture the previous get_db entry in
app.dependency_overrides and restore it afterward, removing only that entry when
no prior override existed; in tests/fixtures/mocks.py lines 18-25, capture the
existing limiter.enabled value before changing it and restore that exact value
during teardown.
🪄 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 Plus
Run ID: 1394aef7-4b6c-4c69-a54f-c4d246a42e34
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
src/ai_trading_discipline_copilot/routers/auth.pytests/README.mdtests/api/test_rate_limiting.pytests/authentication/test_auth.pytests/authentication/test_email_verification.pytests/authentication/test_google_oauth2.pytests/authentication/test_password_reset.pytests/conftest.pytests/factories/__init__.pytests/factories/user_factory.pytests/fixtures/__init__.pytests/fixtures/auth.pytests/fixtures/client.pytests/fixtures/database.pytests/fixtures/mocks.pytests/fixtures/tokens.pytests/fixtures/users.pytests/helpers/__init__.pytests/helpers/assertions.pytests/helpers/database.pytests/helpers/jwt.pytests/mocks/__init__.py
🛑 Comments failed to post (6)
tests/authentication/test_auth.py (1)
277-284: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash sed -n '239,303p' tests/authentication/test_auth.py sed -n '19,40p' tests/fixtures/client.py rg -n 'httpx|AsyncClient' pyproject.toml poetry.lock uv.lock requirements*.txt 2>/dev/null || trueRepository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 3872
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== refresh endpoint locations ==\n' rg -n "refresh_token|set_cookie|/auth/refresh|reuse detection|revoked due to reuse detection" tests app src . 2>/dev/null || true printf '\n== auth route outline ==\n' fd -a 'auth*.py' . 2>/dev/null | head -n 20Repository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 31208
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' import asyncio import httpx from starlette.applications import Starlette from starlette.responses import PlainTextResponse from starlette.routing import Route seen = [] async def login(request): resp = PlainTextResponse("ok") resp.set_cookie("refresh_token", "token_a") return resp async def refresh(request): seen.append(request.cookies.get("refresh_token")) resp = PlainTextResponse("refreshed") if request.cookies.get("refresh_token") == "token_a": resp.set_cookie("refresh_token", "token_b") return resp app = Starlette(routes=[Route("/login", login, methods=["POST"]), Route("/refresh", refresh, methods=["POST"])]) async def main(): async with httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url="http://testserver") as client: r1 = await client.post("/login") print("after login client.cookies:", dict(client.cookies)) print("login response cookies:", dict(r1.cookies)) r2 = await client.post("/refresh") print("after first refresh client.cookies:", dict(client.cookies)) print("first refresh response cookies:", dict(r2.cookies)) r3 = await client.post("/refresh") print("seen cookies:", seen) print("after second refresh client.cookies:", dict(client.cookies)) print("second refresh response status:", r3.status_code) asyncio.run(main()) PYRepository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 560
tests/authentication/test_auth.py:277-284 — Restore
token_abefore the reuse-detection request.AsyncClientcarries the rotatedrefresh_tokenforward inclient.cookies, so the secondPOST /auth/refreshsends the new token instead of revokedtoken_a.Proposed fix
rotate_res = await client.post("/auth/refresh") assert rotate_res.status_code == 200 # 4. Present Session A again (Reuse Detection!) + client.cookies.clear() + client.cookies.set("refresh_token", token_a) response = await client.post("/auth/refresh")📝 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.# 3. Rotate Session A client.cookies.clear() client.cookies.set("refresh_token", token_a) rotate_res = await client.post("/auth/refresh") assert rotate_res.status_code == 200 # 4. Present Session A again (Reuse Detection!) client.cookies.clear() client.cookies.set("refresh_token", token_a) response = await client.post("/auth/refresh")🤖 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 `@tests/authentication/test_auth.py` around lines 277 - 284, Update the reuse-detection flow in the authentication test to restore `token_a` in `client.cookies` after the successful rotation request and before the second `POST /auth/refresh`; ensure the request explicitly presents the revoked original token rather than the rotated cookie.tests/authentication/test_google_oauth2.py (3)
18-22: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- tests/authentication/test_google_oauth2.py ---' cat -n tests/authentication/test_google_oauth2.py printf '\n%s\n' '--- search for google_client_id / google_client_secret usage ---' rg -n "google_client_id|google_client_secret|SecretStr\(" tests src . -g '!**/.git/**' printf '\n%s\n' '--- settings definition search ---' rg -n "class .*Settings|google_client_id|google_client_secret|settings =" . -g '!**/.git/**'Repository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 27703
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- src/ai_trading_discipline_copilot/core/config.py ---' cat -n src/ai_trading_discipline_copilot/core/config.py printf '\n%s\n' '--- search for get_settings cache/dependency pattern ---' rg -n "def get_settings|`@lru_cache`|cached|Settings\\(" src/ai_trading_discipline_copilot/core/config.py src/ai_trading_discipline_copilot -g '!**/.git/**' printf '\n%s\n' '--- other tests mutating settings ---' rg -n "patch\\.object\\(settings|settings\\.[a-z_]+ =" tests -g '!**/.git/**'Repository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 5965
Restore the Google OAuth settings after each test
The autouse fixture mutates the cachedSettingssingleton returned byget_settings(), so later tests can inheritgoogle_client_idandgoogle_client_secretfrom this module. Usemonkeypatchor a save/restore fixture so the overrides are torn down automatically.🤖 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 `@tests/authentication/test_google_oauth2.py` around lines 18 - 22, Update the setup_oauth_config fixture to isolate its Google OAuth overrides by using monkeypatch or explicitly saving and restoring the original settings values after each test. Ensure the cached Settings singleton is returned to its pre-test state for both google_client_id and google_client_secret.
25-35: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Add cross-client and replay rejection tests for OAuth state.
The current tests only verify signature and age. The supplied callback accepts any app-signed state without binding its nonce to the initiating browser or consuming it, so another client can reuse that state during the 10-minute window.
Add tests proving that:
- A state issued to client A is rejected from client B.
- A successfully consumed state cannot be replayed.
This will require persisting and validating a one-time state nonce in the implementation.
Also applies to: 162-194
🤖 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 `@tests/authentication/test_google_oauth2.py` around lines 25 - 35, Extend the OAuth state flow tested by test_google_login_redirect to persist the initiating client’s one-time nonce, require the callback client to match that binding, and consume the nonce after successful validation. Add tests proving a state issued for client A is rejected by client B and that a successfully accepted state cannot be replayed, while preserving the existing signature and age checks.
197-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Verify that rejected inactive-user callbacks do not mutate the account.
login_with_googlecurrently commits the Google link before checkingis_active, so this 403 still persistsgoogle_id. Assert that the failed login is side-effect free; the resulting failure should be fixed by checking account status before linking.Proposed regression assertion
assert response.status_code == 403 assert response.json()["detail"] == "User account is disabled" + await db_session.refresh(inactive_user) + assert inactive_user.google_id is None📝 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.`@pytest.mark.anyio` async def test_google_callback_inactive_user( client: AsyncClient, db_session: AsyncSession ) -> None: """Test Google callback fails if the matching user is inactive.""" # Pre-add inactive user inactive_user = User( username="inactive", email="inactive@example.com", hashed_password="somepasswordhash", # noqa: S106 is_verified=True, is_active=False, ) db_session.add(inactive_user) await db_session.commit() state_payload = { "timestamp": datetime.now(UTC).timestamp(), "nonce": uuid.uuid4().hex, } state_token = jwt.encode( state_payload, settings.secret_key.get_secret_value(), algorithm=settings.algorithm, ) mock_token_resp = MagicMock() mock_token_resp.status_code = 200 mock_token_resp.json.return_value = {"access_token": "google-access-token"} mock_userinfo_resp = MagicMock() mock_userinfo_resp.status_code = 200 mock_userinfo_resp.json.return_value = { "sub": "google-id-inactive", "email": "inactive@example.com", "email_verified": True, } mock_client = AsyncMock() mock_client.__aenter__.return_value = mock_client mock_client.post.return_value = mock_token_resp mock_client.get.return_value = mock_userinfo_resp with patch( "ai_trading_discipline_copilot.routers.auth.httpx.AsyncClient", return_value=mock_client, ): response = await client.get( "/auth/oauth2/google/callback", params={"code": "auth-code", "state": state_token}, follow_redirects=False, ) assert response.status_code == 403 assert response.json()["detail"] == "User account is disabled" await db_session.refresh(inactive_user) assert inactive_user.google_id is None🤖 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 `@tests/authentication/test_google_oauth2.py` around lines 197 - 251, Extend test_google_callback_inactive_user to query the inactive user after the rejected callback and assert its google_id remains unset. Update login_with_google to validate is_active before persisting the Google account link, ensuring the 403 path does not mutate the user.tests/authentication/test_password_reset.py (1)
327-353: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash email_file="$(fd -a '^email_service\.py$' . | head -n1)" ast-grep outline "$email_file" --match send_email --view expanded rg -n -C3 '\bawait\s+EmailService\.send_email\s*\(' .Repository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 1440
🏁 Script executed:
#!/bin/bash set -euo pipefail file="src/ai_trading_discipline_copilot/services/email_service.py" wc -l "$file" rg -n -C4 'def send_email|async def send_email|class EmailService' "$file" sed -n '1,220p' "$file"Repository: TradeDisciplineAI/ai-trading-discipline-copilot
Length of output: 4170
Use
assert_awaited_once()for the reset email mock
EmailService.send_emailis async, soassert_called_once()can pass even if the coroutine is never awaited. Replace it withmock_send.assert_awaited_once().🤖 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 `@tests/authentication/test_password_reset.py` around lines 327 - 353, Update the reset email assertion in the password-reset test to use mock_send.assert_awaited_once() instead of mock_send.assert_called_once(), preserving the existing call_kwargs and email-content assertions.tests/fixtures/client.py (1)
32-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve global state across fixture teardown.
Both fixtures overwrite shared process state rather than restoring exactly what existed before setup:
tests/fixtures/client.py#L32-L40: restore or remove only the priorget_dboverride instead of clearing every dependency override.tests/fixtures/mocks.py#L18-L25: capture and restore the priorlimiter.enabledvalue instead of always enabling it.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 35-35: Configuring an LLM/agent client endpoint over http:// sends prompts and responses (and often API keys) in cleartext, exposing them to interception. Use https for the base_url.
Context: base_url="http://testserver"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.(llm-client-insecure-http-python)
[warning] 35-35: Do not make http calls without encryption
Context: "http://testserver"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.(requests-http)
📍 Affects 2 files
tests/fixtures/client.py#L32-L40(this comment)tests/fixtures/mocks.py#L18-L25🤖 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 `@tests/fixtures/client.py` around lines 32 - 40, Preserve shared state during fixture teardown: in tests/fixtures/client.py lines 32-40, capture the previous get_db entry in app.dependency_overrides and restore it afterward, removing only that entry when no prior override existed; in tests/fixtures/mocks.py lines 18-25, capture the existing limiter.enabled value before changing it and restore that exact value during teardown.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ai_trading_discipline_copilot/routers/auth.py`:
- Around line 709-717: Update the OAuth state cookie configuration in the
RedirectResponse flow to pass settings.cookie_secure via the secure option,
matching the refresh cookie behavior and ensuring production transmits it only
over HTTPS.
🪄 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 Plus
Run ID: e91ebf69-9c84-4661-8270-ff8edfd63447
📒 Files selected for processing (4)
src/ai_trading_discipline_copilot/routers/auth.pysrc/ai_trading_discipline_copilot/services/auth_service.pytests/authentication/test_auth.pytests/authentication/test_google_oauth2.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/authentication/test_auth.py
| redirect_response = RedirectResponse(url=google_auth_url) | ||
| redirect_response.set_cookie( | ||
| key="oauth_state", | ||
| value=state_token, | ||
| httponly=True, | ||
| max_age=600, | ||
| path="/auth", | ||
| samesite="lax", | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Mark the OAuth state cookie as secure in production.
Unlike the refresh cookie, this security-sensitive cookie omits secure=settings.cookie_secure, allowing it to be transmitted over plaintext HTTP.
🔒 Proposed fix
redirect_response.set_cookie(
key="oauth_state",
value=state_token,
httponly=True,
+ secure=settings.cookie_secure,
max_age=600,
path="/auth",
samesite="lax",
)📝 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.
| redirect_response = RedirectResponse(url=google_auth_url) | |
| redirect_response.set_cookie( | |
| key="oauth_state", | |
| value=state_token, | |
| httponly=True, | |
| max_age=600, | |
| path="/auth", | |
| samesite="lax", | |
| ) | |
| redirect_response = RedirectResponse(url=google_auth_url) | |
| redirect_response.set_cookie( | |
| key="oauth_state", | |
| value=state_token, | |
| httponly=True, | |
| secure=settings.cookie_secure, | |
| max_age=600, | |
| path="/auth", | |
| samesite="lax", | |
| ) |
🤖 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/ai_trading_discipline_copilot/routers/auth.py` around lines 709 - 717,
Update the OAuth state cookie configuration in the RedirectResponse flow to pass
settings.cookie_secure via the secure option, matching the refresh cookie
behavior and ensuring production transmits it only over HTTPS.
2 participants
Summary by CodeRabbit
Security Enhancements
Bug Fixes
Documentation