diff --git a/apps/services/gateway/gateway/db.py b/apps/services/gateway/gateway/db.py index 85f9444a1..67e0fbacb 100644 --- a/apps/services/gateway/gateway/db.py +++ b/apps/services/gateway/gateway/db.py @@ -20,17 +20,29 @@ from __future__ import annotations from acb_common.db import ( + TenantUnbound, async_database_url, + bind_tenant, + clear_tenant, + current_tenant, engine_connect_args, get_db, get_engine, get_session_factory, + release_tenant, + tenant_session, ) __all__ = [ + "TenantUnbound", "async_database_url", + "bind_tenant", + "clear_tenant", + "current_tenant", "engine_connect_args", "get_db", "get_engine", "get_session_factory", + "release_tenant", + "tenant_session", ] diff --git a/apps/services/gateway/gateway/main.py b/apps/services/gateway/gateway/main.py index 4b863ffea..09a264ac3 100644 --- a/apps/services/gateway/gateway/main.py +++ b/apps/services/gateway/gateway/main.py @@ -8,6 +8,7 @@ from acb_auth import (UserContext, UserRole, get_current_user, require_authenticated, require_role) from acb_common import configure_logging, get_logger, get_settings +from acb_common.db import clear_tenant, release_tenant from fastapi import BackgroundTasks, Depends, FastAPI from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import StreamingResponse @@ -553,6 +554,31 @@ def docs_enabled(env: str) -> bool: openapi_url="/openapi.json" if _docs_enabled else None, ) +# ── Tenant scope (MT-1c / H2) ── every HTTP request runs inside its own tenant +# scope: opened empty here, filled in by `_with_resolved_access` when the auth +# dependency resolves the caller's organization, released after the response. +# Pure ASGI (no BaseHTTPMiddleware) so the downstream app runs in THIS task and +# the contextvar token round-trips — which is what makes "one request can never +# inherit another's binding" true on any server task model, not just uvicorn's +# task-per-request. Jobs and consumers get no scope from this; they bind +# explicitly or fail closed with TenantUnbound (H4). +class TenantScopeMiddleware: + def __init__(self, asgi_app): # noqa: ANN001 — ASGI protocol shape + self.asgi_app = asgi_app + + async def __call__(self, scope, receive, send): # noqa: ANN001 + if scope["type"] != "http": + await self.asgi_app(scope, receive, send) + return + token = clear_tenant() + try: + await self.asgi_app(scope, receive, send) + finally: + release_tenant(token) + + +app.add_middleware(TenantScopeMiddleware) + # ── CORS ── allow workbench dev server (port 3001) and production origin app.add_middleware( CORSMiddleware, diff --git a/apps/services/gateway/gateway/routes/projects/activities.py b/apps/services/gateway/gateway/routes/projects/activities.py index 04fa5481b..10c92eca1 100644 --- a/apps/services/gateway/gateway/routes/projects/activities.py +++ b/apps/services/gateway/gateway/routes/projects/activities.py @@ -24,7 +24,7 @@ from gateway.routes.projects.core import ( ActivityModel, Page, - _get_db, + _tenant_session, actor, emit, from_jsonb, @@ -85,8 +85,7 @@ async def get_timeline( never withheld — the history of what happened to a task is not editable by the people it happened to. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) params = { @@ -111,8 +110,6 @@ async def get_timeline( "rows": [row_to_dict(r, ActivityModel) for r in rows], "total": int(total), } - finally: - await db.close() @router.post("/tasks/{task_id}/comments", status_code=201) @@ -123,8 +120,7 @@ async def add_comment( body = (payload.body or "").strip() if not body: raise HTTPException(status_code=422, detail="A comment needs a body.") - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) row = await record_activity( @@ -170,13 +166,10 @@ async def add_comment( task_id=task_id, meta={"mentioned": mention_result["notified"]}, ) - await db.commit() result = row_to_dict(row, ActivityModel) # Surfaced, not swallowed: a mention that reached nobody would # otherwise leave the author believing they pulled a colleague in. result["not_notified"] = mention_result["skipped"] - finally: - await db.close() await emit("pm.task.comment_added", {"task_id": task_id}) return result @@ -211,8 +204,7 @@ async def edit_comment( body = (payload.body or "").strip() if not body: raise HTTPException(status_code=422, detail="A comment needs a body.") - db = await _get_db() - try: + async with _tenant_session() as db: comment = await _load_own_comment(db, activity_id, user) # The task must still be visible: a member removed from a Center keeps # authorship of what they wrote but not access to it. @@ -244,12 +236,9 @@ async def edit_comment( task_id=task_id, meta={"mentioned": mention_result["notified"]}, ) - await db.commit() result = row_to_dict(row, ActivityModel) result["not_notified"] = mention_result["skipped"] return result - finally: - await db.close() @router.delete("/comments/{activity_id}") @@ -263,17 +252,13 @@ async def delete_comment( the row hidden, so "deleted" means the words are gone rather than merely filtered out of one read path. """ - db = await _get_db() - try: + async with _tenant_session() as db: await _load_own_comment(db, activity_id, user) await update_row( db, "pm_activities", activity_id, {"deleted_at": now(), "body": None}, ) - await db.commit() return {"deleted": activity_id} - finally: - await db.close() def _restore_custom(task: Any, changes: list[dict]) -> dict | None: @@ -317,8 +302,7 @@ async def revert_change( the timeline shows that a revert happened instead of history appearing never to have contained the change. """ - db = await _get_db() - try: + async with _tenant_session() as db: row = (await db.execute( text( "SELECT * FROM pm_activities WHERE id = CAST(:aid AS uuid) " @@ -378,7 +362,6 @@ async def revert_change( ], extra_meta={"reverted_activity_id": activity_id}, ) - await db.commit() task_id = str(task.id) reverted = sorted( [k for k in restore if k != "custom_fields"] @@ -387,8 +370,6 @@ async def revert_change( if str(c.get("field") or "").startswith(_CUSTOM_PREFIX) ] ) - finally: - await db.close() await emit("pm.task.updated", {"task_id": task_id}) return {"task_id": task_id, "reverted": reverted, "skipped": skipped} diff --git a/apps/services/gateway/gateway/routes/projects/admin.py b/apps/services/gateway/gateway/routes/projects/admin.py index 7caab7e69..6b9b94e67 100644 --- a/apps/services/gateway/gateway/routes/projects/admin.py +++ b/apps/services/gateway/gateway/routes/projects/admin.py @@ -28,7 +28,7 @@ STATUS_CATEGORIES, StatusModel, TypeModel, - _get_db, + _tenant_session, clean_payload, count_where, load_visible_project, @@ -93,8 +93,7 @@ async def _clear_other_defaults(db: Any, table: str, root: str, keep: str) -> No async def list_statuses( project_id: str, user: UserContext = Depends(get_current_user), ) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) root = await _root_for(db, vis, project_id) rows = (await db.execute( @@ -107,8 +106,6 @@ async def list_statuses( return { "rows": [row_to_dict(r, StatusModel) for r in rows], "total": len(rows), } - finally: - await db.close() @router.post("/nodes/{project_id}/statuses", status_code=201) @@ -123,8 +120,7 @@ async def create_status( category = values.get("category") or "todo" validate_choice(category, STATUS_CATEGORIES, "status category") - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) root = await _root_for(db, vis, project_id) row = (await db.execute( @@ -144,10 +140,7 @@ async def create_status( )).fetchone() if row.is_default: await _clear_other_defaults(db, "pm_task_statuses", root, str(row.id)) - await db.commit() return row_to_dict(row, StatusModel) - finally: - await db.close() @router.patch("/statuses/{status_id}") @@ -157,8 +150,7 @@ async def patch_status( ) -> dict: values = clean_payload(payload) validate_choice(values.get("category"), STATUS_CATEGORIES, "status category") - db = await _get_db() - try: + async with _tenant_session() as db: existing = await require_row(db, "pm_task_statuses", status_id, "Status") vis = await resolve_visibility(db, user) await load_visible_project(db, vis, str(existing.project_id)) @@ -169,10 +161,7 @@ async def patch_status( await _clear_other_defaults( db, "pm_task_statuses", str(row.project_id), status_id, ) - await db.commit() return row_to_dict(row, StatusModel) - finally: - await db.close() @router.delete("/statuses/{status_id}") @@ -186,8 +175,7 @@ async def delete_status( that into a 409 naming how many tasks are in the way, which is the number the caller needs to decide what to do next. """ - db = await _get_db() - try: + async with _tenant_session() as db: existing = await require_row(db, "pm_task_statuses", status_id, "Status") vis = await resolve_visibility(db, user) await load_visible_project(db, vis, str(existing.project_id)) @@ -204,10 +192,7 @@ async def delete_status( text("DELETE FROM pm_task_statuses WHERE id = CAST(:sid AS uuid)"), {"sid": status_id}, ) - await db.commit() return {"deleted": status_id, "tasks_affected": 0} - finally: - await db.close() # ── Types ─────────────────────────────────────────────────────────────────── @@ -216,8 +201,7 @@ async def delete_status( async def list_types( project_id: str, user: UserContext = Depends(get_current_user), ) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) root = await _root_for(db, vis, project_id) rows = (await db.execute( @@ -230,8 +214,6 @@ async def list_types( return { "rows": [row_to_dict(r, TypeModel) for r in rows], "total": len(rows), } - finally: - await db.close() @router.post("/nodes/{project_id}/types", status_code=201) @@ -244,8 +226,7 @@ async def create_type( if not name: raise HTTPException(status_code=422, detail="A task type needs a name.") - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) root = await _root_for(db, vis, project_id) row = (await db.execute( @@ -266,10 +247,7 @@ async def create_type( # second root-only type — or, worse, a type that claims Epic's exemption. if row.is_default: await _clear_other_defaults(db, "pm_task_types", root, str(row.id)) - await db.commit() return row_to_dict(row, TypeModel) - finally: - await db.close() @router.patch("/types/{type_id}") @@ -278,8 +256,7 @@ async def patch_type( user: UserContext = Depends(get_current_user), ) -> dict: values = clean_payload(payload) - db = await _get_db() - try: + async with _tenant_session() as db: existing = await require_row(db, "pm_task_types", type_id, "Task type") vis = await resolve_visibility(db, user) await load_visible_project(db, vis, str(existing.project_id)) @@ -298,10 +275,7 @@ async def patch_type( await _clear_other_defaults( db, "pm_task_types", str(row.project_id), type_id, ) - await db.commit() return row_to_dict(row, TypeModel) - finally: - await db.close() @router.delete("/types/{type_id}") @@ -315,8 +289,7 @@ async def delete_type( lane. The count is reported (R7/R8) because "12 tasks became untyped" is not something to discover from a board. """ - db = await _get_db() - try: + async with _tenant_session() as db: existing = await require_row(db, "pm_task_types", type_id, "Task type") vis = await resolve_visibility(db, user) await load_visible_project(db, vis, str(existing.project_id)) @@ -330,7 +303,4 @@ async def delete_type( text("DELETE FROM pm_task_types WHERE id = CAST(:tid AS uuid)"), {"tid": type_id}, ) - await db.commit() return {"deleted": type_id, "tasks_untyped": affected} - finally: - await db.close() diff --git a/apps/services/gateway/gateway/routes/projects/agent_dispatch.py b/apps/services/gateway/gateway/routes/projects/agent_dispatch.py index 95c93b167..e443d0d0c 100644 --- a/apps/services/gateway/gateway/routes/projects/agent_dispatch.py +++ b/apps/services/gateway/gateway/routes/projects/agent_dispatch.py @@ -29,7 +29,20 @@ from typing import Any from acb_common import get_logger -from gateway.routes.projects.core import _get_db, record_activity + +# ⚠️ H4, DELIBERATELY NOT H2 (`saas_multitenancy_handover.md`): this module is +# an EVENT CONSUMER, not a request handler — `on_event` fires from +# `emit_event`'s sink fan-out and `_run_and_record` outlives the request that +# scheduled it. The runbook's rule for that category is "do not let a job +# inherit an ambient tenant", so converting these sites to the ambient +# `tenant_session()` would be exactly the inheritance it forbids. They stay on +# the unbound `get_db()` until H4 threads an EXPLICIT tenant through the event +# payload (`tenant_session(org_id)`), and the H2 ratchet in +# `test_db_engine_seam.py` carries this module as its one named exemption. +# Sequencing is safe: RLS phase 4 (which would starve these reads) is gated on +# H2+H4 both being complete. +from gateway.db import get_db as _get_db +from gateway.routes.projects.core import record_activity from sqlalchemy import text _log = get_logger("projects.agent_dispatch") diff --git a/apps/services/gateway/gateway/routes/projects/attachments.py b/apps/services/gateway/gateway/routes/projects/attachments.py index e7eaa482c..59339568d 100644 --- a/apps/services/gateway/gateway/routes/projects/attachments.py +++ b/apps/services/gateway/gateway/routes/projects/attachments.py @@ -39,7 +39,7 @@ from fastapi.responses import FileResponse from gateway.routes.projects.core import ( ListResponse, - _get_db, + _tenant_session, actor, emit, load_visible_task, @@ -90,8 +90,7 @@ async def attach_file( would mean an unauthorised caller could still make the server do the work. """ email = actor(user) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) task = await load_visible_task(db, vis, task_id) @@ -138,7 +137,6 @@ async def attach_file( meta={"attachment_id": att_id, "name": name, "mime": mime, "size": len(content)}, ) - await db.commit() result = { "attachment_id": att_id, "name": name, "mime": mime, "size": len(content), "added_by": email, @@ -146,8 +144,6 @@ async def attach_file( "url": f"/api/projects/attachments/{att_id}/{name}", } project_id = str(task.project_id) - finally: - await db.close() await emit("pm.task.updated", {"task_id": task_id, "project_id": project_id, "attachment_added": att_id}) @@ -158,8 +154,7 @@ async def attach_file( async def list_attachments( task_id: str, user: UserContext = Depends(get_current_user), ) -> ListResponse: - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) rows = (await db.execute( @@ -173,8 +168,6 @@ async def list_attachments( ), {"tid": task_id}, )).fetchall() - finally: - await db.close() items = [descriptor(r) for r in rows] return ListResponse(rows=items, total=len(items)) @@ -194,8 +187,7 @@ async def serve_attachment( 404 for "not attached to anything you can see" as well as "no such file" (R5). A 403 here would confirm the file exists. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) params: dict[str, Any] = {"aid": attachment_id} clauses = ["ta.attachment_id = CAST(:aid AS uuid)"] @@ -216,8 +208,6 @@ async def serve_attachment( ), params, )).fetchone() - finally: - await db.close() if row is None or not Path(row.path).is_file(): raise HTTPException(status_code=404, detail="Attachment not found") return FileResponse( @@ -243,8 +233,7 @@ async def detach_file( half-failed request safe. """ email = actor(user) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) result = await db.execute( @@ -262,7 +251,4 @@ async def detach_file( task_id=task_id, body="Removed an attachment", meta={"attachment_id": attachment_id, "removed": True}, ) - await db.commit() - finally: - await db.close() return {"task_id": task_id, "attachment_id": attachment_id, "removed": removed} diff --git a/apps/services/gateway/gateway/routes/projects/bulk.py b/apps/services/gateway/gateway/routes/projects/bulk.py index 877bacc18..4a90926a9 100644 --- a/apps/services/gateway/gateway/routes/projects/bulk.py +++ b/apps/services/gateway/gateway/routes/projects/bulk.py @@ -40,7 +40,7 @@ apply_task_patch, ) from gateway.routes.projects.core import ( - _get_db, + _tenant_session, actor, emit, load_visible_task, @@ -341,8 +341,7 @@ async def bulk_edit( newly_assigned: dict[str, list[str]] = {} changed_ids: list[str] = [] - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) for task_id in ids: try: @@ -383,9 +382,6 @@ async def bulk_edit( f"and {len(tasks) - 1} other task(s)" if len(tasks) > 1 else None ), ) - await db.commit() - finally: - await db.close() # Emitted after the commit, and per task: `emit` is best-effort by # construction, so an automation that fails cannot roll back a re-triage. diff --git a/apps/services/gateway/gateway/routes/projects/calendar.py b/apps/services/gateway/gateway/routes/projects/calendar.py index b0ce9baec..b79c37e07 100644 --- a/apps/services/gateway/gateway/routes/projects/calendar.py +++ b/apps/services/gateway/gateway/routes/projects/calendar.py @@ -51,7 +51,7 @@ from fastapi import Depends, HTTPException, Query from gateway.routes.projects.core import ( TaskModel, - _get_db, + _tenant_session, load_visible_project, resolve_visibility, router, @@ -218,8 +218,7 @@ async def get_calendar( """ window_from, window_to = window_bounds(date_from, date_to) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) clauses: list[str] = [task_visibility_clause(vis)] params: dict[str, Any] = dict(vis.params) @@ -289,8 +288,6 @@ async def get_calendar( # has no dependencies" must not be confused with "nobody asked". "links": await window_links(db, window_rows) if include_links else [], } - finally: - await db.close() __all__ = [ diff --git a/apps/services/gateway/gateway/routes/projects/core.py b/apps/services/gateway/gateway/routes/projects/core.py index ce7b065c6..3d6ded7e8 100644 --- a/apps/services/gateway/gateway/routes/projects/core.py +++ b/apps/services/gateway/gateway/routes/projects/core.py @@ -44,7 +44,15 @@ from acb_auth import UserContext, require_feature_router from acb_common import get_logger from fastapi import APIRouter, HTTPException, Query -from gateway.db import get_db as _get_db # noqa: F401 — the shared seam (BO-10) + +# The shared seam (BO-10 → MT-1c/H2). `_tenant_session` IS +# `acb_common.db.tenant_session`, aliased per-package for the same reason +# `_get_db` was: every submodule imports it from here BY NAME, which is the +# seam `tests/unit/_projects_fakes.bind_db` patches per module. The tenant +# comes from the request context — bound once in `_with_resolved_access` — +# so no call site passes one (H2). A call outside a bound request raises +# `TenantUnbound` rather than defaulting: fail closed, never "the usual org". +from gateway.db import tenant_session as _tenant_session from pydantic import BaseModel from sqlalchemy import text diff --git a/apps/services/gateway/gateway/routes/projects/custom_fields.py b/apps/services/gateway/gateway/routes/projects/custom_fields.py index 71c73ffbc..e94c7ff56 100644 --- a/apps/services/gateway/gateway/routes/projects/custom_fields.py +++ b/apps/services/gateway/gateway/routes/projects/custom_fields.py @@ -29,7 +29,7 @@ from acb_auth import UserContext, get_current_user from fastapi import Depends, HTTPException from gateway.routes.projects.core import ( - _get_db, + _tenant_session, actor, clean_payload, load_visible_project, @@ -366,14 +366,11 @@ async def _count_with_key(db: Any, root: str, key: str) -> int: async def list_fields( project_id: str, user: UserContext = Depends(get_current_user), ) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) root = await _root_for(db, vis, project_id) rows = await load_definitions(db, root) return {"rows": rows, "total": len(rows)} - finally: - await db.close() @router.post("/nodes/{project_id}/fields", status_code=201) @@ -394,8 +391,7 @@ async def create_field( key = validate_key(str(values.get("field_key") or slugify_key(name))) options = clean_options(field_type, values.get("options")) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) root = await _root_for(db, vis, project_id) existing = await load_definitions(db, root) @@ -428,10 +424,7 @@ async def create_field( "by": actor(user), }, )).fetchone() - await db.commit() return _definition_row(row) - finally: - await db.close() @router.patch("/fields/{field_id}") @@ -465,8 +458,7 @@ async def patch_field( f"One of: {list(FIELD_TYPES)}.", ) - db = await _get_db() - try: + async with _tenant_session() as db: existing = await require_row(db, "pm_custom_fields", field_id, "Custom field") vis = await resolve_visibility(db, user) await load_visible_project(db, vis, str(existing.project_id)) @@ -501,10 +493,7 @@ async def patch_field( values["options"] = _json(options) row = await update_row(db, "pm_custom_fields", field_id, values) - await db.commit() return _definition_row(row) - finally: - await db.close() @router.delete("/fields/{field_id}") @@ -523,8 +512,7 @@ async def delete_field( The count is reported (R7/R8), for the same reason ``delete_view`` reports its cascade: losing data silently is what makes people distrust a delete. """ - db = await _get_db() - try: + async with _tenant_session() as db: existing = await require_row(db, "pm_custom_fields", field_id, "Custom field") vis = await resolve_visibility(db, user) await load_visible_project(db, vis, str(existing.project_id)) @@ -541,14 +529,11 @@ async def delete_field( text("DELETE FROM pm_custom_fields WHERE id = CAST(:fid AS uuid)"), {"fid": field_id}, ) - await db.commit() return { "deleted": field_id, "field_key": existing.field_key, "cascaded": {"values_cleared": int(cleared)}, } - finally: - await db.close() async def _options_in_use( diff --git a/apps/services/gateway/gateway/routes/projects/import_clickup.py b/apps/services/gateway/gateway/routes/projects/import_clickup.py index 31ef79a0b..e4e73d25e 100644 --- a/apps/services/gateway/gateway/routes/projects/import_clickup.py +++ b/apps/services/gateway/gateway/routes/projects/import_clickup.py @@ -37,7 +37,7 @@ class this app cannot make silently. The plan is a read; the import writes only from acb_common import get_logger from fastapi import Depends, HTTPException from gateway.routes.projects.core import ( - _get_db, + _tenant_session, actor, insert_row, record_activity, @@ -121,8 +121,7 @@ async def _resolve_provider(account_id: str) -> Any: from gateway.routes.tasks.core import _key_store from gateway.routes.tasks.providers import build_provider - db = await _get_db() - try: + async with _tenant_session() as db: row = (await db.execute( text( "SELECT provider, workspace_id, credentials_encrypted " @@ -130,8 +129,6 @@ async def _resolve_provider(account_id: str) -> Any: ), {"id": account_id}, )).mappings().first() - finally: - await db.close() if row is None: raise HTTPException(status_code=404, detail="Task account not found") if row["provider"] != "clickup": @@ -213,8 +210,7 @@ async def plan_import( workspace_id = getattr(provider, "_workspace_id", None) facts = await _gather(provider, str(workspace_id)) - db = await _get_db() - try: + async with _tenant_session() as db: centers = await load_centers(db) group_members = await load_group_members(db) existing = await _existing_mappings(db) @@ -249,8 +245,6 @@ async def plan_import( else suggestion.as_dict() ), }) - finally: - await db.close() return { "account_id": payload.account_id, @@ -291,8 +285,7 @@ async def import_clickup( facts = await _gather(provider, workspace_id) chosen = {m.space_id: m.center for m in payload.mappings} - db = await _get_db() - try: + async with _tenant_session() as db: centers = await load_centers(db) unknown = sorted( {c for c in chosen.values() if c and c not in centers} @@ -318,10 +311,7 @@ async def import_clickup( # Nothing is committed, and the caller is told so in the response # rather than left to infer it from a counts-only payload. return {"dry_run": True, **summary.as_dict(facts)} - await db.commit() return {"dry_run": False, **summary.as_dict(facts)} - finally: - await db.close() @dataclass diff --git a/apps/services/gateway/gateway/routes/projects/import_tasks.py b/apps/services/gateway/gateway/routes/projects/import_tasks.py index e464a411e..d5ba8f0b9 100644 --- a/apps/services/gateway/gateway/routes/projects/import_tasks.py +++ b/apps/services/gateway/gateway/routes/projects/import_tasks.py @@ -52,7 +52,7 @@ from acb_common import get_logger from fastapi import Depends from gateway.routes.projects.core import ( - _get_db, + _tenant_session, actor, insert_row, next_task_number, @@ -377,8 +377,7 @@ async def import_from_tasks( tally = _Tally() department = (payload.department or "Company").strip() or "Company" - db = await _get_db() - try: + async with _tenant_session() as db: organization_id = await require_organization_of(db, who.lower()) root_id = await _root_department( db, department, who, tally, payload.dry_run, @@ -465,9 +464,6 @@ async def import_from_tasks( f"ClickUp mirror into '{department}'" ), ) - await db.commit() - finally: - await db.close() return tally.as_dict(department=department, dry_run=payload.dry_run) diff --git a/apps/services/gateway/gateway/routes/projects/intake.py b/apps/services/gateway/gateway/routes/projects/intake.py index 34e8d653e..5f9d02f39 100644 --- a/apps/services/gateway/gateway/routes/projects/intake.py +++ b/apps/services/gateway/gateway/routes/projects/intake.py @@ -54,7 +54,7 @@ TRIAGE_CATEGORY, Page, TaskModel, - _get_db, + _tenant_session, actor, apply_status_transition, emit, @@ -270,8 +270,7 @@ async def capture_intake( raise HTTPException(status_code=422, detail="A capture needs a project_id.") source = (payload.source or "").strip() or None - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) # R5 — capturing INTO a project requires seeing it; an unreadable id # answers 404, never 403. @@ -306,10 +305,7 @@ async def capture_intake( meta={"intake": "captured", "source": source, "source_ref": getattr(wrapper, "source_ref", None)}, ) - await db.commit() result = _shape(task, wrapper) - finally: - await db.close() await emit("pm.intake.captured", { "task_id": result["task"]["id"], @@ -335,8 +331,7 @@ async def list_intake( not open, and a `project_id` the caller cannot see is a 404 (R5) rather than an empty queue that confirms the project exists. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) params: dict[str, Any] = dict(vis.params) scope = "" @@ -373,8 +368,6 @@ async def list_intake( } out.append(item) return {"rows": out, "total": int(total)} - finally: - await db.close() # ── The four rulings ──────────────────────────────────────────────────────── @@ -391,8 +384,7 @@ async def accept_intake( `status_change` activity a board drag would. The wrapper flips to `accepted` and stays forever. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) task = await load_visible_task(db, vis, task_id) wrapper = await _load_wrapper(db, task_id) @@ -424,10 +416,7 @@ async def accept_intake( task_id=task_id, body="Accepted from intake", meta={"intake": "accepted", "status_id": str(destination.id)}, ) - await db.commit() result = _shape(moved["row"], wrapper) - finally: - await db.close() await emit("pm.intake.accepted", {"task_id": task_id}) return result @@ -442,8 +431,7 @@ async def decline_intake( Archived, not deleted — the standing soft-delete, so a decline is revertible the way any archive is, and the capture's timeline survives. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) wrapper = await _load_wrapper(db, task_id) @@ -458,10 +446,7 @@ async def decline_intake( task_id=task_id, body="Declined at intake", meta={"intake": "declined"}, ) - await db.commit() result = _shape(task, wrapper) - finally: - await db.close() await emit("pm.intake.declined", {"task_id": task_id}) return result @@ -483,8 +468,7 @@ async def duplicate_intake( raise HTTPException( status_code=422, detail="A task cannot be a duplicate of itself.", ) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) await load_visible_task(db, vis, original) @@ -501,10 +485,7 @@ async def duplicate_intake( task_id=task_id, body="Marked duplicate at intake", meta={"intake": "duplicate", "duplicate_of_task_id": original}, ) - await db.commit() result = _shape(task, wrapper) - finally: - await db.close() await emit("pm.intake.duplicate", { "task_id": task_id, "duplicate_of_task_id": original, @@ -529,8 +510,7 @@ async def snooze_intake( raise HTTPException( status_code=422, detail="'until' must be in the future.", ) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) task = await load_visible_task(db, vis, task_id) wrapper = await _load_wrapper(db, task_id) @@ -544,10 +524,7 @@ async def snooze_intake( task_id=task_id, body="Snoozed at intake", meta={"intake": "snoozed", "until": until.isoformat()}, ) - await db.commit() result = _shape(task, wrapper) - finally: - await db.close() await emit("pm.intake.snoozed", { "task_id": task_id, "until": until.isoformat(), diff --git a/apps/services/gateway/gateway/routes/projects/me.py b/apps/services/gateway/gateway/routes/projects/me.py index fd4fff530..e195baafa 100644 --- a/apps/services/gateway/gateway/routes/projects/me.py +++ b/apps/services/gateway/gateway/routes/projects/me.py @@ -23,7 +23,7 @@ ListResponse, Page, TaskModel, - _get_db, + _tenant_session, actor, resolve_organization_id, router, @@ -77,8 +77,7 @@ async def assigned_to_me( ) where = " WHERE " + " AND ".join(clauses) - db = await _get_db() - try: + async with _tenant_session() as db: # A caller the directory does not know binds NULL and matches nothing, # which is the same fail-closed shape every other read here has. scope = {"who": email, "vis_org": await resolve_organization_id(db, email)} @@ -96,5 +95,3 @@ async def assigned_to_me( return ListResponse( rows=[row_to_dict(r, TaskModel) for r in rows], total=int(total), ) - finally: - await db.close() diff --git a/apps/services/gateway/gateway/routes/projects/notifications.py b/apps/services/gateway/gateway/routes/projects/notifications.py index 0d5434c74..8c6128f63 100644 --- a/apps/services/gateway/gateway/routes/projects/notifications.py +++ b/apps/services/gateway/gateway/routes/projects/notifications.py @@ -35,7 +35,7 @@ from fastapi import Depends, Query from gateway.routes.projects.core import ( Page, - _get_db, + _tenant_session, actor, insert_row, resolve_visibility, @@ -292,8 +292,7 @@ async def list_notifications( address (R3). """ me = actor(user).lower() - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) clause = vis.project_clause("t.root_project_id") sql = _LIST_SQL.format( @@ -319,8 +318,6 @@ async def list_notifications( ), {"me": me, **vis.params}, )).fetchone() - finally: - await db.close() return { "rows": [_row(r) for r in rows], "total": len(rows), @@ -351,8 +348,7 @@ async def mark_read( me = actor(user).lower() if not payload.all and not payload.ids: return {"marked": 0} - db = await _get_db() - try: + async with _tenant_session() as db: if payload.all: sql = ( "UPDATE pm_notifications SET read_at = now() " @@ -367,9 +363,6 @@ async def mark_read( ) params = {"me": me, "ids": list(payload.ids)} result = await db.execute(text(sql), params) - await db.commit() - finally: - await db.close() return {"marked": int(getattr(result, "rowcount", 0) or 0)} diff --git a/apps/services/gateway/gateway/routes/projects/personal.py b/apps/services/gateway/gateway/routes/projects/personal.py index 7506bf62c..dac80ec2d 100644 --- a/apps/services/gateway/gateway/routes/projects/personal.py +++ b/apps/services/gateway/gateway/routes/projects/personal.py @@ -38,7 +38,7 @@ ListResponse, Page, TaskModel, - _get_db, + _tenant_session, actor, clean_payload, coerce_write_values, @@ -202,26 +202,19 @@ async def ensure_personal_project(db: Any, email: str) -> Any: async def get_my_project(user: UserContext = Depends(get_current_user)) -> dict: """My personal project, or 404 if I have never captured anything.""" email = actor(user).lower() - db = await _get_db() - try: + async with _tenant_session() as db: row = await _load_personal_project(db, email) if row is None: raise HTTPException(status_code=404, detail="No personal project yet") return {"id": str(row.id), "name": row.name} - finally: - await db.close() @router.post("/my/project", status_code=201) async def create_my_project(user: UserContext = Depends(get_current_user)) -> dict: email = actor(user).lower() - db = await _get_db() - try: + async with _tenant_session() as db: row = await ensure_personal_project(db, email) - await db.commit() return {"id": str(row.id), "name": row.name} - finally: - await db.close() @router.post("/my/tasks", status_code=201) @@ -240,8 +233,7 @@ async def capture( raise HTTPException(status_code=422, detail="A task needs a title.") email = actor(user).lower() - db = await _get_db() - try: + async with _tenant_session() as db: project = await ensure_personal_project(db, email) project_id = str(project.id) status = await load_default_status(db, project_id) @@ -273,10 +265,7 @@ async def capture( db, activity_type="system", created_by=email, task_id=task_id, body="Captured", ) - await db.commit() result = row_to_dict(task, TaskModel) - finally: - await db.close() await emit("pm.task.created", {"task_id": task_id, "project_id": project_id, "title": title}) @@ -333,8 +322,7 @@ async def set_personal( ) email = actor(user).lower() - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) # Seeing the task is the floor. Assignment already satisfies it # (`load_visible_task`), so a task delegated across a Center boundary is @@ -347,10 +335,7 @@ async def set_personal( # member looking at it. values["clarified_at"] = now() row = await _upsert_personal(db, task_id, email, values) - await db.commit() return _personal_to_dict(row) - finally: - await db.close() def _personal_to_dict(row: Any) -> dict[str, Any]: @@ -452,12 +437,9 @@ async def my_inbox( params["context"] = context.strip().lower() sql = _MY_TASKS_SQL + ("".join(f" AND {c}" for c in clauses)) - db = await _get_db() - try: + async with _tenant_session() as db: params["vis_org"] = await resolve_organization_id(db, email) rows = (await db.execute(text(sql), params)).fetchall() - finally: - await db.close() items: list[dict[str, Any]] = [] for row in rows: @@ -495,8 +477,7 @@ async def my_contexts(user: UserContext = Depends(get_current_user)) -> dict: and GTD contexts are personal by nature. """ email = actor(user).lower() - db = await _get_db() - try: + async with _tenant_session() as db: rows = (await db.execute( text( "SELECT p.context AS context, count(*) AS total " @@ -514,8 +495,6 @@ async def my_contexts(user: UserContext = Depends(get_current_user)) -> dict: ], "total": len(rows), } - finally: - await db.close() # ── Completion, from the personal side ────────────────────────────────────── @@ -536,8 +515,7 @@ async def complete_task( from gateway.routes.projects.core import apply_status_transition email = actor(user).lower() - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) task = await load_visible_task(db, vis, task_id) done = (await db.execute( @@ -559,10 +537,7 @@ async def complete_task( # And the member's own view of it follows, so a completed task does not # sit in their Next list contradicting the board. await _upsert_personal(db, task_id, email, {"disposition": "DONE"}) - await db.commit() result = row_to_dict(moved["row"], TaskModel) - finally: - await db.close() await emit("pm.task.status_changed", { "task_id": task_id, "from": moved["from"].name, "to": moved["to"].name, @@ -584,14 +559,10 @@ async def defer_task( unaffected, because deferring is a statement about my attention, not about the work.""" email = actor(user).lower() - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) row = await _upsert_personal(db, task_id, email, { "defer_until": payload.until, "disposition": "SOMEDAY", }) - await db.commit() return _personal_to_dict(row) - finally: - await db.close() diff --git a/apps/services/gateway/gateway/routes/projects/recurrence.py b/apps/services/gateway/gateway/routes/projects/recurrence.py index fd975c420..652457e97 100644 --- a/apps/services/gateway/gateway/routes/projects/recurrence.py +++ b/apps/services/gateway/gateway/routes/projects/recurrence.py @@ -31,7 +31,7 @@ from acb_auth import UserContext, get_current_user from fastapi import Depends, HTTPException from gateway.routes.projects.core import ( - _get_db, + _tenant_session, actor, clean_payload, insert_row, @@ -387,8 +387,7 @@ async def spawn_successor(db: Any, task: Any, *, actor_id: str) -> str | None: async def get_recurrence( task_id: str, user: UserContext = Depends(get_current_user), ) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) task = await load_visible_task(db, vis, task_id) if task.recurrence_id is None: @@ -398,8 +397,6 @@ async def get_recurrence( {"rid": str(task.recurrence_id)}, )).fetchone() return {"rule": rule_of(row) if row else None} - finally: - await db.close() @router.put("/tasks/{task_id}/recurrence") @@ -413,8 +410,7 @@ async def set_recurrence( "change the cadence" is the same act as "give it one". """ rule = validate_rule(clean_payload(payload)) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) task = await load_visible_task(db, vis, task_id) root = str(task.root_project_id) @@ -431,10 +427,7 @@ async def set_recurrence( await update_row( db, "pm_tasks", task_id, {"recurrence_id": str(row.id)}, ) - await db.commit() return {"rule": rule_of(row)} - finally: - await db.close() @router.delete("/tasks/{task_id}/recurrence") @@ -447,8 +440,7 @@ async def clear_recurrence( some of it finished, and a "stop repeating this" button that swept away three months of completed reports would be the last time anybody pressed it. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) task = await load_visible_task(db, vis, task_id) if task.recurrence_id is None: @@ -467,10 +459,7 @@ async def clear_recurrence( text("DELETE FROM pm_recurrences WHERE id = CAST(:rid AS uuid)"), {"rid": rule_id}, ) - await db.commit() return {"cleared": True, "cascaded": {"tasks_detached": detached}} - finally: - await db.close() __all__ = [ diff --git a/apps/services/gateway/gateway/routes/projects/relations.py b/apps/services/gateway/gateway/routes/projects/relations.py index c54d569db..57b09fe7c 100644 --- a/apps/services/gateway/gateway/routes/projects/relations.py +++ b/apps/services/gateway/gateway/routes/projects/relations.py @@ -38,7 +38,7 @@ from gateway.routes.projects.core import ( CLOSING_CATEGORIES, MAX_DEPTH, - _get_db, + _tenant_session, load_visible_task, resolve_visibility, router, @@ -200,8 +200,7 @@ async def get_relations( and three round trips to fill one block is three chances to paint a half-drawn dependency section. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) visible = task_visibility_clause(vis) @@ -238,8 +237,6 @@ async def get_relations( "links": links, "blocked_by": blocked_by_open(blockers), } - finally: - await db.close() __all__ = [ diff --git a/apps/services/gateway/gateway/routes/projects/search.py b/apps/services/gateway/gateway/routes/projects/search.py index 39e74cf07..8410c8cf3 100644 --- a/apps/services/gateway/gateway/routes/projects/search.py +++ b/apps/services/gateway/gateway/routes/projects/search.py @@ -54,7 +54,7 @@ from fastapi import Depends from gateway.routes.projects.core import ( MAX_DEPTH, - _get_db, + _tenant_session, load_visible_task, resolve_visibility, router, @@ -231,8 +231,7 @@ async def search_tasks( cap = max(1, min(int(limit), MAX_HITS)) escaped = like_escape(term) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) exclude_sql = "" exclude_params: dict[str, Any] = {} @@ -286,8 +285,6 @@ async def search_tasks( "truncated": len(rows) > cap, "query": term, } - finally: - await db.close() __all__ = ["MAX_HITS", "MIN_QUERY", "relative_task_ids", "task_number"] diff --git a/apps/services/gateway/gateway/routes/projects/tags.py b/apps/services/gateway/gateway/routes/projects/tags.py index fafc79d96..8945b1560 100644 --- a/apps/services/gateway/gateway/routes/projects/tags.py +++ b/apps/services/gateway/gateway/routes/projects/tags.py @@ -36,7 +36,7 @@ from acb_auth import UserContext, get_current_user from fastapi import Depends, HTTPException from gateway.routes.projects.core import ( - _get_db, + _tenant_session, actor, clean_payload, load_visible_project, @@ -270,8 +270,7 @@ async def list_tags( you which of two near-duplicate tags to merge into the other, and computing it per row in the browser would mean shipping every task to get it. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) root = await _root_for(db, vis, project_id) rows = (await db.execute( @@ -292,8 +291,6 @@ async def list_tags( "rows": [_row(r, int(r.task_count or 0)) for r in rows], "total": len(rows), } - finally: - await db.close() @router.post("/nodes/{project_id}/tags", status_code=201) @@ -305,8 +302,7 @@ async def create_tag( name = normalise_tag(values.get("name")) if name is None: raise HTTPException(status_code=422, detail="A tag needs a name.") - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) root = await _root_for(db, vis, project_id) registry = await load_registry(db, root) @@ -330,10 +326,7 @@ async def create_tag( row = await update_row(db, "pm_tags", str(row.id), { k: v for k, v in values.items() if k in ("color", "description") }) - await db.commit() return _row(row, 0) - finally: - await db.close() @router.patch("/tags/{tag_id}") @@ -353,8 +346,7 @@ async def patch_tag( if "name" in values and new_name is None: raise HTTPException(status_code=422, detail="A tag needs a name.") - db = await _get_db() - try: + async with _tenant_session() as db: existing = await require_row(db, "pm_tags", tag_id, "Tag") vis = await resolve_visibility(db, user) await load_visible_project(db, vis, str(existing.project_id)) @@ -376,10 +368,7 @@ async def patch_tag( if new_name: write["name"] = new_name row = await update_row(db, "pm_tags", tag_id, write) if write else existing - await db.commit() return {**_row(row), "retagged": renamed} - finally: - await db.close() @router.post("/tags/{tag_id}/merge") @@ -394,8 +383,7 @@ async def merge_tag( ends with the target **once** — `merged_tags` is where that is decided, and it is pure so the case can be asserted directly. """ - db = await _get_db() - try: + async with _tenant_session() as db: source = await require_row(db, "pm_tags", tag_id, "Tag") target = await require_row(db, "pm_tags", payload.into_tag_id, "Tag") vis = await resolve_visibility(db, user) @@ -418,12 +406,9 @@ async def merge_tag( text("DELETE FROM pm_tags WHERE id = CAST(:tid AS uuid)"), {"tid": tag_id}, ) - await db.commit() return { "merged": source.name, "into": target.name, "retagged": moved, } - finally: - await db.close() @router.delete("/tags/{tag_id}") @@ -437,8 +422,7 @@ async def delete_tag( recreates the name. The count is reported (R7/R8), because losing data silently is what makes people distrust a delete. """ - db = await _get_db() - try: + async with _tenant_session() as db: existing = await require_row(db, "pm_tags", tag_id, "Tag") vis = await resolve_visibility(db, user) await load_visible_project(db, vis, str(existing.project_id)) @@ -454,13 +438,10 @@ async def delete_tag( text("DELETE FROM pm_tags WHERE id = CAST(:tid AS uuid)"), {"tid": tag_id}, ) - await db.commit() return { "deleted": tag_id, "name": existing.name, "cascaded": {"tasks_untagged": int(stripped)}, } - finally: - await db.close() async def _rewrite(db: Any, root: str, before: str, after: str) -> int: diff --git a/apps/services/gateway/gateway/routes/projects/tasks.py b/apps/services/gateway/gateway/routes/projects/tasks.py index b37d5094c..f0236e919 100644 --- a/apps/services/gateway/gateway/routes/projects/tasks.py +++ b/apps/services/gateway/gateway/routes/projects/tasks.py @@ -32,7 +32,7 @@ Page, TaskIn, TaskModel, - _get_db, + _tenant_session, actor, apply_status_transition, assert_epic_has_no_parent, @@ -164,8 +164,7 @@ async def list_tasks( detail=f"Unknown sort direction '{direction}'. One of: asc, desc.", ) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) clauses: list[str] = [task_visibility_clause(vis)] params: dict[str, Any] = dict(vis.params) @@ -228,16 +227,13 @@ async def list_tasks( await attach_assignees(db, page_rows) await attach_relation_counts(db, page_rows) return ListResponse(rows=page_rows, total=int(total)) - finally: - await db.close() @router.get("/tasks/{task_id}") async def get_task( task_id: str, user: UserContext = Depends(get_current_user), ) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) row = await load_visible_task(db, vis, task_id) result = row_to_dict(row, TaskModel) @@ -250,8 +246,6 @@ async def get_task( )).fetchall() result["assignees"] = [r.assignee for r in assignees] return result - finally: - await db.close() # ── Writes ────────────────────────────────────────────────────────────────── @@ -271,8 +265,7 @@ async def create_task( values["title"] = title values["created_by"] = actor(user) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_project(db, vis, str(project_id)) root = await root_project_id(db, str(project_id)) @@ -309,10 +302,7 @@ async def create_task( # WS-27j author-hears-about-comments behaviour once the audience is # watchers ∪ assignees (migration 165 seeds the same for older tasks). await ensure_watchers(db, task_id, [actor(user)], by=actor(user)) - await db.commit() result = row_to_dict(row, TaskModel) - finally: - await db.close() await emit("pm.task.created", { "task_id": task_id, "project_id": str(project_id), "title": title, @@ -347,8 +337,7 @@ async def patch_task( new_status = values.pop("status_id", None) custom = values.pop("custom_fields", None) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) before = await load_visible_task(db, vis, task_id) @@ -423,10 +412,7 @@ async def patch_task( db, task_id, mentioned["notified"], by=actor(user), ) - await db.commit() result = row_to_dict(after, TaskModel) - finally: - await db.close() await emit("pm.task.updated", {"task_id": task_id}) if moved is not None: @@ -449,8 +435,7 @@ async def move_task( the status, because statuses are per-root: carrying the old status across would leave the task in a lane the destination board does not render. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) task = await load_visible_task(db, vis, task_id) values: dict[str, Any] = {} @@ -489,10 +474,7 @@ async def move_task( meta={"from_project": str(task.project_id), "from_number": getattr(task, "task_number", None)}, ) - await db.commit() result = row_to_dict(row, TaskModel) - finally: - await db.close() await emit("pm.task.moved", {"task_id": task_id}) return result @@ -510,8 +492,7 @@ async def delete_task( the exact class of lie the N8 purge shipped: a count that reassures in the wrong direction. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) promoted = await count_where(db, "pm_tasks", "parent_task_id", task_id) @@ -536,9 +517,6 @@ async def delete_task( text("DELETE FROM pm_tasks WHERE id = CAST(:tid AS uuid)"), {"tid": task_id}, ) - await db.commit() - finally: - await db.close() await emit("pm.task.deleted", {"task_id": task_id}) return DeleteResponse( @@ -571,8 +549,7 @@ async def archive_task( WS-27z's sweeper depends on this guard shipping first. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) task = await load_visible_task(db, vis, task_id) status = await require_row( @@ -596,10 +573,7 @@ async def archive_task( db, activity_type="system", created_by=actor(user), task_id=task_id, body="Task archived", ) - await db.commit() result = row_to_dict(row, TaskModel) - finally: - await db.close() await emit("pm.task.archived", {"task_id": task_id}) return result @@ -617,8 +591,7 @@ async def unarchive_task( writes ``archived_at``, and the PATCH surface deliberately does not accept it. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) task = await load_visible_task(db, vis, task_id) if getattr(task, "archived_at", None) is None: @@ -628,10 +601,7 @@ async def unarchive_task( db, activity_type="system", created_by=actor(user), task_id=task_id, body="Task restored from the archive", ) - await db.commit() result = row_to_dict(row, TaskModel) - finally: - await db.close() await emit("pm.task.unarchived", {"task_id": task_id}) return result @@ -654,8 +624,7 @@ async def set_assignees( off, so it carries the added assignees rather than the whole set: a re-assert of an existing assignee must not re-dispatch a run. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) @@ -717,9 +686,6 @@ async def set_assignees( # dispatched, never subscribed), and the rows survive a later # unassignment — having held the work is a reason to keep hearing. await ensure_watchers(db, task_id, sorted(added), by=actor(user)) - await db.commit() - finally: - await db.close() if added: await emit("pm.task.assigned", { @@ -753,8 +719,7 @@ async def create_link( raise HTTPException( status_code=422, detail="A task cannot be linked to itself.", ) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) # Both ends must be visible: a link is readable from either side, so @@ -783,14 +748,11 @@ async def create_link( db, activity_type="link", created_by=actor(user), task_id=task_id, meta={"target": str(payload.target_task_id), "type": payload.link_type}, ) - await db.commit() return { "id": str(row.id), "source_task_id": task_id, "target_task_id": str(payload.target_task_id), "link_type": payload.link_type, } - finally: - await db.close() @router.delete("/tasks/{task_id}/links/{link_id}") @@ -798,8 +760,7 @@ async def delete_link( task_id: str, link_id: str, user: UserContext = Depends(get_current_user), ) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) row = (await db.execute( @@ -812,7 +773,4 @@ async def delete_link( )).fetchone() if row is None: raise HTTPException(status_code=404, detail="Link not found") - await db.commit() return {"deleted": link_id} - finally: - await db.close() diff --git a/apps/services/gateway/gateway/routes/projects/tree.py b/apps/services/gateway/gateway/routes/projects/tree.py index abedeee04..1b11868f8 100644 --- a/apps/services/gateway/gateway/routes/projects/tree.py +++ b/apps/services/gateway/gateway/routes/projects/tree.py @@ -33,7 +33,7 @@ GrantModel, ProjectIn, ProjectModel, - _get_db, + _tenant_session, actor, assert_no_project_cycle, clean_payload, @@ -152,11 +152,8 @@ async def get_tree(user: UserContext = Depends(get_current_user)) -> dict: as a root here — that is not a bug to fix by hiding it, it is the shape of a subtree granted to a Center without its parent department. """ - db = await _get_db() - try: + async with _tenant_session() as db: rows = await _visible_projects(db, user) - finally: - await db.close() nodes = {str(r.id): {**row_to_dict(r, ProjectModel), "children": []} for r in rows} roots: list[dict] = [] @@ -168,11 +165,8 @@ async def get_tree(user: UserContext = Depends(get_current_user)) -> dict: @router.get("/nodes") async def list_nodes(user: UserContext = Depends(get_current_user)) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: rows = await _visible_projects(db, user) - finally: - await db.close() return {"rows": [row_to_dict(r, ProjectModel) for r in rows], "total": len(rows)} @@ -180,13 +174,10 @@ async def list_nodes(user: UserContext = Depends(get_current_user)) -> dict: async def get_node( project_id: str, user: UserContext = Depends(get_current_user), ) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) row = await load_visible_project(db, vis, project_id) return row_to_dict(row, ProjectModel) - finally: - await db.close() # ── Writes ────────────────────────────────────────────────────────────────── @@ -239,8 +230,7 @@ async def create_node( values["name"] = name values["created_by"] = actor(user) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) parent_id = values.get("parent_project_id") if parent_id: @@ -279,10 +269,7 @@ async def create_node( db, activity_type="system", created_by=actor(user), project_id=project_id, body=f"Project '{name}' created", ) - await db.commit() result = row_to_dict(row, ProjectModel) - finally: - await db.close() await emit("pm.project.created", {"project_id": project_id, "name": name}) return result @@ -305,8 +292,7 @@ async def patch_node( detail="Use POST /projects/nodes/{id}/move to re-parent a project.", ) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) before = await load_visible_project(db, vis, project_id) _refuse_lifecycle_on_child( @@ -325,10 +311,7 @@ async def patch_node( db, created_by=actor(user), project_id=project_id, changes=changes, ) - await db.commit() result = row_to_dict(after, ProjectModel) - finally: - await db.close() await emit("pm.project.updated", {"project_id": project_id}) return result @@ -346,8 +329,7 @@ async def move_node( move that left it stale would leave tasks pointing at another project's status rows — visible immediately as lanes that do not exist on the board. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_project(db, vis, project_id) new_parent = payload.parent_project_id @@ -378,10 +360,7 @@ async def move_node( db, activity_type="system", created_by=actor(user), project_id=project_id, body="Project moved", ) - await db.commit() result = row_to_dict(row, ProjectModel) - finally: - await db.close() await emit("pm.project.moved", {"project_id": project_id}) return result @@ -397,8 +376,7 @@ async def delete_node( and the honest number is unobtainable, which is how a destructive route ends up reporting zero. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_project(db, vis, project_id) @@ -431,9 +409,6 @@ async def delete_node( text("DELETE FROM pm_projects WHERE id = CAST(:pid AS uuid)"), {"pid": project_id}, ) - await db.commit() - finally: - await db.close() await emit("pm.project.deleted", {"project_id": project_id}) return DeleteResponse( @@ -455,8 +430,7 @@ async def delete_node( async def list_grants( project_id: str, user: UserContext = Depends(get_current_user), ) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_project(db, vis, project_id) rows = (await db.execute( @@ -469,8 +443,6 @@ async def list_grants( return { "rows": [row_to_dict(r, GrantModel) for r in rows], "total": len(rows), } - finally: - await db.close() @router.post("/nodes/{project_id}/grants", status_code=201) @@ -486,8 +458,7 @@ async def create_grant( fixture INSERT proving nothing about the API. """ subject = validate_grant_subject(payload.subject) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_project(db, vis, project_id) row = (await db.execute( @@ -503,10 +474,7 @@ async def create_grant( db, activity_type="system", created_by=actor(user), project_id=project_id, body=f"Granted to {subject}", ) - await db.commit() return row_to_dict(row, GrantModel) - finally: - await db.close() @router.delete("/nodes/{project_id}/grants/{grant_id}") @@ -514,8 +482,7 @@ async def delete_grant( project_id: str, grant_id: str, user: UserContext = Depends(get_current_user), ) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_project(db, vis, project_id) row = (await db.execute( @@ -532,7 +499,4 @@ async def delete_grant( db, activity_type="system", created_by=actor(user), project_id=project_id, body=f"Revoked {row.subject}", ) - await db.commit() return {"deleted": grant_id, "subject": row.subject} - finally: - await db.close() diff --git a/apps/services/gateway/gateway/routes/projects/views.py b/apps/services/gateway/gateway/routes/projects/views.py index 14d12f6bb..92c0dba4d 100644 --- a/apps/services/gateway/gateway/routes/projects/views.py +++ b/apps/services/gateway/gateway/routes/projects/views.py @@ -24,7 +24,7 @@ from fastapi import Depends, HTTPException from gateway.routes.projects.core import ( ViewModel, - _get_db, + _tenant_session, actor, clean_payload, insert_row, @@ -69,8 +69,7 @@ class PositionsIn(BaseModel): async def list_views( project_id: str, user: UserContext = Depends(get_current_user), ) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_project(db, vis, project_id) rows = (await db.execute( @@ -83,8 +82,6 @@ async def list_views( return { "rows": [row_to_dict(r, ViewModel) for r in rows], "total": len(rows), } - finally: - await db.close() @router.post("/nodes/{project_id}/views", status_code=201) @@ -102,8 +99,7 @@ async def create_view( status_code=422, detail=f"Unknown view type '{view_type}'. One of: {list(VIEW_TYPES)}.", ) - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_project(db, vis, project_id) row = await insert_row(db, "pm_views", { @@ -115,10 +111,7 @@ async def create_view( "position": values.get("position"), "created_by": actor(user), }) - await db.commit() return row_to_dict(row, ViewModel) - finally: - await db.close() @router.patch("/views/{view_id}") @@ -133,8 +126,7 @@ async def patch_view( detail=f"Unknown view type '{values['view_type']}'. " f"One of: {list(VIEW_TYPES)}.", ) - db = await _get_db() - try: + async with _tenant_session() as db: existing = await require_row(db, "pm_views", view_id, "View") vis = await resolve_visibility(db, user) await load_visible_project(db, vis, str(existing.project_id)) @@ -145,10 +137,7 @@ async def patch_view( if "config" in values: values["config"] = normalise_view_config(values["config"]) row = await update_row(db, "pm_views", view_id, values) - await db.commit() return row_to_dict(row, ViewModel) - finally: - await db.close() @router.delete("/views/{view_id}") @@ -162,8 +151,7 @@ async def delete_view( losing a hand-arranged board silently is exactly the surprise that makes people distrust a delete button. """ - db = await _get_db() - try: + async with _tenant_session() as db: existing = await require_row(db, "pm_views", view_id, "View") vis = await resolve_visibility(db, user) await load_visible_project(db, vis, str(existing.project_id)) @@ -178,10 +166,7 @@ async def delete_view( text("DELETE FROM pm_views WHERE id = CAST(:vid AS uuid)"), {"vid": view_id}, ) - await db.commit() return {"deleted": view_id, "cascaded": {"positions": int(positions)}} - finally: - await db.close() # ── Manual order ──────────────────────────────────────────────────────────── @@ -197,8 +182,7 @@ async def _load_visible_view(db: Any, user: UserContext, view_id: str) -> Any: async def get_positions( view_id: str, user: UserContext = Depends(get_current_user), ) -> dict: - db = await _get_db() - try: + async with _tenant_session() as db: await _load_visible_view(db, user, view_id) rows = (await db.execute( text( @@ -218,8 +202,6 @@ async def get_positions( ], "total": len(rows), } - finally: - await db.close() @router.put("/views/{view_id}/positions") @@ -240,8 +222,7 @@ async def set_positions( detail=f"At most {MAX_POSITIONS} positions per request; " f"got {len(payload.positions)}.", ) - db = await _get_db() - try: + async with _tenant_session() as db: await _load_visible_view(db, user, view_id) for entry in payload.positions: await db.execute( @@ -259,7 +240,4 @@ async def set_positions( "pos": entry.position, "grp": entry.group_key, }, ) - await db.commit() return {"view_id": view_id, "written": len(payload.positions)} - finally: - await db.close() diff --git a/apps/services/gateway/gateway/routes/projects/watchers.py b/apps/services/gateway/gateway/routes/projects/watchers.py index 092cdfaf1..322e963f3 100644 --- a/apps/services/gateway/gateway/routes/projects/watchers.py +++ b/apps/services/gateway/gateway/routes/projects/watchers.py @@ -27,7 +27,7 @@ from acb_auth import UserContext, get_current_user from fastapi import Depends from gateway.routes.projects.core import ( - _get_db, + _tenant_session, actor, load_visible_task, resolve_visibility, @@ -106,17 +106,13 @@ async def watch_task( accepted a ``watcher`` parameter would let anyone subscribe anyone else to a stream of that task's titles. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) # R5: an invisible task is 404, never 403 — "not yours" and "no such # task" must be one answer, or this endpoint becomes an oracle for # which ids exist. await load_visible_task(db, vis, task_id) await ensure_watchers(db, task_id, [actor(user)], by=actor(user)) - await db.commit() - finally: - await db.close() return {"task_id": task_id, "watching": True} @@ -130,8 +126,7 @@ async def unwatch_task( assignees, so somebody who holds the work keeps hearing about it. That is the contract's own shape, not an oversight. """ - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) await db.execute( @@ -141,9 +136,6 @@ async def unwatch_task( ), {"tid": task_id, "who": actor(user).lower()}, ) - await db.commit() - finally: - await db.close() return {"task_id": task_id, "watching": False} @@ -153,13 +145,10 @@ async def list_watchers( ) -> dict: """Who watches this task, and whether the caller does — one read, so the panel's toggle can render without a second round trip.""" - db = await _get_db() - try: + async with _tenant_session() as db: vis = await resolve_visibility(db, user) await load_visible_task(db, vis, task_id) watchers = await watchers_of(db, task_id) - finally: - await db.close() return { "watchers": watchers, "watching": actor(user).lower() in set(watchers), diff --git a/packages/acb_auth/acb_auth/deps.py b/packages/acb_auth/acb_auth/deps.py index 17c174355..5a1803edd 100644 --- a/packages/acb_auth/acb_auth/deps.py +++ b/packages/acb_auth/acb_auth/deps.py @@ -62,6 +62,7 @@ async def pull_sales(req: PullRequest): from fastapi import Depends, Header, HTTPException, Request from acb_common import get_logger +from acb_common.db import bind_tenant from acb_auth.access import SERVICE_ACCESS, resolve_access, resolve_identity from acb_auth.permissions import NO_ACCESS @@ -274,6 +275,19 @@ async def _with_resolved_access(user: UserContext) -> UserContext: access, user_id=user_id, organization_id=organization_id ) + # MT-1c / H2 — the ONE place a request binds its tenant + # (`saas_multitenancy_handover.md` H2: "bind once, centrally, from the + # authenticated session", never from a header/query/body — R11; the + # organization_id above came from the app_user row, not from the caller). + # `tenant_session()` then needs no argument anywhere on the request path. + # No release here: the gateway's TenantScopeMiddleware opened this + # request's scope and releases it after the response, so the binding + # cannot outlive the request even on servers that reuse a task. An + # unresolved identity (no app_user row) binds nothing, and a converted + # handler then fails closed with TenantUnbound rather than defaulting. + if enriched.organization_id: + bind_tenant(enriched.organization_id) + # Keep the legacy coarse role consistent with the org model, so a member # promoted to `admin` in the members UI immediately passes the # require_role(EXECUTIVE) routes that have not migrated to permissions yet. diff --git a/packages/acb_common/acb_common/db.py b/packages/acb_common/acb_common/db.py index 76e3ba09b..ae078af3c 100644 --- a/packages/acb_common/acb_common/db.py +++ b/packages/acb_common/acb_common/db.py @@ -177,8 +177,20 @@ def bind_tenant(organization_id: str) -> Token[str | None]: return _TENANT.set(str(organization_id)) +def clear_tenant() -> Token[str | None]: + """Open a fresh, empty tenant scope. Returns a reset token. + + The gateway's request middleware calls this at the top of every HTTP + request and :func:`release_tenant` after the response, so one request can + never inherit another's binding — whatever the server's task model does + with context propagation. Inside the scope, the auth dependency's + :func:`bind_tenant` is what fills it in. + """ + return _TENANT.set(None) + + def release_tenant(token: Token[str | None] | None) -> None: - """Undo :func:`bind_tenant`. Never raises.""" + """Undo :func:`bind_tenant` / :func:`clear_tenant`. Never raises.""" if token is None: return try: @@ -232,8 +244,19 @@ async def tenant_session(organization_id: str | None = None) -> AsyncIterator[An session = get_session_factory()() try: await session.begin() + # ``set_config(..., is_local := true)`` IS ``SET LOCAL`` — same + # transaction-scoped reset on commit/rollback — but it is a function + # call, so the tenant can be a BOUND parameter. The literal + # ``SET LOCAL app.tenant_id = :tenant`` form is a Postgres syntax + # error through the extended protocol (``SET`` takes no bind + # parameters), which every hermetic test missed because a Python fake + # does not parse SQL; the first live run found it (2026-08-10, the + # same lesson as WS-27r's CAST). Interpolating the id into the + # statement instead would be the injection seam this module exists to + # avoid handing out. await session.execute( - text("SET LOCAL app.tenant_id = :tenant"), {"tenant": str(tenant)} + text("SELECT set_config('app.tenant_id', :tenant, true)"), + {"tenant": str(tenant)}, ) yield session await session.commit() diff --git a/project-docs/specs/saas_multitenancy.md b/project-docs/specs/saas_multitenancy.md index 105414e62..7717eb62f 100644 --- a/project-docs/specs/saas_multitenancy.md +++ b/project-docs/specs/saas_multitenancy.md @@ -428,6 +428,15 @@ async def get_db(tenant_id: str | None = None) -> AsyncSession: await s.execute(text("SET LOCAL app.tenant_id = :t"), {"t": tenant_id or _ctx_tenant()}) return s ``` +> ⚠️ **Correction (2026-08-10, found by the first live H2 run):** the literal +> `SET LOCAL app.tenant_id = :t` is a Postgres **syntax error** through the extended +> protocol — `SET` cannot take a bind parameter. Every hermetic test was green with it; +> real Postgres refused it on the first converted handler. The shipped encoding in +> `acb_common.db.tenant_session()` is `SELECT set_config('app.tenant_id', :tenant, true)` +> — identical transaction-local semantics (`is_local = true` IS `SET LOCAL`), but +> parameterizable. The warning below still binds; only the spelling changed. +> `test_tenant_session.py` pins the `set_config` form and refuses both the literal and +> an `is_local = false` variant. > ⚠️ **`SET LOCAL`, never `SET`.** The pool recycles connections across requests > (`pool_size` + `max_overflow`, `db.py:114-120`). A session-scoped `SET` survives the > connection's return to the pool and becomes a cross-tenant read on the next borrower. diff --git a/project-docs/specs/saas_multitenancy_handover.md b/project-docs/specs/saas_multitenancy_handover.md index 769606de4..521a05e33 100644 --- a/project-docs/specs/saas_multitenancy_handover.md +++ b/project-docs/specs/saas_multitenancy_handover.md @@ -187,6 +187,46 @@ stated values, **and** the baseline test set still passes. ## H2 · Convert 561 session-acquisition sites to `tenant_session()` · 🟢 AGENT-SAFE · **the long pole** +> ### ◐ H2 STARTED 2026-08-10 — central binding SHIPPED + the Projects slice converted +> +> **The "do this first" step is done:** `_with_resolved_access` (acb_auth/deps.py) calls +> `bind_tenant(organization_id)` when identity resolves — the one place, from the +> `app_user` row, never a header (R11) — and the gateway's `TenantScopeMiddleware` +> (main.py) opens a fresh scope per request and releases it after the response. +> `tests/unit/test_tenant_request_binding.py` pins both, including +> "`system:internal` binds nothing" and no-leak-across-sequential-requests. +> +> **`routes/projects` is converted** (84 sites, the largest single package): every +> handler is `async with _tenant_session() as db:` where `_tenant_session` IS the +> shared seam (identity asserted in `test_db_engine_seam.py`). One named exemption: +> `agent_dispatch.py` (2 sites) is an **event consumer**, so per this document's own +> rule it stays on `get_db()` until H4 threads an explicit tenant through the event +> payload — inheriting the ambient one is exactly what H4 forbids. +> +> **Ratchets** (test_db_engine_seam.py): `routes/projects` must stay at ZERO +> unconverted sites; the remainder elsewhere is frozen at **`H2_BASELINE_ELSEWHERE = +> 494`** and only ratchets down (progress must be banked by lowering the constant). +> +> ⚠️ **A live run found a defect in `tenant_session()` itself:** the literal +> `SET LOCAL app.tenant_id = :tenant` is a Postgres syntax error through the extended +> protocol — `SET` cannot bind a parameter, and every hermetic test was green with it. +> Fixed to `SELECT set_config('app.tenant_id', :tenant, true)` (identical +> transaction-local semantics), pinned by `test_tenant_session.py`, and proven by a +> live scratch-Postgres smoke: unbound → `TenantUnbound`; converted handlers write +> under the GUC; rows stamped with the bound org. **Converters of the remaining +> packages: the runbook below stands unchanged** — but test against real Postgres at +> least once per package; this class of defect is invisible to fakes. +> +> Conversion notes that generalize (learned on the Projects slice): handlers' explicit +> `await db.commit()` goes away (the wrapper commits on clean exit — and a mid-block +> commit would END the transaction and drop the GUC for everything after it, so a +> handler that genuinely needs two transactions needs two `async with` blocks); +> read-only endpoints now commit an empty transaction, so tests asserting +> `committed == 0` as a "writes nothing" proxy must assert on statements/rows instead; +> hermetic fakes swap in via an `asynccontextmanager` patched over the package's +> `_tenant_session` alias, commit-on-clean-exit so one-transaction contracts stay +> observable. + `acb_common.db.tenant_session()` exists and is tested. `get_db()` still exists, is documented as **not** tenant-bound, and every one of the 561 sites still uses it. diff --git a/project-docs/specs/saas_multitenancy_implementation.md b/project-docs/specs/saas_multitenancy_implementation.md index 94b16d1c0..caa83ed53 100644 --- a/project-docs/specs/saas_multitenancy_implementation.md +++ b/project-docs/specs/saas_multitenancy_implementation.md @@ -168,7 +168,10 @@ async def tenant_session(tenant_id: str | None = None): try: await session.begin() # SET LOCAL needs a transaction await session.execute( - text("SET LOCAL app.tenant_id = :t"), {"t": str(tid)}) + # ⚠️ corrected 2026-08-10: the SET LOCAL literal cannot bind a + # parameter (extended-protocol syntax error, found live); + # set_config(..., true) is the same transaction-local semantics. + text("SELECT set_config('app.tenant_id', :t, true)"), {"t": str(tid)}) yield session await session.commit() except Exception: diff --git a/project-docs/work_plan.md b/project-docs/work_plan.md index cc4df0368..96768d685 100644 --- a/project-docs/work_plan.md +++ b/project-docs/work_plan.md @@ -195,7 +195,7 @@ owning specs are the archive; this file owns ordering, gates and states only. | WS | Workstream | State | Owning spec · record | Gates · next (verified) | |---|---|---|---|---| -| WS-29 | **Multi-tenancy — turning CommandCenter into a product sold to other companies** | ◐ H1 done · H2 next | **`specs/saas_multitenancy.md`** (architecture; §11 tickets) · ⭐ **`specs/saas_multitenancy_handover.md`** (H1→H8 runbook — hand THIS to the executing agent) · `specs/saas_multitenancy_implementation.md` (shapes) · board record 2026-08-09 in the parent spec | **Phase 0 ✅** (MT-0a · 0b · 0c-1 · 0d, pending review) · **H1 ✅ CLOSED**: scratch-verified 2026-08-09, **PR #404 merged and migrations 157/158/159 CONFIRMED on prod the same day** (ledger line "157 already recorded"; box self-applied via pull timer). · MT-1: 1a schema ✅ (identity cutover = H6, open) · 1b generated ✅ · 1c seam + ratchets ✅ — **561 call sites across 138 files unconverted = H2, the long pole** · 1e wrapper ✅ (~58 key sites unconverted = H5) · 1i ✅ (two-org DB fixture owed) · **MT-2/MT-3/MT-4 owner inputs ALL ANSWERED — final pricing shape D23/D24 2026-08-10 (§2.4b Center packages; ladder 600/1200/1800/2400/3000)** — MT-2's scope includes `center_package` + `plan_catalog` + seat `source` + the one-assignment act; spec detailing may start on all three; the customer console is WS-30 · 🔴 MT-0c-2 parked (D16; §6 first blockquote) · §5.1 cutover trigger **ADOPTED 2026-08-09** — owner checks monthly · ⚠️ **#399 (MERGED 2026-08-09) carried a second WS-29 thread** (`specs/multi_tenancy.md`, superseded for architecture — measured record only): migration **161** keys all 17 `pm_*` + a parent-consistency trigger, **162** makes `app_user` unique on `lower(email)`, S1-1 fixed a cross-tenant **write** into access control, S1-4 removed the process-global agent identity, plus a 14-finding leak audit. **It also found a defect in MT-1b:** the generator scoped `crm_contacts`/`crm_deals`/`crm_activities` by column name, but their `organization_id` references `crm_organizations` — phase 2 would have aborted mid-window. Gated at generation time now (`HOMONYM_BLOCKED`); those three tables carry **no isolation** pending a rename — 🔴 owner call. **Next: dispatch H2.** (2026-08-10) | +| WS-29 | **Multi-tenancy — turning CommandCenter into a product sold to other companies** | ◐ H1 done · H2 next | **`specs/saas_multitenancy.md`** (architecture; §11 tickets) · ⭐ **`specs/saas_multitenancy_handover.md`** (H1→H8 runbook — hand THIS to the executing agent) · `specs/saas_multitenancy_implementation.md` (shapes) · board record 2026-08-09 in the parent spec | **Phase 0 ✅** (MT-0a · 0b · 0c-1 · 0d, pending review) · **H1 ✅ CLOSED**: scratch-verified 2026-08-09, **PR #404 merged and migrations 157/158/159 CONFIRMED on prod the same day** (ledger line "157 already recorded"; box self-applied via pull timer). · MT-1: 1a schema ✅ (identity cutover = H6, open) · 1b generated ✅ · 1c seam + ratchets ✅ — **561 call sites across 138 files unconverted = H2, the long pole** · 1e wrapper ✅ (~58 key sites unconverted = H5) · 1i ✅ (two-org DB fixture owed) · **MT-2/MT-3/MT-4 owner inputs ALL ANSWERED — final pricing shape D23/D24 2026-08-10 (§2.4b Center packages; ladder 600/1200/1800/2400/3000)** — MT-2's scope includes `center_package` + `plan_catalog` + seat `source` + the one-assignment act; spec detailing may start on all three; the customer console is WS-30 · 🔴 MT-0c-2 parked (D16; §6 first blockquote) · §5.1 cutover trigger **ADOPTED 2026-08-09** — owner checks monthly · ⚠️ **#399 (MERGED 2026-08-09) carried a second WS-29 thread** (`specs/multi_tenancy.md`, superseded for architecture — measured record only): migration **161** keys all 17 `pm_*` + a parent-consistency trigger, **162** makes `app_user` unique on `lower(email)`, S1-1 fixed a cross-tenant **write** into access control, S1-4 removed the process-global agent identity, plus a 14-finding leak audit. **It also found a defect in MT-1b:** the generator scoped `crm_contacts`/`crm_deals`/`crm_activities` by column name, but their `organization_id` references `crm_organizations` — phase 2 would have aborted mid-window. Gated at generation time now (`HOMONYM_BLOCKED`); those three tables carry **no isolation** pending a rename — 🔴 owner call. ◐ **H2 STARTED 2026-08-10**: central `bind_tenant` binding + `TenantScopeMiddleware` SHIPPED; `routes/projects` converted (84 sites; `agent_dispatch` H4-exempt by name); ratchets in `test_db_engine_seam.py` (projects=0, elsewhere frozen at 494, bank-your-progress rule). ⚠️ **The first live run found `tenant_session()` itself broken** — `SET LOCAL … = :param` is a Postgres syntax error (SET can't bind); fixed to `set_config(..., true)`, pinned + live-proven. See the handover's H2 box. **Next: convert the remaining 494 sites package-by-package (email/wa/crm/tasks/notes/workflows…), each with one live-Postgres smoke.** (2026-08-10) | | — | **Future modules roadmap** (KB · Marketing · Support · dashboards-colour · Builder/Workflows slicing rule) *(named 2026-08-09, D21)* | 🔴 not dispatchable | `specs/future_modules_roadmap.md` | No WS rows until each earns a §1-contract spec; Dashboards colour lands in WS-15's acceptance; the Builder/Workflows visibility-tier-at-creation rule binds reviews now (D12). | | WS-30 | **Subscription Console — customer-facing billing surface** *(minted 2026-08-09)* | 🔴 MT-2 first | **`specs/subscription_console.md`** | Manage-only at launch: **Centers & add-ons panel · users × Centers seat grid (D23)** · credit monitor · seat writes under D19.3's hard cap · role presets (D24.5) · change-request flow (fulfilment 🔴 OWNER-GATE during silo phase). Sequencing: MT-2 tables → SC-1 → SC-2/SC-3 → SC-4 with MT-4. Business inputs answered (D19 + D23 + **D24** — framing closed); blockers: MT-2's substrate and the MT-2/MT-3 ticket contracts. (2026-08-10) | diff --git a/tests/unit/_projects_fakes.py b/tests/unit/_projects_fakes.py index f2ab3933d..bf44a74ec 100644 --- a/tests/unit/_projects_fakes.py +++ b/tests/unit/_projects_fakes.py @@ -47,6 +47,7 @@ from __future__ import annotations import re +from contextlib import asynccontextmanager from datetime import UTC, datetime, timedelta from types import SimpleNamespace from typing import Any @@ -1571,16 +1572,40 @@ def page(number: int = 1, size: int = 50) -> Any: def bind_db(monkeypatch: Any, fake: FakeProjectsDB, modules: tuple[Any, ...]) -> None: - """Point each submodule's ``_get_db`` seam at ``fake``. + """Point each submodule's ``_tenant_session`` seam at ``fake``. - Per-module and not per-package because each module imports ``_get_db`` from + Per-module and not per-package because each module imports the seam from ``core`` by name, so patching ``core`` alone would not reach them. + + H2 note: the real seam is ``acb_common.db.tenant_session`` — an async + context manager that begins a transaction, issues ``SET LOCAL + app.tenant_id`` and commits on exit. The fake mirrors only the SHAPE + (``async with … as db``); transaction semantics stay out of the mirror + exactly as ``commit``/``close`` no-ops did before, because what these + tests pin is SQL behaviour, not the GUC plumbing — + ``test_tenant_session.py`` owns that. """ + @asynccontextmanager + async def _tenant_session(organization_id: str | None = None) -> Any: + # Commit-on-clean-exit, exactly like the real wrapper — so "these + # writes share one transaction" stays an OBSERVABLE fact + # (`db.committed == 1`) rather than a comment, and a handler that + # raises mid-block commits nothing here just as it commits nothing + # against Postgres. + yield fake + await fake.commit() + async def _get_db() -> FakeProjectsDB: return fake for module in modules: - monkeypatch.setattr(module, "_get_db", _get_db) + if hasattr(module, "_tenant_session"): + monkeypatch.setattr(module, "_tenant_session", _tenant_session) + else: + # The one H4 exemption: `agent_dispatch` is an event consumer and + # deliberately stays on the unbound `get_db` seam until H4 threads + # an explicit tenant through the event payload. + monkeypatch.setattr(module, "_get_db", _get_db) def silence_events(monkeypatch: Any, modules: tuple[Any, ...]) -> list[tuple[str, dict]]: diff --git a/tests/unit/test_db_engine_seam.py b/tests/unit/test_db_engine_seam.py index 21d402df5..77e51ba35 100644 --- a/tests/unit/test_db_engine_seam.py +++ b/tests/unit/test_db_engine_seam.py @@ -39,6 +39,7 @@ from __future__ import annotations import ast +import re from functools import cache from pathlib import Path @@ -265,6 +266,11 @@ def test_route_packages_resolve_to_the_shared_factory(module_path: str) -> None: "_get_session_factory": shared.get_session_factory, "get_db": shared.get_db, "_get_db": shared.get_db, + # H2 (MT-1c): a converted package drops `_get_db` for the tenant-bound + # context manager — which must still BE the shared seam, not a wrapper + # with its own pool or its own GUC discipline. + "tenant_session": shared.tenant_session, + "_tenant_session": shared.tenant_session, } found = [n for n in expected if hasattr(mod, n)] assert found, f"{module_path} exposes no DB seam name at all" @@ -284,3 +290,71 @@ def test_acb_auth_shares_the_pool() -> None: from acb_common import db as shared assert access._get_session_factory is shared.get_session_factory + + +# ── H2 ratchet — get_db() call sites only go DOWN ─────────────────────────── +# +# `saas_multitenancy_handover.md` H2: every `await get_db()` is a session that +# will read ZERO rows once the RLS phase-4 policies apply, so the workstream's +# done-when is "the grep returns 0". Converting 500+ sites takes many PRs; +# this pair of tests is what stops the number creeping back up in between. + +_GET_DB_CALL = re.compile(r"await _?get_db\(\)") + +#: Sites that are ALLOWED to stay on the unbound seam, each with the reason. +#: An entry here is a decision, not a grandfathering — H4 owns retiring them. +H2_EXEMPT_FILES: dict[str, str] = { + "apps/services/gateway/gateway/routes/projects/agent_dispatch.py": + "event consumer, not a request handler — H4 threads an explicit " + "tenant through the event payload; ambient inheritance is forbidden", +} + +#: The unconverted remainder OUTSIDE routes/projects at the time the Projects +#: slice landed (2026-08-10). Lower it as packages convert; never raise it. +H2_BASELINE_ELSEWHERE = 494 + + +def _get_db_sites() -> dict[str, int]: + out: dict[str, int] = {} + for base in ("apps", "packages"): + for path in (_REPO / base).rglob("*.py"): + n = len(_GET_DB_CALL.findall(path.read_text(encoding="utf-8"))) + if n: + out[str(path.relative_to(_REPO)).replace("\\", "/")] = n + return out + + +def test_routes_projects_is_converted_and_stays_converted() -> None: + """The Projects package acquires sessions ONLY through `tenant_session`. + + A new `get_db()` here is a handler whose queries will silently return + nothing under RLS — the fail-closed symptom H2's runbook warns about. + """ + sites = _get_db_sites() + offenders = { + f: n for f, n in sites.items() + if f.startswith("apps/services/gateway/gateway/routes/projects/") + and f not in H2_EXEMPT_FILES + } + assert offenders == {}, ( + f"unbound get_db() in converted package: {offenders} — use " + f"`async with _tenant_session() as db:` (core.py) instead" + ) + + +def test_get_db_sites_elsewhere_only_ratchet_down() -> None: + total = sum( + n for f, n in _get_db_sites().items() + if not f.startswith("apps/services/gateway/gateway/routes/projects/") + ) + assert total <= H2_BASELINE_ELSEWHERE, ( + f"{total} unbound get_db() sites outside routes/projects — above the " + f"frozen H2 baseline of {H2_BASELINE_ELSEWHERE}. New code must use " + f"tenant_session(); see saas_multitenancy_handover.md H2." + ) + if total < H2_BASELINE_ELSEWHERE: + # Progress must be BANKED, or the headroom becomes new-debt budget. + assert total > H2_BASELINE_ELSEWHERE - 25, ( + f"H2 progress: {total} sites remain — lower H2_BASELINE_ELSEWHERE " + f"to {total} in this PR to bank it" + ) diff --git a/tests/unit/test_projects_attachments.py b/tests/unit/test_projects_attachments.py index ff8965ff5..762155deb 100644 --- a/tests/unit/test_projects_attachments.py +++ b/tests/unit/test_projects_attachments.py @@ -20,6 +20,7 @@ import asyncio import re +from contextlib import asynccontextmanager from pathlib import Path from types import SimpleNamespace from typing import Any @@ -111,8 +112,12 @@ def db() -> FakeDB: @pytest.fixture(autouse=True) def wiring(monkeypatch, db, tmp_path): - async def _get_db(): - return db + # H2: the module's seam is `_tenant_session` (an async context manager), + # mirrored commit-on-clean-exit like `_projects_fakes.bind_db`. + @asynccontextmanager + async def _tenant_session(organization_id=None): + yield db + await db.commit() async def _resolve(_db, _user): from gateway.routes.projects.core import Visibility @@ -130,7 +135,7 @@ async def _record(*_a, **_k): async def _emit(*_a, **_k): return None - monkeypatch.setattr(pm_attachments, "_get_db", _get_db) + monkeypatch.setattr(pm_attachments, "_tenant_session", _tenant_session) monkeypatch.setattr(pm_attachments, "resolve_visibility", _resolve) monkeypatch.setattr(pm_attachments, "load_visible_task", _load_visible) monkeypatch.setattr(pm_attachments, "record_activity", _record) diff --git a/tests/unit/test_projects_import_mapping.py b/tests/unit/test_projects_import_mapping.py index b7add1c74..7cff66a3c 100644 --- a/tests/unit/test_projects_import_mapping.py +++ b/tests/unit/test_projects_import_mapping.py @@ -158,7 +158,11 @@ async def test_the_plan_writes_nothing( if s.split(None, 1)[0].upper() in ("INSERT", "UPDATE", "DELETE") ] assert writes == [] - assert db.committed == 0 + # H2 note: the session wrapper now commits on every clean exit — including + # a read-only transaction, exactly as `tenant_session` does against + # Postgres — so a commit COUNT no longer distinguishes a write from a + # read. "Writes nothing" is the two assertions around this comment: no + # write statement was issued, and no row appeared. assert db.rows("pm_projects") == [] @@ -562,8 +566,9 @@ async def test_a_dry_run_writes_nothing_and_says_so( assert result["dry_run"] is True assert result["projects"]["created"] > 0 + # H2: commit count no longer distinguishes reads from writes (see the + # plan-writes-nothing test) — the absence of rows IS the dry-run proof. assert db.rows("pm_projects") == [] - assert db.committed == 0 # ── Gating ────────────────────────────────────────────────────────────────── @@ -615,8 +620,11 @@ async def test_a_non_clickup_account_is_refused( ) -> None: """The importer speaks ClickUp's shapes; handed an Asana account it would silently import nothing rather than say why.""" - async def _fake_db() -> FakeProjectsDB: - return db + from contextlib import asynccontextmanager + + @asynccontextmanager + async def _fake_session(organization_id=None): + yield db class _Row(dict): pass @@ -634,7 +642,7 @@ async def _execute(sql, params=None): "credentials_encrypted": "x"}) monkeypatch.setattr(db, "execute", _execute) - monkeypatch.setattr(pm_import, "_get_db", _fake_db) + monkeypatch.setattr(pm_import, "_tenant_session", _fake_session) with pytest.raises(HTTPException) as exc: await pm_import._resolve_provider("acct-9") diff --git a/tests/unit/test_projects_import_tasks.py b/tests/unit/test_projects_import_tasks.py index 6d688f825..c681eaa98 100644 --- a/tests/unit/test_projects_import_tasks.py +++ b/tests/unit/test_projects_import_tasks.py @@ -262,10 +262,15 @@ def gtd_item(**over) -> SimpleNamespace: @pytest.fixture() def bind(monkeypatch): def _bind(db: FakeDB): - async def _get_db(): - return db + # H2: the module's seam is `_tenant_session`, commit-on-clean-exit. + from contextlib import asynccontextmanager - monkeypatch.setattr(sut, "_get_db", _get_db, raising=False) + @asynccontextmanager + async def _tenant_session(organization_id=None): + yield db + await db.commit() + + monkeypatch.setattr(sut, "_tenant_session", _tenant_session) # `_seed_root` lives in tree.py and writes its own rows; it is exercised # by the tree suite, and stubbing it keeps this suite about the import. async def _seed(*_a, **_k): diff --git a/tests/unit/test_projects_notifications.py b/tests/unit/test_projects_notifications.py index 2516dcb76..e2fecc338 100644 --- a/tests/unit/test_projects_notifications.py +++ b/tests/unit/test_projects_notifications.py @@ -249,11 +249,16 @@ def _user(email: str, *grants: str) -> UserContext: def bind(monkeypatch, db: FakeDB, *modules) -> None: - async def _get_db(): - return db + # H2: the seam is `_tenant_session`, commit-on-clean-exit. + from contextlib import asynccontextmanager + + @asynccontextmanager + async def _tenant_session(organization_id=None): + yield db + await db.commit() for module in modules or (pm_notify,): - monkeypatch.setattr(module, "_get_db", _get_db, raising=False) + monkeypatch.setattr(module, "_tenant_session", _tenant_session) # ── Rule 3: nobody hears about a task they cannot open ────────────────────── diff --git a/tests/unit/test_tenant_request_binding.py b/tests/unit/test_tenant_request_binding.py new file mode 100644 index 000000000..2c3e79283 --- /dev/null +++ b/tests/unit/test_tenant_request_binding.py @@ -0,0 +1,139 @@ +"""MT-1c / H2 — the request path binds its tenant once, centrally. + +`saas_multitenancy_handover.md` H2: *"Add `bind_tenant(user.organization_id)` +in the gateway middleware / the app-wide dependency that already resolves +`UserContext`, and release it after the response. Then the 561 sites need no +tenant argument at all."* + +The claims pinned here are the ones a refactor could silently lose: + +* resolving an authenticated identity BINDS the tenant contextvar — this is + what lets every converted `tenant_session()` call site take no argument; +* the tenant comes from the `app_user` row (`resolve_identity`), NEVER from a + header the caller controls (R11) — asserted by construction: the fake + resolver ignores the asserted role header entirely; +* an identity with no organization binds NOTHING, so a converted handler fails + closed with `TenantUnbound` instead of defaulting to "the usual tenant"; +* the `system:internal` service identity binds nothing — jobs bind their own + tenant explicitly (H4) or fail closed; +* the gateway middleware opens a fresh scope per request and releases it after + the response, so a binding cannot leak from one request into the next even + on a server that reuses one task for sequential requests. + +Hermetic: `resolve_access` / `resolve_identity` are monkeypatched; no DB. +""" + +from __future__ import annotations + +import pytest +from acb_common.db import bind_tenant, clear_tenant, current_tenant, release_tenant + +import acb_auth.deps as deps +from acb_auth.permissions import NO_ACCESS + +ORG = "11111111-1111-1111-1111-111111111111" + + +@pytest.fixture(autouse=True) +def _fresh_scope(): + """Every test runs in its own empty tenant scope, released afterwards.""" + token = clear_tenant() + yield + release_tenant(token) + + +@pytest.fixture +def identity(monkeypatch: pytest.MonkeyPatch): + """Resolve every email to (user-id, ORG) without a database.""" + async def fake_access(email, legacy_role=None, record_request=False): + return NO_ACCESS + + async def fake_identity(email): + return ("22222222-2222-2222-2222-222222222222", ORG) + + monkeypatch.setattr(deps, "resolve_access", fake_access) + monkeypatch.setattr(deps, "resolve_identity", fake_identity) + monkeypatch.setattr(deps, "_get_internal_token", lambda: "tok") + + +async def test_an_authenticated_request_binds_its_organization(identity): + user = await deps.get_current_user( + x_user_email="priya@fracktal.in", + x_user_role="employee", + authorization="Bearer tok", + ) + assert user.organization_id == ORG + assert current_tenant() == ORG + + +async def test_the_tenant_comes_from_identity_resolution_not_the_caller(identity): + """R11 by construction: the resolver saw only the email; the asserted role + header (the only other caller-controlled input) cannot steer the tenant.""" + await deps.get_current_user( + x_user_email="priya@fracktal.in", + x_user_role="executive", + authorization="Bearer tok", + ) + assert current_tenant() == ORG + + +async def test_an_unresolved_identity_binds_nothing(monkeypatch): + async def fake_access(email, legacy_role=None, record_request=False): + return NO_ACCESS + + async def fake_identity(email): + return (None, None) + + monkeypatch.setattr(deps, "resolve_access", fake_access) + monkeypatch.setattr(deps, "resolve_identity", fake_identity) + monkeypatch.setattr(deps, "_get_internal_token", lambda: "tok") + + user = await deps.get_current_user( + x_user_email="stranger@fracktal.in", + x_user_role="employee", + authorization="Bearer tok", + ) + assert user.organization_id is None + assert current_tenant() is None + + +async def test_the_service_identity_binds_nothing(identity): + """`system:internal` has no organization. A job that needs one binds it + explicitly from its own record (H4) — never inherits an ambient one.""" + user = await deps.get_current_user(authorization="Bearer tok") + assert user.email == "system:internal" + assert current_tenant() is None + + +async def test_the_middleware_scope_does_not_leak_across_requests(identity): + """Two sequential requests in ONE task: the second starts empty even + though the first bound a tenant and never explicitly unbound it.""" + from gateway.main import TenantScopeMiddleware + + seen: list[str | None] = [] + + async def app(scope, receive, send): + seen.append(current_tenant()) + bind_tenant(ORG) # what _with_resolved_access does mid-request + + mw = TenantScopeMiddleware(app) + http = {"type": "http"} + await mw(http, None, None) + await mw(http, None, None) + assert seen == [None, None] + assert current_tenant() is None + + +async def test_a_non_http_scope_is_passed_through_untouched(identity): + from gateway.main import TenantScopeMiddleware + + called: list[str] = [] + + async def app(scope, receive, send): + called.append(scope["type"]) + + bind_tenant(ORG) + await TenantScopeMiddleware(app)({"type": "lifespan"}, None, None) + assert called == ["lifespan"] + # a lifespan passthrough neither opens nor closes a scope + assert current_tenant() == ORG diff --git a/tests/unit/test_tenant_session.py b/tests/unit/test_tenant_session.py index beb6d786a..031cd8f7a 100644 --- a/tests/unit/test_tenant_session.py +++ b/tests/unit/test_tenant_session.py @@ -70,19 +70,30 @@ def calls(monkeypatch): # ========================================================================== # 1 — SET LOCAL, not SET. # ========================================================================== -def test_the_seam_uses_set_local_not_set() -> None: +def test_the_seam_uses_transaction_local_set_config() -> None: """A string assertion, deliberately. - ``SET`` vs ``SET LOCAL`` is a one-word difference whose only symptom is a - cross-tenant read under connection reuse. No behavioural unit test - reproduces that reliably, so the source is pinned instead. + Transaction-local vs session-scoped is a one-argument difference whose + only symptom is a cross-tenant read under connection reuse. No behavioural + unit test reproduces that reliably, so the source is pinned instead. + + ⚠️ The encoding is ``set_config('app.tenant_id', :tenant, true)`` and NOT + the literal ``SET LOCAL app.tenant_id = :tenant`` — the latter is a + Postgres SYNTAX ERROR through the extended protocol (``SET`` cannot take + a bind parameter). Every hermetic run was green with the broken literal; + the first live handler run failed on it (2026-08-10). ``set_config``'s + third argument ``true`` is what makes it ``SET LOCAL`` semantics; ``false`` + (or omitting it) is the session-scoped poison this test exists to refuse. """ src = inspect.getsource(tenant_session.__wrapped__) # type: ignore[attr-defined] - assert "SET LOCAL app.tenant_id" in src - assert "SET app.tenant_id" not in src.replace("SET LOCAL app.tenant_id", ""), ( - "a session-scoped SET survives the connection's return to the pool — " - "the next borrower reads the previous tenant" - ) + assert "set_config('app.tenant_id', :tenant, true)" in src + # Pin the EXECUTED statement, not the prose around it: no text("SET ...") + # may reappear (the literal form cannot bind a parameter — it only ever + # worked against fakes), and no is_local=false variant may sneak in (a + # session-scoped set survives the connection's return to the pool and the + # next borrower reads the previous tenant). + assert 'text("SET' not in src and "text('SET" not in src + assert "set_config('app.tenant_id', :tenant, false" not in src @pytest.mark.asyncio @@ -92,7 +103,7 @@ async def test_binding_is_issued_as_a_bound_parameter(calls) -> None: sets = [c for c in calls if c[0] == "execute"] assert len(sets) == 1 sql, params = sets[0][1] - assert "SET LOCAL app.tenant_id" in sql + assert "set_config('app.tenant_id'" in sql assert params == {"tenant": _ORG_A} assert _ORG_A not in sql, "the tenant id was interpolated into the statement"