Unit and integration tests#18
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: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR updates CI and project guidance, adds fail-fast HTTP-client guards and validation checks in core and workspace/team/user APIs, and introduces shared test fixtures plus new unit and integration coverage. ChangesRepository validation and test coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 9
🧹 Nitpick comments (7)
.github/workflows/ci.yml (2)
10-46: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an explicit least-privilege
permissionsblock.The workflow has no
permissions:declaration, so the job inherits the repository defaultGITHUB_TOKENscopes, which can be broader than this CI job needs (it only checks out and runs tooling). Scoping to read-only reduces blast radius if a step is compromised.🔒 Proposed least-privilege permissions
jobs: ci: runs-on: ubuntu-latest + permissions: + contents: read strategy:🤖 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 @.github/workflows/ci.yml around lines 10 - 46, Add an explicit least-privilege permissions block to the ci job/workflow so the default GITHUB_TOKEN does not inherit broader repository scopes. Update the workflow around the existing ci job and its checkout/tooling steps to declare read-only access only, since this job only needs to read the repository and run checks; use the workflow/job permissions setting alongside actions/checkout, setup-uv, and the test/lint steps.Source: Linters/SAST tools
25-26: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: pin
actions/setup-pythonto a commit SHA.
actions/setup-python@v5(like the other tag-pinned actions here) relies on a mutable tag; a blanket supply-chain policy flagged this. Pinning to a full commit SHA hardens against tag-retargeting. Apply consistently tocheckout,setup-uv, andsetup-pythonif you adopt this.🤖 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 @.github/workflows/ci.yml around lines 25 - 26, The CI workflow currently uses tag-pinned GitHub Actions, and the review asks to harden supply-chain security by pinning them to immutable commit SHAs. Update the action references in the workflow so the existing uses of checkout, setup-uv, and setup-python are switched from mutable version tags to full commit SHA pins, keeping the same setup behavior while making the identifiers for those steps more secure and consistent.Source: Linters/SAST tools
README.md (1)
22-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code blocks.
markdownlint (MD040) flags the two fenced blocks (Lines 22 and 32) for missing language specifiers. Tag them as shell for proper rendering/linting.
📝 Proposed fix
-``` +```shell uv run pytest # full suite with coverage (configured in pyproject.toml)-``` +```shell uvx pyright --pythonpath .venv/bin/python api tests🤖 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 `@README.md` around lines 22 - 35, The fenced code blocks in the README are missing language specifiers and are triggering markdownlint MD040. Update the two fenced blocks around the pytest commands and the type-check/format commands to use a shell language tag, matching the existing command examples, so the blocks render and lint correctly.Source: Linters/SAST tools
api/core/security.py (1)
173-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmpty
#@test:placeholder.This annotation has no test description, unlike the other authoritative
@testoutlines. Either fill it in to describevalidate_token's intended coverage (cache hit/miss, token rotation eviction, 401 paths) or remove it.Want me to draft the
@test:outline lines forvalidate_token?🤖 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 `@api/core/security.py` around lines 173 - 175, The `# `@test`:` annotation under `validate_token` is empty and should either be completed or removed. Update the placeholder to describe the intended test coverage for `validate_token`—including cache hit/miss behavior, token rotation eviction, and 401 response paths—or delete the annotation if it is not meant to be used, keeping it consistent with the other `@test` outlines in this module.api/src/workspaces/routes.py (1)
159-159: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
assertfor a runtime invariant is stripped underpython -O.This
assertdoubles as Pyright narrowing forworkspace.id: Optional[int], but if the app ever runs with-O, the check vanishes and aNoneid would silently flow intoassign_member_role(...)and the response. Prefer an explicit guard that raises regardless of optimization level. Same pattern exists inapi/src/workspaces/schemas.py:238.♻️ Suggested explicit guard
- assert workspace.id is not None # freshly persisted workspace has an id + if workspace.id is None: # freshly persisted workspace must have an id + raise RuntimeError("Persisted workspace is missing an id")🤖 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 `@api/src/workspaces/routes.py` at line 159, The runtime invariant in workspaces routing is currently enforced with an assert, which disappears under optimized Python runs. Replace the assert in the workspace member assignment flow with an explicit guard that raises a real exception if workspace.id is None, so the check still runs before assign_member_role(...) and response construction; apply the same pattern anywhere else this is used, including the similar Optional[int] narrowing case in the schemas code.api/src/workspaces/schemas.py (1)
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor grammar typo in
@testannotation (repeated)."any values from Python and properly serialized" should read "any values from Python are properly serialized". Since these comments drive AI test generation, the typo also appears at Lines 112 and 261.
🤖 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 `@api/src/workspaces/schemas.py` at line 84, The repeated `@test` annotation text has a grammar typo in the schema comments, where “any values from Python and properly serialized” should be “any values from Python are properly serialized.” Update the affected `@test` comment(s) in the schemas definitions, including the repeated occurrences referenced by this same wording, so the test-generation annotation reads correctly and consistently.tests/unit/test_teams_schemas.py (1)
21-27: 📐 Maintainability & Code Quality | 🔵 TrivialHoist the shared schema helpers
_tableand_fk_targetsare duplicated intests/unit/test_users_schemas.py,tests/unit/test_workspaces_schemas.py, and this file; move them intotests/supportto keep a single definition.🤖 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/unit/test_teams_schemas.py` around lines 21 - 27, The shared schema helper functions `_table` and `_fk_targets` are duplicated across multiple schema test files, so move their single implementation into a reusable helper under `tests/support` and update `test_teams_schemas`, `test_users_schemas`, and `test_workspaces_schemas` to import and use it. Keep the helper names or equivalent unique symbols consistent so the schema tests still reference the same `__table__` and foreign key target logic from one central place.Sources: Coding guidelines, Linters/SAST tools
🤖 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 `@api/core/jwt.py`:
- Around line 8-12: The `@test` outline in JWT-related comments is describing JSON
schema validation and network failure handling, which does not match the actual
responsibilities of the JWT logic in core JWT code. Update the `@test` comments in
the JWT class/module to describe JWT-specific behavior that matches the symbols
in this diff and the surrounding JWT implementation, so the authoritative test
contract reflects what this module באמת does. Keep the comments aligned with the
actual JWT parsing/validation/authentication flow rather than generic JSON
payload validation.
In `@api/src/teams/schemas.py`:
- Around line 9-12: The authoritative `@test` annotations in the schema comments
contain a typo that makes the test contract unclear. Update the duplicated `@test`
comment in the schema definition near the relevant class comments so it reads
that values from Python are properly serialized to the database, and keep the
wording consistent wherever that annotation appears. Use the existing schema
comment block in schemas.py as the source of truth and fix the phrasing without
changing the intended test coverage.
In `@api/src/workspaces/repository.py`:
- Around line 151-159: The `quest.url` fetch in `repository.py` is still an SSRF
sink because `QuestSettingsPatch` only validates presence, not safety. Before
`client.get(...)` in the `quest.url` branch, add URL validation/allowlisting to
reject unsafe schemes, non-allowed hosts, and private or internal IP ranges, and
only proceed when the URL passes those checks. Keep the logic localized around
the existing `get_http_client()` and `client.get` flow so the restriction is
enforced right where the server-side request is made.
In `@api/src/workspaces/routes.py`:
- Around line 355-361: Remove the redundant JSON definition guard in the
long_quest_data handling path: `validate_quest_settings()` already enforces that
`QuestDefinitionTypeName.JSON` includes a definition, so the `HTTPException` 400
branch in this route is unreachable. Update the `long_quest_data.type ==
QuestDefinitionTypeName.JSON` block in `routes.py` to rely on the existing
validation and keep only the schema validation call via
`validate_quest_definition_schema`.
In `@tests/README.md`:
- Line 79: The fenced code block in the README opens without a language tag,
triggering markdownlint MD040. Update the fenced block at the referenced spot to
include an appropriate language identifier, using the markdown content around
that block to choose the best fit. Locate the unlabeled fence in the README and
ensure the opening fence is explicitly tagged so the block remains valid
Markdown.
In `@tests/support/factories.py`:
- Around line 65-67: Rename the factory parameters that shadow Python builtins
to explicit names to satisfy Ruff A002; update the workspace factory signature
in create_workspace and the other affected factory helpers that currently use id
and type. Use names like workspace_id, workspace_type, user_id, and team_id, and
make sure all internal references and any call sites in
tests/support/factories.py are updated consistently.
In `@tests/support/fakes.py`:
- Around line 139-140: FakeSession.exec currently consumes queued responses for
session-setup statements, unlike FakeSession.execute, which breaks the shared
call-order semantics. Update FakeSession.exec in FakeSession so it also detects
and ignores SET-style session-setup statements before calling _next(), matching
the documented FakeSession behavior used by integration tests. Keep the response
queue aligned so only real query statements advance the queued result order.
In `@tests/unit/test_config.py`:
- Around line 12-23: The defaults test in test_defaults_loaded_when_env_unset is
still reading ambient process environment variables because _env_file=None only
skips .env loading. Update the test setup to clear or isolate the relevant
Settings inputs before instantiating Settings, so assertions like PROJECT_NAME,
DEBUG, SENTRY_DSN, CORS_ORIGINS, and the database URL fields always reflect
declared defaults. Apply the same guard to test_value_types_and_formats if it
depends on the same Settings behavior, using the Settings class as the main
entry point to locate the fix.
In `@tests/unit/test_users_schemas.py`:
- Around line 20-22: The helper _table currently uses getattr(model,
"__table__"), which is intentional for Pyright but triggers Ruff B009; update
that getattr call to include a targeted noqa for B009 only. Keep the behavior in
_table unchanged so SQLAlchemy runtime access still works while satisfying both
Ruff and Pyright.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 10-46: Add an explicit least-privilege permissions block to the ci
job/workflow so the default GITHUB_TOKEN does not inherit broader repository
scopes. Update the workflow around the existing ci job and its checkout/tooling
steps to declare read-only access only, since this job only needs to read the
repository and run checks; use the workflow/job permissions setting alongside
actions/checkout, setup-uv, and the test/lint steps.
- Around line 25-26: The CI workflow currently uses tag-pinned GitHub Actions,
and the review asks to harden supply-chain security by pinning them to immutable
commit SHAs. Update the action references in the workflow so the existing uses
of checkout, setup-uv, and setup-python are switched from mutable version tags
to full commit SHA pins, keeping the same setup behavior while making the
identifiers for those steps more secure and consistent.
In `@api/core/security.py`:
- Around line 173-175: The `# `@test`:` annotation under `validate_token` is empty
and should either be completed or removed. Update the placeholder to describe
the intended test coverage for `validate_token`—including cache hit/miss
behavior, token rotation eviction, and 401 response paths—or delete the
annotation if it is not meant to be used, keeping it consistent with the other
`@test` outlines in this module.
In `@api/src/workspaces/routes.py`:
- Line 159: The runtime invariant in workspaces routing is currently enforced
with an assert, which disappears under optimized Python runs. Replace the assert
in the workspace member assignment flow with an explicit guard that raises a
real exception if workspace.id is None, so the check still runs before
assign_member_role(...) and response construction; apply the same pattern
anywhere else this is used, including the similar Optional[int] narrowing case
in the schemas code.
In `@api/src/workspaces/schemas.py`:
- Line 84: The repeated `@test` annotation text has a grammar typo in the schema
comments, where “any values from Python and properly serialized” should be “any
values from Python are properly serialized.” Update the affected `@test`
comment(s) in the schemas definitions, including the repeated occurrences
referenced by this same wording, so the test-generation annotation reads
correctly and consistently.
In `@README.md`:
- Around line 22-35: The fenced code blocks in the README are missing language
specifiers and are triggering markdownlint MD040. Update the two fenced blocks
around the pytest commands and the type-check/format commands to use a shell
language tag, matching the existing command examples, so the blocks render and
lint correctly.
In `@tests/unit/test_teams_schemas.py`:
- Around line 21-27: The shared schema helper functions `_table` and
`_fk_targets` are duplicated across multiple schema test files, so move their
single implementation into a reusable helper under `tests/support` and update
`test_teams_schemas`, `test_users_schemas`, and `test_workspaces_schemas` to
import and use it. Keep the helper names or equivalent unique symbols consistent
so the schema tests still reference the same `__table__` and foreign key target
logic from one central place.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f384fde8-9c47-4d89-ad73-8844a96b465b
📒 Files selected for processing (41)
.github/workflows/ci.ymlCLAUDE.mdREADME.mdapi/core/config.pyapi/core/json_schema.pyapi/core/jwt.pyapi/core/security.pyapi/main.pyapi/src/teams/repository.pyapi/src/teams/routes.pyapi/src/teams/schemas.pyapi/src/users/repository.pyapi/src/users/routes.pyapi/src/users/schemas.pyapi/src/workspaces/repository.pyapi/src/workspaces/routes.pyapi/src/workspaces/schemas.pypyproject.tomltests/README.mdtests/conftest.pytests/integration/__init__.pytests/integration/test_health.pytests/integration/test_proxy.pytests/integration/test_teams.pytests/integration/test_users.pytests/integration/test_workspaces.pytests/support/__init__.pytests/support/factories.pytests/support/fakes.pytests/support/http.pytests/test_main.pytests/unit/__init__.pytests/unit/test_config.pytests/unit/test_json_schema.pytests/unit/test_jwt.pytests/unit/test_security.pytests/unit/test_teams_schemas.pytests/unit/test_user_info.pytests/unit/test_users_schemas.pytests/unit/test_workspace_repository.pytests/unit/test_workspaces_schemas.py
💤 Files with no reviewable changes (1)
- tests/test_main.py
| if quest.url: | ||
| client = get_http_client() | ||
| if client is None: | ||
| return None | ||
| try: | ||
| response = await get_http_client().get(quest.url, timeout=10) | ||
| response = await client.get(quest.url, timeout=10) | ||
| return response.text | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 '\b(url)\b' api/src/workspaces/schemas.py
ast-grep run --pattern $'`@field_validator`($$$)' --lang python api/src/workspaces/schemas.pyRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 2039
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the touched code and any URL validation/fetch helpers around it.
sed -n '130,180p' api/src/workspaces/repository.py
printf '\n--- schemas ---\n'
sed -n '1,240p' api/src/workspaces/schemas.py
printf '\n--- search for validation/fetch helpers ---\n'
rg -n "url|http_client|get_http_client|model_validator|field_validator|private|loopback|169\\.254|ssrf" api/src/workspaces -SRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 11558
Restrict quest.url before fetching
quest.url is still user-influenced data fetched server-side without any scheme/host/private-range checks. QuestSettingsPatch only enforces presence or absence by type, so this remains an SSRF sink; add URL allowlisting/restrictions before client.get(...).
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 158-158: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@api/src/workspaces/repository.py` around lines 151 - 159, The `quest.url`
fetch in `repository.py` is still an SSRF sink because `QuestSettingsPatch` only
validates presence, not safety. Before `client.get(...)` in the `quest.url`
branch, add URL validation/allowlisting to reject unsafe schemes, non-allowed
hosts, and private or internal IP ranges, and only proceed when the URL passes
those checks. Keep the logic localized around the existing `get_http_client()`
and `client.get` flow so the restriction is enforced right where the server-side
request is made.
There was a problem hiding this comment.
This is a fair point, thanks Rabbit. @cyrossignol thoughts?
There was a problem hiding this comment.
@jeffmaki Thanks — agreed, I think this is worth a quick maintainer call.
My concern is specifically the server-side fetch path in WorkspaceRepository.resolve_quest_def(...): even with presence validation, a user-controlled quest.url can still target internal/private destinations unless we add URL restrictions at fetch time.
If @cyrossignol prefers to keep this PR tightly scoped to tests, I’m also fine with tracking it as a small follow-up hardening change rather than blocking the whole PR.
🧠 Learnings used
Learnt from: cyrossignol
Repo: TaskarCenterAtUW/workspaces-backend PR: 1
File: api/src/workspaces/repository.py:238-244
Timestamp: 2026-02-19T03:37:38.382Z
Learning: In api/src/workspaces/repository.py, get_current_user() intentionally uses scalar_one() instead of scalar_one_or_none() so that a missing authenticated user surfaces as a 500 error (systemic fault) rather than being treated as NotFound. This should be maintained as a guideline: treat absence of an authenticated user in the DB as a systemic error, not a normal not-found case. Document this rationale and ensure tests cover that exceptions bubble up appropriately, preserving the intended error signaling.
| if long_quest_data.type == QuestDefinitionTypeName.JSON: | ||
| if long_quest_data.definition is None: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="'definition' is required for JSON quest type.", | ||
| ) | ||
| await validate_quest_definition_schema(long_quest_data.definition) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C5 'class QuestSettingsPatch' api/src/workspaces/schemas.py
ast-grep run --pattern $'`@model_validator`($$$)' --lang python api/src/workspaces/schemas.pyRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 639
🏁 Script executed:
#!/bin/bash
sed -n '151,190p' api/src/workspaces/schemas.py
printf '\n---\n'
sed -n '340,370p' api/src/workspaces/routes.pyRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 3018
🏁 Script executed:
#!/bin/bash
sed -n '151,190p' api/src/workspaces/schemas.py
printf '\n---\n'
sed -n '340,370p' api/src/workspaces/routes.pyRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 3018
🏁 Script executed:
#!/bin/bash
sed -n '151,190p' api/src/workspaces/schemas.py
printf '\n---\n'
sed -n '340,370p' api/src/workspaces/routes.pyRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 3018
Remove the redundant JSON definition check. QuestSettingsPatch.validate_quest_settings() already rejects type=JSON without definition with a 422, so this 400 branch in api/src/workspaces/routes.py is unreachable.
🤖 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 `@api/src/workspaces/routes.py` around lines 355 - 361, Remove the redundant
JSON definition guard in the long_quest_data handling path:
`validate_quest_settings()` already enforces that `QuestDefinitionTypeName.JSON`
includes a definition, so the `HTTPException` 400 branch in this route is
unreachable. Update the `long_quest_data.type == QuestDefinitionTypeName.JSON`
block in `routes.py` to rely on the existing validation and keep only the schema
validation call via `validate_quest_definition_schema`.
|
|
||
| ## Layout | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced code block.
Line 79 opens a fenced block without a language, which triggers markdownlint MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/README.md` at line 79, The fenced code block in the README opens
without a language tag, triggering markdownlint MD040. Update the fenced block
at the referenced spot to include an appropriate language identifier, using the
markdown content around that block to choose the best fit. Locate the unlabeled
fence in the README and ensure the opening fence is explicitly tagged so the
block remains valid Markdown.
Source: Linters/SAST tools
| id: int | None = 1, | ||
| title: str = "Test Workspace", | ||
| type: WorkspaceType = WorkspaceType.OSW, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid shadowing Python builtins in factory parameters.
Lines 65/67/87/97 use id and type, which are builtin names and flagged by Ruff (A002). Rename to explicit names like workspace_id, workspace_type, user_id, team_id.
Also applies to: 87-87, 97-97
🧰 Tools
🪛 Ruff (0.15.18)
[error] 65-65: Function argument id is shadowing a Python builtin
(A002)
[error] 67-67: Function argument type is shadowing a Python builtin
(A002)
🤖 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/support/factories.py` around lines 65 - 67, Rename the factory
parameters that shadow Python builtins to explicit names to satisfy Ruff A002;
update the workspace factory signature in create_workspace and the other
affected factory helpers that currently use id and type. Use names like
workspace_id, workspace_type, user_id, and team_id, and make sure all internal
references and any call sites in tests/support/factories.py are updated
consistently.
Source: Linters/SAST tools
| async def exec(self, statement, *args, **kwargs): | ||
| return self._raise_if_exc(self._next()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
FakeSession.exec should also ignore session-setup statements.
Line 139 currently consumes queued responses for SET ... statements, while execute skips them. This breaks the queued call-order contract and can misalign subsequent results in integration tests.
As per coding guidelines, tests/support/**/*.py fakes must support the integration-test data-fetcher boundary with documented FakeSession behavior and query-order semantics.
Suggested fix
async def exec(self, statement, *args, **kwargs):
- return self._raise_if_exc(self._next())
+ if self._is_session_setup(statement):
+ return FakeResult(rows=[])
+ return self._raise_if_exc(self._next())📝 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.
| async def exec(self, statement, *args, **kwargs): | |
| return self._raise_if_exc(self._next()) | |
| async def exec(self, statement, *args, **kwargs): | |
| if self._is_session_setup(statement): | |
| return FakeResult(rows=[]) | |
| return self._raise_if_exc(self._next()) |
🤖 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/support/fakes.py` around lines 139 - 140, FakeSession.exec currently
consumes queued responses for session-setup statements, unlike
FakeSession.execute, which breaks the shared call-order semantics. Update
FakeSession.exec in FakeSession so it also detects and ignores SET-style
session-setup statements before calling _next(), matching the documented
FakeSession behavior used by integration tests. Keep the response queue aligned
so only real query statements advance the queued result order.
Source: Coding guidelines
| def test_defaults_loaded_when_env_unset(): | ||
| # _env_file=None ignores any local .env so we observe the declared defaults. | ||
| s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg | ||
|
|
||
| assert s.PROJECT_NAME == "Workspaces API" | ||
| assert s.CORS_ORIGINS == [] | ||
| assert s.DEBUG is False | ||
| assert s.SENTRY_DSN == "" | ||
| assert s.WS_OSM_HOST == "http://osm-web" | ||
| assert s.TDEI_OIDC_REALM == "tdei" | ||
| assert s.TASK_DATABASE_URL.startswith("postgresql+asyncpg://") | ||
| assert s.OSM_DATABASE_URL.startswith("postgresql+asyncpg://") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Defaults test is vulnerable to ambient environment variables.
_env_file=None only disables .env file loading; it does not stop pydantic-settings from reading process environment variables. If the test environment exports any of these names (e.g. PROJECT_NAME, DEBUG, SENTRY_DSN, CORS_ORIGINS), this test (and test_value_types_and_formats) will assert against the override instead of the declared default and flake. Clear the relevant vars first.
🛡️ Suggested guard
-def test_defaults_loaded_when_env_unset():
+def test_defaults_loaded_when_env_unset(monkeypatch):
# _env_file=None ignores any local .env so we observe the declared defaults.
+ for var in ("PROJECT_NAME", "CORS_ORIGINS", "DEBUG", "SENTRY_DSN",
+ "WS_OSM_HOST", "TDEI_OIDC_REALM", "TASK_DATABASE_URL", "OSM_DATABASE_URL"):
+ monkeypatch.delenv(var, raising=False)
s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg📝 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.
| def test_defaults_loaded_when_env_unset(): | |
| # _env_file=None ignores any local .env so we observe the declared defaults. | |
| s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg | |
| assert s.PROJECT_NAME == "Workspaces API" | |
| assert s.CORS_ORIGINS == [] | |
| assert s.DEBUG is False | |
| assert s.SENTRY_DSN == "" | |
| assert s.WS_OSM_HOST == "http://osm-web" | |
| assert s.TDEI_OIDC_REALM == "tdei" | |
| assert s.TASK_DATABASE_URL.startswith("postgresql+asyncpg://") | |
| assert s.OSM_DATABASE_URL.startswith("postgresql+asyncpg://") | |
| def test_defaults_loaded_when_env_unset(monkeypatch): | |
| # _env_file=None ignores any local .env so we observe the declared defaults. | |
| for var in ("PROJECT_NAME", "CORS_ORIGINS", "DEBUG", "SENTRY_DSN", | |
| "WS_OSM_HOST", "TDEI_OIDC_REALM", "TASK_DATABASE_URL", "OSM_DATABASE_URL"): | |
| monkeypatch.delenv(var, raising=False) | |
| s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg | |
| assert s.PROJECT_NAME == "Workspaces API" | |
| assert s.CORS_ORIGINS == [] | |
| assert s.DEBUG is False | |
| assert s.SENTRY_DSN == "" | |
| assert s.WS_OSM_HOST == "http://osm-web" | |
| assert s.TDEI_OIDC_REALM == "tdei" | |
| assert s.TASK_DATABASE_URL.startswith("postgresql+asyncpg://") | |
| assert s.OSM_DATABASE_URL.startswith("postgresql+asyncpg://") |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 19-19: Do not make http calls without encryption
Context: "http://osm-web"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🤖 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/unit/test_config.py` around lines 12 - 23, The defaults test in
test_defaults_loaded_when_env_unset is still reading ambient process environment
variables because _env_file=None only skips .env loading. Update the test setup
to clear or isolate the relevant Settings inputs before instantiating Settings,
so assertions like PROJECT_NAME, DEBUG, SENTRY_DSN, CORS_ORIGINS, and the
database URL fields always reflect declared defaults. Apply the same guard to
test_value_types_and_formats if it depends on the same Settings behavior, using
the Settings class as the main entry point to locate the fix.
| def _table(model): | ||
| # __table__ is added by SQLAlchemy at runtime; getattr keeps type-checkers happy. | ||
| return getattr(model, "__table__") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ruff B009 will flag this getattr call; add a noqa to satisfy both Ruff and Pyright.
The getattr(model, "__table__") is intentional to keep Pyright quiet, but Ruff reports B009 (constant attribute), which can fail the lint step in CI. Suppress only this rule rather than switching to plain attribute access (which would reintroduce the Pyright complaint).
🔧 Proposed fix
def _table(model):
# __table__ is added by SQLAlchemy at runtime; getattr keeps type-checkers happy.
- return getattr(model, "__table__")
+ return getattr(model, "__table__") # noqa: B009 -- avoids Pyright __table__ error📝 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.
| def _table(model): | |
| # __table__ is added by SQLAlchemy at runtime; getattr keeps type-checkers happy. | |
| return getattr(model, "__table__") | |
| def _table(model): | |
| # __table__ is added by SQLAlchemy at runtime; getattr keeps type-checkers happy. | |
| return getattr(model, "__table__") # noqa: B009 -- avoids Pyright __table__ error |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 22-22: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🤖 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/unit/test_users_schemas.py` around lines 20 - 22, The helper _table
currently uses getattr(model, "__table__"), which is intentional for Pyright but
triggers Ruff B009; update that getattr call to include a targeted noqa for B009
only. Keep the behavior in _table unchanged so SQLAlchemy runtime access still
works while satisfying both Ruff and Pyright.
Source: Linters/SAST tools
1 participant
Tests generated via AI via "@test: XXYYZZ" annotations on top of method defs.
This PR adds broad unit and integration test coverage across the backend, along with supporting test infrastructure and documentation.
Highlights:
@testcoverage annotations and repository guidance inCLAUDE.md, plus expanded test-running instructions inREADME.mdandtests/README.md.