feat(auth): add owner-aware OAuth storage foundation - #1529
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces owner-aware builtin OAuth storage to support actor-owned credentials alongside ordinary user credentials. It adds a nullable resource_owner_key column to the user_oauth table, replaces the previous unique constraint with partial unique indexes, and updates services and API endpoints to enforce owner-scoped queries. Feedback on the changes highlights that replacing db.get with db.query().filter().first() in the new lookup functions get_scoped_user_oauth_account and get_user_oauth_account_by_id can lead to stale attribute reads, recommending the use of .populate_existing() to ensure attributes are properly refreshed.
Finalization SummaryPush
Resolved
RejectedNone. UnresolvedNone. Totals
|
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Summary
This PR (stack 1/4) adds UserOAuth.resource_owner_key (nullable) to distinguish ordinary user-owned OAuth credentials from a new "actor-owned" credential class, replacing the previous owner-blind uniqueness constraint with two partial unique indexes (NULL -> ordinary, NOT NULL -> actor-owned). It scopes every OAuth consumer call site to ordinary credentials, ships a 249-line hand-written Alembic migration with PostgreSQL/SQLite-specific DDL sequencing, and adds docs/deployment.md. The call-site sweep is genuinely complete -- no owner-blind runtime access sites remain.
This is a re-review. The prior review (gemini-code-assist bot) flagged two stale identity-map reads in get_scoped_user_oauth_account and get_user_oauth_account_by_id; both are verified fixed (commits 3e2bf5f91 and c8cb2a3af, each with a regression test that correctly fails without the fix).
This independent pass found 3 new major issues and 13 minor issues.
Blocking: yes -- recommended event: REQUEST_CHANGES
Design/approach note
Round 0 verdict: acceptable-with-reservations. The underlying problem (owner-blind uniqueness blocking a second, actor-owned credential class) is real, and the chosen shape -- a nullable resource_owner_key discriminator plus two partial unique indexes -- solves it correctly for existing data with no backfill required (provably safe, since the old constraint's row set is a subset of the new "ordinary" partial index's row set).
One design-level concern worth flagging even though it isn't blocking: this introduces a second, incompatible encoding of "resource_owner_key" in the codebase. MCPOAuthGrant/MCPOAuthFlowState (src/xagent/web/models/mcp_oauth.py, not touched by this diff) already use resource_owner_key NOT NULL with sentinel xagent:user:{id} for "ordinary" -- the opposite convention from this PR's NULL=ordinary. No current call site bridges the two tables, so this isn't a live bug, but it's a naming/convention trap for future work: normalize_user_oauth_resource_owner_key (src/xagent/web/services/user_oauth.py) has no reserved-prefix check, so a future actor key of xagent:user:5 would alias onto MCP's sentinel if the systems were ever bridged. Similarly, GmailWatchState.oauth_account (src/xagent/web/models/gmail_watch.py:44, also not touched by this diff) is an unfiltered relationship("UserOAuth") with no primaryjoin, unlike User.oauth_accounts which is correctly narrowed to resource_owner_key IS NULL -- currently unreachable (zero .oauth_account accesses anywhere) but a latent foot-gun if a future change reads through it.
Findings
Major
1. SQLite migration has an unrecoverable window with zero uniqueness protection
src/xagent/migrations/versions/20260818_add_user_oauth_resource_owner.py, batch_alter_table block (~L183-189), _create_owner_indexes() (~L199), re-run branch (~L166-171)
On SQLite, the batch rebuild adds the new column and drops the old unique constraint in one self-committing DDL step (SQLite DDL auto-commits; rollback does not undo it), while the new owner indexes are created in a later, separate step. A crash between the two steps (disk full, OOM-kill, kill -9 during the documented "quiesce SQLite" maintenance window) leaves user_oauth with the column added, the old constraint gone, the new indexes partial/missing, and alembic_version not bumped. Re-running alembic upgrade head then hits raise RuntimeError("owner-aware UserOAuth schema has incorrect indexes") with no automated repair -- the table has zero uniqueness enforcement until an operator manually intervenes, and docs/deployment.md has no SQLite recovery guidance for this state (only PostgreSQL's same-name-relation case is documented).
Suggested fix: document an operator recovery procedure in docs/deployment.md (inspect via PRAGMA index_list/sqlite_master.sql, manually re-run _create_owner_indexes()), and/or make the "incorrect indexes" branch self-healing.
2. Misleading test asserts the opposite of what its name claims
tests/alembic/test_20260818_add_user_oauth_resource_owner.py, test_postgresql_owner_index_creation_does_not_accept_existing_names (~L480-498)
The test patches migration._index_names to return "existing" names, then asserts _create_owner_indexes() creates all three indexes anyway. But _create_owner_indexes() never calls _index_names (only used in downgrade()) -- the patch is a no-op. The test actually proves the opposite of its name: there's no pre-flight name-collision check for PostgreSQL; it relies solely on the DB raising on CREATE INDEX for a real collision, triggering transactional rollback (which is what test_postgresql_owner_index_collision_allows_retry_after_remediation actually covers).
Suggested fix: rename this test and cross-reference the real safety-net test.
3. No test proves the PR's central security property: cross-user credential isolation
tests/web/services/test_user_oauth_ownership.py, tests/web/test_user_oauth_ordinary_consumers.py, tests/web/test_user_oauth_actor_ownership.py
Every test across this ~500-line new OAuth-ownership suite uses a single User row. The enforcement that matters most -- user B passing user A's account_id and being refused user A's credential -- is implemented in code (scoped_user_oauth_query's user_id filter) but never asserted by a test. The closest test, test_direct_id_lookup_requires_the_expected_owner, only varies resource_owner_key while holding user_id constant -- that's actor-namespace isolation within one user, not cross-user isolation. Not a live bug today (verified zero unscoped access sites exist currently), but a real regression-protection gap for the PR's own stated security goal.
Suggested fix: add an explicit two-user negative test -- create a credential for user A, call the scoped getter as user B with A's account_id, assert None/empty.
Minor
4. src/xagent/migrations/versions/20260818_add_user_oauth_resource_owner.py:171 -- RuntimeError("UserOAuth schema is partially owner-aware") is untested and not mentioned in docs/deployment.md, unlike the sibling "incorrect indexes" failure (which has a test) and the PG collision case (which has documented remediation). Suggest adding a test and a docs note.
5. tests/alembic/test_20260818_add_user_oauth_resource_owner.py (~L453-524) -- the "indexes created before old constraint dropped" ordering claim for PostgreSQL is asserted only against SimpleNamespace mocks with _create_owner_indexes patched out, not real DDL. The only real-PG test can't distinguish ordering (Alembic wraps the whole migration in one transaction). Not blocking; suggest a comment clarifying these tests verify call order, not real-DDL behavior.
6. tests/migrations/test_migration_integration.py (~L450) -- the prior full-chain (~141 revision) PostgreSQL idempotence test was replaced by a test that stamps past most of the chain, dropping the re-run/version-stability assertion. Justified only by an inline comment, consistent with an existing pre-established workaround pattern elsewhere in the same file, but untracked. Suggest filing a tracked issue referencing the incompatible early migration(s) (candidate: c7dfa28cc67a_add_user_oauth_table.py's conditional FK creation).
7. src/xagent/web/models/user_oauth.py:54 -- no CHECK constraint or @validates on resource_owner_key; normalize_user_oauth_resource_owner_key is applied only on reads, never before insert. No live writer of non-null keys ships in this PR, but a future writer bypassing the normalizer could persist an unreachable "" or treat 'k'/' k ' as distinct rows. Suggest routing future writers through the normalizer or adding a DB-level CHECK constraint.
8. src/xagent/web/services/user_oauth.py:105 -- get_user_oauth_account_by_id is the one lookup in this module that omits the user_id filter. Its single caller (gmail_provisioning.py:1006) already has AgentTrigger.user_id available at the call site. Not exploitable today (input is server-derived), but untested for cross-user safety. Suggest threading user_id through and replacing with get_scoped_user_oauth_account, or at minimum renaming to signal the danger and adding a cross-user regression test.
9. src/xagent/web/services/user_oauth.py:154 -- delete_scoped_user_oauth_accounts hardcodes synchronize_session=False, differing from the prior inline .delete() calls' default ('auto', which evicts matching in-session objects). No current caller pre-loads a matching row, so latent. Suggest a short code comment explaining why False is safe today.
10. src/xagent/web/services/user_oauth.py:57 -- scoped_user_oauth_query's docstring's second paragraph ("Keep the caller's SQL value unchanged. The legacy OAuth callback persists before it treats malformed state-claim coercion...") is unrelated to this function (a simple filtered query builder) and reads like a misplaced comment. Please remove it.
11. src/xagent/web/services/gmail_provisioning.py (~L1290-1305, ~L1380-1385) and src/xagent/web/services/gmail_triggers.py (~L549-553) -- when a scoped OAuth lookup returns None due to an owner mismatch (a new class of None introduced by this PR -- previously None almost exclusively meant "row deleted"), the code silently skips watch().stop() and deletes the local watch-state row, leaving a live server-side Gmail push subscription with no cleanup or operator visibility. Suggest adding logger.warning in these branches.
12. docs/deployment.md:84 -- (a) no remediation steps for the SQLite index-name-collision case, unlike the equivalent PostgreSQL case; (b) neither hard-failure message ("partially owner-aware", "incorrect indexes") is mentioned; (c) the claim that the actor partial-unique-index "reserves distinct actor-owned namespaces for later callers" overstates the guarantee -- the PR's own test suite proves two actor rows with the same resource_owner_key and NULL provider_user_id both persist (SQL NULL semantics). Suggest adding the missing remediation steps and caveating the NULL provider_user_id case.
13. tests/web/api/test_public_mcp_connector_visibility.py:412 -- test_remote_connector_ignores_actor_owned_oauth_accounts asserts only that a connectability-check hook was never invoked (checked_providers == []), never inspecting the response body (is_connected, connected_account). Sibling tests in this file do check the response body -- a regression leaking an actor-owned account's status into the response would pass here undetected. Suggest asserting the response body directly.
14. tests/web/test_user_oauth_actor_ownership.py:25 -- checks only SQLAlchemy model metadata (.unique is True, substring-matching _where(...)), not real DB enforcement via Base.metadata.create_all() -- the actual production schema-creation path. The only real IntegrityError-on-duplicate test runs against the migration-built schema instead. Suggest adding one test that builds the schema via create_all() and attempts a duplicate insert, asserting IntegrityError.
Simplification opportunities
src/xagent/migrations/versions/20260818_add_user_oauth_resource_owner.pyL170-171: yagni the "partially owner-aware" XOR guard (needs_column != has_old_constraint) -- unreachable in practice (SQLite batch-rebuild failure modes don't produce this exact XOR; PostgreSQL runs the whole revision transactionally), and untested, unlike the load-bearing SQLite-collision and idempotent-rerun guards elsewhere in this same migration (which have dedicated regression tests and directly detect/prevent the confirmed SQLite corruption window above). The subsequent code already appliesadd_column/drop_constraintindependently per-flag. Replacement: drop theraise RuntimeError("UserOAuth schema is partially owner-aware")branch and let the per-flag logic run unconditionally.
net: -3 lines possible
Blocking status & recommended decision
Blocking: yes
Recommended event: REQUEST_CHANGES
Blocking issues list:
- [new]
src/xagent/migrations/versions/20260818_add_user_oauth_resource_owner.py-- SQLite migration has an unrecoverable window with zero uniqueness protection - [new]
tests/alembic/test_20260818_add_user_oauth_resource_owner.py:480-- misleading test asserts the opposite of what its name claims - [new]
tests/web/services/test_user_oauth_ownership.py-- no test proves cross-user credential isolation, the PR's stated core security property
These are newly identified in this independent review pass; they are distinct from the two issues flagged in the prior review round.
Prior review findings -- resolved
Both issues from the prior review (gemini-code-assist bot) are verified fixed:
get_scoped_user_oauth_accountstale identity-map read -> fixed via.populate_existing(), commit3e2bf5f91, with a regression test that correctly fails without the fix.get_user_oauth_account_by_idstale identity-map read -> fixed via.populate_existing(), commitc8cb2a3af, with a regression test that correctly fails without the fix.
Finalization SummaryPush
Resolved
Rejected
UnresolvedNone. Totals
|
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Summary
This PR lays the schema and query-layer foundation for owner-aware OAuth storage: a nullable resource_owner_key discriminator column on UserOAuth, two partial unique indexes (one for the existing "ordinary" NULL-key rows, one for a future actor-owned key class) replacing the old single unique constraint, an Alembic migration to install this on both SQLite and PostgreSQL, and a centralized services/user_oauth.py query layer that all current call sites have been swept onto. It is PR 1 of a 4-PR stack; no code in this PR yet creates actor-owned rows.
Blocking: yes — recommended event: REQUEST_CHANGES
Re-review note: This is a second pass over the same PR. Disposition of prior-round findings: 9 fixed, 2 partial (still open, minor), 1 not-fixed (major, author's rebuttal independently re-verified and refuted — see below), 3 waived (author's rebuttal verified correct, dropped).
Round 0 design verdict (unchanged): Sound as a foundation commit — acceptable with reservations. The nullable resource_owner_key discriminator + two partial unique indexes correctly solve owner-blind uniqueness for a future actor-owned credential class, with zero behavioral change to existing data (the new "ordinary" partial index is provably equivalent to the old constraint on all-NULL current data). No backfill needed, downgrade correctly refused when actor rows exist, and the call-site sweep centralizing owner-scoping in services/user_oauth.py is genuinely complete (verified: zero owner-blind UserOAuth queries remain in production code). As stack 1/4, shipping unused index/query surface ahead of the PR-2 consumer is legitimate incremental design, not premature — confirmed by re-verification.
Findings
[MAJOR — prior, re-raised, rebuttal REFUTED] SQLite migration has an unrecoverable window with zero uniqueness protection.
On SQLite, the batch rebuild adds the new column and drops the OLD unique constraint in one DDL step, while the new owner partial-unique indexes are created in a LATER, separate step (src/xagent/migrations/versions/20260818_add_user_oauth_resource_owner.py, batch_alter_table block ~L183-189 vs _create_owner_indexes() ~L199). A crash between these two steps leaves the table with zero uniqueness enforcement and no automated repair; a re-run hits RuntimeError("owner-aware UserOAuth schema has incorrect indexes") requiring manual operator intervention, and docs/deployment.md has no SQLite recovery guidance for this specific state.
The author's reply argued this window doesn't exist because the migration runner wraps the upgrade in connection.begin()/context.begin_transaction(), and that Python's SQLite driver hasn't implicitly committed before DDL since Python 3.6. This was independently re-verified and found technically incorrect for this codebase's actual configuration: the SQLite engine (src/xagent/db/sqlite.py, src/xagent/web/models/database.py) has no isolation_level override and no begin-event hook, so SQLAlchemy's pysqlite dialect uses its default no-op do_begin() — transaction control is left to the sqlite3 driver's legacy mode, which auto-emits BEGIN only before DML, never before DDL (CREATE TABLE/ALTER TABLE/CREATE INDEX all autocommit in this mode). The outer connection.begin() calls the author cited do not actually wrap these DDL statements into one atomic unit on SQLite. The migration's own two-step structure confirms the split the original finding described.
Please either (a) document an operator recovery procedure for this specific SQLite partial-index state, or (b) make the "incorrect indexes" branch self-healing (idempotently drop+recreate the owner indexes), or (c) provide concrete evidence that pysqlite's isolation_level is actually configured to force real transactional DDL somewhere — the current reply's stated mechanism does not hold up under inspection of this codebase's actual SQLite engine configuration.
[MAJOR — new] PostgreSQL migration tests never execute in any CI job.
The PR description claims PostgreSQL owner migration tests (collision rollback/retry, metadata idempotence, cascade, downgrade) "passed without skips" — true only on the author's machine. .github/workflows/ci.yml (~L302, L458) runs migration tests with -m "not slow and not postgresql", deselecting everything marked postgresql. .github/workflows/test-migrations.yml is the only job with a live Postgres service, running -m postgresql against a hand-enumerated file allowlist (~L328-425) — neither tests/migrations/test_migration_integration.py nor tests/alembic/test_20260818_add_user_oauth_resource_owner.py appears in that allowlist. Compounding this, test_migration_integration.py's if __name__ == "__main__": block (~L598-656), which IS invoked by test-migrations.yml (~L296-317 via --db postgresql upgrade|idempotence|incremental|downgrade), only calls raw alembic.command.upgrade/downgrade/stamp directly — it never runs the actual pytest test methods (test_postgresql_upgrade, test_postgresql_owner_index_collision_allows_retry_after_remediation, test_postgresql_user_delete_cascades_actor_owned_oauth_rows, test_postgresql_owner_migration_downgrade_restores_legacy_schema, test_postgresql_owner_migration_accepts_current_metadata, test_postgresql_incremental_upgrade). Fix: add both test files to the -m postgresql pytest invocation in test-migrations.yml (the path-trigger gate at ~L11-49 already covers these paths — only the pytest step enumeration is missing).
[MAJOR — new] ensure_gmail_mailbox_provisioned reads an expired ORM attribute after a commit, violating its own concurrency-lock contract.
src/xagent/web/services/gmail_provisioning.py:896. _gmail_watch_transition_lock calls db.commit() at line 122 on the PostgreSQL path, and its docstring (~L100-104) explicitly documents avoiding "checking out a second pooled connection during remote API calls" specifically because of this commit. The new code evaluates user_id=int(oauth_account.user_id) at line 896, inside the with block, after that commit. The session (sessionmaker(autocommit=False, autoflush=False, bind=_engine), web/models/database.py:206, no expire_on_commit=False) defaults to expire_on_commit=True, so this access triggers a lazy-load — checking out a second pooled connection while the advisory lock connection is held, exactly what the docstring warns against. Under small pool sizes or long-held locks during remote Gmail/Pub-Sub API calls, this risks pool exhaustion; if the row was concurrently deleted it raises ObjectDeletedError instead of the GmailProvisioningError the caller contract implies. Fix: capture user_id before entering the lock, alongside the already-correctly-pre-captured oauth_account_id at line 889.
[MAJOR — new] docs/deployment.md's PostgreSQL rollout procedure doesn't warn that an old worker restarting mid-window will hard-crash.
Doc (~L113-115) says "Existing workers can continue non-OAuth work while the transactional DDL runs," implying the window is safe for existing workers. But _check_revision_is_known (src/xagent/db/migration.py ~L318-341) raises RuntimeError when alembic_version holds a revision missing from a worker's local versions/ directory, and this propagates uncaught through try_upgrade_db (~L411-413) on every process/worker startup (_initialize_database_schema, web/models/database.py:227). Any old (pre-migration) worker that restarts during the window — crash, OOM-kill, k8s eviction, autoscaler event — will fail startup hard. Please add this caveat to the deployment doc.
Minor (carried-forward partials + new)
- [PARTIAL, prior]
f207b9b70's newtest_create_all_enforces_owner_aware_uniqueness(tests/web/test_user_oauth_actor_ownership.py:95-126) is a real improvement, but verified (by removing thesqlite_where/postgresql_wherepredicate and re-running) that it only inserts identical duplicate rows, which fail under full uniqueness too — it never exercises the case that actually distinguishes partial from full uniqueness: an ordinary row and an actor row sharing(user_id, provider, provider_user_id)coexisting. Suggest adding that case. - [PARTIAL, prior]
f65ebb4e6addedlogger.warningat 3 of 4 owner-mismatch lookup-returns-None sites but missed_provision_in_fresh_session(src/xagent/web/services/gmail_provisioning.py:1006-1012), which still does a barereturnwith no logging. Confirmed downstream effect:provision_gmail_triggersees noGmailWatchStatecreated,state is None, and reports the trigger asPENDING/error=Noneindefinitely for actor-owned accounts, with zero log signal. Please add the same warning here. - Two related documentation-accuracy gaps: (a) "transactional PostgreSQL retry" overstates what shipped —
transaction_per_migration=is_postgresql(migrations/env.py:179) predates this PR; what's new is a fail-closed guard making a manual re-run safe, not automatic retry. (b) "Quiescent SQLite migration" has no code enforcement beyond asqlite_mastername-collision precheck; quiescence is purely an operator instruction. (c) The doc's "short OAuth-write pause" claim undersells the PostgreSQL lock:ADD COLUMN+ 3 non-concurrent index builds + constraint drop run in one transaction, and the ACCESS EXCLUSIVE lock fromADD COLUMNblocks reads too for the full duration. docs/deployment.md's verification query checksindisvalid(~L134-151), but since the migration never usesCREATE INDEX CONCURRENTLY(the doc itself confirms this at ~L86),indisvalidis always true for any successfully created index — a vacuous check that could mislead an operator.docs/deployment.md(~L104-107) recommends the standalonealembic upgrade headCLI for SQLite, but that path lacks the FK-off guard + FK-violation-delta check the app-startup path provides (src/xagent/db/migration.py:252-298) — relevant since the rebuild affectsgmail_watch_states.oauth_account_id'sON DELETE CASCADE. Works today only by accident (NullPool defaults to FK-off;env.py's pragma call lands on a different connection). Recommend documenting the asymmetry or preferring the app-startup path.ix_user_oauth_owner_provider(new non-partial, non-unique index) has no current reader not already served byuq_user_oauth_ordinary_account, and being non-partial it indexes every row, adding migration-time DDL cost for no current benefit. Recommend deferring to the PR that introduces actual actor-scoped reads.- "Keep actor-owned credential creation disabled" reads as an operational switch, but there is no feature flag/env var/config toggle — the only enforcement is two usage-time rejections on already-loaded rows (
gmail_provisioning.py:885,gmail_triggers.py:195). Accurate today, but worth rewording so it doesn't imply a toggle. - Inconsistent error-handling convention for the same "actor cannot do X" rule:
services/user_oauth.pyexpresses it via filtering (returnsNone/empty), whilegmail_provisioning.py:885-887andgmail_triggers.py:194-198raise exceptions for the identical condition. Not a live bug (no shared callers), but worth documenting the intended convention. - Actor-owned rows now depend on
PRAGMA foreign_keys=ONsucceeding (best-effort,src/xagent/db/sqlite.py:99-102) for SQLite cleanup on user deletion, since the ORMdelete-orphancascade is now scoped toresource_owner_key IS NULL(web/models/user.py:66-73). Previously the unrestricted cascade guaranteed cleanup regardless of pragma state; a silent pragma failure could now leak actor-owned rows past user deletion. Worth a comment noting the dependency, orpassive_deletes=True+ an explicit unfiltered cleanup path. USER_OAUTH_RESOURCE_OWNER_KEY_MAX_LENGTH = 512(web/models/user_oauth.py:7) andOWNER_LENGTH = 512in the migration (~L42) are duplicated with no cross-check test — a future change to one without the other would silently desync model and migration._resolve_gmail_resource(src/xagent/web/services/triggers.py~L597-609) lost a defensiveint()coercion when replaced with the new scoped-query helper. Low real-world risk (user_idis type-hintedintend-to-end from an authenticated session), but a minor defense-in-depth regression.- The SQLite index-name-collision preflight (migration ~L130-142) queries
sqlite_master WHERE type = 'index'only; a pre-existing TABLE or VIEW with a colliding name would slip past it and surface as a less-clear rawCREATE INDEXfailure. Confirmed the migration still rolls back cleanly (no data-loss risk) — a diagnostics-quality gap, not correctness. _normalize_index_predicate(migration ~L98-104) strips matching leading/trailing parens in a loop; for a compound predicate like"(a) and (b)"this would corrupt it to"a) and (b". Currently unreachable (both real predicates are simple single terms), but latent if a future compound predicate is added.- (Restating a non-blocking design aside from the prior round, still unaddressed)
UserOAuth.resource_owner_keyuses NULL-means-ordinary, while the pre-existing, unrelatedMCPOAuthGrant/MCPOAuthFlowState.resource_owner_keyuses the opposite convention (NOT NULL, sentinelxagent:user:{id}). No current call site bridges the two tables, but it's a naming/convention trap for future work.
Test quality (new)
- No test on real PostgreSQL DDL asserts the
postgresql_where/uniquepredicate correctness of the three owner-aware indexes —test_create_owner_indexes_attempts_all_postgresql_indexes(tests/alembic/..., ~L505-521) discards those kwargs and only checks index names were attempted; the real-PostgreSQL integration tests only check name presence/absence, never insert rows to prove the boundary is enforced. A regression droppingpostgresql_where=entirely would pass every existing test — compounds with the CI-unenforced Major above. test_upgrade_preserves_nullable_provider_identity_semantics(tests/alembic/..., ~L251-275) asserts onlySELECT count(*) == 4; its name claims nullable-identity-semantics coverage, but the siblingtest_upgrade_preserves_rows_and_installs_owner_aware_identity(~L200-221) actually covers that. Misleading name, not a live coverage gap. Suggest renaming or consolidating.test_none_provider_filter_deletes_all_for_owner_without_committing(tests/web/services/test_user_oauth_ownership.py, ~L222-256) opens a second session on the same file-backed SQLite DB while the first holds an uncommitted DELETE, with nobusy_timeout/WAL pragma configured (unlike sibling concurrency-sensitive test files). Latent flake risk under parallel test execution, not observed to fail. Preventive hardening suggested.- (Optional, very minor)
tests/web/test_user_oauth_ordinary_consumers.py:87'sassert db.get(UserOAuth, int(actor.id)) is not Nonehits the session identity map rather than re-querying the DB, since the code path under test returns 404 before any delete call — confirms "no side effects on early-return" only, limited value.
Simplification opportunities
services/user_oauth.pycentralizes predicate-building, but theresource_owner_key=Nonesentinel is still spelled out literally at 21 call sites across 8 files (auth.py,cloud_storage.py,mcp.py,gmail_provisioning.py,gmail_triggers.py,triggers.py,tools/config.py, plus the module itself). Nothing makes the safe/ordinary choice the default or the unsafe choice hard —db.query(UserOAuth)remains legal anywhere with no lint enforcement. Suggest a thinordinary_user_oauth_query(db, *, user_id)wrapper (and equivalents for the other helpers) so ordinary callers never type the sentinel. Verified as trivial given existing kwarg signatures — a real, low-effort improvement.ix_user_oauth_owner_providershould be deferred to the PR introducing actual actor-scoped reads (see Minor finding above) — it adds migration-time DDL cost with no current reader, and its column order can be chosen against real query shapes once one exists.- Two items flagged by an automated simplification-lens pass — "the new
uq_user_oauth_actor_accountunique index" and "the entire newservices/user_oauth.pymodule" being unused — were reviewed and DROPPED as inapplicable: this is PR 1 of an explicit 4-PR stack, and shipping schema/interface ahead of the PR-2 consumer is deliberate, correct foundation-laying, not dead flexibility.
net: mostly inapplicable to a foundation PR; one real opportunity (ordinary_user_oauth_query wrapper) and one index deferral, neither large enough to quantify as a meaningful line-count reduction.
Blocking status & recommended decision
Blocking: yes
Recommended event: REQUEST_CHANGES
Blocking issues:
- [prior] SQLite migration has an unrecoverable zero-uniqueness window between dropping the old constraint and creating the new owner indexes. Previously raised; author's rebuttal (transactional DDL via
connection.begin()) was independently re-verified against this codebase's actual SQLite engine configuration and found incorrect — pysqlite's default no-opdo_begin()means DDL autocommits regardless of the outerbegin()call. Needs a documented recovery procedure, a self-healing repair path, or concrete evidence closing the gap. - [new] PostgreSQL migration tests (collision/rollback, cascade, downgrade, idempotence) never actually run in CI — the CI job excludes
postgresql-marked tests, and the one job with live Postgres never invokes the pytest methods, only raw Alembic commands. The PR's central verification claim is currently CI-unenforced. - [new]
ensure_gmail_mailbox_provisionedreadsoauth_account.user_idafter a commit inside the advisory-lock block, triggering a lazy-load that checks out a second pooled connection — violating the documented contract of_gmail_watch_transition_lockand risking pool exhaustion or an unexpectedObjectDeletedError. - [new]
docs/deployment.md's PostgreSQL rollout guidance omits that an old worker restarting mid-migration-window will hard-crash on startup due to_check_revision_is_known, contradicting its own "existing workers can continue" framing.
|
|
||
| def _normalize_index_predicate(predicate: object | None) -> str | None: | ||
| if predicate is None: | ||
| return None |
There was a problem hiding this comment.
[Minor, new] _normalize_index_predicate strips matching leading/trailing parens in a loop; for a compound predicate like "(a) and (b)" this would incorrectly corrupt it to "a) and (b". Currently unreachable since both real predicates are simple single terms, but latent if a future compound predicate is added to OWNER_INDEX_DEFINITIONS.
There was a problem hiding this comment.
We are not making this change because the private helper only compares the two fixed single-term predicates in this revision; no compound predicate is accepted (predicate definitions).
Generalizing normalization for hypothetical compound predicates would add speculative code without correcting current behavior (predicate definitions).
| ) | ||
|
|
||
|
|
||
| def test_upgrade_preserves_nullable_provider_identity_semantics(tmp_path) -> None: |
There was a problem hiding this comment.
[Test quality, new] This test's name claims to verify nullable-identity-semantics preservation, but it only asserts SELECT count(*) == 4 — the actual identity-semantics coverage lives in the sibling test_upgrade_preserves_rows_and_installs_owner_aware_identity (~L200-221). Not a live coverage gap, but suggest renaming or consolidating to avoid the misleading name.
There was a problem hiding this comment.
We are not making this change because successful insertion of four otherwise-colliding rows with null provider identities is itself the semantics assertion, and the count proves those rows persisted (nullable identity test).
The existing name accurately describes the behavior exercised by the insert-and-count sequence (nullable identity test).
|
|
||
| assert account is ordinary_row | ||
| assert account.access_token == "ordinary-refreshed" | ||
| finally: |
There was a problem hiding this comment.
[Test quality, new] This test opens a second session on the same file-backed SQLite DB while the first holds an uncommitted DELETE, with no busy_timeout/WAL pragma configured (unlike sibling concurrency-sensitive test files in this repo). Latent flake risk under parallel test execution; not observed to fail in practice, but preventive hardening is suggested.
There was a problem hiding this comment.
We are not making this change because the second session performs only a read while the first session holds the uncommitted delete (concurrency test).
SQLite permits readers while a writer holds a RESERVED lock, so the described lock state does not establish the proposed flake mechanism (SQLite locking documentation).
Finalization SummaryPush
Resolved
Rejected
UnresolvedNone. Totals
|
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Summary
This PR (stack 1/4) lays the schema and query-layer foundation for owner-aware OAuth storage: a nullable resource_owner_key discriminator column on UserOAuth, two partial unique indexes (NULL = existing "ordinary" user credential, NOT NULL = a future actor-owned credential class) replacing the old owner-blind unique constraint, an Alembic migration installing this on both PostgreSQL and SQLite, and a centralized services/user_oauth.py query layer that every current OAuth consumer has been swept onto. No code in this PR creates actor-owned rows yet — that ships in PR 2+.
Blocking: yes — recommended event: REQUEST_CHANGES
Re-review note: This is round 3 of review on this PR. Disposition of prior-round findings, independently re-verified against the current code (not just the author's claim): 9 fixed, 3 unchanged and still WAIVED (already independently verified and accepted in an earlier round — no CHECK constraint needed on resource_owner_key since no reachable non-null writer exists in this PR; no user_id threading needed on get_user_oauth_account_by_id since its sole caller's account_id is already ownership-validated upstream; the PostgreSQL full-chain idempotence coverage gap is intentionally tracked in open issue #1534, out of scope for this PR).
Fixed and re-verified this round:
- SQLite migration's unrecoverable zero-uniqueness window now has a concrete, actionable operator recovery procedure in
docs/deployment.md(mandatory pre-rebuild backup plus exact restore/verify steps for the specific crash state). - PostgreSQL migration tests now actually execute in CI against a live Postgres service (
.github/workflows/test-migrations.ymlgained a-m postgresqlpytest step), closing the previously CI-unenforced verification claim. ensure_gmail_mailbox_provisioned's lazy-load-after-commit bug is fixed —user_idis now captured before entering the advisory-lock block.docs/deployment.mdnow explicitly warns that an old worker restarting mid-migration-window will fail startup, with a concrete mitigation.test_create_all_enforces_owner_aware_uniquenessnow inserts an ordinary row and an actor row sharing the same(user_id, provider, provider_user_id)and asserts both persist — genuinely proves partial-index semantics rather than just duplicate rejection._provision_in_fresh_session's missinglogger.warningon owner mismatch was added, matching the other 3 call sites, with a regression test.- The vacuous
indisvalid-only PostgreSQL index-verification query was replaced with one that actually checksindisunique/predicate/definition text. - The FK-off-guard asymmetry between the app-startup and standalone-CLI SQLite migration paths is now documented with a concrete mitigation (
PRAGMA foreign_key_checkbefore/after). - The SQLite name-collision preflight now checks TABLE/VIEW collisions too, not just indexes (with a regression test); the unused
ix_user_oauth_owner_providerindex was removed entirely (deferred to the PR that will actually read it) with no vestigial test assertions left behind; and a real-PostgreSQL test now asserts the actualpostgresql_where/uniquepredicate content of the owner-aware indexes, plus a new integration test provingIntegrityErrorenforcement on real Postgres DDL.
Round 0 design verdict: acceptable-with-reservations. This remains a sound foundation commit. The call-site sweep to ordinary-scoped queries is verified complete (zero owner-blind UserOAuth queries remain in production code). A comparison to the repo's own MCPOAuthGrant precedent (NOT-NULL synthetic owner key vs. this PR's nullable discriminator) was investigated as a possible simpler alternative; the core observation is correct — a synthetic key would eliminate the dialect gate and the predicate-normalization/idempotence-check machinery — but the alternative is not as clearly superior as it first appears (SQLite's batch-rebuild requirement and the name-collision preflight would remain regardless of key nullability, and a proposed relationship-primaryjoin simplification for it does not actually hold up), and it carries its own comparable backfill/consumer-migration cost. This is a non-blocking design note, not a redesign requirement — worth a code comment pointing at the precedent for future readers.
Findings
[CRITICAL — new] The documented rollback procedure bricks every worker on restart (multi-head alembic_version).
docs/deployment.md:180 says: "Stop workers, run alembic downgrade 20260818_seed_jira_mcp_app, and deploy the old version." This target revision is reachable from HEAD via two independent branches in the revision graph — it is an ancestor of both 20260819_merge_jira_and_linear_heads (src/xagent/migrations/versions/20260819_merge_jira_and_linear_heads.py, down_revision = ("20260818_seed_jira_mcp_app", "20260818_seed_linear_mcp_app")) and directly of 20260818_user_oauth_resource_owner (src/xagent/migrations/versions/20260818_add_user_oauth_resource_owner.py:36, down_revision = "20260818_seed_jira_mcp_app"), both of which feed into the merge revision 20260821_merge_oauth_owner_and_mcp_heads.py:16-18.
Empirically reproduced end-to-end against a real Alembic/SQLite chain: running the exact documented command splits alembic_version into two rows (20260818_seed_jira_mcp_app, 20260818_seed_linear_mcp_app), and xagent.db.migration.get_alembic_revision() (src/xagent/db/migration.py:307) then raises CommandError: Version table 'alembic_version' has more than one head present — meaning every worker fails to start after anyone follows this exact documented procedure. There is also no alternative single-revision downgrade target that both removes resource_owner_key and leaves a single head: downgrading instead to 20260819_merge_jira_and_linear_heads is also multi-head ({20260819_merge_jira_and_linear_heads, 20260818_user_oauth_resource_owner}) and leaves the resource_owner_key column in place (verified via PRAGMA table_info).
Fix: the rollback procedure needs a downgrade path that correctly un-merges to a single head (e.g., downgrading each branch explicitly before the merge point, or restructuring the merge revision), verified against a real Alembic run before this ships.
[MAJOR — new] User.oauth_accounts's primaryjoin is asymmetric with UserOAuth.user's back_populates, allowing an actor-owned row into the "ordinary-only" in-memory collection.
src/xagent/web/models/user.py:68-75 scopes User.oauth_accounts to resource_owner_key IS NULL via a restrictive primaryjoin, with cascade="all, delete-orphan". src/xagent/web/models/user_oauth.py:65 defines the reverse side, UserOAuth.user = relationship("User", back_populates="oauth_accounts"), with no such filter.
Empirically reproduced (SQLAlchemy 2.0.51, in-memory SQLite, the actual model classes): assigning actor_row.user = some_user on a transient actor-owned row (resource_owner_key set) causes SQLAlchemy's Python-side backref sync to append that row into some_user.oauth_accounts in memory — the IS NULL predicate is only consulted for the SQL SELECT that would normally populate the collection, never for this in-memory backref append. Combined with cascade="all, delete-orphan", a future code path that assigns .user on an actor-owned row and later removes it from the collection could trigger an unintended ORM-level delete of a row meant to be protected.
Verified: no code in this PR today performs .user = assignment on an actor-owned row (all actor/ordinary access goes through the query/filter helpers in src/xagent/web/services/user_oauth.py), so this is not exploitable yet. It is, however, a landmine for the actor-owned writer this foundation PR is explicitly being built to support in PR 2+.
Fix: give UserOAuth.user's relationship the same resource_owner_key IS NULL primaryjoin restriction (or otherwise make the two sides symmetric) before any writer starts touching .user on actor-owned rows.
[MAJOR — new] Three Gmail service functions silently no-op on legacy rows where watch.user_id != account.user_id, with no automated detection beyond a manual runbook query.
docs/deployment.md already acknowledges such legacy-mismatch rows can exist and gives operators a one-time manual SQL query to check for them before rollout — but none of the three affected functions escalate at runtime beyond a log line:
release_gmail_mailbox_if_unused(src/xagent/web/services/gmail_provisioning.py:1256-1342): on mismatch, skips the remoteservice.users().stop()call but still deletes the local tracking row and returnsTrue— permanently orphaning a live server-side Gmail watch with no local record left to ever find it again.tests/web/services/test_gmail_provisioning.py::test_release_warns_when_watch_account_is_not_ordinaryliterally assertsreleased is Truefor this case, codifying the leak as expected behavior rather than flagging it as one.collect_gmail_pubsub_events(src/xagent/web/services/gmail_triggers.py:524-563): on mismatch, drops all incoming Gmail push events for that mailbox forever, with only a warning log.sweep_gmail_provisioning(src/xagent/web/services/gmail_provisioning.py:1345-1420): on mismatch, the mailbox is never reconciled — and since the mismatch condition persists indefinitely, every future sweep run hits the identical skip.
Fix: at minimum, emit a metric or alert (not just a log line) on these three paths so an operator learns about orphaned watches or dropped events without having to proactively run the manual pre-flight SQL query, and reconsider whether release_gmail_mailbox_if_unused should still delete local tracking state when the remote stop() call could not even be attempted.
Minor (new)
src/xagent/web/models/user.py's actor-owned-row cleanup on user deletion depends entirely on the databaseON DELETE CASCADE, which on SQLite requiresPRAGMA foreign_keys=ON— set best-effort (logged, not fail-closed) only for engines built throughsrc/xagent/db/sqlite.py's factory. The production admin user-delete path does go through that factory correctly, and no production writer for actor-owned rows exists yet, so this is not currently exploitable — but the one test written to justify this design (tests/web/test_user_oauth_actor_ownership.py::test_sqlite_user_delete_cascades_hidden_actor_oauth_rows) registers its own ad hoc FK-pragma listener rather than exercising the realconfigure_db()engine path, so it proves nothing about production wiring. Recommend a follow-up test against the actual production engine factory before actor-owned row creation is enabled.src/xagent/web/models/gmail_watch.py:44'sGmailWatchState.oauth_account = relationship("UserOAuth")has no owner-scopingprimaryjoin, unlike every otherUserOAuthaccess path in this PR (verified: zero owner-blindUserOAuthqueries remain anywhere else insrc/, and this relationship attribute has zero current callers). Nothing structurally prevents this from becoming the next owner-blind door once it acquires a caller. Recommend a guard test (e.g., assertingquery(UserOAuth)/select(UserOAuth)only appears inservices/user_oauth.py) or scoping this relationship's primaryjoin the same wayUser.oauth_accountsis scoped.docs/deployment.md:92's documented mitigation for the standalonealembic upgrade headSQLite path — recordingPRAGMA foreign_key_checkbefore/after — cannot actually catch the failure mode it's meant to guard against. Empirically confirmed: an FK-enabled batch rebuild of a parent table silently cascade-deletes child rows viaON DELETE CASCADE(relevant here becausegmail_watch_states.oauth_account_idhasondelete="CASCADE"ontouser_oauth), andPRAGMA foreign_key_checkreports no violations before or after, since a cascade-delete produces no violation to detect. Tracing the standalone CLI path's engine construction (src/xagent/migrations/env.py) shows it never explicitly enables SQLite foreign keys, so this likely isn't reachable via the exact documented invocation today, but the documented check itself is not a valid safety net for this specific risk. Recommend replacing/supplementing it with a row-count snapshot ofgmail_watch_states(and any other tables with an FK ontouser_oauth) before and after the migration.
Simplification opportunities
delete_scoped_user_oauth_accounts'sproviders=Nonebranch (src/xagent/web/services/user_oauth.py:124-151, meaning "delete every credential in the owner namespace") has zero production callers — every real call site (auth.py,mcp.py) passes an explicit non-empty list, some behind anif providers_to_delete:guard. Since the falsy value (None) is the destructive one, this is an inverted-sentinel shape that invites a futureproviders=filtered or Nonemistake, but it's not a live risk today. Recommend dropping theNone/delete-all case and requiring aSequence[str], reintroducing an explicit delete-all only if a real caller ever needs one.get_user_oauth_account_by_id(src/xagent/web/services/user_oauth.py:100-121) duplicatesget_scoped_user_oauth_account's query/filter/populate_existing()structure, differing only in omitting theuser_idfilter, and has exactly one production caller (gmail_provisioning.py:1007). A prior review exchange already justified why omitting theuser_idfilter is safe here, but did not address the duplication itself. Recommend makinguser_idoptional onget_scoped_user_oauth_accountand dropping the second function.- (Design note, not a line-level change) Consider a short code comment on
UserOAuth.resource_owner_keycross-referencingMCPOAuthGrant's NOT-NULL synthetic-owner-key convention, so a future reader understands this table deliberately chose a different (nullable-discriminator) convention for the same concept rather than assuming an oversight.
net: two small opportunities (a few lines each) and one design note; not large enough to quantify as a meaningful line-count reduction.
Blocking status & recommended decision
Blocking: yes
Recommended event: REQUEST_CHANGES
Blocking issues:
- [new]
docs/deployment.md:180's documented rollback command splitsalembic_versioninto two heads, and every worker fails to start afterward — empirically reproduced, with no valid single-revision alternative that both reverts the schema and leaves a single head. - [new]
src/xagent/web/models/user.py:68-75vs.src/xagent/web/models/user_oauth.py:65— mismatchedprimaryjoin/back_populateslets an actor-owned row enter the ordinary-only, delete-orphan-cascading collection in memory; latent today, but a landmine for the actor-owned writer this PR is explicitly foundation for. - [new]
release_gmail_mailbox_if_unused,collect_gmail_pubsub_events, andsweep_gmail_provisioning(gmail_provisioning.py,gmail_triggers.py) all silently no-op on a legacy mismatched-owner state the deployment doc already acknowledges can exist, with no automated detection beyond a manual pre-flight query.
| ) | ||
|
|
||
|
|
||
| def get_user_oauth_account_by_id( |
There was a problem hiding this comment.
[Simplification] This duplicates get_scoped_user_oauth_account's query/filter/populate_existing() structure, differing only in omitting the user_id filter, and has exactly one production caller (gmail_provisioning.py:1007). A prior review exchange already justified why omitting the user_id filter is safe here, but didn't address the duplication itself. Consider making user_id optional on get_scoped_user_oauth_account and dropping this function.
There was a problem hiding this comment.
We are not making this change because the two short helpers intentionally expose different trust contracts: one requires a user boundary, while the Gmail foreign-key reload uses a globally unique server-derived key after upstream ownership validation (scoped and foreign-key lookup contracts; validated Gmail worker path).
Making user_id optional on the scoped API would hide that distinction and weaken the safer required-argument contract for negligible reduction. This matches the earlier review disposition (prior review disposition).
Finalization SummaryPush
Resolved
Rejected
UnresolvedNone. Totals
Additional fixes included in the pushed headThese seven post-resolution fixes were found during independent acceptance review. They are not additional GitHub-comment findings and do not change the totals above.
Local verification for the complete pushed head: 213 tests passed; 4 PostgreSQL-dependent tests were skipped because |
3 participants
Summary
UserOAuth.resource_owner_keyVerification
20260821_merge_oauth_owner_and_mcp_headsRollout
Do not deploy a writer that creates actor-owned credentials in this foundation release. Follow
docs/deployment.mdfor the database-specific migration procedure.