Open
Conversation
jeffmaki
added a commit
to TaskarCenterAtUW/workspaces-frontend
that referenced
this pull request
Feb 18, 2026
This adds UIs for joining and managing teams. It includes an admin-facing settings page for managing teams and participant-facing landing page for joining teams/invitations. Backend PR: TaskarCenterAtUW/workspaces-backend#1
jeffmaki
approved these changes
Feb 18, 2026
api/main.py
Outdated
|
|
||
| # Optional: Run migrations on startup | ||
| run_migrations() | ||
| #run_migrations() |
Collaborator
Author
There was a problem hiding this comment.
Ehh, I'm lukewarm about it. I put it back for now. We just need to remember that it doesn't execute until Uvicorn services a request and boots the app.
Contributor
There was a problem hiding this comment.
Ahh I see--do you want to move to a place where it runs on start instead? I'm just afraid disabling migrations entirely isn't good...
Contributor
|
Good once you respond to that one question I raised about the commented out migrations--nice work! |
Contributor
|
@coderabbitai can you review this? |
|
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds workspace teams feature (schemas, repository, routes), introduces TDEI OIDC/backend settings and reorders JWT_SECRET in env, standardizes AsyncSession imports to sqlmodel, changes workspace user auth types/relationships, and updates security to use dynamic OIDC URL with relaxed JWT audience verification. Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as "Teams API Route"
participant WRepo as "WorkspaceRepository"
participant TRepo as "WorkspaceTeamRepository"
participant DB as "Database"
Client->>API: GET /workspaces/{workspace_id}/teams
API->>WRepo: get(workspace_id) (auth/perm)
WRepo->>DB: SELECT workspace
DB-->>WRepo: workspace
WRepo-->>API: workspace
API->>TRepo: get_all(workspace_id)
TRepo->>DB: SELECT teams JOIN team_users JOIN users (selectinload)
DB-->>TRepo: teams + members
TRepo-->>API: list[WorkspaceTeamItem]
API-->>Client: 200 list[WorkspaceTeamItem]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/core/security.py (2)
1-1:⚠️ Potential issue | 🟡 MinorFix import ordering to resolve CI failure.
The pipeline reports
isort: Imports are incorrectly sorted and/or formatted. Runisorton this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/core/security.py` at line 1, Run isort on this module and reorder the imports to follow standard grouping (standard library, third-party, local) and formatting; specifically ensure the existing import json is placed in the standard library group and any other imports in this file are sorted alphabetically within their groups so the file passes `isort`/CI checks.
180-185:⚠️ Potential issue | 🟡 MinorDouble slash in URL due to trailing slash in base URL.
settings.TDEI_BACKEND_URLdefaults to"https://portal-api-dev.tdei.us/api/v1/"(with trailing slash), and the concatenation adds"/project-group-roles/"(with leading slash), resulting in.../api/v1//project-group-roles/....While most HTTP servers normalize double slashes, it's better to be explicit:
🔧 Proposed fix
authorizationUrl = ( - settings.TDEI_BACKEND_URL - + "/project-group-roles/" + settings.TDEI_BACKEND_URL.rstrip("/") + + "/project-group-roles/" + user_id + "?page_no=1&page_size=50" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/core/security.py` around lines 180 - 185, The URL concatenation for authorizationUrl can produce a double slash because settings.TDEI_BACKEND_URL may end with a slash; update the construction (where authorizationUrl is built in api/core/security.py) to normalize parts by either stripping any trailing slash from settings.TDEI_BACKEND_URL before adding the endpoint "/project-group-roles/" or by using a URL join utility (e.g., urllib.parse.urljoin) so the final URL contains a single slash between the base and the path and still appends the query string "?page_no=1&page_size=50".api/core/config.py (1)
1-1:⚠️ Potential issue | 🟡 MinorFix import ordering to resolve CI failure.
The pipeline reports
isort: Imports are incorrectly sorted and/or formatted. Runisorton this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/core/config.py` at line 1, The import ordering in api/core/config.py is failing isort; reorder imports to follow isort rules (standard library, third-party, local application groups) and ensure third-party imports like BaseSettings and SettingsConfigDict from pydantic_settings are grouped together; run isort on the file (or apply your project's isort config) so the single-line import "from pydantic_settings import BaseSettings, SettingsConfigDict" is correctly placed/formatted to satisfy CI.
🧹 Nitpick comments (1)
api/main.py (1)
33-33: Address commented-out migration code.The
run_migrations()call is commented out. This was noted by@jeffmakiin the PR review. Consider either removing this code if migrations are handled externally, or adding a comment explaining when/how migrations should be run.Would you like me to help document the intended migration strategy?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/main.py` at line 33, The commented-out run_migrations() call should be resolved: either remove the commented line if DB migrations are performed outside the app, or re-enable it and/or add a concise comment explaining the intended migration strategy and when run_migrations() should execute (e.g., on startup for dev only, gated by an env var like MIGRATE_ON_START, or handled by CI/CD). Locate the run_migrations() occurrence and update the code to reflect the chosen approach and, if kept commented, add a short explanatory comment referencing run_migrations() and the deployment/CI process that runs migrations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/teams/repository.py`:
- Around line 1-13: Remove the unused SQLAlchemy symbol by deleting "update"
from the import line (from sqlalchemy import delete, exists, select[, update])
so it no longer conflicts with the local method name `update`; update the import
to only include delete, exists, select and run the linter to confirm Ruff F811
is resolved.
- Around line 107-123: In remove_member, replace the non-idiomatic membership
check "if not team in user.teams" with the standard form "if team not in
user.teams" to follow Python idioms; update the conditional that references team
and user.teams in the remove_member method so it uses "team not in user.teams"
before returning and leaving the rest of the logic (user.teams.remove(team),
self.session.add(user), await self.session.commit()) unchanged.
In `@api/src/teams/routes.py`:
- Around line 65-75: The endpoint get_team_for_workspace calls
workspace_repo.getById to validate workspace access but then returns
team_repo.get_item(team_id) without verifying the returned team's workspace_id
matches workspace_id; update this to fetch and enforce binding (e.g., use an
existing team_repo method that takes both team_id and workspace_id like
get_by_id_and_workspace or after calling team_repo.get_item(team_id) assert
team.workspace_id == workspace_id and raise a 404/permission error if not) so a
user with access to one workspace cannot retrieve a team from another.
In `@api/src/teams/schemas.py`:
- Around line 1-4: The import block in api/src/teams/schemas.py is not sorted
per the project's isort rules; reorder the imports (from pydantic, sqlmodel,
sqlalchemy, sqlalchemy.orm) into the project's expected order (or simply run
isort on this module) so the lines importing BaseModel,
Field/Integer/Relationship/SQLModel, func/select/text, and column_property are
sorted and CI will pass.
- Around line 36-46: The custom __init__ on WorkspaceTeamItem bypasses
Pydantic/SQLModel initialization; remove the __init__ and add a classmethod
factory like from_team(cls, team) that returns cls(id=team.id, name=team.name,
member_count=len(team.users)) so Pydantic initialization/validation is
preserved; then update call sites that currently do WorkspaceTeamItem(x) and
WorkspaceTeamItem(team) to use WorkspaceTeamItem.from_team(x) and
WorkspaceTeamItem.from_team(team) respectively.
- Around line 22-33: Add a conditional import for the User type using
typing.TYPE_CHECKING and import User only inside that TYPE_CHECKING block so the
forward-reference string "User" in WorkspaceTeam.users is recognized by type
checkers without creating a circular runtime import; update the top of the
module to import TYPE_CHECKING from typing and, within an if TYPE_CHECKING:
block, import User from its module (where User is defined) so WorkspaceTeam, the
users Relationship and WorkspaceTeamUser link_model remain unchanged but type
checkers can resolve the User symbol.
In `@api/src/workspaces/repository.py`:
- Around line 238-243: The get_current_user query compares User.auth_uid (str)
with UserInfo.user_uuid which is annotated as UUID; this causes type-checker
warnings — update the comparison to use str(current_user.user_uuid) in
get_current_user (method get_current_user) so the types align, or alternatively
fix the annotation on the UserInfo.user_uuid declaration to be str; also
consider replacing result.scalar_one() with a safer retrieval (e.g.,
scalar_one_or_none and raise or handle NoResultFound) if missing-user behavior
needs to be explicit.
In `@api/src/workspaces/schemas.py`:
- Line 11: The import ordering in api/src/workspaces/schemas.py is failing
isort; reorder the imports so they follow isort rules (standard library first,
then third-party, then local application imports) and/or run the project's isort
configuration to automatically fix it; specifically ensure the line importing
WorkspaceTeamUser from api.src.teams.schemas is placed with other local imports
according to isort formatting (or run `isort` on the file) so the import
statement for WorkspaceTeamUser matches the repo's sorted import style.
---
Outside diff comments:
In `@api/core/config.py`:
- Line 1: The import ordering in api/core/config.py is failing isort; reorder
imports to follow isort rules (standard library, third-party, local application
groups) and ensure third-party imports like BaseSettings and SettingsConfigDict
from pydantic_settings are grouped together; run isort on the file (or apply
your project's isort config) so the single-line import "from pydantic_settings
import BaseSettings, SettingsConfigDict" is correctly placed/formatted to
satisfy CI.
In `@api/core/security.py`:
- Line 1: Run isort on this module and reorder the imports to follow standard
grouping (standard library, third-party, local) and formatting; specifically
ensure the existing import json is placed in the standard library group and any
other imports in this file are sorted alphabetically within their groups so the
file passes `isort`/CI checks.
- Around line 180-185: The URL concatenation for authorizationUrl can produce a
double slash because settings.TDEI_BACKEND_URL may end with a slash; update the
construction (where authorizationUrl is built in api/core/security.py) to
normalize parts by either stripping any trailing slash from
settings.TDEI_BACKEND_URL before adding the endpoint "/project-group-roles/" or
by using a URL join utility (e.g., urllib.parse.urljoin) so the final URL
contains a single slash between the base and the path and still appends the
query string "?page_no=1&page_size=50".
---
Nitpick comments:
In `@api/main.py`:
- Line 33: The commented-out run_migrations() call should be resolved: either
remove the commented line if DB migrations are performed outside the app, or
re-enable it and/or add a concise comment explaining the intended migration
strategy and when run_migrations() should execute (e.g., on startup for dev
only, gated by an env var like MIGRATE_ON_START, or handled by CI/CD). Locate
the run_migrations() occurrence and update the code to reflect the chosen
approach and, if kept commented, add a short explanatory comment referencing
run_migrations() and the deployment/CI process that runs migrations.
cyrossignol
force-pushed
the
teams-roles
branch
from
5a59962 to
a103a32
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/core/security.py (1)
179-184:⚠️ Potential issue | 🟡 MinorPotential double-slash in authorization URL.
If
settings.TDEI_BACKEND_URLends with a trailing slash (as in.env.example:https://portal-api-dev.tdei.us/api/v1/), concatenating with"/project-group-roles/"produces a double-slash:.../api/v1//project-group-roles/....🔧 Proposed fix
authorizationUrl = ( - settings.TDEI_BACKEND_URL - + "/project-group-roles/" + settings.TDEI_BACKEND_URL.rstrip("/") + + "/project-group-roles/" + user_id + "?page_no=1&page_size=50" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/core/security.py` around lines 179 - 184, authorizationUrl construction can produce a double-slash if settings.TDEI_BACKEND_URL ends with a slash; update the code that builds authorizationUrl (currently referencing settings.TDEI_BACKEND_URL and user_id) to normalize the base URL before concatenation (e.g., rstrip the trailing slash or use a URL join utility) so the path "/project-group-roles/" and the user_id are appended exactly once and the query string remains intact.api/main.py (1)
100-101:⚠️ Potential issue | 🟡 MinorUnreachable code after
raise.The
returnstatement on line 101 is unreachable sinceraise HTTPExceptionalways exits the function. Same issue on line 110.🧹 Proposed fix
raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid authentication credentials", headers={"WWW-Authenticate": "Bearer"}, ) - return else: if not any( re.search(pattern, request.url.path) for pattern in AUTH_WHITELIST_PATHS ): raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="You must set your workspace in the X-Workspace header to access OSM methods.", ) - return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/main.py` around lines 100 - 101, Remove the unreachable return statements that follow raise HTTPException in api/main.py: locate the blocks where an HTTPException is raised (the raise HTTPException(...) calls) and delete the subsequent return lines (the stray "return" after the raise and the similar one around line 110) so control flow is correct; ensure no additional cleanup is required in those code paths and keep the raise as the final statement in each block.
🧹 Nitpick comments (5)
api/src/teams/routes.py (1)
52-62: Consider authorization for team creation.Any user with workspace access can create teams. If team creation should be restricted to workspace leads, add an authorization check using
current_user.isWorkspaceLead(workspace_id).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/teams/routes.py` around lines 52 - 62, The create_team_for_workspace endpoint currently allows any user with workspace access to create teams; add an authorization guard after the workspace_repo.getById(...) call that checks current_user.isWorkspaceLead(workspace_id) and if false raise an HTTPException(status_code=403) to block creation; then only call team_repo.create(workspace_id, team) when the lead check passes. Ensure you reference the existing symbols create_team_for_workspace, workspace_repo.getById, current_user.isWorkspaceLead(workspace_id), and team_repo.create when making the change.api/main.py (1)
32-33: Clarify commented-out migration call.The
run_migrations()call is commented out. A PR comment asked about this. Consider adding a comment explaining why it's disabled, or remove the dead code if migrations are handled separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/main.py` around lines 32 - 33, The commented-out run_migrations() line is ambiguous; either remove the dead code or add a clear explanatory comment above the line explaining why migrations are disabled here (e.g., migrations run in deployment, handled by CI/CD, or using a separate service). Update the area around run_migrations() so reviewers know the intended migration strategy by referencing the run_migrations() call and describing where/when migrations are executed.api/src/teams/repository.py (2)
61-70: Return type mismatch:team.idcould beNone.The method signature declares
-> int, butWorkspaceTeam.idis likely typed asOptional[int](withdefault=None). Aftercommit()andrefresh(), the ID will be set, but the type checker may flag this.💡 Suggestion to satisfy type checker
await self.session.refresh(team) - return team.id + return team.id # type: ignore[return-value] # ID is set after commitOr assert the ID is not None:
assert team.id is not None return team.id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/teams/repository.py` around lines 61 - 70, The return type annotation for create conflicts with WorkspaceTeam.id which can be Optional[int]; in the create method (async def create(self, workspace_id: int, data: WorkspaceTeamCreate) -> int) ensure the ID is non-None before returning by asserting team.id is not None (or alternatively change the signature to -> Optional[int] if you prefer); add the assertion right after await self.session.refresh(team) so the type checker knows team.id is set and you can safely return team.id.
79-81: Consider verifying team exists before deletion.The
deletemethod executes the DELETE statement without checking if the team exists. If no rows match, it silently succeeds. Depending on requirements, you may want to raiseNotFoundExceptionfor consistency with other methods.💡 Proposed fix for consistent behavior
async def delete(self, id: int) -> None: + await self.get(id) # Raises NotFoundException if not found await self.session.exec(delete(WorkspaceTeam).where(WorkspaceTeam.id == id)) await self.session.commit()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/teams/repository.py` around lines 79 - 81, The delete method currently issues a DELETE for WorkspaceTeam by id and commits without checking affected rows; change it to first query for existence (e.g., use self.session.exec(select(WorkspaceTeam).where(WorkspaceTeam.id == id)) or a count query) and if no team is found raise NotFoundException, otherwise perform the delete/commit as before; update the method that uses WorkspaceTeam, delete, self.session.exec and NotFoundException accordingly to ensure consistent behavior with other repository methods.api/src/workspaces/schemas.py (1)
147-149: ImportWorkspaceTeamfor proper type hint resolution and IDE support.The forward reference
"WorkspaceTeam"relies on SQLModel's registry to resolve at runtime. While this may work because theteamsmodule is imported viaWorkspaceTeamUser, explicitly importingWorkspaceTeamimproves type checking and IDE support. Use aTYPE_CHECKINGguard to avoid any potential issues:Suggested improvement
from typing import Any, Optional +from typing import TYPE_CHECKING from uuid import UUID from geoalchemy2 import Geometry from sqlalchemy import JSON as SAJson from sqlalchemy import Column, SmallInteger, TypeDecorator, Unicode from sqlmodel import Field, Relationship, SQLModel from api.src.teams.schemas import WorkspaceTeamUser + +if TYPE_CHECKING: + from api.src.teams.schemas import WorkspaceTeam🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/workspaces/schemas.py` around lines 147 - 149, The teams relationship uses a forward-reference string "WorkspaceTeam" which should be explicitly imported to improve type checking and IDE support; add an import for WorkspaceTeam (the model class referenced) guarded by from typing import TYPE_CHECKING and an if TYPE_CHECKING: block so runtime import cycles are avoided, placing the import alongside the existing WorkspaceTeamUser usage and keeping the field definition teams: list["WorkspaceTeam"] = Relationship(back_populates="users", link_model=WorkspaceTeamUser) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/core/security.py`:
- Line 164: Replace the disabled audience check in jwt.decode by enabling
audience verification: remove or change options={"verify_aud": False} to pass
verify_aud=True and supply the expected audience (e.g., the service/client ID)
via the audience parameter to jwt.decode (or set options accordingly) so
jwt.decode(jwt_token, key, algorithms=..., audience=EXPECTED_AUD,
options={"verify_aud": True}) performs proper OIDC audience validation; update
the code path that calls jwt.decode in api/core/security.py (the JWT decoding
logic) to use the expected audience value or, if intentional, add a clear
justification comment and an alternative anti-substitution check.
In `@api/src/teams/repository.py`:
- Around line 20-27: The get_all method is constructing WorkspaceTeamItem
instances with the wrong call signature (WorkspaceTeamItem(x)), which will fail
because WorkspaceTeamItem expects specific fields and provides a factory; update
the return comprehension in get_all to call the factory method
WorkspaceTeamItem.from_team for each WorkspaceTeam (use
WorkspaceTeamItem.from_team(x) for x in result.scalars()) so items are created
correctly from the ORM objects.
In `@api/src/teams/routes.py`:
- Around line 154-166: The delete_member_from_workspace_team endpoint lacks an
authorization check allowing any authenticated workspace user to remove team
members; before calling team_repo.remove_member, add an explicit permission
check (e.g., call a repo method like team_repo.assert_user_is_team_admin or
workspace_repo.assert_user_is_admin) that verifies current_user has the required
role/permission for team_id/workspace_id, reuse existing helpers such as
workspace_repo.getById and team_repo.assert_team_in_workspace for context, and
only proceed to await team_repo.remove_member(team_id, user_id) if that
authorization assertion passes.
- Around line 139-151: The add_member_to_workspace_team (and the corresponding
delete_member_from_workspace_team) endpoints currently only validate membership;
update them to enforce workspace lead privileges by either adding the
isWorkspaceLead() dependency (e.g., add a parameter like is_lead: bool =
Depends(isWorkspaceLead)) or by calling the workspace-repo lead-check helper
before modifying team membership (e.g., invoke a method such as await
workspace_repo.ensure_workspace_lead(current_user, workspace_id) or similar),
then proceed to call await team_repo.add_member(...) / await
team_repo.remove_member(...); reference add_member_to_workspace_team,
delete_member_from_workspace_team, validate_token, isWorkspaceLead,
workspace_repo.getById, and team_repo.add_member/remove_member to locate the
spots to change.
---
Outside diff comments:
In `@api/core/security.py`:
- Around line 179-184: authorizationUrl construction can produce a double-slash
if settings.TDEI_BACKEND_URL ends with a slash; update the code that builds
authorizationUrl (currently referencing settings.TDEI_BACKEND_URL and user_id)
to normalize the base URL before concatenation (e.g., rstrip the trailing slash
or use a URL join utility) so the path "/project-group-roles/" and the user_id
are appended exactly once and the query string remains intact.
In `@api/main.py`:
- Around line 100-101: Remove the unreachable return statements that follow
raise HTTPException in api/main.py: locate the blocks where an HTTPException is
raised (the raise HTTPException(...) calls) and delete the subsequent return
lines (the stray "return" after the raise and the similar one around line 110)
so control flow is correct; ensure no additional cleanup is required in those
code paths and keep the raise as the final statement in each block.
---
Nitpick comments:
In `@api/main.py`:
- Around line 32-33: The commented-out run_migrations() line is ambiguous;
either remove the dead code or add a clear explanatory comment above the line
explaining why migrations are disabled here (e.g., migrations run in deployment,
handled by CI/CD, or using a separate service). Update the area around
run_migrations() so reviewers know the intended migration strategy by
referencing the run_migrations() call and describing where/when migrations are
executed.
In `@api/src/teams/repository.py`:
- Around line 61-70: The return type annotation for create conflicts with
WorkspaceTeam.id which can be Optional[int]; in the create method (async def
create(self, workspace_id: int, data: WorkspaceTeamCreate) -> int) ensure the ID
is non-None before returning by asserting team.id is not None (or alternatively
change the signature to -> Optional[int] if you prefer); add the assertion right
after await self.session.refresh(team) so the type checker knows team.id is set
and you can safely return team.id.
- Around line 79-81: The delete method currently issues a DELETE for
WorkspaceTeam by id and commits without checking affected rows; change it to
first query for existence (e.g., use
self.session.exec(select(WorkspaceTeam).where(WorkspaceTeam.id == id)) or a
count query) and if no team is found raise NotFoundException, otherwise perform
the delete/commit as before; update the method that uses WorkspaceTeam, delete,
self.session.exec and NotFoundException accordingly to ensure consistent
behavior with other repository methods.
In `@api/src/teams/routes.py`:
- Around line 52-62: The create_team_for_workspace endpoint currently allows any
user with workspace access to create teams; add an authorization guard after the
workspace_repo.getById(...) call that checks
current_user.isWorkspaceLead(workspace_id) and if false raise an
HTTPException(status_code=403) to block creation; then only call
team_repo.create(workspace_id, team) when the lead check passes. Ensure you
reference the existing symbols create_team_for_workspace,
workspace_repo.getById, current_user.isWorkspaceLead(workspace_id), and
team_repo.create when making the change.
In `@api/src/workspaces/schemas.py`:
- Around line 147-149: The teams relationship uses a forward-reference string
"WorkspaceTeam" which should be explicitly imported to improve type checking and
IDE support; add an import for WorkspaceTeam (the model class referenced)
guarded by from typing import TYPE_CHECKING and an if TYPE_CHECKING: block so
runtime import cycles are avoided, placing the import alongside the existing
WorkspaceTeamUser usage and keeping the field definition teams:
list["WorkspaceTeam"] = Relationship(back_populates="users",
link_model=WorkspaceTeamUser) unchanged.
a103a32 to
b6ed442
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/core/security.py (1)
1-15:⚠️ Potential issue | 🟡 MinorFix import ordering to satisfy isort.
api.core.loggingis imported before third‑party packages. Reorder to stdlib → third‑party → local.🧹 Suggested import order
-import json -from enum import StrEnum -import requests - -from api.core.logging import get_logger -import jwt -import cachetools -from fastapi import Depends, HTTPException, status -from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer -from sqlalchemy import text -from sqlmodel.ext.asyncio.session import AsyncSession - -from api.core.config import settings +import json +from enum import StrEnum + +import cachetools +import jwt +import requests +from fastapi import Depends, HTTPException, status +from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer +from sqlalchemy import text +from sqlmodel.ext.asyncio.session import AsyncSession + +from api.core.config import settings +from api.core.logging import get_logger🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/core/security.py` around lines 1 - 15, Reorder the imports in this module to satisfy isort: place standard library imports (json, enum.StrEnum) first, then third‑party packages (requests, jwt, cachetools, fastapi.FastAPI/Depends/HTTPException/status, fastapi.security.HTTPAuthorizationCredentials/HTTPBearer, sqlalchemy.text, sqlmodel.ext.asyncio.session.AsyncSession), and finally local project imports (api.core.logging.get_logger, api.core.config.settings, api.core.database.get_osm_session/get_task_session, api.src.workspaces.schemas.WorkspaceUserRoleType); update the import block in api/core/security.py so the listed symbols appear in that order and remove any blank-line ordering issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 1-6: Reorder the environment variables in .env.example to satisfy
dotenv-linter's UnorderedKey rule by sorting the keys alphabetically;
specifically reorder entries like TASK_DATABASE_URL, OSM_DATABASE_URL,
TDEI_BACKEND_URL, TDEI_OIDC_URL, TDEI_OIDC_REALM, and JWT_SECRET into
alphabetical order so the linter no longer reports UnorderedKey.
In `@api/src/teams/schemas.py`:
- Around line 5-6: Update the TYPE_CHECKING import in the TYPE_CHECKING block:
replace the incorrect import "from api.src.users.schemas import User" with the
correct module export "from api.src.workspaces.schemas import User" so the User
type used by the teams schemas resolves to the actual definition; locate the
TYPE_CHECKING block in teams/schemas.py and change that single import statement.
In `@api/src/workspaces/repository.py`:
- Around line 238-243: In get_current_user, replace result.scalar_one() with
result.scalar_one_or_none() and explicitly handle a missing user by raising
NotFoundException; specifically, after executing
select(User).where(User.auth_uid == current_user.user_uuid) check if the scalar
result is None and raise NotFoundException(...) (with an appropriate
message/context) instead of allowing scalar_one() to throw.
In `@api/src/workspaces/schemas.py`:
- Around line 139-143: The User.auth_uid field is currently declared as str
while WorkspaceUserRole.auth_user_uid and the DB migration expect a UUID,
causing a type mismatch; fix this by changing User.auth_uid to UUID (update the
import to UUID and the Field type on the User model) so it matches
WorkspaceUserRole.auth_user_uid and the existing migration, then run/adjust
migrations or regenerate schema as needed; alternatively, if you prefer string
IDs instead, change WorkspaceUserRole.auth_user_uid to str and update its
foreign_key to "users.auth_uid" and adjust the migration to create a
text/varchar column.
- Around line 147-149: The undefined WorkspaceTeam name comes from using a
forward-reference type in the Relationship declaration inside the Workspace
schema; add a typing guard to avoid runtime import cycles by importing
TYPE_CHECKING (from typing) at the top and then, under "if TYPE_CHECKING:",
import WorkspaceTeam from its module (the model that defines WorkspaceTeam) so
the type checker sees WorkspaceTeam but the import isn't executed at runtime;
leave the Relationship as list["WorkspaceTeam"] = Relationship(...).
---
Outside diff comments:
In `@api/core/security.py`:
- Around line 1-15: Reorder the imports in this module to satisfy isort: place
standard library imports (json, enum.StrEnum) first, then third‑party packages
(requests, jwt, cachetools, fastapi.FastAPI/Depends/HTTPException/status,
fastapi.security.HTTPAuthorizationCredentials/HTTPBearer, sqlalchemy.text,
sqlmodel.ext.asyncio.session.AsyncSession), and finally local project imports
(api.core.logging.get_logger, api.core.config.settings,
api.core.database.get_osm_session/get_task_session,
api.src.workspaces.schemas.WorkspaceUserRoleType); update the import block in
api/core/security.py so the listed symbols appear in that order and remove any
blank-line ordering issues.
---
Duplicate comments:
In `@api/core/security.py`:
- Around line 160-165: The JWT decode call currently disables audience
verification via options={"verify_aud": False} in jwt.decode_complete; update
the logic in the jwt.decode_complete usage to enable audience checks (remove or
set verify_aud=True) and pass the expected audience (fetch from your
configuration—e.g., an OIDC_AUDIENCE/expected_audience config entry) using the
audience parameter to jwt.decode_complete, or if you intentionally cannot
enforce an aud, add a clear runtime guard that fetches and verifies token["aud"]
against the configured expected audience and document that safeguard in the same
module (referencing jwt.decode_complete, token, signing_key, and the config key
that supplies the expected audience).
In `@api/src/teams/routes.py`:
- Around line 139-166: Both endpoints (add_member_to_workspace_team and
delete_member_from_workspace_team) currently only check workspace access; add a
privileged lead/admin check so only workspace leads can add/remove team members.
After the existing workspace_repo.getById(...) call (and before performing
team_repo.add_member/remove_member), call your existing lead-check helper (e.g.,
assert_user_is_lead or ensure_workspace_lead) with current_user and workspace_id
(importing it if necessary) and return/raise if the check fails; apply this
change to both functions so the lead assertion runs prior to
team_repo.assert_team_in_workspace(...) or the add/remove call as per your
existing patterns.
745c2ff to
cd75492
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/core/security.py (2)
200-241:⚠️ Potential issue | 🟠 MajorGuard UUID parsing of token
subclaim to return 401 instead of 500.The code at line 202 calls
UUID(payload.get("sub", "unknown"))without error handling. While thesubclaim is validated for presence at lines 170–172, it is not validated as a well-formed UUID. Ifsubexists but contains an invalid UUID string (e.g., a truncated or corrupted value), theUUID()constructor will raise an unhandledValueError, causing the request to return a 500 error instead of the expected 401 Unauthorized response.Wrap the UUID parsing in a try-except block to catch
ValueErrorand raisecredentials_exception:🛠️ Proposed fix
- r.user_uuid = UUID(payload.get("sub", "unknown")) + sub = payload.get("sub") + try: + r.user_uuid = UUID(sub) + except ValueError: + raise credentials_exception🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/core/security.py` around lines 200 - 241, The UUID(...) conversion of the token "sub" claim can raise ValueError and cause a 500; wrap the parsing of payload.get("sub", ...) into a try/except that catches ValueError and raises the existing credentials_exception instead of allowing the exception to bubble up: replace the direct assignment r.user_uuid = UUID(payload.get("sub", "unknown")) with a guarded parse (try UUID(...) assign to r.user_uuid; except ValueError: raise credentials_exception) so malformed UUIDs produce a 401.
155-186:⚠️ Potential issue | 🟠 MajorFix double-slash in BACKEND_URL concatenation.
The
TDEI_BACKEND_URLsetting ("https://portal-api-dev.tdei.us/api/v1/") ends with a trailing slash, and the path concatenation (+ "/project-group-roles/") starts with a slash, creating a malformed URL with double slashes. Normalize the base URL before concatenation.Note: The
TDEI_OIDC_URLusage at line 156 currently works correctly (concatenates with "realms" which lacks a leading slash), but normalizing it is still good defensive practice.🔧 Proposed fix
- jwks_client = jwt.PyJWKClient( - f"{settings.TDEI_OIDC_URL}realms/{settings.TDEI_OIDC_REALM}/protocol/openid-connect/certs" - ) + oidc_base = settings.TDEI_OIDC_URL.rstrip("/") + jwks_client = jwt.PyJWKClient( + f"{oidc_base}/realms/{settings.TDEI_OIDC_REALM}/protocol/openid-connect/certs" + ) @@ - authorizationUrl = ( - settings.TDEI_BACKEND_URL - + "/project-group-roles/" - + user_id - + "?page_no=1&page_size=50" - ) + backend_base = settings.TDEI_BACKEND_URL.rstrip("/") + authorizationUrl = ( + f"{backend_base}/project-group-roles/{user_id}?page_no=1&page_size=50" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/core/security.py` around lines 155 - 186, The URL for authorizationUrl is being built by naive string concatenation using settings.TDEI_BACKEND_URL which may end with a trailing slash, producing a double-slash; update the construction to normalize the base URL first (e.g., strip any trailing slash from settings.TDEI_BACKEND_URL or use a proper URL join) before appending "/project-group-roles/" + user_id + "?page_no=1&page_size=50"; also apply the same normalization defensively when building the jwks_client URL from settings.TDEI_OIDC_URL so both TDEI_BACKEND_URL and TDEI_OIDC_URL are normalized before concatenation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/core/security.py`:
- Around line 1-15: Reorder the top-level imports to satisfy isort: place
standard library imports first (json, StrEnum from enum, UUID from uuid), then
third-party packages (requests, jwt, cachetools, fastapi and fastapi.security
HTTPAuthorizationCredentials/HTTPBearer, sqlalchemy.text,
sqlmodel.ext.asyncio.session.AsyncSession), and finally local application
imports (get_logger, settings, get_osm_session, get_task_session); update the
import block containing json, StrEnum, UUID, requests, jwt, cachetools,
HTTPAuthorizationCredentials, HTTPBearer, text, AsyncSession, get_logger,
settings, get_osm_session, and get_task_session to follow that order.
In `@api/main.py`:
- Around line 32-33: The startup migration call has been commented out—uncomment
the run_migrations() invocation in api/main.py (or alternatively add an explicit
migration step) so DB schema changes are applied before the app serves requests;
update the Dockerfile, docker-compose.yml or your CI/CD pipeline to run the same
migration command (or an entrypoint script that calls run_migrations()) to
ensure migrations for the teams feature and future schema changes are executed
during deployment.
In `@api/src/workspaces/schemas.py`:
- Around line 143-144: Change the User.id annotation to allow None so the DB can
autoincrement: update the declaration "id: int = Field(default=None,
primary_key=True)" to use a nullable type (e.g., "id: int | None =
Field(default=None, primary_key=True)" or "id: Optional[int] = Field(...)" with
an appropriate import), matching the pattern used in teams/schemas.py and
SQLModel docs; keep the Field(...) params the same so inserts can omit id and
the database will generate it.
---
Outside diff comments:
In `@api/core/security.py`:
- Around line 200-241: The UUID(...) conversion of the token "sub" claim can
raise ValueError and cause a 500; wrap the parsing of payload.get("sub", ...)
into a try/except that catches ValueError and raises the existing
credentials_exception instead of allowing the exception to bubble up: replace
the direct assignment r.user_uuid = UUID(payload.get("sub", "unknown")) with a
guarded parse (try UUID(...) assign to r.user_uuid; except ValueError: raise
credentials_exception) so malformed UUIDs produce a 401.
- Around line 155-186: The URL for authorizationUrl is being built by naive
string concatenation using settings.TDEI_BACKEND_URL which may end with a
trailing slash, producing a double-slash; update the construction to normalize
the base URL first (e.g., strip any trailing slash from
settings.TDEI_BACKEND_URL or use a proper URL join) before appending
"/project-group-roles/" + user_id + "?page_no=1&page_size=50"; also apply the
same normalization defensively when building the jwks_client URL from
settings.TDEI_OIDC_URL so both TDEI_BACKEND_URL and TDEI_OIDC_URL are normalized
before concatenation.
---
Duplicate comments:
In @.env.example:
- Around line 1-6: Reorder the environment variable entries in .env.example so
keys are alphabetical; specifically arrange the lines for TASK_DATABASE_URL,
TDEI_BACKEND_URL, TDEI_OIDC_REALM, TDEI_OIDC_URL, JWT_SECRET, OSM_DATABASE_URL
(or refer to each key by name) into ascending alphabetical order to satisfy
dotenv-linter; update the file so the keys appear sorted and preserve their
values and formatting.
In `@api/src/teams/routes.py`:
- Around line 139-166: The add/remove endpoints currently call
workspace_repo.getById and then directly invoke
team_repo.add_member/remove_member; before those calls enforce an authorization
check that only workspace leads/admins can modify team membership by calling the
existing workspace lead/admin guard (e.g., workspace_repo.isWorkspaceLead or
workspace_repo.ensure_workspace_lead — or the project’s equivalent lead/admin
check used elsewhere) and return 403 if the check fails; update
add_member_to_workspace_team and delete_member_from_workspace_team to perform
that lead/admin check (using current_user and workspace_id) prior to calling
team_repo.add_member or team_repo.remove_member.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
api/src/teams/repository.py (1)
79-81: Silent no-op when deleting non-existent team.The
delete()method doesn't verify the team exists before deletion. If the team doesn't exist, this silently succeeds. Consider callingawait self.get(id)first to raiseNotFoundException, matching the pattern in other methods.🔧 Suggested fix
async def delete(self, id: int) -> None: + await self.get(id) # Raises NotFoundException if not found await self.session.exec(delete(WorkspaceTeam).where(WorkspaceTeam.id == id)) await self.session.commit()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/teams/repository.py` around lines 79 - 81, The delete method in repository.py silently no-ops when the WorkspaceTeam doesn't exist; update async def delete(self, id: int) to first call await self.get(id) so it raises NotFoundException for missing teams (consistent with other methods), then proceed with the existing await self.session.exec(delete(WorkspaceTeam).where(WorkspaceTeam.id == id)) and commit; reference the delete method, self.get, NotFoundException and WorkspaceTeam to locate and apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/teams/repository.py`:
- Around line 83-86: In get_members,
session.exec(select(User).where(User.teams.any(id=id))) returns a ScalarResult,
so calling .scalars().all() is invalid; replace the final call with result.all()
(i.e., return the ScalarResult's .all()) to return the list of User
objects—update the return in the get_members function accordingly.
- Around line 20-27: The code calls .scalars() on the return value of
session.exec, which already yields a ScalarResult and makes .scalars() invalid;
in get_all replace the list comprehension that iterates over result.scalars()
with one that iterates directly over result (e.g.,
[WorkspaceTeamItem.from_team(x) for x in result]) so WorkspaceTeamItem.from_team
receives each WorkspaceTeam row from the ScalarResult returned by
session.exec(select(WorkspaceTeam).options(selectinload(WorkspaceTeam.users)).where(WorkspaceTeam.workspace_id
== workspace_id)).
In `@api/src/teams/routes.py`:
- Line 1: Remove the unused HTTPException import from the top of the file (the
line importing APIRouter, Depends, HTTPException, status) and re-run isort to
correct import ordering/formatting; specifically update the import to only
include APIRouter, Depends, and status (referencing the import line that
currently mentions HTTPException) and then run `isort` on
api/src/teams/routes.py to apply proper sorting and formatting.
In `@api/src/workspaces/schemas.py`:
- Around line 11-14: Reorder the import block at the top of the module so it
conforms to isort rules: place standard library imports first (including
typing.TYPE_CHECKING), then third-party, then local app imports; specifically
ensure TYPE_CHECKING/typing import appears before or grouped correctly with
other typing imports and that the local imports for WorkspaceTeamUser and the
TYPE_CHECKING-only WorkspaceTeam are ordered/formatted per isort. You can run
`isort` on the file or manually move the `from typing import TYPE_CHECKING` (or
the TYPE_CHECKING import group) so the imports are correctly grouped and
alphabetized around the local imports WorkspaceTeamUser and WorkspaceTeam.
---
Duplicate comments:
In @.env.example:
- Around line 1-6: The .env.example currently triggers dotenv-linter
UnorderedKey warnings; reorder the environment keys alphabetically to satisfy
the linter (for example sort lines containing TASK_DATABASE_URL,
TDEI_BACKEND_URL, TDEI_OIDC_REALM, TDEI_OIDC_URL, OSM_DATABASE_URL, JWT_SECRET)
so the file is lexicographically ordered; update the file in-place preserving
values and formatting, then re-run dotenv-linter to confirm the UnorderedKey
warnings are gone.
In `@api/src/workspaces/repository.py`:
- Around line 238-244: Replace the use of result.scalar_one() in
get_current_user with result.scalar_one_or_none(), check for a None return and
raise the same NotFoundException used by getById/get (or the repository's
standard NotFoundException) with a clear message (e.g., "User not found"
including current_user.user_uuid) so the method returns a 404 instead of a
generic 500 when the user is missing.
In `@api/src/workspaces/schemas.py`:
- Line 143: The User model's primary key annotation uses "id: int =
Field(default=None, primary_key=True)" which prevents SQLModel from treating it
as nullable/autoincrementable; change the type annotation to allow None (e.g.,
use "int | None" or "Optional[int]") while keeping Field(default=None,
primary_key=True) so the database can auto-generate the id (update the User.id
declaration to match the pattern used in teams/schemas.py).
---
Nitpick comments:
In `@api/src/teams/repository.py`:
- Around line 79-81: The delete method in repository.py silently no-ops when the
WorkspaceTeam doesn't exist; update async def delete(self, id: int) to first
call await self.get(id) so it raises NotFoundException for missing teams
(consistent with other methods), then proceed with the existing await
self.session.exec(delete(WorkspaceTeam).where(WorkspaceTeam.id == id)) and
commit; reference the delete method, self.get, NotFoundException and
WorkspaceTeam to locate and apply the change.
Comment on lines
+20
to
+27
| async def get_all(self, workspace_id: int) -> list[WorkspaceTeamItem]: | ||
| result = await self.session.exec( | ||
| select(WorkspaceTeam) | ||
| .options(selectinload(WorkspaceTeam.users)) | ||
| .where(WorkspaceTeam.workspace_id == workspace_id) | ||
| ) | ||
|
|
||
| return [WorkspaceTeamItem.from_team(x) for x in result.scalars()] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the actual file to see the code in context
echo "=== File: api/src/teams/repository.py (lines 15-35) ==="
cat -n api/src/teams/repository.py | sed -n '15,35p'
echo ""
echo "=== Search for session.exec usage patterns in the codebase ==="
rg -n "session\.exec" api/src --type py -B 1 -A 2 | head -50Repository: TaskarCenterAtUW/workspaces-backend
Length of output: 3669
🏁 Script executed:
#!/bin/bash
# Check if there are any imports or type hints that clarify the return type
echo "=== Check imports in repository.py ==="
head -20 api/src/teams/repository.py
echo ""
echo "=== Look for other .scalars() calls in the codebase ==="
rg "\.scalars\(\)" api/src --type py -B 1 -A 1Repository: TaskarCenterAtUW/workspaces-backend
Length of output: 1410
🌐 Web query:
SQLModel session.exec() return type ScalarResult documentation
💡 Result:
In SQLModel, Session.exec() is overloaded so its return type depends on the statement you pass:
- If you execute a scalar select (
SelectOfScalar[T], e.g.select(Hero)orselect(Hero.name)),exec()returnssqlalchemy.engine.ScalarResult[T]. (raw.githubusercontent.com) - If you execute a tuple/multi-column select (
Select[Tuple[...]], e.g.select(Hero, Team)),exec()returnssqlalchemy.engine.TupleResult[Tuple[...]]. (raw.githubusercontent.com) - If you execute an UPDATE/DELETE (
UpdateBase),exec()returnssqlalchemy.engine.CursorResult. (raw.githubusercontent.com)
Where to find the docs you’re looking for:
- SQLModel implementation + type hints (source of truth for the exact return types):
sqlmodel/orm/session.pyoverloads forSession.exec()(raw.githubusercontent.com) - SQLModel docs explaining “exec() auto-calls .scalars()” for scalar selects: SQLModel “Read Data - SELECT” tutorial (sqlmodel.tiangolo.com)
- SQLAlchemy docs for what
ScalarResultis / how to use it (.all(),.first(),.one(), etc.): SQLAlchemy “Working with Engines and Connections” →ScalarResultAPI (docs.sqlalchemy.org) - SQLAlchemy ORM doc explicitly stating
Session.scalars()returnsScalarResult: SQLAlchemySession.scalars()docs (docs.sqlalchemy.org)
Citations:
- 1: https://raw.githubusercontent.com/fastapi/sqlmodel/main/sqlmodel/orm/session.py
- 2: https://raw.githubusercontent.com/fastapi/sqlmodel/main/sqlmodel/orm/session.py
- 3: https://raw.githubusercontent.com/fastapi/sqlmodel/main/sqlmodel/orm/session.py
- 4: https://raw.githubusercontent.com/fastapi/sqlmodel/main/sqlmodel/orm/session.py
- 5: https://sqlmodel.tiangolo.com/tutorial/select/?utm_source=openai
- 6: https://docs.sqlalchemy.org/en/20/core/connections.html
- 7: https://docs.sqlalchemy.org/en/14/orm/session_api.html?highlight=scalars&utm_source=openai
Remove redundant .scalars() call—session.exec() already returns ScalarResult.
When using SQLModel's session.exec() with a scalar select statement (like select(WorkspaceTeam)), it returns ScalarResult directly. The method automatically applies scalar semantics, so calling .scalars() again is redundant and will raise AttributeError: 'ScalarResult' object has no attribute 'scalars' at runtime.
Iterate directly over the result:
Diff
- return [WorkspaceTeamItem.from_team(x) for x in result.scalars()]
+ return [WorkspaceTeamItem.from_team(x) for x in result]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/teams/repository.py` around lines 20 - 27, The code calls .scalars()
on the return value of session.exec, which already yields a ScalarResult and
makes .scalars() invalid; in get_all replace the list comprehension that
iterates over result.scalars() with one that iterates directly over result
(e.g., [WorkspaceTeamItem.from_team(x) for x in result]) so
WorkspaceTeamItem.from_team receives each WorkspaceTeam row from the
ScalarResult returned by
session.exec(select(WorkspaceTeam).options(selectinload(WorkspaceTeam.users)).where(WorkspaceTeam.workspace_id
== workspace_id)).
Collaborator
Author
There was a problem hiding this comment.
This comment is wrong. The result type is a ChunkedIteratorResult. Calling .scalars() works fine.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SQLModel AsyncSession exec return type ChunkedIteratorResult
💡 Result:
In SQLModel, AsyncSession.exec() is typed to return SQLAlchemy result objects, not your ORM models directly:
await session.exec(select(Model))→TupleResult[Model]await session.exec(select(Model.some_col))(scalar select) →ScalarResult[T]await session.exec(update(Model)... )/ other DML →CursorResult[Any][1][2]
If you’re seeing a ChunkedIteratorResult, that’s what SQLAlchemy’s AsyncSession.execute() (or SQLModel’s deprecated AsyncSession.execute()) can return for ORM selects; you then need to extract rows/scalars, e.g. result.scalars().one()/all() [3]. SQLModel’s exec() exists largely to do the “scalar handling” automatically (so you can usually call .all(), .first(), .one(), etc. on the returned ScalarResult/TupleResult) [1][2].
🌐 Web query:
SQLAlchemy AsyncSession execute ChunkedIteratorResult scalars
💡 Result:
AsyncSession.execute() returns a SQLAlchemy Result object (often printed as ChunkedIteratorResult). You don’t access ORM attributes on it directly—you consume it.
ORM entity (e.g., select(User))
result = await session.execute(select(User).where(User.id == 1))
user = result.scalars().one() # or .first(), .one_or_none()
print(user.name)scalars() “unwraps” the first column (your User entity) so you get User objects instead of Row(...). [2]
Shortcut: AsyncSession.scalars()
users = (await session.scalars(select(User))).all()AsyncSession.scalars() is a built-in shorthand for await execute(...); result.scalars(). [1]
Single scalar value (e.g., select(User.id), select(func.count()))
value = (await session.execute(select(func.count(User.id)))).scalar_one()
# or:
value = await session.scalar(select(func.count(User.id)))AsyncSession.scalar() / AsyncConnection.scalar() are shorthand helpers for “execute then return a scalar”. [1]
Async/streaming consumption
If you want to iterate without loading everything:
async_result = await session.stream_scalars(select(User))
async for user in async_result:
...stream_scalars() returns an async scalar stream. [1]
@cyrossignol, you're absolutely right, and I apologize for the incorrect review comment.
With AsyncSession.exec(), the return type is indeed ChunkedIteratorResult (a SQLAlchemy Result object), which requires calling .scalars() to unwrap the ORM entities. I confused this with the synchronous Session.exec() behavior in SQLModel, where scalar semantics are applied automatically.
Your code is correct as written—the .scalars() call is necessary here. Thank you for the correction!
Comment on lines
+83
to
+86
| async def get_members(self, id: int) -> list[User]: | ||
| result = await self.session.exec(select(User).where(User.teams.any(id=id))) | ||
|
|
||
| return result.scalars().all() |
There was a problem hiding this comment.
Redundant .scalars() call and potential runtime error.
Same issue as line 27: session.exec() returns a ScalarResult, not a Result. Calling .scalars().all() will fail.
- return result.scalars().all()
+ return list(result.all())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/teams/repository.py` around lines 83 - 86, In get_members,
session.exec(select(User).where(User.teams.any(id=id))) returns a ScalarResult,
so calling .scalars().all() is invalid; replace the final call with result.all()
(i.e., return the ScalarResult's .all()) to return the list of User
objects—update the return in the get_members function accordingly.
Collaborator
Author
There was a problem hiding this comment.
This comment is wrong. The result type is a ChunkedIteratorResult. Calling .scalars().all() works fine.
There was a problem hiding this comment.
@cyrossignol, you're absolutely right, and I apologize for the incorrect review comment.
In SQLModel/SQLAlchemy async contexts, session.exec(select(Model)) returns a Result (specifically a ChunkedIteratorResult in async sessions), not a ScalarResult. Calling .scalars() on this result is the correct way to extract the scalar values, and then .all() materializes them into a list. The code at line 85 is correct as written.
Thank you for the correction!
✏️ Learnings added
Learnt from: cyrossignol
Repo: TaskarCenterAtUW/workspaces-backend PR: 1
File: api/src/teams/repository.py:83-86
Timestamp: 2026-02-19T03:10:52.535Z
Learning: In SQLModel async sessions, `session.exec(select(Model))` returns a `Result` object (specifically `ChunkedIteratorResult`), not a `ScalarResult`. The pattern `result.scalars().all()` is correct: `.scalars()` extracts scalar values returning a `ScalarResult`, then `.all()` materializes them into a list.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+11
to
+14
| from api.src.teams.schemas import WorkspaceTeamUser | ||
|
|
||
| if TYPE_CHECKING: | ||
| from api.src.teams.schemas import WorkspaceTeam |
There was a problem hiding this comment.
Fix import ordering to resolve CI failure.
The pipeline reports isort: Imports are incorrectly sorted and/or formatted. Run isort api/src/workspaces/schemas.py to fix the import ordering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/workspaces/schemas.py` around lines 11 - 14, Reorder the import block
at the top of the module so it conforms to isort rules: place standard library
imports first (including typing.TYPE_CHECKING), then third-party, then local app
imports; specifically ensure TYPE_CHECKING/typing import appears before or
grouped correctly with other typing imports and that the local imports for
WorkspaceTeamUser and the TYPE_CHECKING-only WorkspaceTeam are ordered/formatted
per isort. You can run `isort` on the file or manually move the `from typing
import TYPE_CHECKING` (or the TYPE_CHECKING import group) so the imports are
correctly grouped and alphabetized around the local imports WorkspaceTeamUser
and WorkspaceTeam.
Collaborator
Author
There was a problem hiding this comment.
isort claims there are no changes needed here.
There was a problem hiding this comment.
@cyrossignol, you're absolutely right—I apologize for the incorrect flag. The imports in api/src/workspaces/schemas.py are properly ordered and conform to isort's rules. The file already has:
- Standard library imports (datetime, enum, typing, uuid)
- Third-party imports (geoalchemy2, sqlalchemy, sqlmodel)
- Local imports (api.src.teams.schemas)
- TYPE_CHECKING conditional import
If there was an isort failure in CI, it must have been for a different file, not this one. Thank you for verifying!
cyrossignol
force-pushed
the
teams-roles
branch
from
cd75492 to
60151ff
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds the initial API for the teams features of TaskarCenterAtUW/workspaces-frontend#42.
Summary by CodeRabbit
New Features
Chores
API