feat(cloud): workspace bootstrap, resolution & management API (C2)#367
Open
OBenner wants to merge 8 commits into
Open
feat(cloud): workspace bootstrap, resolution & management API (C2)#367OBenner wants to merge 8 commits into
OBenner wants to merge 8 commits into
Conversation
Second slice of Track C — the workspace-context layer that makes the C1 models usable. - CLOUD_MODE config flag (single|team, validated; default single). - services/workspace_service.py: get_or_create_personal_workspace (idempotent single-mode default), list_accessible_workspaces (owned ∪ member), create_workspace. - core/permissions.py: get_current_workspace dependency — single mode resolves the caller's auto-created Personal workspace; team mode requires ?workspace_id and enforces >= viewer access. Extracted a shared _current_user_id helper (reused by require_workspace_access). - api/routes/workspaces.py: GET /api/workspaces (list with role), GET /api/workspaces/current, POST /api/workspaces; registered in main.py. - Personal-workspace bootstrap hooked into register + login (idempotent; backfills pre-C2 accounts). - Tests: bootstrap idempotency, owned∪member listing, single/team resolution (+400/403), and an end-to-end HTTP test of the router. Restores the owner-access test's membership-absence assertion dropped by the C1 squash. 11 workspace tests pass; existing auth/user route tests (49) unaffected; ruff clean. No schema change: per-resource scoping (repos/runs) lands as each gains DB persistence (C3/C6); today only 'repositories' is a DB model and has no CRUD routes yet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds workspace schemas, cloud-mode validation, persistence helpers, workspace resolution, new workspace routes, app wiring, migrations, and tests for bootstrap, access, and member management. ChangesWorkspace multitenancy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@apps/web-backend/api/routes/workspaces.py`:
- Around line 63-67: The workspace listing in the route that builds
WorkspaceListResponse has an N+1 query pattern because the loop over
list_accessible_workspaces calls user_role_in_workspace for each workspace. Fix
this by deriving the role without re-fetching each row in the loop, using
ws.owner_id to detect OWNER and a single batched membership lookup keyed by
workspace_id for the remaining workspaces, then pass the resolved role into
_to_response.
- Line 56: The FastAPI route signatures in workspaces.py should be updated to
match the newer idioms flagged by SonarCloud: remove the redundant
response_model from the `@router.get/`@router.post decorators where the return
type annotation already defines the response, and convert any dependency
parameters that currently use default-value Depends(...) to Annotated[T,
Depends(...)] in the affected route functions. Apply this consistently to the
route handlers identified by the decorated endpoints and their dependency
parameters so the signatures are idiomatic without changing behavior.
In `@apps/web-backend/core/permissions.py`:
- Around line 149-157: The workspace fetch in the team-mode permission path is
duplicated because `check_workspace_access` already performs the `Workspace`
lookup via `user_role_in_workspace`. Update the resolver around the existing
`check_workspace_access` call to avoid calling
`db.query(Workspace).filter(...).first()` up front; instead, perform the access
check first and only fetch/reuse the `Workspace` after it succeeds, while
keeping the same `HTTPException` behavior for denied access.
In `@apps/web-backend/services/workspace_service.py`:
- Around line 33-52: The get_or_create_personal_workspace flow is vulnerable to
duplicate inserts because the existing lookup and insert are not atomic. Update
the Workspace creation path to be idempotent by using a database-level
uniqueness guarantee on the personal workspace identity (for example, owner_id
plus name) and handle conflicts in get_or_create_personal_workspace, or switch
the Workspace insert to an upsert/ON CONFLICT pattern so concurrent calls cannot
create multiple Personal rows for the same user.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4deee8f8-d264-4a2c-99da-7fe59fc69e9c
📒 Files selected for processing (8)
apps/web-backend/api/models/workspace.pyapps/web-backend/api/routes/users.pyapps/web-backend/api/routes/workspaces.pyapps/web-backend/core/config.pyapps/web-backend/core/permissions.pyapps/web-backend/main.pyapps/web-backend/services/workspace_service.pyapps/web-backend/tests/test_workspaces.py
… (C2 review) - workspace_service.create_workspace: stop logging the user-supplied name (CodeQL log-injection); log id + owner_id only. - workspaces.list_workspaces: replace the per-row user_role_in_workspace call (N+1) with a single membership query; owners derived from owner_id. - permissions.get_current_workspace (team mode): check access first, fetch the workspace row only on success (no wasted lookup on denial). - workspaces routes: drop redundant return-type annotations (keep response_model= in the decorator, matching the rest of the codebase) to clear the SonarCloud duplication hint. Skipped (with reason): Annotated[...] deps (whole codebase uses = Depends; trivial/stylistic) and a unique-constraint for concurrent Personal-workspace creation (needs a migration, out of C2's no-schema scope; low-probability + reads self-heal). 11 tests pass; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rounds out the workspace API with team-member management — and the first real consumer of C1's require_workspace_access dependency (listing needs >= viewer, mutations need owner).
- Endpoints (api/routes/workspaces.py): GET /api/workspaces/{id}/members (owner first), POST (add by user_id+role), PATCH /{user_id} (change role), DELETE /{user_id} (remove). Validates: unknown workspace/user -> 404, adding the owner -> 400, duplicate member -> 409.
- Service (services/workspace_service.py): list_workspace_members, get_membership, add_member, update_member_role, remove_member.
- Models: WorkspaceMemberAddRequest/UpdateRequest (role validated via Literal) + member response schemas.
- Tests: service CRUD + an end-to-end HTTP flow (add/list/dup-409/owner-400/update/non-owner-403/viewer-can-list/remove-204). 13 workspace tests pass; ruff clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
list_workspace_members now joinedload's WorkspaceUser.user so the members route reads wu.user.email without a per-row query — same fix as list_workspaces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The 'Insufficient workspace permissions' literal is now used 3x in permissions.py; hoist it to a module constant. (The remaining Sonar S8410 'use Annotated for deps' hints are left as-is: the whole web-backend uses = Depends(...), per the convention CodeRabbit recorded as a repo learning.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…in asserts (C2) - workspace_service.add_member: remove the info log (user_id/role/workspace_id are request-derived → CodeQL log-injection; member-change audit is C3/C7). - tests: extract client.post/get/delete calls out of assert expressions so python -O can't strip the action (CodeQL py/side-effect-in-assert). 13 tests pass; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…p (C2) Closes the concurrent-duplicate edge case flagged in review (previously deferred). Adds a partial unique index uq_personal_workspace_per_owner on workspaces(owner_id) WHERE name='Personal' (model __table_args__ + migration 004; SQLite + Postgres partial-WHERE). get_or_create_personal_workspace is now truly idempotent under concurrency: the race loser catches IntegrityError and reuses the winner's workspace. Test asserts a second 'Personal' is rejected while other names stay unconstrained. Note: this adds the first C2 migration (004), upgrading the slice from no-schema to a single additive index. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web-backend/api/routes/workspaces.py (1)
109-117: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate team-only workspace management when
CLOUD_MODE=single.
singlemode is defined as one auto-created Personal workspace per user, but these routes still allow creating additional workspaces and sharing/managing members. Add a team-mode guard to the create and member-management endpoints.Proposed fix
+def require_team_mode() -> None: + if settings.CLOUD_MODE != "team": + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail="Workspace management is only available in team mode", + ) + + async def create_workspace_endpoint( request: WorkspaceCreateRequest, + _team_mode: None = Depends(require_team_mode), auth: dict = Depends(require_auth), db: Session = Depends(get_db), ): @@ async def list_members( workspace_id: int, + _team_mode: None = Depends(require_team_mode), _role: WorkspaceRole = Depends(require_workspace_access(WorkspaceRole.VIEWER)), db: Session = Depends(get_db), ): @@ async def add_workspace_member( workspace_id: int, request: WorkspaceMemberAddRequest, + _team_mode: None = Depends(require_team_mode), _role: WorkspaceRole = Depends(require_workspace_access(WorkspaceRole.OWNER)), db: Session = Depends(get_db), ): @@ async def update_workspace_member( workspace_id: int, user_id: int, request: WorkspaceMemberUpdateRequest, + _team_mode: None = Depends(require_team_mode), _role: WorkspaceRole = Depends(require_workspace_access(WorkspaceRole.OWNER)), db: Session = Depends(get_db), ): @@ async def remove_workspace_member( workspace_id: int, user_id: int, + _team_mode: None = Depends(require_team_mode), _role: WorkspaceRole = Depends(require_workspace_access(WorkspaceRole.OWNER)), db: Session = Depends(get_db), ):Also applies to: 130-220
🤖 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 `@apps/web-backend/api/routes/workspaces.py` around lines 109 - 117, Add a team-mode guard to the workspace creation and member-management routes so they are unavailable when CLOUD_MODE is single. Update create_workspace_endpoint and the related member/sharing handlers in the same routes module to check the cloud mode before calling create_workspace or modifying members, and reject the request with the same auth/validation style used elsewhere in the API. Use the existing workspace route helpers and symbols like create_workspace_endpoint, _require_user_id, and the member-management endpoint functions to place the guard consistently.
🤖 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 `@apps/web-backend/api/routes/workspaces.py`:
- Around line 177-182: The duplicate-member check in the workspace member route
is still race-prone because concurrent requests can both pass get_membership()
before add_member() commits. Update the handler around add_member() to run in a
transaction and catch the database IntegrityError from the insert; on that
failure, roll back and return the same 409 Conflict response instead of letting
it surface as a 500. Keep the existing get_membership() pre-check, but make
add_member() the authoritative commit-time guard in this route.
In `@apps/web-backend/migrations/versions/004_unique_personal_workspace.py`:
- Around line 33-42: The `upgrade()` migration in
`004_unique_personal_workspace.py` can fail when duplicate `Personal` workspaces
already exist for the same `owner_id`. Add a cleanup step before
`op.create_index` that identifies duplicate `workspaces` rows for `name =
'Personal'`, keeps one row per `owner_id`, and deletes the older extras so the
unique partial index can be created successfully. Use the
`PERSONAL_WORKSPACE_NAME` and `Workspace` creation path in
`workspace_service.py` as the context for the affected records.
---
Outside diff comments:
In `@apps/web-backend/api/routes/workspaces.py`:
- Around line 109-117: Add a team-mode guard to the workspace creation and
member-management routes so they are unavailable when CLOUD_MODE is single.
Update create_workspace_endpoint and the related member/sharing handlers in the
same routes module to check the cloud mode before calling create_workspace or
modifying members, and reject the request with the same auth/validation style
used elsewhere in the API. Use the existing workspace route helpers and symbols
like create_workspace_endpoint, _require_user_id, and the member-management
endpoint functions to place the guard consistently.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46c2d3f7-5415-4a73-accd-cef82ab4ce22
📒 Files selected for processing (6)
apps/web-backend/api/models/workspace.pyapps/web-backend/api/routes/workspaces.pyapps/web-backend/core/permissions.pyapps/web-backend/migrations/versions/004_unique_personal_workspace.pyapps/web-backend/services/workspace_service.pyapps/web-backend/tests/test_workspaces.py
… (C2 review) Address CodeRabbit review on the member-management + uniqueness work: - routes: gate workspace creation and all member endpoints behind a require_team_mode dependency (400 in single mode) — single mode is one auto-Personal workspace, not shared. - add_workspace_member: catch IntegrityError from the uq_workspace_user constraint and return 409 instead of a 500 on the concurrent-add race (the get_membership pre-check is now just the fast path). - migration 004: delete any pre-existing duplicate Personal workspaces (keep the oldest per owner) before creating the partial unique index, so the upgrade can't fail on legacy duplicates. - tests: single-mode create now asserts 400 + a team-mode create asserts 201; added a member-routes-gated-in-single-mode test. 15 tests pass; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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.
Second slice of Track C (cloud multitenancy — see docs/strategy/roadmap.md), building on the merged C1 models/permissions. C2 is the workspace-context layer that makes those models usable.
What's delivered
CLOUD_MODEconfig flag (single|team, validated; defaultsingle).services/workspace_service.py:get_or_create_personal_workspace(idempotent single-mode default),list_accessible_workspaces(owned ∪ member),create_workspace.get_current_workspacedependency (core/permissions.py): single mode → the caller's auto-created Personal workspace; team mode →?workspace_idwith a>= vieweraccess check (400 if missing, 403 if denied). Extracted a shared_current_user_idhelper (now reused byrequire_workspace_access).api/routes/workspaces.py, registered inmain.py):GET /api/workspaces(list with role),GET /api/workspaces/current,POST /api/workspaces— the data the frontend workspace-switcher needs./register+/login(idempotent; backfills pre-C2 accounts).Tests
11 workspace tests (bootstrap idempotency, owned∪member listing, single/team resolution incl. 400/403, plus an end-to-end HTTP test of the router). Existing auth/user route tests (49) unaffected;
ruffclean; full app imports cleanly and the routes resolve in the OpenAPI schema. Also restores the owner-access test's membership-absence assertion that the C1 squash-merge dropped.Scope note
No schema change. Per-resource scoping (specs/runs/repos by
workspace_id) lands as each resource gains DB persistence — today onlyrepositoriesis a DB model and it has no CRUD routes yet, so resource scoping is deferred to C3 (runs/AgentExecution) and C6 (OAuth repos), per the roadmap.🤖 Generated with Claude Code
Summary by CodeRabbit