Conversation
Fix double CREATE TYPE for workspace_role enum
Unit and integration tests
📝 WalkthroughWalkthroughThis PR updates CI and repository guidance, adds OSM changeset and workspace auto-flag wiring, hardens shared client initialization and type-checking annotations, and expands test support plus unit and integration coverage. ChangesCI, Tooling, and Documentation
OSM Changesets and Workspace Auto-Flag
Runtime Guards, Pyright Suppressions, and Test Comments
Test Support, Unit Tests, and Integration Tests
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant scripts_ci_sh
participant Pyright
participant Pytest
GitHubActions->>scripts_ci_sh: ./scripts/ci.sh --integration
scripts_ci_sh->>Pyright: run type checks
scripts_ci_sh->>Pytest: run unit and integration tests
sequenceDiagram
participant Client
participant OSMRouter
participant OSMRepository
participant Database
Client->>OSMRouter: GET /changesets/{id}/adiff
OSMRouter->>OSMRepository: getChangesetAdiff()
OSMRepository->>Database: SELECT * FROM osm_augmented_diff()
Database-->>OSMRouter: rows
OSMRouter-->>Client: AugmentedDiffResponse
sequenceDiagram
participant Test
participant ClientFixture
participant FastAPIApp
participant FakeSession
Test->>ClientFixture: request app/client
ClientFixture->>FastAPIApp: override DB/auth dependencies
Test->>FastAPIApp: HTTP request
FastAPIApp->>FakeSession: execute queued DB results
FastAPIApp-->>Test: response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/workspaces/routes.py (1)
330-360: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd a
@testline for the new "definition is required" validation.The new
HTTPException(400)for a missingdefinitionwhentype == JSON(lines 356-360) is a distinct behavior from the existing "@test: ...validates the long quest definition against the JSON schema and returns a 400 if the definition is invalid" outline — the latter implies a malformed definition, not a missing one.# `@test`: Test that this endpoint properly validates the long quest definition against the JSON schema and returns a 400 if the definition is invalid +# `@test`: Test that this endpoint returns a 400 when type is JSON and 'definition' is None/missingAs per path instructions, "When adding behavior, add matching
#@test:lines and tests in modules that use#@test:comments; treat those comments as authoritative over code if they disagree."🤖 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 330 - 360, Add a new # `@test` entry for the missing-definition validation in update_long_quest_settings, since the current tests only cover invalid JSON schema input and not the distinct HTTPException(400) raised when long_quest_data.type is QuestDefinitionTypeName.JSON and long_quest_data.definition is None. Update the nearby test annotations to explicitly cover this case, and add or adjust tests for update_long_quest_settings so they assert the 400 response and message for a missing definition separately from the existing JSON-schema invalidity path.Source: Path instructions
🧹 Nitpick comments (4)
api/src/workspaces/repository.py (1)
152-159: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUninitialized HTTP client silently returns
Noneinstead of surfacing the failure.Unlike
json_schema._require_http_client()andsecurity._validate_token_uncached(), which raiseHTTPException(503)when the shared client is missing, this path returnsNone, indistinguishable from a legitimately empty quest definition. Callers (e.g. the long-quest settings endpoint) may silently serve no content instead of surfacing a 503 for an operational fault.if quest.url: client = get_http_client() if client is None: logger.warning("HTTP client not initialized; cannot resolve quest URL %s", quest.url) return 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 `@api/src/workspaces/repository.py` around lines 152 - 159, The quest URL fetch path in the repository currently returns None when get_http_client() is missing, which hides an operational failure; update the client-missing branch in the quest retrieval flow to surface the same kind of 503 error used by json_schema._require_http_client() and security._validate_token_uncached(). Keep the existing fetch logic in the quest URL handling path, but when client is None, raise HTTPException(503) with a clear message instead of returning None, so callers like the long-quest settings endpoint can distinguish service unavailability from an empty result.api/main.py (1)
188-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate header-building/upstream-send logic between
capabilitiesandcatch_all.Both handlers construct the same
req_headerslist (strip/forward headers) and the same timeout/connect-error handling aroundclient.build_request/client.send. Extracting a small helper (e.g._build_proxy_headers(client, request)and a shared "send with 502/504 mapping" helper) would reduce the risk of the two paths drifting when one is updated but not the other.♻️ Sketch of a shared helper
+def _proxy_request_headers(client: httpx.AsyncClient, request: Request) -> list[tuple[bytes, bytes]]: + client_host = request.client.host if request.client else "unknown" + return [ + (k.encode(), v.encode()) + for k, v in request.headers.items() + if k.lower() not in STRIP_REQUEST_HEADERS + ] + [ + (b"Host", client.base_url.host.encode()), + (b"X-Real-IP", client_host.encode()), + (b"X-Forwarded-For", client_host.encode()), + (b"X-Forwarded-Host", (request.url.hostname or "").encode()), + (b"X-Forwarded-Proto", request.url.scheme.encode()), + ]Also applies to: 234-313
🤖 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/main.py` around lines 188 - 206, The `capabilities` and `catch_all` paths duplicate the same proxy header construction and upstream request/send error handling, so extract shared helpers to keep them consistent. Move the repeated `req_headers` building into a helper like `_build_proxy_headers(client, request)` and centralize the `client.build_request`/`client.send(..., stream=True)` timeout and connect-error mapping into one reusable helper. Update both handlers to call these shared helpers so future changes to forwarding behavior or 502/504 handling only need to be made in one place.tests/support/factories.py (1)
63-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParameter names shadow Python builtins (
id,type).Ruff flags
id/typeas builtin-shadowing keyword args inmake_workspace,make_user, andmake_team. Not a functional bug here (the builtins aren't used inside these bodies), but if Ruff'sA002rule is enforced as a CI gate, this will fail lint.♻️ Suggested rename
def make_workspace( *, - id: int | None = 1, + workspace_id: int | None = 1, title: str = "Test Workspace", - type: WorkspaceType = WorkspaceType.OSW, + type_: WorkspaceType = WorkspaceType.OSW, tdei_project_group_id: str = DEFAULT_PG_ID, created_by: str = DEFAULT_USER_ID, created_by_name: str = "Test User", **extra, ) -> Workspace: return Workspace( - id=id, + id=workspace_id, title=title, - type=type, + type=type_, ... )(Analogous renames apply to
make_user's andmake_team'sidparameter, e.g.user_id/team_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 `@tests/support/factories.py` around lines 63 - 107, Rename the builtin-shadowing keyword arguments in the factory helpers to satisfy Ruff A002: update `make_workspace`, `make_user`, and `make_team` to avoid parameters named `id` and `type` by using clearer names like `workspace_id`, `workspace_type`, `user_id`, and `team_id`. Make sure the corresponding constructor calls in `Workspace`, `User`, and `WorkspaceTeam` still receive the same values through the new parameter names, and keep the public helper behavior unchanged.Source: Linters/SAST tools
tests/integration/test_proxy.py (1)
24-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
tests.support.http.osm_mock_clientinstead of duplicating transport setup.
osm_mock_client()already builds theStreamingMockTransport+AsyncClientpair;mock_osm/install_osmre-implement the same logic inline.♻️ Proposed refactor
-from tests.support.http import StreamingMockTransport +from tests.support.http import osm_mock_client `@pytest.fixture` def mock_osm(monkeypatch): """Default upstream returning 200 text/xml; records the forwarded request.""" - transport = StreamingMockTransport( - lambda req: (200, {"content-type": "text/xml"}, b"<osm version='0.6'/>") - ) - monkeypatch.setattr( - api.main, - "_osm_client", - httpx.AsyncClient(transport=transport, base_url="http://osm-web"), - ) + client, transport = osm_mock_client() + monkeypatch.setattr(api.main, "_osm_client", client) return transport def install_osm(monkeypatch, handler): - transport = StreamingMockTransport(handler) - monkeypatch.setattr( - api.main, - "_osm_client", - httpx.AsyncClient(transport=transport, base_url="http://osm-web"), - ) + client, transport = osm_mock_client(handler) + monkeypatch.setattr(api.main, "_osm_client", client) return transport🤖 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/integration/test_proxy.py` around lines 24 - 45, The fixture setup in mock_osm and install_osm duplicates the same StreamingMockTransport and httpx.AsyncClient construction already provided by tests.support.http.osm_mock_client. Refactor both helpers to call osm_mock_client() with the appropriate handler/default response and use its returned client/transport instead of building the pair inline, while keeping the monkeypatch to api.main._osm_client in place.
🤖 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 @.github/workflows/ci.yml:
- Around line 47-50: The CI test step only runs the default pytest suite, so it
skips the DB-backed integration path in tests/integration because _pg_urls and
the integration marker are never selected. Update the workflow by adding a
separate integration job or test step that enables the integration marker and
provides the required Postgres/testcontainers environment, while keeping the
existing unit test run for the non-DB suite. Use the existing pytest invocation
in the Run tests step and the integration behavior in _pg_urls/tests/integration
as the reference points when wiring this in.
In @.vscode/settings.json:
- Around line 6-17: The VS Code pytest settings are pointing Test Explorer at
the wrong directory because enabling python.testing.pytestEnabled will honor the
existing python.testing.pytestArgs value. Update the pytest configuration in
settings.json so the args match the real test tree, or remove
python.testing.pytestArgs if it is no longer needed, and keep the change aligned
with the Python settings block.
In `@api/src/tasking/projects/repository.py`:
- Around line 1-8: The Pyright suppression at the top of repository.py is too
broad and hides unrelated errors across the whole module. Remove the file-level
disables and scope the exceptions only to the SQLModel false-positive sites in
the relevant query methods (for example around the where()/select()/selectinload
and deleted_at.is_(None) expressions), keeping the rest of the module subject to
normal Pyright checks.
In `@api/src/tasking/tasks/repository.py`:
- Around line 1-8: The file-level Pyright suppression in repository.py is too
broad and hides unrelated type issues. Narrow the suppression in the task
repository code by removing the global pyright directives and applying targeted
inline ignores only at the specific SQLModel false-positive sites in the
repository methods that use where(), select(), exec(), selectinload(), and
nullable .is_(None) checks. Keep the rest of the module under normal Pyright
checking so real errors remain visible.
In `@api/src/teams/repository.py`:
- Around line 1-6: The current top-of-file Pyright suppression in repository.py
is too broad and masks unrelated issues beyond the SQLModel false positives.
Remove the file-level disables and instead apply the Pyright suppression only at
the specific SQLModel call sites in the relevant repository methods (such as the
`where()`, `exec()`, `select()`, `selectinload`, and `rowcount` usages) so the
rest of the file continues to be checked normally.
In `@api/src/users/repository.py`:
- Around line 1-6: The Pyright suppression in the repository module is too broad
because it disables checks for the whole file instead of only the SQLModel false
positives. Remove the file-wide directives and apply targeted per-line ignores
only at the specific `where()`, `exec()`, `select()`, `selectinload`, or
`rowcount` call sites in `repository.py`, following the pattern used in
`workspaces/repository.py` and `teams/repository.py`. Keep the rest of the file
under normal Pyright checking so unrelated type errors are still reported.
In `@api/src/workspaces/repository.py`:
- Around line 1-7: Remove the file-level Pyright suppression in repository.py
and scope the ignores only to the specific SQLModel false-positive sites. Keep
the existing SQLModel-related queries and constructors in Repository methods
like the ones using where(), select(), exec(), and rowcount, but replace the
blanket pyright directives with inline, targeted ignores at those call sites
only. Leave unrelated type checking enabled so genuine errors elsewhere in the
file are still reported.
In `@README.md`:
- Around line 22-35: The Markdown code fences in the README examples are missing
language tags, so update both fenced blocks to use bash. Locate the two command
example blocks in the README and add the bash label to each opening fence while
keeping the command contents unchanged.
In `@tests/integration/test_users.py`:
- Around line 35-40: The integration-test fixture named evictions is
monkeypatching users_routes.evict_user_from_cache, which goes beyond the allowed
mock boundary and skips the real cache-eviction path. Remove that monkeypatch
and adjust the tests to exercise the actual eviction behavior through the
routes, while only mocking the approved session/token/client boundaries
(get_task_session, get_osm_session, validate_token, and api.main._osm_client).
If you still need to assert eviction happened, verify its observable effect
instead of replacing evict_user_from_cache itself.
In `@tests/README.md`:
- Around line 79-94: The repository layout block in tests/README.md is missing a
language tag, triggering markdownlint warnings. Update the fenced code block
that shows the tests directory tree to use a text language tag so the markdown
is recognized correctly, keeping the content unchanged; this applies to the
README section containing the tests/, support/, unit/, and integration/ layout
snippet.
---
Outside diff comments:
In `@api/src/workspaces/routes.py`:
- Around line 330-360: Add a new # `@test` entry for the missing-definition
validation in update_long_quest_settings, since the current tests only cover
invalid JSON schema input and not the distinct HTTPException(400) raised when
long_quest_data.type is QuestDefinitionTypeName.JSON and
long_quest_data.definition is None. Update the nearby test annotations to
explicitly cover this case, and add or adjust tests for
update_long_quest_settings so they assert the 400 response and message for a
missing definition separately from the existing JSON-schema invalidity path.
---
Nitpick comments:
In `@api/main.py`:
- Around line 188-206: The `capabilities` and `catch_all` paths duplicate the
same proxy header construction and upstream request/send error handling, so
extract shared helpers to keep them consistent. Move the repeated `req_headers`
building into a helper like `_build_proxy_headers(client, request)` and
centralize the `client.build_request`/`client.send(..., stream=True)` timeout
and connect-error mapping into one reusable helper. Update both handlers to call
these shared helpers so future changes to forwarding behavior or 502/504
handling only need to be made in one place.
In `@api/src/workspaces/repository.py`:
- Around line 152-159: The quest URL fetch path in the repository currently
returns None when get_http_client() is missing, which hides an operational
failure; update the client-missing branch in the quest retrieval flow to surface
the same kind of 503 error used by json_schema._require_http_client() and
security._validate_token_uncached(). Keep the existing fetch logic in the quest
URL handling path, but when client is None, raise HTTPException(503) with a
clear message instead of returning None, so callers like the long-quest settings
endpoint can distinguish service unavailability from an empty result.
In `@tests/integration/test_proxy.py`:
- Around line 24-45: The fixture setup in mock_osm and install_osm duplicates
the same StreamingMockTransport and httpx.AsyncClient construction already
provided by tests.support.http.osm_mock_client. Refactor both helpers to call
osm_mock_client() with the appropriate handler/default response and use its
returned client/transport instead of building the pair inline, while keeping the
monkeypatch to api.main._osm_client in place.
In `@tests/support/factories.py`:
- Around line 63-107: Rename the builtin-shadowing keyword arguments in the
factory helpers to satisfy Ruff A002: update `make_workspace`, `make_user`, and
`make_team` to avoid parameters named `id` and `type` by using clearer names
like `workspace_id`, `workspace_type`, `user_id`, and `team_id`. Make sure the
corresponding constructor calls in `Workspace`, `User`, and `WorkspaceTeam`
still receive the same values through the new parameter names, and keep the
public helper behavior unchanged.
🪄 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: 775f9fc7-1d5d-4cad-9054-1c63a127b428
📒 Files selected for processing (48)
.github/workflows/ci.yml.pre-commit-config.yaml.vscode/settings.jsonCLAUDE.mdREADME.mdalembic_osm/versions/9221408912dd_add_user_role_table.pyapi/core/config.pyapi/core/json_schema.pyapi/core/jwt.pyapi/core/security.pyapi/main.pyapi/src/tasking/projects/repository.pyapi/src/tasking/tasks/repository.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/conftest.pytests/integration/test_health.pytests/integration/test_proxy.pytests/integration/test_tasks_flow.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/test_aoi_normalisation.pytests/unit/test_config.pytests/unit/test_dtos_validation.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
| "python.testing.pytestEnabled": true, | ||
| "[python]": { | ||
| "editor.defaultFormatter": "ms-python.black-formatter", | ||
| "editor.formatOnSave": true, | ||
| "editor.codeActionsOnSave": { | ||
| "source.organizeImports": "explicit" | ||
| } | ||
| }, | ||
| "isort.args": [ | ||
| "--profile", | ||
| "black" | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Point VS Code pytest at the actual test tree.
Enabling python.testing.pytestEnabled will now make VS Code honor the existing python.testing.pytestArgs: ["alembic"], so Test Explorer will still look in the migration directory instead of the repo’s test suite. Update those args (or remove them) together with this change.
🛠️ Proposed fix
- "python.testing.pytestArgs": [
- "alembic"
- ],
+ "python.testing.pytestArgs": [
+ "tests"
+ ],📝 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.
| "python.testing.pytestEnabled": true, | |
| "[python]": { | |
| "editor.defaultFormatter": "ms-python.black-formatter", | |
| "editor.formatOnSave": true, | |
| "editor.codeActionsOnSave": { | |
| "source.organizeImports": "explicit" | |
| } | |
| }, | |
| "isort.args": [ | |
| "--profile", | |
| "black" | |
| ] | |
| "python.testing.pytestArgs": [ | |
| "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 @.vscode/settings.json around lines 6 - 17, The VS Code pytest settings are
pointing Test Explorer at the wrong directory because enabling
python.testing.pytestEnabled will honor the existing python.testing.pytestArgs
value. Update the pytest configuration in settings.json so the args match the
real test tree, or remove python.testing.pytestArgs if it is no longer needed,
and keep the change aligned with the Python settings block.
| ``` | ||
| uv run pytest # full suite with coverage (configured in pyproject.toml) | ||
| uv run pytest --no-cov -q # quick run, no coverage | ||
| uv run pytest tests/unit # unit tests only | ||
| uv run pytest tests/integration # integration tests only | ||
| uv run pytest -k workspaces # filter by keyword | ||
| ``` | ||
|
|
||
| Type-check and format (matches the pre-commit hooks): | ||
|
|
||
| ``` | ||
| uvx pyright --pythonpath .venv/bin/python api tests | ||
| uv run black api tests && uv run isort api tests | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the fenced code blocks.
Both command examples are missing a language tag, so markdownlint will keep flagging them. Add bash to each fence.
🛠️ Proposed fix
-```
+```bash
uv run pytest # full suite with coverage (configured in pyproject.toml)
uv run pytest --no-cov -q # quick, no coverage
uv run pytest tests/unit # one layer
uv run pytest -k workspaces # by keyword
-```
+```
@@
-```
+```bash
uvx pyright --pythonpath .venv/bin/python api tests
uv run black api tests && uv run isort api tests
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 32-32: 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 `@README.md` around lines 22 - 35, The Markdown code fences in the README
examples are missing language tags, so update both fenced blocks to use bash.
Locate the two command example blocks in the README and add the bash label to
each opening fence while keeping the command contents unchanged.
Source: Linters/SAST tools
| @pytest.fixture | ||
| def evictions(monkeypatch): | ||
| """Record evict_user_from_cache(...) calls made by the routes.""" | ||
| calls = [] | ||
| monkeypatch.setattr(users_routes, "evict_user_from_cache", calls.append) | ||
| return calls |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Monkeypatching evict_user_from_cache exceeds the allowed integration-test mock boundary.
As per coding guidelines, integration tests should "mock only the data-fetcher boundary: override get_task_session / get_osm_session with FakeSession, validate_token with a real UserInfo, and api.main._osm_client with the proxy transport mock." Monkeypatching evict_user_from_cache on users_routes replaces real cache-eviction logic instead of exercising it, so a bug in the real implementation (wrong key, silent exception, etc.) wouldn't be caught by these 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 `@tests/integration/test_users.py` around lines 35 - 40, The integration-test
fixture named evictions is monkeypatching users_routes.evict_user_from_cache,
which goes beyond the allowed mock boundary and skips the real cache-eviction
path. Remove that monkeypatch and adjust the tests to exercise the actual
eviction behavior through the routes, while only mocking the approved
session/token/client boundaries (get_task_session, get_osm_session,
validate_token, and api.main._osm_client). If you still need to assert eviction
happened, verify its observable effect instead of replacing
evict_user_from_cache itself.
Source: Path instructions
| ``` | ||
| tests/ | ||
| conftest.py # fixtures: client, login, task_session, osm_session | ||
| support/ | ||
| fakes.py # FakeSession + FakeResult + result builders | ||
| factories.py # make_user_info / make_workspace / make_user / make_team | ||
| http.py # streamable mock transport for the OSM proxy | ||
| unit/ | ||
| test_user_info.py | ||
| test_workspace_repository.py | ||
| integration/ | ||
| test_health.py | ||
| test_workspaces.py | ||
| test_teams.py | ||
| test_proxy.py | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the layout fence.
The repo tree block should be fenced as text so markdownlint stops warning.
🛠️ Proposed fix
-```
+```text
tests/
conftest.py # fixtures: client, login, task_session, osm_session
support/
...
-```
+```📝 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.
| ``` | |
| tests/ | |
| conftest.py # fixtures: client, login, task_session, osm_session | |
| support/ | |
| fakes.py # FakeSession + FakeResult + result builders | |
| factories.py # make_user_info / make_workspace / make_user / make_team | |
| http.py # streamable mock transport for the OSM proxy | |
| unit/ | |
| test_user_info.py | |
| test_workspace_repository.py | |
| integration/ | |
| test_health.py | |
| test_workspaces.py | |
| test_teams.py | |
| test_proxy.py | |
| ``` |
🧰 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` around lines 79 - 94, The repository layout block in
tests/README.md is missing a language tag, triggering markdownlint warnings.
Update the fenced code block that shows the tests directory tree to use a text
language tag so the markdown is recognized correctly, keeping the content
unchanged; this applies to the README section containing the tests/, support/,
unit/, and integration/ layout snippet.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@alembic_osm/versions/5303f61d7b3a_add_osm_augmented_diff_function.py`:
- Line 4: The migration header has a stale Revisions docstring that does not
match the actual Alembic chain. Update the Revises comment in
5303f61d7b3a_add_osm_augmented_diff_function.py to reflect the same parent
revision used by down_revision, so the header stays consistent with the
migration metadata. Use the down_revision value in the migration module as the
source of truth and keep the docstring aligned with it.
In `@alembic_task/versions/b3f8a2c91e04_add_auto_flag_review.py`:
- Around line 11-13: The import block in this Alembic revision has an extra
blank line between third-party imports, which violates isort grouping. Update
the top-level imports in this migration so `sqlalchemy` and `alembic` are kept
together in one third-party group with no separating blank line, preserving the
existing order conventions used by isort.
In `@api/main.py`:
- Around line 320-325: The XML handling in the request-processing path uses the
standard ElementTree parser on client-controlled input, which is vulnerable to
XXE/entity-expansion attacks. Update the parsing and serialization in the
request body flow around ET.fromstring and ET.tostring to use
defusedxml.ElementTree instead, keeping the same logic for locating the
changeset element and adding the review_requested tag.
In `@api/src/osm/repository.py`:
- Around line 43-51: The OSM repository query path leaves the session
search_path changed on a pooled connection, so update the logic in the method
that sets up the session and runs osm_augmented_diff to avoid leaking workspace
schema state. Use transaction-scoped search_path handling with SET LOCAL inside
a transaction, or explicitly RESET search_path before the session is returned,
and make sure the fix is applied around the existing self.session.execute calls
in the repository method that fetches the augmented diff.
🪄 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: 4b112a75-7276-4298-84b1-0ff98565f234
📒 Files selected for processing (11)
alembic_osm/sql/osm_augmented_diff.sqlalembic_osm/versions/5303f61d7b3a_add_osm_augmented_diff_function.pyalembic_task/versions/b3f8a2c91e04_add_auto_flag_review.pyapi/main.pyapi/src/osm/__init__.pyapi/src/osm/repository.pyapi/src/osm/routes.pyapi/src/osm/schemas.pyapi/src/workspaces/repository.pyapi/src/workspaces/routes.pyapi/src/workspaces/schemas.py
💤 Files with no reviewable changes (1)
- api/src/workspaces/repository.py
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/workspaces/schemas.py
| body = await request.body() | ||
| root = ET.fromstring(body) | ||
| changeset_el = root.find("changeset") | ||
| if changeset_el is not None: | ||
| ET.SubElement(changeset_el, "tag", k="review_requested", v="yes") | ||
| request_content = ET.tostring(root, encoding="unicode").encode("utf-8") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does defusedxml.ElementTree provide tostring, or only parsing functions?
💡 Result:
Yes, defusedxml.ElementTree provides a tostring function [1]. While the defusedxml library is designed primarily to provide secure, attack-resistant versions of parsing and loading functions (such as parse, iterparse, and fromstring) [2][3][4], it does include a tostring function in the defusedxml.ElementTree module [1]. However, it is important to note that the defusedxml modules are not intended to be full drop-in replacements for the standard library's xml.etree.ElementTree [2][3]. The library's documentation explicitly advises that for all features beyond parsing and loading, you should use the classes, functions, and constants from the standard library [2][3]. In common practice, this means using defusedxml for the initial secure parsing and the standard library (xml.etree.ElementTree) for subsequent manipulation and serialization (like tostring) [2][3].
Citations:
- 1: https://github.com/tiran/defusedxml/blob/master/defusedxml/cElementTree.py
- 2: https://github.com/tiran/defusedxml
- 3: https://github.com/tiran/defusedxml/blob/main/README.md
- 4: https://runebook.dev/en/docs/python/library/xml/xml-vulnerabilities
Parse the changeset XML with a hardened parser.
ET.fromstring() is parsing client-controlled XML on the API server and is exposed to XXE/entity-expansion attacks. Switch this path to defusedxml.ElementTree; it still provides tostring(), so a straight import swap covers both parse and serialize.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 321-321: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
🤖 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/main.py` around lines 320 - 325, The XML handling in the
request-processing path uses the standard ElementTree parser on
client-controlled input, which is vulnerable to XXE/entity-expansion attacks.
Update the parsing and serialization in the request body flow around
ET.fromstring and ET.tostring to use defusedxml.ElementTree instead, keeping the
same logic for locating the changeset element and adding the review_requested
tag.
Source: Linters/SAST tools
| await self.session.execute( | ||
| text(f"SET search_path TO 'workspace-{int(workspace_id)}', public") | ||
| ) | ||
| result = await self.session.execute( | ||
| text("SELECT * FROM osm_augmented_diff(:changeset_id)"), | ||
| {"changeset_id": changeset_id}, | ||
| ) | ||
|
|
||
| return result.mappings().all() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect osm engine/session config for search_path reset behavior.
fd -e py database.py api/core | xargs rg -nP 'get_osm_session|reset_on_return|search_path|create_async_engine' -C2Repository: TaskarCenterAtUW/workspaces-backend
Length of output: 737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== database.py =="
sed -n '1,220p' api/core/database.py
echo
echo "== osm repository =="
sed -n '1,220p' api/src/osm/repository.py
echo
echo "== search_path / session usage =="
rg -n "SET search_path|search_path|get_osm_session|async_osm_session|reset_on_return|pool_reset|checkin|begin\(" api -C 2Repository: TaskarCenterAtUW/workspaces-backend
Length of output: 11790
Reset search_path before returning pooled OSM connections. SET search_path is session-scoped, and get_osm_session() returns pooled connections without any reset_on_return/SET LOCAL handling. A later query on the same connection can inherit the previous workspace schema; use SET LOCAL inside a transaction or RESET search_path before release.
🤖 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/osm/repository.py` around lines 43 - 51, The OSM repository query
path leaves the session search_path changed on a pooled connection, so update
the logic in the method that sets up the session and runs osm_augmented_diff to
avoid leaking workspace schema state. Use transaction-scoped search_path
handling with SET LOCAL inside a transaction, or explicitly RESET search_path
before the session is returned, and make sure the fix is applied around the
existing self.session.execute calls in the repository method that fetches the
augmented diff.
|
@MashB Please back-merge this to dev and stage after you merge this. CC: @sureshgaussian |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
api/src/tasking/projects/repository.py (1)
635-638: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a small helper to de-duplicate the
TaskingProject.id == project.idpredicate.The same 3-line
.where(TaskingProject.id == project.id # pyright: ignore[reportArgumentType])pattern is repeated at 9 call sites in this file (patch, soft_delete, activate, close, reset, upload_aoi, delete_aoi, etc.). A small helper (e.g._by_id(project.id)) would remove the duplication and centralize the pyright suppression to one spot if the underlying SQLModel typing issue is ever fixed.♻️ Example
+ `@staticmethod` + def _id_eq(project_id: int): + return TaskingProject.id == project_id # pyright: ignore[reportArgumentType]Then replace each
.where(TaskingProject.id == project.id # pyright: ignore[reportArgumentType])with.where(self._id_eq(project.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/tasking/projects/repository.py` around lines 635 - 638, The repeated TaskingProject.id == project.id predicate should be de-duplicated in the repository methods. Add a small helper on the repository class (for example, a private _id_eq or _by_id helper) that builds the TaskingProject.id comparison and keeps the pyright ignore in one place, then update the repeated call sites in methods like patch, soft_delete, activate, close, reset, upload_aoi, and delete_aoi to use that helper.
🤖 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/src/tasking/projects/repository.py`:
- Line 91: The AOI validity error message is using a nonexistent geometry
attribute, so update the repository validation path to use the Shapely
module-level helper instead. In repository.py, adjust the AOI polygon check
around the detail message to import is_valid_reason from shapely and call
is_valid_reason(geom) directly, keeping the existing fallback text only if
needed for unsupported cases.
In `@pyproject.toml`:
- Around line 48-55: The pytest config currently only registers the integration
marker, so `@pytest.mark.integration` tests still run in the default suite. Update
the [tool.pytest.ini_options] addopts setting in pyproject.toml to exclude
integration tests by default (for example by adding a marker filter), or apply
the same exclusion in scripts/ci.sh, so the normal pytest entrypoint stays
Docker-free. Refer to the pytest configuration block and the integration marker
definition when making the change.
In `@scripts/ci.sh`:
- Line 14: The initial directory change in the script should be fail-fast so the
rest of the relative commands don’t run from the wrong location. Update the
startup `cd` near the top of `scripts/ci.sh` to guard against failure from
`dirname "$0"` or `cd` itself, and ensure the script exits immediately if the
repository root cannot be resolved. Refer to the existing top-level `cd
"$(dirname "$0")/.."` step when making the change.
---
Nitpick comments:
In `@api/src/tasking/projects/repository.py`:
- Around line 635-638: The repeated TaskingProject.id == project.id predicate
should be de-duplicated in the repository methods. Add a small helper on the
repository class (for example, a private _id_eq or _by_id helper) that builds
the TaskingProject.id comparison and keeps the pyright ignore in one place, then
update the repeated call sites in methods like patch, soft_delete, activate,
close, reset, upload_aoi, and delete_aoi to use that helper.
🪄 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: 43867a6b-6990-4e01-a939-bd1378dff407
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/ci.ymlCLAUDE.mdalembic_osm/versions/5303f61d7b3a_add_osm_augmented_diff_function.pyalembic_task/versions/b3f8a2c91e04_add_auto_flag_review.pyapi/main.pyapi/src/osm/repository.pyapi/src/osm/routes.pyapi/src/tasking/audit/repository.pyapi/src/tasking/projects/repository.pyapi/src/tasking/tasks/repository.pyapi/src/teams/repository.pyapi/src/users/repository.pyapi/src/workspaces/repository.pyapi/src/workspaces/schemas.pypyproject.tomlscripts/ci.shscripts/fix-lint.shtests/integration/conftest.pytests/integration/test_audit_flow.py
💤 Files with no reviewable changes (1)
- alembic_task/versions/b3f8a2c91e04_add_auto_flag_review.py
✅ Files skipped from review due to trivial changes (4)
- scripts/fix-lint.sh
- api/src/users/repository.py
- CLAUDE.md
- api/src/tasking/tasks/repository.py
🚧 Files skipped from review as they are similar to previous changes (5)
- api/src/osm/routes.py
- alembic_osm/versions/5303f61d7b3a_add_osm_augmented_diff_function.py
- api/src/workspaces/repository.py
- api/src/osm/repository.py
- api/main.py
| raise HTTPException( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
| detail=f"AOI is not a valid polygon: {geom.is_valid_reason if hasattr(geom, 'is_valid_reason') else 'self-intersection or invalid ring'}", | ||
| detail=f"AOI is not a valid polygon: {geom.is_valid_reason if hasattr(geom, 'is_valid_reason') else 'self-intersection or invalid ring'}", # pyright: ignore[reportAttributeAccessIssue] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does shapely 2.1 provide an is_valid_reasonattribute on Geometry objects, or only the module-levelshapely.is_valid_reason() function?
💡 Result:
In Shapely 2.1, is_valid_reason is provided only as a module-level function [1][2][3][4]. It is not available as an attribute on Geometry objects (e.g., you cannot call my_geometry.is_valid_reason) [4]. To use it, you must import and call the function by passing the geometry object as an argument [2][4]: from shapely import is_valid_reason from shapely.geometry import Polygon geom = Polygon([(0, 0), (1, 1), (1, 2), (1, 1), (0, 0)]) print(is_valid_reason(geom)) This function is part of Shapely's predicates module [4][5]. While older versions of Shapely (prior to the 2.0 rewrite) sometimes offered different method-based access patterns, the 2.x architecture emphasizes these functional, often vectorized, approaches [4][6].
Citations:
- 1: https://shapely.readthedocs.io/en/2.1.2/reference/shapely.is_valid_reason.html
- 2: https://shapely.readthedocs.io/en/latest/reference/shapely.is_valid_reason.html
- 3: https://shapely.readthedocs.io/en/2.1.1/reference/shapely.is_valid_reason.html
- 4: https://github.com/shapely/shapely/blob/eba985c6e0170ecdd90c83592fd0afa7ae793cb8/shapely/predicates.py
- 5: https://shapely.readthedocs.io/en/stable/predicates.html
- 6: https://shapely.readthedocs.io/en/2.1.2/release/2.x.html
🏁 Script executed:
sed -n '1,140p' api/src/tasking/projects/repository.pyRepository: TaskarCenterAtUW/workspaces-backend
Length of output: 4934
Use the module-level validity reason helper here. geom.is_valid_reason is never present on Shapely geometries, so this branch always falls back to the generic message. Import is_valid_reason from shapely and call is_valid_reason(geom) instead.
🤖 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/tasking/projects/repository.py` at line 91, The AOI validity error
message is using a nonexistent geometry attribute, so update the repository
validation path to use the Shapely module-level helper instead. In
repository.py, adjust the AOI polygon check around the detail message to import
is_valid_reason from shapely and call is_valid_reason(geom) directly, keeping
the existing fallback text only if needed for unsupported cases.
| [tool.pytest.ini_options] | ||
| addopts = "-v --cov=api --cov-report=term-missing" | ||
| testpaths = ["tests"] | ||
| asyncio_mode = "auto" | ||
| asyncio_default_fixture_loop_scope = "function" | ||
| markers = [ | ||
| "integration: tests that require a live PostGIS database (Docker + testcontainers)", | ||
| ] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- pyproject.toml (pytest section) ---'
nl -ba pyproject.toml | sed -n '40,70p'
echo
echo '--- scripts/ci.sh ---'
nl -ba scripts/ci.sh | sed -n '1,120p'
echo
echo '--- integration marker usage ---'
rg -n '\bintegration\b' -S .Repository: TaskarCenterAtUW/workspaces-backend
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- pyproject.toml (pytest section) ---'
sed -n '40,70p' pyproject.toml | cat -n
echo
echo '--- scripts/ci.sh ---'
sed -n '1,140p' scripts/ci.sh | cat -n
echo
echo '--- integration marker usage ---'
rg -n '\bintegration\b' -S .Repository: TaskarCenterAtUW/workspaces-backend
Length of output: 9116
Exclude integration tests from the default pytest run
markers = [...] only registers the marker; it does not prevent @pytest.mark.integration tests from running under uv run pytest tests. Add -m "not integration" to addopts (or filter them in scripts/ci.sh) so the default test step stays Docker-free.
🤖 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 `@pyproject.toml` around lines 48 - 55, The pytest config currently only
registers the integration marker, so `@pytest.mark.integration` tests still run in
the default suite. Update the [tool.pytest.ini_options] addopts setting in
pyproject.toml to exclude integration tests by default (for example by adding a
marker filter), or apply the same exclusion in scripts/ci.sh, so the normal
pytest entrypoint stays Docker-free. Refer to the pytest configuration block and
the integration marker definition when making the change.
| # therefore needs a running Docker daemon | ||
| set -uo pipefail | ||
|
|
||
| cd "$(dirname "$0")/.." |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the cd against failure.
If dirname "$0" resolution or the cd fails, the script silently continues from the wrong directory and all subsequent relative commands misbehave.
🛠️ Proposed fix
-cd "$(dirname "$0")/.."
+cd "$(dirname "$0")/.." || exit 1📝 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.
| cd "$(dirname "$0")/.." | |
| cd "$(dirname "$0")/.." || exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 14-14: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 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 `@scripts/ci.sh` at line 14, The initial directory change in the script should
be fail-fast so the rest of the relative commands don’t run from the wrong
location. Update the startup `cd` near the top of `scripts/ci.sh` to guard
against failure from `dirname "$0"` or `cd` itself, and ensure the script exits
immediately if the repository root cannot be resolved. Refer to the existing
top-level `cd "$(dirname "$0")/.."` step when making the change.
Source: Linters/SAST tools
3 participants
Tests from
main, as well as new Cy features fromdeploy, plus updates to tests.Merges
mainintodevelop, bringing in major updates across CI/testing, API/OSM proxy functionality, typing/test guidance, schema changes, migrations, and substantially expanded test coverage.Key highlights:
cijob (concurrencycancels in-progress runs). Updates GitHub Actions versions (checkout/uv/Python) and switches execution to./scripts/ci.sh --integration, which runs the same checks as CI previously did (uv sync, isort, black, pyright, pytest) and conditionally enables PostGIS/testcontainers integration viapytest -m integration. Adds local CI helpers (scripts/ci.sh,scripts/fix-lint.sh). Updates pre-commit Black to24.10.0, and updates editor formatting settings (.vscode) for Black/isort on save.CLAUDE.md, a repository guidance doc focused on enforced permissions and test architecture. ExpandsREADME.mdwith a “Running the tests” section and addstests/README.mddescribing unit vs integration layers and the integration mocking boundaries.api/core/json_schema.py: Ensures the shared async HTTP client is initialized; otherwise schema fetches fail with 503. Refactors schema fetching to use caching + per-schema locking.api/core/security.py: Improves token/TDEI validation behavior to return 503 when the TDEI client is uninitialized, with clearer messaging.api/main.py: Adds OSM client initialization enforcement (503) and updates proxy routing to use it. Improves hop-by-hop header stripping (excludescontent-length). Adds catch-all OSM changeset-create request buffering/rewrite to inject areview_requested=yestag when the workspace’sautoFlagReviewis enabled and the user is effectively a contributor.GET .../adiff,PUT .../resolve) under the/api/v1app (router wired fromapi/src/osm/routes.py).OSMRepositorywith per-workspacesearch_pathhandling and support for calling the newosm_augmented_diff(...)SQL function plus changeset resolution tag updates.workspaces.autoFlagReviewend-to-end: Alembic migration for the new column, Pydantic schema/response support, and request validation behavior;WorkspaceResponse.from_workspace(...)now assertsworkspace.idand includesautoFlagReview.users.auth_uid/workspace role Alembic migration to unconditionally create/drop the enum/table/constraints.osm_augmented_diff(p_changeset_id bigint)SQL + Alembic migration to install it, plus new Pydantic models for augmented diff responses (api/src/osm/schemas.py).Quest/imagery schema testing and error mapping improvements via new JSON schema unit tests and related code changes.tests/support/fakes.py(FakeSession/FakeResult) to drive deterministic queued DB interactions.tests/support/http.py).tests/conftest.pyand integrationtests/integration/conftest.pyto cleanly separate integration vs non-integration wiring, install dependency overrides at the session boundaries, and manage per-test async DB engines safely.test_proxy.py) including tenant gating and streaming/error fidelity.tests/test_main.py) in favor of integration smoke coverage.#@test:) across ORM-heavy modules and repository/query code; updates pytest/asyncio loop scope and related warning filters inpyproject.toml.