WS-29 H2 (waves 1+2): nine gateway packages converted to tenant_session — 111 classified sites remain - #417
Merged
Conversation
61 of the package's 94 get_db() sites move to the tenant-bound seam:
every user-identity request handler now runs `async with _tenant_session()
as db:` (commit-on-clean-exit; explicit db.commit() lines deleted), with
the alias exported from routes/notes/core.py exactly as routes/projects
does it.
The 33 sites that stay on get_db() are each marked in place:
- H4 background consumers: pipeline.run_transcription, summaries.
generate_notes/enqueue_summary, dispatch's auto_dispatch/_dispatch/_mark/
_audit/_dispatch_document, copilot's _persist/_deep_context, meeting_bot's
_refresh_bot/_poll_bot/_ingest_recording, live_session.end,
live_speakers.apply_live_names, glossary.glossary_prompt,
speaker_id.infer_speaker_names — all reachable from spawned tasks or the
poller where no ambient tenant exists; each names the row to derive from.
- H4/H6 service identity: live.live_wanted (meeting-bot worker calls
/live/wanted under MEETING_BOT_TOKEN; system identity binds no tenant).
- Dual-use settings/copilot_agenda/copilot_context helpers consumed by the
copilot orchestrator task stay unbound with the same markers.
Tests: _install_db in test_notes_owner_scoping.py now patches the module's
_tenant_session (asynccontextmanager, commit-on-clean-exit) alongside
_get_db, so both seam shapes stay hermetically testable per module.
H2_BASELINE_ELSEWHERE banked 494 → 433.
Verified against a real scratch Postgres 16 per the handover's warning:
unbound → TenantUnbound; bound → create/list/patch/read commit under the
GUC (set_config('app.tenant_id', …)); patch_meeting's read-after-write
holds in the single wrapper transaction.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
All 52 get_db() sites classified before touching any (this package is
ingestion-heavy, and a wrong CONVERT is a runtime 500):
- 38 request-handler sites (Depends(get_current_user), tenant bound
centrally by _with_resolved_access) are now
`async with _tenant_session() as db:` — the per-package alias in
whatsapp/core.py IS acb_common.db.tenant_session, mirroring
projects/core.py; identity asserted by the existing seam test.
Explicit commits removed (the wrapper commits on clean exit; a
mid-block commit would end the transaction and drop the GUC), the
bootstrap/update commit-then-reread shapes now read their own writes
in the one transaction, and the saved-replies inner excepts keep
their 409 mapping inside the block with the wrapper owning rollback.
- 7 service-identity sites STAY on _get_db with `# H4/H6` markers: the
Meta webhook pair (signature-authed, no member session) and the Go
bridge's five shared-secret push routes (ingest/reclassify/labels/
avatars/paired — grep of whatsapp_bridge/gateway.go is the evidence).
system:internal binds NO tenant, so tenant_session() there would 500
every inbound batch; H4/H6 derives the tenant from the wa_accounts
row instead. bridge.py carries both seams deliberately: its /connect
and /status routes are user-facing and converted.
- 7 background-consumer sites STAY with `# H4` markers: the enrichment
scheduler pair, the post-sync hooks (intent.process_new_messages,
replyzero.classify_chats), the two scheduled enrichment passes
(transcribe_pending, summarize_stale_groups) and the Action Broker
broadcast handler — inheriting an ambient tenant is exactly what the
runbook forbids for jobs.
- Ratchets (test_db_engine_seam.py): H2_BASELINE_ELSEWHERE banked
494 → 456 (measured), and a new pin holds routes/whatsapp at exactly
its named remainder file-by-file, so a new unbound handler fails the
build and a retired site must shrink the dict.
- Hermetic seams re-pointed: test_whatsapp_calls/_connect now patch
_tenant_session with an @asynccontextmanager fake that commits on
clean exit (the _projects_fakes.bind_db shape); the closed-is-True
resource proxy became a committed==1 assertion.
Verification: the 27 test_whatsapp_*.py files + test_db_engine_seam.py
pass by name (335 tests); ruff F821/F601/F602/F502/F7/B006 clean on all
touched files; live smoke on scratch Postgres 16 with the repo
migrations applied — unbound converted handler raises TenantUnbound,
bound GET /whatsapp/accounts answers through the real
set_config('app.tenant_id',…) round-trip, converted DELETE commits via
the wrapper, and the GUC does not leak to a fresh session.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
Baseline conflict resolved by recomputing: 494 - 61 (notes) - 38 (whatsapp) = 395; both slices' provenance comments kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
The largest remaining H2 slice: 98 of routes/email's 125 get_db() sites are now `async with _tenant_session() as db:` — the per-package alias IS acb_common.db.tenant_session (email/core.py re-exports it beside the thin `_get_db` wrapper the seam test already exempts; notes/dispatch and the 27 B/C-class sites still import that wrapper). Every site was classified before conversion: - A (converted, 98): member-identity request handlers plus helpers called only from them (chat context builder, retry_failed_executions, restore_provider_labels) — the central binding has bound the tenant. - B (left, 2): service-identity routes — the OAuth provider callback (trust = HMAC-signed state) and the Microsoft Graph webhook. Marked with the row each would derive its tenant from (app_user via the signed state's user; email_accounts via the subscription). - C (left, 25): scheduler post-sync hooks, BackgroundTask jobs, create_task paths (_compose_assist_run's streaming leg), and helpers with mixed route+background callers (sweep_uncategorized, apply_thread_status_correction, _maybe_send_follow_up_reminders). Each carries an `# H4:` marker naming why. Mid-block commits removed per the runbook (the wrapper commits on clean exit; a mid-block commit ends the transaction and drops the GUC). Four shared helpers that committed on a caller-passed session were the real traps and are restructured honestly: - core.hydrate_message_body no longer commits (cache write; callers own the boundary), - senders._apply_newsletter_status and messages._hydrate_attachments and contacts._remember_contact lose their commits (only converted callers, and BackgroundTasks run post-response, i.e. after the exit commit), - drafting._store_ai_draft grows commit=False for its converted caller while background callers keep committing. rule_policies keeps its defensive rollback (documented: nothing touches the DB after it, so the dropped GUC is moot). Tests: tests/unit/_email_fakes.py::bind_db mirrors the seam shape (commit-on-clean-exit, entry counter replacing get_db.assert_not_awaited); ~90 patch sites across 27 email test files re-pointed; the reclassify fixture doubles BOTH seams (its job stays on get_db until H4); the download-attachment "commits so rotated creds land" pin now asserts the tenant seam owns that boundary. H2_BASELINE_ELSEWHERE banked 494 → 396. Verification: all 96 tests/unit/test_email_*.py files + seam/tenancy pins green (1167 tests); ruff F821,F601,F602,F502,F7,B006 clean. NOT yet smoked against live Postgres — the runbook asks one live smoke per package; cheapest converted read is GET /email/senders/categories. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
Baseline recomputed: 494 - 61 (notes) - 38 (whatsapp) - 98 (email) = 297. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
73 of the package's 79 get_db() sites — every user-identity request handler — now acquire sessions as 'async with _tenant_session() as db:' where _tenant_session IS acb_common.db.tenant_session (aliased in core.py, imported by every submodule by name, mirroring the Projects exemplar). The tenant comes from the request context bound centrally in _with_resolved_access; no call site passes one. The 6 sites that stay on get_db() are background consumers, each with an H4 comment naming why ambient inheritance is forbidden for them: broker_handlers.py (1, action-broker execute), scheduler.py (3, asyncio.create_task sync loops), calendar.py (2, the nightly rollover sweep — cross-tenant by construction). Restructures where a mid-block commit would have dropped the tenant GUC (never a naive swap): - commit-then-push handlers (patch_item, merge_into, archive, bulk- archive, organize, delegate) become TWO tenant blocks: the local edit commits on the first block's clean exit, the best-effort provider back-sync runs in a second — same "the edit is saved before upstream" contract as before. - _push_patch_upstream / _push_pending_item / the calendar pending-plan helpers no longer commit; the owning handler block does (which also makes the *-today apply atomic). - _refresh_schema no longer commits or calls _reconcile_people; handlers run both inside tenant blocks, the (unbound, H4) scheduler commits explicitly and sequences the reconcile itself. - accounts/settings handlers that must be COMMITTED before the scheduler re-reads on its own session (sync_enabled / background_sync toggles, put_day_state's echo) commit by block boundary, then act. - _sync_account / embed_pending_people keep their trailing commit and are documented as sole-occupant-of-a-block helpers (commit is their last statement, wrapper exit is an empty no-op). Tests: the tasks hermetic seams (test_tasks_people_scoping, test_people_write) patch an @asynccontextmanager _tenant_session with commit-on-clean-exit; test_tasks_gtd's commit-before-push source pin now asserts the two-block shape. H2_BASELINE_ELSEWHERE banked 494 → 421. Live scratch-Postgres smoke (Postgres 16, FORCE RLS on gtd_items/ gtd_waiting/gtd_projects/task_accounts, non-superuser role): unbound handler raises TenantUnbound; capture_item writes under the GUC with the row stamped by the bound org; the same read handlers return the row under org A and zero rows under org B; delete_item's tombstone persists via the wrapper commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
Baseline recomputed: 494 - 61 - 38 - 98 - 73 = 224. All four wave-1 slices now merged (email/notes/tasks/whatsapp). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
26 of 43 get_db() sites converted to `async with _tenant_session() as db:` (crud 7, publish 4, modules 6, runs 5, copilot 2, catalog 1, search 1) — every member-reached handler in the package. The 17 that remain are the unattended half, each H4-annotated in place: service.py (13 — run lifecycle, engine module loads, F13 programmatic entries; start_run/resume_run are dual-use and must not inherit an ambient tenant), scheduler.py (2 — the cron loop), triggers.py (1 — event sink), hooks.py (1 — hook_token service identity; tenant derives from the workflow row's organization). core.py gains the `_tenant_session` alias of the shared seam (gateway.db.tenant_session); submodules import it by name. update_workflow's mid-block commit is gone — its re-reads now see the same transaction's own writes. Hermetic fakes re-pointed to @asynccontextmanager patches with commit-on-clean-exit. H2_BASELINE_ELSEWHERE banked 494 → 468. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
Baseline recomputed: 224 - 26 = 198. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
32 of 38 get_db() sites in gateway/routes/apps now acquire sessions via `async with _tenant_session() as db:` — the package alias of acb_common.db.tenant_session, exposed from _common.py and imported by name per submodule (the seam test_db_engine_seam.py asserts identity on). Explicit commits are gone: the wrapper owns the one commit on clean exit, so patch_app's re-read moved inside the same transaction and sync_workspace_to_store / _remember_tool_grant no longer commit themselves (their callers' blocks do). Six sites stay on the unbound seam, each with an H4 marker naming the tenant source (the app row's organization): - actions.py (4): execute_app_action + _run_storage_* are dual-audience — the HTTP route AND the orchestrator's in-process agent tools (orchestrator/app_tools.py) call them with no request and no bound tenant; ambient inheritance there is what the H2 runbook forbids. - _common.py (1): record_app_audit is reached from that same agent path; under the ambient seam its except-all would silently drop audit rows. - tools.py (1): _apply_publish_review is a broker-invoked action handler running on proposal approval, outside the publishing request. Ratchet banked: H2_BASELINE_ELSEWHERE 494 -> 462, plus a per-file exact- count pin (H2_APPS_EXEMPT_SITES + test_routes_apps_is_converted_and_ stays_converted) so the six exemptions can neither grow nor silently linger once retired. Hermetic fakes re-pointed to asynccontextmanager patches with commit-on-clean-exit (test_app_grants / test_app_tools / test_app_runtime_activity); test_app_actions keeps its _get_db patches because actions.py deliberately stays. NOT verified against live Postgres in this environment (no DB available); the handover requires one live smoke per package — cheapest converted read: GET /apps/pins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
Baseline recomputed: 198 - 32 = 166; whatsapp and apps remainder pin tests both kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
The previous merge commit shipped with one stray '<<<<<<< HEAD' line in test_db_engine_seam.py (a piped test run masked the failure). Ratchet and apps suites green again (116 passed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
… to tenant_session (slice) 55 of the three packages' 58 get_db() sites now acquire their session through the tenant-bound seam (`async with _tenant_session() as db:`), with the seam aliased in each package's central module (crm/core.py, admin/_common.py, people/core.py) exactly as routes/projects did: - crm: activities (4), admin (8), deal_contacts (3), import_zoho (1), pipeline (2), records (5), reports (4), stage_metadata (1) — all member-reached request handlers (the CRM agent reaches them over HTTP with the acting member's identity, so the central binding covers it). Three files stay on the unbound seam BY DECISION, each annotated and named in H2_EXEMPT_FILES: auto_lead (background, email scheduler hook), sync_zoho (scheduled engine, service identity, per-phase commits), broker_handlers (approval-time handler; tenant belongs to the proposal payload, never the approver's ambient binding) — H4 owns all three. - people: directory (4) — all request handlers; core.py's dead `get_session` wrapper replaced by the shared-seam alias and the module added to the seam-identity parametrize list. - admin: members (8), roles (5), groups (6), access_requests (3), me (1) — since S1-1 admin resolves the CALLER's org from app_user, the same source the central bind_tenant reads; the explicit organization_id predicates stay as defense in depth. `_common.get_db` remains exported only for routes/agent_skills.py (not this package's to convert). Handlers' explicit commits are gone (the wrapper commits on clean exit); post-commit re-reads now read their own uncommitted writes in the same transaction. Hermetic fakes swap in via @asynccontextmanager patches with commit-on-clean-exit, so one-transaction contracts stay observable; `committed == 0` proxies on read-only successes became statement assertions, and the purge route's one-commit structural test now pins the `async with` shape instead of a literal db.commit(). Ratchets: H2_BASELINE_ELSEWHERE 494 → 439 (banked); the projects-only converted-package test is now parametrized over all four converted packages, with a stale-exemption check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
Ratchet composition: kept the whatsapp/apps exact-count pin tests AND the slice's generalisations (H2_CONVERTED_PACKAGES parametrized zero check now covering projects/crm/people/admin, plus the stale-exemption check). Baseline recomputed: 166 - 55 = 111. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
…remain Handover H2 box and WS-29 board row updated to the post-wave state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
… + mid-block commit; cross-file edge ratchet P0: auto_lead._create_lead calls the H2-converted create_record from the background mail path, where no ambient tenant exists — every message raised TenantUnbound, _consider counted it failed, and the watermark never advanced (auto-lead dead behind a WARNING). Fixed the H4 way, done early because it was a today-failure: the mailbox owner's organization is resolved from their app_user row on the caller's unbound session and bound explicitly around the create_record call, released after. P1: actions.py passed the enclosing tenant session to _store_ai_draft with the default commit=True — a mid-block commit that ends the transaction and drops the tenant GUC for everything after it (partial commits today; zero-row reads under RLS phase 4). Now commit=False; all three _apply_rule_actions callers hold wrapper sessions. New ratchet: test_exempt_files_do_not_reach_converted_sessions_unreviewed — no H4-exempt file may call a tenant_session-opening function without a reviewed edge in H2_EXEMPT_CALL_EDGES (AST calls, so marker comments can't false-positive). The exemption markers are file-scoped but the call graph is not; this is the mechanical catch for the P0 class, plus a stale-edge companion test. CRM fake taught the app_user lower()=lower() lookup arm; auto-lead tests seed the owner's app_user row. Full suite: 5807 passed. Both findings from the D29 adversarial diff review; neither was visible to any hermetic suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the request-handler half of H2 (
saas_multitenancy_handover.md), on top of #416's central binding + Projects slice. Seven parallel slice-agents across two waves, integrated with recomputed ratchets, followed by an adversarial diff review (D29 loop) whose two confirmed findings are fixed in the final commit.Converted (this PR)
execute_app_actionpath, broker publish-review, audit helperH2_BASELINE_ELSEWHEREbanked stepwise 494 → 111. Every remaining site carries an in-place# H4:(background consumer) or# H4/H6:(service-identity route) marker naming its tenant-derivation source — the classification IS the deliverable for H4/H6. Ratchets: zero-remainder packages pinned by the parametrizedtest_converted_packages_stay_converted(+H2_EXEMPT_FILESwith reasons); whatsapp/apps pinned at exact per-file counts; the elsewhere baseline only ratchets down with a bank-your-progress guard.The adversarial review's two catches (fixed here, both invisible to every hermetic suite)
process_new_mail→_create_lead) calls the now-convertedcreate_recordwith no ambient tenant →TenantUnboundon every message →_considercounts it failed → the watermark never advances. Fixed the H4 way, early: the mailbox owner's organization is resolved from theirapp_userrow and bound explicitly around the call. A new AST ratchet (test_exempt_files_do_not_reach_converted_sessions_unreviewed) now catches this whole class mechanically — the exemption markers are file-scoped but the call graph is not, so any exempt file calling atenant_session-opening function needs a reviewed edge inH2_EXEMPT_CALL_EDGES._store_ai_draft(db, …)defaultedcommit=Trueunder the tenant wrapper — ending the transaction and dropping the GUC for everything after it (partial commits today; zero-row tails under RLS phase 4). Nowcommit=Falseat the one call site whose callers all hold wrapper sessions.Conversion discipline (uniform across slices)
Every site classified BEFORE converting: user-identity request handler (convert — the central binding from #416 has the tenant) vs. service-identity route (
system:internalbinds nothing — leave) vs. background consumer (ambient inheritance forbidden by the runbook — leave). Explicitdb.commit()calls removed (the wrapper commits on clean exit; mid-block commits drop the GUC), with honest two-block restructures where local-commit-before-provider-sync ordering matters (tasks, email helpers, apps publish). Hermetic fakes re-pointed toasynccontextmanagerseams with commit-on-clean-exit;committed == 0"writes nothing" proxies replaced by statement/row assertions.Verification
TenantUnboundeverywhere.Not in this PR
🤖 Generated with Claude Code
https://claude.ai/code/session_01VmFScimSbeyHcLdut7RT4W
Generated by Claude Code