feat(auth): add owner-aware OAuth storage foundation - #1588
Conversation
Finalization SummaryPush
Carry-over auditI also re-audited the accepted PR #1529 findings against this split PR and its stack:
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 service foundation for owner-aware builtin OAuth credentials: UserOAuth gains a nullable resource_owner_key column, the owner-blind uq_user_provider_account constraint is replaced by two partial unique indexes (uq_user_oauth_ordinary_account on resource_owner_key IS NULL, uq_user_oauth_actor_account on IS NOT NULL), a user_id -> users.id ON DELETE CASCADE FK is backfilled, and a new src/xagent/web/services/user_oauth.py centralizes owner-scoped read/delete of OAuth rows. Non-Gmail consumers (api/auth.py, api/cloud_storage.py, api/mcp.py, tools/config.py) are converted to the scoped helpers, and the migration carries substantial SQLite batch-rebuild repair machinery plus a dialect allowlist shared with app startup.
Deliberately, no production code writes an actor-owned row yet — verified: all 13 resource_owner_key= sites in src/ pass literal None. Gmail's full ownership boundary is deferred to stacked PR #82. The motivation is a multi-PR stack toward actor-owned / delegated OAuth credentials, where an actor-owned row shares the same user_id as its owning user and is distinguished only by resource_owner_key.
Approach verdict: acceptable-with-reservations
The schema/migration work is careful and well-tested, and owner-scoping is cohesive — one service module used at every converted call site. Three reservations:
- Filtered relationship trades a real guarantee for hiding invisible rows.
User.oauth_accountsisprimaryjoin-filtered to ordinary rows, which buys nothing today (nothing reads actor rows) at the cost of dropping them out of the ORM cascade. - Nullable owner key diverges from this codebase's own convention (
MCPOAuthGrant/MCPOAuthFlowStateuse NOT NULL + synthetic sentinel), and the divergence carries a long complexity chain as its price. - The boundary to PR #82 is enforced by review convention and prose only, not by code. Finding M3 below is direct proof that convention-only enforcement has already missed a site inside this very PR.
Major findings (blocking)
M1. The documented rollback procedure silently deletes an unrelated Stripe MCP catalog row
docs/deployment.md:179 instructs alembic downgrade b1efe0dbe0af. But src/xagent/migrations/versions/0108d2704fc1_merge_stripe_and_oauth_owner_heads.py:13-16 merges two independent branches off that same parent: this PR's 20260818_user_oauth_resource_owner and an unrelated 20260818_seed_stripe_mcp_app (pulled in by an upstream-main merge during this PR's life). Downgrading straight to b1efe0dbe0af necessarily unwinds both branches, and 20260818_seed_stripe_mcp_app.py:86-98 executes DELETE FROM public_mcp_apps WHERE app_id='stripe'.
So the documented rollback removes the Stripe catalog entry and orphans MCPServer / UserMCPServer rows for users who already connected Stripe — with zero mention in the doc and zero test coverage. tests/migrations/test_migration_integration.py:484-493 runs this exact downgrade and never asserts anything about public_mcp_apps.
Please scope the documented rollback to unwind only the OAuth-owner branch (e.g. stepwise alembic downgrade -1 through 0108d2704fc1 then the OAuth-owner revision), or explicitly document and accept the Stripe-seed revert if that is genuinely intended — plus a test asserting the Stripe row's fate across this downgrade path.
M2. FK-cascade gap on a from-empty pure-Alembic bootstrap is only partially fixed, and the one test that exercised the scenario was changed to stop exercising it
This was raised in round 1, rejected on the basis of a test that did not cover it, then reproduced directly in round 2 (empty SQLite DB → alembic upgrade head → user_oauth created with no FK at all, because c7dfa28cc67a_add_user_oauth_table.py:36 only adds the FK when users already exists) and reopened as blocking. Commits df272eb1 + 140e22ef claim a fix.
Current state, verified: the fix converts silent corruption into a hard failure — src/xagent/migrations/versions/20260818_add_user_oauth_resource_owner.py:226-230 now raises RuntimeError when users is absent. That is a genuine improvement, but it does not guarantee the FK ends up present. A from-empty pure-Alembic run now aborts mid-chain, leaving a half-migrated database that needs manual operator recovery.
The larger concern is the test change. test_empty_database_alembic_upgrade_to_head_completes was renamed in the final commit f0e712f2 to test_database_with_core_users_table_alembic_upgrade_to_head_completes and altered to pre-create users before the upgrade (tests/alembic/test_20260706_add_connector_runtime_context.py:160-173) — so it no longer covers the empty-database path and does not assert the new fail-closed behavior. The same pattern appears on the CLI-invocation path the round-2 reviewer used to reproduce the bug: tests/migrations/test_migration_integration.py:488 now calls create_metadata_owned_users_table() before upgrading.
This is the second time in this PR's history that a response to this specific finding did not hold up on close reading, so it stays blocking. Please either (a) make the migration chain guarantee the FK end-to-end from a genuinely empty database without aborting, or (b) if fail-closed-with-manual-recovery is the accepted design, restore an automated test that runs the full chain from empty and asserts the RuntimeError, and document the manual recovery procedure.
M3. services/triggers.py:603 is an owner-blind, user-controlled UserOAuth lookup missed by this PR's scoping pass
account = db.query(UserOAuth).filter(UserOAuth.id == int(oauth_account_id)).first()src/xagent/web/services/triggers.py:603, in _resolve_gmail_resource, guarded only by the subsequent account.user_id == user_id check on line 604. oauth_account_id arrives straight from user-supplied Gmail trigger config (GmailTriggerConfig.oauth_account_id, validated at src/xagent/web/services/triggers.py:708-713) — an ordinary request body field.
This PR introduced user_oauth_owner_clause / get_scoped_user_oauth_account precisely to stop a direct-ID lookup from widening into another namespace, and migrated api/auth.py, api/cloud_storage.py, api/mcp.py, tools/config.py — but missed this file. src/xagent/web/services/triggers.py is entirely unchanged by this PR, which is consistent with the scoping pass having implicitly scanned gmail_*-prefixed filenames; this is the generic, non-gmail-prefixed trigger service.
It is latent today (no writer of actor-owned rows exists). But per this PR's own model docstring, an actor-owned row shares the same user_id as its owning user and differs only in resource_owner_key. The moment #82 adds an actor-owned writer, an ordinary user can bind a Gmail trigger's oauth_account_id to their own actor-owned credential, bypassing exactly the ordinary/actor separation this PR exists to establish.
Please either route this query through the new helper now — cheap while it is harmless — or record it as a hard blocker on #82's review checklist. It should not be left implicitly for #82, since #82 is scoped to gmail_* files and would plausibly miss this same non-gmail-named file a second time.
M4. The PR's central regression test cannot detect the regression it exists to catch
tests/web/test_github_oauth.py:173-177 asserts that an OAuth callback preserves an actor-owned row:
preserved_actor = db.get(UserOAuth, actor_account_id)
assert preserved_actor is actor_account
assert preserved_actor.access_token == "actor-token"delete_scoped_user_oauth_accounts (src/xagent/web/services/user_oauth.py:149) hardcodes query.delete(synchronize_session=False) — a deliberate change from the implicit "auto" used by the pre-PR callback code. As a result the identity-mapped actor_account object created earlier in the same session is never evicted or expired, and db.get() returns the cached in-memory object with its pre-delete attribute values regardless of what the DELETE actually did. assert preserved_actor is actor_account is in fact an assertion about the identity map, not the database. The test would pass even if the callback's delete wrongly matched and removed the actor row.
Verified separately: the WHERE clause itself (user_oauth_owner_clause, scoped to resource_owner_key IS NULL) is correct today, so there is no live production bug — only a test that cannot protect the guarantee it names. Please assert real database state: db.expire_all() before re-fetching, db.get(..., populate_existing=True), or a COUNT(*) on a separate session/connection.
Design-level notes (non-blocking)
D1. User.oauth_accounts (src/xagent/web/models/user.py:68-79) is primaryjoin-filtered to ordinary rows, which removes actor-owned rows from the delete-orphan cascade at the one ORM user-delete site, src/xagent/web/api/admin_users.py:234. Deletion of actor rows on user delete now rests entirely on the DB-level ON DELETE CASCADE. Mitigating: SQLite's pragma setup in src/xagent/db/sqlite.py is fail-closed (raises if PRAGMA foreign_keys=ON does not take, rather than degrading silently), and there is no writer of actor rows yet. So this is a dormant, forward-looking tradeoff — worth an explicit note carried into the #82 stack rather than a current defect.
D2. The nullable resource_owner_key diverges from the sibling MCPOAuthGrant / MCPOAuthFlowState convention (always NOT NULL with a synthetic sentinel). The cost is a real complexity chain: a dialect allowlist enforced at app startup, two partial unique indexes instead of one plain index, SQLite batch-rebuild-with-interrupted-state-repair machinery, and a filtered relationship. The implementation is sound, but the PR description should carry a written rationale for diverging from the established pattern.
D3. src/xagent/web/api/mcp.py uses the field name resource_owner_key for two semantically different conventions in the same file — this PR's nullable UserOAuth convention and the pre-existing always-non-null MCPOAuthGrant / MCPOAuthFlowState convention. Each site is individually correct; this is a maintainability nit worth a comment at minimum.
D4. src/xagent/migrations/versions/20260818_add_user_oauth_resource_owner.py:33 imports xagent.db.migration_support — the only migration version file in the repo importing application code. The shared-invariant rationale (accepted in a prior round) is sound, but it creates forward coupling: if that module is moved or renamed, this one historical revision stops being replayable against existing databases, unlike every other self-contained revision. Informational only.
Minor findings
N1. The SQLite interrupted-migration retry branch (20260818_add_user_oauth_resource_owner.py:247-260) calls _missing_owner_index_definitions() but never _sqlite_global_owner_relation_names(), so a same-named unrelated table/index/view collision on this narrow path surfaces as a raw sqlite3.OperationalError instead of the guided RuntimeError the fresh-install branch at line 264-271 produces. Narrow (needs an interrupted migration and a coincidental name collision), untested.
N2. downgrade() (20260818_add_user_oauth_resource_owner.py:313-358) has no leftover-_alembic_tmp_user_oauth guard, unlike upgrade() (which got one in commit 2c41cbcb, lines 217-222). An interrupted downgrade, retried, fails with a raw OperationalError instead of the guided message. Rollback path only.
N3. The PR description's "one Alembic head: 20260818_user_oauth_resource_owner" claim is stale again — the upstream-main merge that brought in the Stripe seed migration made 0108d2704fc1 the actual single head. This exact claim was corrected once earlier in this PR's life and has drifted a second time. No code anchor; please refresh the description.
Checked, no further action
- SQLite batch-rebuild DROP-before-RENAME data-loss claim (round 2, C1) — the rebuttal (a preceding INSERT opens an implicit pysqlite transaction that a subsequent DROP does not force-commit, so a mid-sequence crash rolls back cleanly) was independently verified against this codebase's actual SQLAlchemy/Alembic/pysqlite configuration: no
isolation_leveloverride, andApplyBatchImplkeeps DROP+RENAME in that same implicit transaction. Waiver stands. - PostgreSQL idempotence test narrowing (
test_postgresql_owner_migration_accepts_current_metadataviacommand.stamp()) — unchanged since the last round and already tracked in open issue #1534. Correctly deferred; not a new gap. test_postgresql_upgrade's apparently-lost assertions (models/users/taskstables,_api_key_encryptedcolumn) — those checked tables that no Alembic migration in this repo has ever created (they are SQLAlchemy-metadata-owned only), so the assertions were unreachable before this PR touched the test. Not a coverage regression.- Previously waived round-1 items — predicate paren-balance robustness note, owner-column-length constant duplication between model and migration, the query-scoping "convention not enforced" ask, and migration code-quality nits 8/9/11/12: all previously rejected with unrebutted technical rationale, and no new information changes that.
Simplification opportunities (optional, not blocking)
A separate mechanical pass over the diff (not line-by-line re-verified — treat as suggestions):
L5: shrink: OWNER_AWARE_UNIQUE_INDEX_DIALECTS frozenset has one use site. Inline {"sqlite", "postgresql"}.
L15: delete: redundant `not isinstance(dialect, str)` clause in migration_support.py — a non-str already fails the `not in` check.
L55: delete: the third tuple field in OWNER_INDEX_DEFINITIONS is always True — hardcode unique=True.
L67: shrink: _require_partial_unique_index_support is a one-line alias with two callers — call the helper directly.
L71: shrink: _table_exists / _users_table_exists are the same one-liner with a different constant — collapse to _has_table(name).
L100: delete: get_user_oauth_account_by_id has zero production callers — drop until #82 needs it.
L191: delete: dead postgresql_where fallback in a test helper that only ever inspects SQLite.
The fuller pass found roughly 18 similar items, mostly delete: / shrink: of near-duplicate branches in the migration-repair helpers; net: approximately -60 to -80 lines possible. Happy to enumerate the rest if useful.
Blocking status & recommended decision
Blocking: yes — 4 major findings.
- M1
[new]— the documented rollback procedure silently deletes the Stripe MCP catalog row via the merged sibling branch, untested and undocumented. - M2
[prior, reopened — partially fixed]— from-empty FK gap now fails closed instead of corrupting, but does not guarantee the FK, and the one test covering the scenario was changed to pre-createusersrather than assert the new behavior. - M3
[new]—services/triggers.py:603is an owner-blind user-controlledUserOAuthlookup missed by the scoping pass; an ownership bypass the moment #82 lands. - M4
[new]— the actor-row-preservation test asserts against the identity map, not the database, so it cannot catch a regression of this PR's central safety guarantee.
Recommended event: REQUEST_CHANGES.
| 1. Stop all workers before the downgrade. | ||
| 2. If the database is SQLite, create a current database backup. | ||
| 3. If the database is SQLite, run `PRAGMA integrity_check;` against the backup and record `SELECT count(*) FROM gmail_watch_states;`. The integrity result must be `ok`. | ||
| 4. Run `alembic downgrade b1efe0dbe0af`. |
There was a problem hiding this comment.
Major: this documented rollback silently deletes an unrelated Stripe catalog row.
0108d2704fc1_merge_stripe_and_oauth_owner_heads.py:13-16 merges two independent branches off b1efe0dbe0af: this PR's 20260818_user_oauth_resource_owner and the unrelated 20260818_seed_stripe_mcp_app (pulled in by the upstream-main merge). Downgrading straight to b1efe0dbe0af necessarily unwinds both, and 20260818_seed_stripe_mcp_app.py:86-98 runs DELETE FROM public_mcp_apps WHERE app_id='stripe' — removing the Stripe catalog entry and orphaning MCPServer/UserMCPServer rows for users who already connected Stripe. Neither this doc nor any test mentions it; tests/migrations/test_migration_integration.py:484-493 runs this exact downgrade without asserting on public_mcp_apps.
Please scope the documented rollback to the OAuth-owner branch only (stepwise alembic downgrade -1 through 0108d2704fc1, then the OAuth-owner revision), or explicitly document and accept the Stripe-seed revert — and add a test pinning the Stripe row's fate across this path.
There was a problem hiding this comment.
We are not making a separate change for finding PRRT_kwDORQ4Kr86bWYo8 because it is a duplicate of canonical finding PRR_kwDORQ4Kr88AAAABKfaP0Q:M1, which covers the same Stripe-preserving rollback outcome (canonical review finding).
The underlying collateral deletion occurs because the Stripe revision's downgrade deletes the Stripe catalog row (Stripe downgrade implementation). This outcome is handled once by M1 rather than through a second independent resolution.
| return | ||
|
|
||
| if not _users_table_exists(): | ||
| raise RuntimeError( |
There was a problem hiding this comment.
Major (prior finding, reopened — partially fixed): this converts the from-empty FK gap from silent corruption into a hard failure, which is a real improvement, but it does not guarantee the FK ends up present. c7dfa28cc67a_add_user_oauth_table.py:36 still creates user_oauth without any FK when users is absent, so a from-empty pure-Alembic run now aborts here mid-chain and leaves a half-migrated database requiring manual operator recovery.
Please either make the chain guarantee the FK end-to-end from a genuinely empty database without aborting, or — if fail-closed-with-manual-recovery is the intended design — document the manual recovery procedure and restore an automated test that runs the full chain from empty and asserts this RuntimeError (see the related comments on the two tests that were changed to pre-create users).
There was a problem hiding this comment.
We are not making a separate change for finding PRRT_kwDORQ4Kr86bWYo_ because it is a duplicate of canonical finding PRR_kwDORQ4Kr88AAAABKfaP0Q:M2, which covers the same full-chain negative-coverage and recovery-documentation outcome (canonical review finding).
Pure-Alembic empty bootstrap is intentionally outside the supported application-startup path, which stamps an empty database before metadata-owned tables are created (startup migration contract). M2 accounts for the remaining fail-closed test and recovery guidance without treating unsupported bootstrap as a supported initialization mechanism.
| with engine.begin() as connection: | ||
| # Application startup creates metadata-owned core tables before | ||
| # Alembic upgrades the migration-owned schema. | ||
| User.__table__.create(bind=connection) |
There was a problem hiding this comment.
Major (test coverage): this was the one end-to-end test exercising the disputed scenario. test_empty_database_alembic_upgrade_to_head_completes was renamed to test_database_with_core_users_table_... and altered to pre-create users before the upgrade, so it no longer covers the empty-database path — and it does not assert the new fail-closed RuntimeError either.
That leaves the reopened FK-cascade finding with no automated coverage in either direction. Please keep a test that runs the full chain from a genuinely empty database and asserts whichever behavior is intended (FK present, or RuntimeError raised), in addition to this core-tables-present case.
There was a problem hiding this comment.
We are not making a separate change for finding PRRT_kwDORQ4Kr86bWYpE because it is a duplicate of canonical finding PRR_kwDORQ4Kr88AAAABKfaP0Q:M2, which contains the same required full-chain negative-coverage outcome (canonical review finding).
The existing supported-path test deliberately creates the metadata-owned parent before migration (supported startup-path test); M2 separately accounts for the genuinely empty fail-closed case rather than duplicating that resolution here.
| """Rollback removes owner storage and retains the existing merge head.""" | ||
| parent = "b1efe0dbe0af" | ||
|
|
||
| sqlite_tester.create_metadata_owned_users_table() |
There was a problem hiding this comment.
Same concern as the tests/alembic change: this is the CLI-invocation path that was used to reproduce the original missing-FK bug, and it now pre-creates users via create_metadata_owned_users_table() before upgrading — so it no longer reaches the empty-database branch. Please add (not replace) a case that upgrades from empty on this path and asserts the intended outcome.
Separately, this test runs command.downgrade(..., "b1efe0dbe0af") — the exact path flagged in docs/deployment.md:179 — and would be the natural place to assert what happens to the public_mcp_apps Stripe row.
There was a problem hiding this comment.
We are not making separate changes for findings PRRT_kwDORQ4Kr86bWYpJ:finding-1 and PRRT_kwDORQ4Kr86bWYpJ:finding-2 because they duplicate canonical findings PRR_kwDORQ4Kr88AAAABKfaP0Q:M2 and PRR_kwDORQ4Kr88AAAABKfaP0Q:M1, respectively (canonical review findings).
M2 owns the empty-bootstrap full-chain coverage outcome, while M1 owns the Stripe-preserving rollback and regression-test outcome. Handling each outcome once avoids independent, overlapping resolutions for this combined thread.
| assert oauth_account.email == "octocat" | ||
|
|
||
| preserved_actor = db.get(UserOAuth, actor_account_id) | ||
| assert preserved_actor is actor_account |
There was a problem hiding this comment.
Major: this assertion cannot detect the regression it exists to catch.
delete_scoped_user_oauth_accounts (src/xagent/web/services/user_oauth.py:149) hardcodes query.delete(synchronize_session=False), a deliberate change from the implicit "auto" the pre-PR callback used. The identity-mapped actor_account object created earlier in this same session is therefore never evicted or expired, so db.get() returns the cached object with its pre-delete attribute values regardless of what the DELETE actually did. assert preserved_actor is actor_account asserts on the identity map, not the database — the test passes even if the callback's delete wrongly matched and removed the actor row.
The WHERE clause is correct today (user_oauth_owner_clause scopes to resource_owner_key IS NULL), so there is no live bug — but this is the PR's central safety guarantee and it is currently unprotected. Please assert real DB state: db.expire_all() before re-fetching, db.get(UserOAuth, actor_account_id, populate_existing=True), or a COUNT(*) on a separate session/connection.
There was a problem hiding this comment.
We are not making this change for finding PRRT_kwDORQ4Kr86bWYpM because it duplicates M4's incorrect identity-map premise. The callback commits before the assertion (callback commit).
SQLAlchemy sessions default to expire_on_commit=True, so that commit fully expires persistent instances and subsequent access reloads current database state (SQLAlchemy expire_on_commit documentation). Consequently, the existing db.get() assertion does not silently accept a deleted actor row, and an additional explicit expiration would be redundant.
| # the first replacement index is created. Existing indexes were | ||
| # validated above, so creating only the missing definitions safely | ||
| # completes that exact interrupted state without accepting drift. | ||
| _create_owner_indexes(missing_indexes) |
There was a problem hiding this comment.
Minor: this interrupted-migration retry branch calls _missing_owner_index_definitions() but never _sqlite_global_owner_relation_names(), unlike the fresh-install branch at lines 264-271. On this path a same-named unrelated table/index/view surfaces as a raw sqlite3.OperationalError rather than the guided RuntimeError operators get elsewhere. Narrow (needs both an interrupted migration and a coincidental name collision) and currently untested, but the guard is one call.
There was a problem hiding this comment.
We are not making a separate change for finding PRRT_kwDORQ4Kr86bWYpP because it is a duplicate of canonical finding PRR_kwDORQ4Kr88AAAABKfaP0Q:N1, which contains the same interrupted-repair collision diagnostic and focused regression-test outcome (canonical review finding).
|
|
||
| has_old_constraint = OLD_CONSTRAINT in _constraint_names() | ||
| dialect = op.get_bind().dialect.name | ||
| if dialect == "sqlite" and (OWNER_COLUMN in columns or not has_old_constraint): |
There was a problem hiding this comment.
Minor: downgrade() has no leftover-_alembic_tmp_user_oauth guard, unlike upgrade() (lines 217-222, added in 2c41cbcb). An interrupted SQLite downgrade that is retried fails with a raw OperationalError instead of the guided "restore the verified backup" message — which matters here, since docs/deployment.md explicitly tells operators not to retry a failed downgrade against the changed database. Rollback path only.
There was a problem hiding this comment.
We are not making a separate change for finding PRRT_kwDORQ4Kr86bWYpR because it is a duplicate of canonical finding PRR_kwDORQ4Kr88AAAABKfaP0Q:N2, which contains the same interrupted-downgrade temporary-table diagnostic and regression-test outcome (canonical review finding).
| import sqlalchemy as sa | ||
| from alembic import op | ||
|
|
||
| from xagent.db.migration_support import require_owner_aware_unique_index_dialect |
There was a problem hiding this comment.
Informational (no action required): this is the only migration version file in the repo that imports application code. The shared-invariant rationale accepted in the earlier round is sound, but note the forward coupling — if xagent.db.migration_support is ever moved or renamed, this historical revision stops being replayable against existing databases, unlike every other self-contained revision here. Worth a short comment recording that constraint for whoever refactors that module later.
Minor cleanup while you are here: OWNER_AWARE_UNIQUE_INDEX_DIALECTS has a single use site and could be inlined, and the not isinstance(dialect, str) clause in migration_support.py:15 is redundant since a non-str already fails the not in check.
There was a problem hiding this comment.
We are not making the import-comment change for PRRT_kwDORQ4Kr86bWYpU:finding-1 because the helper module already documents that it deliberately shares one startup/migration invariant, while a comment cannot enforce future import-path compatibility (shared invariant module). The earlier review likewise accepted this shared import as intentional (prior review disposition).
We are not making a separate change for PRRT_kwDORQ4Kr86bWYpU:finding-2 because it duplicates L5. The named dialect constant communicates the shared invariant rather than adding accidental complexity (shared invariant module).
We are not making a separate change for PRRT_kwDORQ4Kr86bWYpU:finding-3 because it duplicates L15 and is technically unsafe. The helper deliberately accepts an object-valued runtime boundary and converts invalid values to RuntimeError (dialect validation helper); set membership requires hashable values, so removing the explicit type guard can instead expose TypeError for unhashable non-strings (Python set-type documentation).
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
This PR adds the nullable UserOAuth.resource_owner_key ownership boundary, ordinary-only OAuth consumers, and SQLite/PostgreSQL migration and recovery coverage for the staged actor-credential rollout. It intentionally leaves actor writers and the Gmail ownership lifecycle for the downstream stack, while changing the migration graph and operational rollback procedure. Blocking: yes — recommended event: REQUEST_CHANGES
Since the previous reviewed head 140e22ef, commit 70922bef merged upstream main and the Stripe branch, leaving the Stripe seed and owner revision as two heads. Commit 8c4d2383 added 0108d2704fc1_merge_stripe_and_oauth_owner_heads, making 0108d2704fc1 the current sole head and making the rollback traversal material. Commit f0e712f2 added metadata-bootstrap handling in the migration tests.
Round 0 approach verdict
Verdict: acceptable-with-reservations. The nullable ownership model, ordinary-only read boundary, transaction ownership, and bounded SQLite recovery design fit the staged rollout. The reservation is the migration graph: the new owner revision still descends from b1efe0dbe0af even though the base Stripe seed revision has the same parent, so the no-op merge couples an OAuth rollback to an unrelated Stripe data downgrade. That macro concern is the same canonical root as N1 below, not a second finding.
Confirmed findings
Major
N1 — Documented OAuth rollback deletes the Stripe catalog row [new]
docs/deployment.md:179 directs operators to run alembic downgrade b1efe0dbe0af. The added merge revision src/xagent/migrations/versions/0108d2704fc1_merge_stripe_and_oauth_owner_heads.py:12-16 has both 20260818_user_oauth_resource_owner and 20260818_seed_stripe_mcp_app as parents, and both branches descend from b1efe0dbe0af; Alembic therefore traverses and downgrades the Stripe sibling as well as the OAuth branch. The Stripe downgrade at src/xagent/migrations/versions/20260818_seed_stripe_mcp_app.py:86-98 unconditionally deletes public_mcp_apps.app_id='stripe'. Because administrators can customize the Stripe catalog row, re-upgrading can reseed defaults but cannot restore those persisted customizations.
Please either linearize the new owner revision after 20260818_seed_stripe_mcp_app and remove the no-op merge, or keep the merge and change the runbook target/expected revision to 20260818_seed_stripe_mcp_app. Add SQLite and PostgreSQL downgrade coverage that edits the Stripe row before rollback and asserts both row presence and the customization afterward.
Minor
N2 — OAuth state owner validation still coerces malformed numeric claims [new]
At src/xagent/web/api/auth.py:1712, int(user_id_claim) accepts booleans and floats (True becomes 1, and 7.9 becomes 7). A signed malformed state can consequently reach the ordinary-account delete/recreate path, while an infinity-sized float can raise OverflowError; the outer callback handler begins later and does not catch that exception. This is a narrow validation/error-boundary defect, not a newly introduced major authentication bypass, because normal state issuance uses an existing integer User.id.
Require exact positive-integer semantics without coercion, for example type(user_id_claim) is int and user_id_claim > 0, preserve the explicit legacy None policy, and reject/catch overflow before provider exchange. Add malformed-state tests for booleans, fractional and integral floats, Infinity/overflow, and oversized values, asserting an invalid-state response, no provider call, and no database mutation.
N4 — SQLite recovery documentation and tests omit the valid both-indexes-present state [new]
docs/deployment.md:92 says automatic recovery is limited to zero or one existing owner-aware index. The migration safely accepts both exact replacement indexes when SQLite DDL completed before Alembic recorded the revision: _missing_owner_index_definitions() returns no missing definitions and the retry proceeds without duplicate CREATE INDEX calls. Update the runbook to describe zero, one, or both exact indexes, explain that both-present means DDL completed before the revision stamp, and add (ORDINARY_INDEX, ACTOR_INDEX) to the recovery parameterization at tests/alembic/test_20260818_add_user_oauth_resource_owner.py:258-264.
Minor/nit
N5 — Post-commit helper retains the pre-validation raw-claim contract [new]
After validation moved before provider exchange, _run_post_commit_oauth_side_effects in src/xagent/web/api/auth.py:82-126 still uses user_id: Any, its :91-95 docstring says it receives the raw OAuth-state claim and performs coercion inside the helper, and :126 repeats int(user_id). The current callback passes an already-normalized integer under the non-null guard, so this is documentation and local-contract drift rather than a reachable malformed-state bug. Change the parameter to int, describe the validated callback boundary, and remove the redundant conversion, or explicitly document why defensive conversion is intentionally retained.
Prior body-only finding still open
P22 — NOT FIXED (body-only, prior). The PR description still says 20260818_user_oauth_resource_owner is the sole Alembic head. After 8c4d2383, the current sole head is 0108d2704fc1; update the Verification section to name the merge head. Source: PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-14, prior review.
Prior findings checklist
| Root | Status | Source IDs / URLs and current-code disposition |
|---|---|---|
| P1 | REFACTORED | PRR_kwDORQ4Kr88AAAABKZxBXQ:F1; inline 3830054546, 3830054557, 3830054567, 3830054577; replies 3830866389, 3830866392, 3830866398, 3830866400; review. Gmail lifecycle moved to bsbds/xagent#82; no current #1588 Gmail diff. |
| P2 | REFACTORED | PRR_kwDORQ4Kr88AAAABKZxBXQ:F2; inline 3830054584; reply 3830866395; review. The Gmail lock path moved to #82 and is absent from this PR. |
| P3 | FIXED | PRR_kwDORQ4Kr88AAAABKZxBXQ:F3, PRR_kwDORQ4Kr88AAAABKbXviA:F3; inline 3830054587; reply 3830866385; reviews 4993073501 / 4994756488. Supported paths require the users/cascade invariant and the unsupported bare-empty path now fails closed. |
| P4 | FIXED | PRR_kwDORQ4Kr88AAAABKZxBXQ:F4; inline 3830054587; reply 3830866385; review. Bounded exact-index recovery and tests are present. |
| P5 | REFACTORED | PRR_kwDORQ4Kr88AAAABKZxBXQ:F5a; review. Gmail behavior and its regression tests moved to #82. |
| P6 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:F5b; review. The sessionless test intentionally covers Python-side sync_backref=False; persisted and production cascade tests cover separate seams. |
| P7 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:F5c; review. An adjacent production-engine test verifies real SQLite pragma wiring. |
| P8 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-1; review. Gmail relationship scoping belongs to and exists in #82. |
| P9 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-2; review. The current SQLite docstring already explains mandatory FK integrity versus optional concurrency pragmas. |
| P10 | DROPPED | PRR_kwDORQ4Kr88AAAABKZLRPw:finding-1, PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-3; inline 3829556093, reply 3830866377; bot review, duplicate review. Sequence[str] and upstream candidate filtering make nullable elements unreachable for current callers. |
| P11 | DROPPED | Body occurrences in reviews 4993073501 and 4994756488; predecessor inline IDs 3827121809, 3828379577, 3820038532, 3820300940 in #1529. The ID-only and user-scoped helpers intentionally have different trust contracts. |
| P12 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-4; review. A future raw-query CI guard is not an actionable current defect; no changed caller bypasses the scoped service. |
| P13 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-5, PRR_kwDORQ4Kr88AAAABKbXviA:minor-6; inline 3831403706; reply 3831834089; reviews 4993073501 / 4994756488. Current model and immutable historical migration lengths are separate contracts. |
| P14 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-6; review. Callback normalization and the shared non-null guard make the nullable annotation non-actionable. |
| P15 | FIXED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-7; review. _owner_index_names is removed and no dead replacement remains. |
| P16 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-8; review. Validation and later dialect access use the same migration bind; the observation is harmless style. |
| P17 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-9; review. SQLite permits same-named triggers and indexes; trigger probing would not prevent a failure. |
| P18 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-10; review. Hatch packages the shared helper and Alembic already imports the application package. |
| P19 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-11; review. object is intentional because runtime type rejection is part of the validation boundary. |
| P20 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-12; review. Original exception text is logged and re-raised. |
| P21 | DROPPED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-13, PRR_kwDORQ4Kr88AAAABKbXviA:minor-3; inline 3831403690; reply 3831833484; reviews 4993073501 / 4994756488. The bounded test is intentional and the historical full-chain follow-up is tracked in issue #1534. |
| P22 | NOT FIXED | PRR_kwDORQ4Kr88AAAABKZxBXQ:finding-14; review. Body-only stale head description; update it to 0108d2704fc1. |
| P23 | DROPPED | PRR_kwDORQ4Kr88AAAABKbXviA:C1; inline 3831403662; reply 3831832426; review. Configured SQLite transaction semantics protect copy/drop; the leftover-temp retry issue is separate P24. |
| P24 | FIXED | PRR_kwDORQ4Kr88AAAABKbXviA:minor-1; inline 3831403673; reply 3831832691; review. Early temp-table detection and both interruption-state tests are present. |
| P25 | FIXED | PRR_kwDORQ4Kr88AAAABKbXviA:minor-2; inline 3831403687; reply 3831833199; review. Rollback now records and compares the Gmail watch-state count. |
| P26 | FIXED | PRR_kwDORQ4Kr88AAAABKbXviA:minor-4; inline 3831403700; reply 3831833769; review. Bare provider strings now raise before query construction and have regression coverage. |
| P27 | FIXED | PRR_kwDORQ4Kr88AAAABKbXviA:minor-8; review. Both temporary-table interruption states are covered. |
| P28 | FIXED | PRR_kwDORQ4Kr88AAAABKbXviA:minor-9; review. MCP read/delete actor isolation tests exercise the changed endpoints. |
| P29 | FIXED | PRR_kwDORQ4Kr88AAAABKbXviA:minor-10; review. Generic callback coverage preserves a same-user, same-provider actor row. |
| P30 | DROPPED | PRR_kwDORQ4Kr88AAAABKbXviA:minor-5; inline 3831403681; reply 3831832946; predecessor IDs 3823039337, 3823443205 in #1529. The private normalizer receives only two fixed single-term predicates. |
| P31 | DROPPED | PRR_kwDORQ4Kr88AAAABKbXviA:minor-7; inline 3831403716; reply 3831834353; review. Existing user/owner predicates remain in force because callers only append conjunctive filters. |
Review coverage and limitations
No local tests, builds, linters, or formatters were run by review policy; the supplied CI preflight recorded 15/15 successful checks, and the changed tests were statically inspected. The complete 27-file Round 0/1 scope, current/base migration graph, caller contracts, and complete review history were inspected; no dropped, fixed, or refactored root is being re-reported. The Simplification Lens was unavailable because of usage_limit_reached, so no simplification opportunity is asserted. Static verification cannot substitute for executing the migration or malformed-state cases locally; the requested regression tests are therefore part of the fixes above.
Blocking status & recommended decision
Blocking: yes
Recommended event: REQUEST_CHANGES
Blocking issues:
docs/deployment.md:179— major — the documented downgrade traverses the Stripe sibling and can delete customizedpublic_mcp_apps.app_id='stripe'data; [new]
| 1. Stop all workers before the downgrade. | ||
| 2. If the database is SQLite, create a current database backup. | ||
| 3. If the database is SQLite, run `PRAGMA integrity_check;` against the backup and record `SELECT count(*) FROM gmail_watch_states;`. The integrity result must be `ok`. | ||
| 4. Run `alembic downgrade b1efe0dbe0af`. |
There was a problem hiding this comment.
Blocking [new] — alembic downgrade b1efe0dbe0af is not an OAuth-only rollback here: merge revision 0108d2704fc1 has both owner and Stripe parents (see src/xagent/migrations/versions/0108d2704fc1_merge_stripe_and_oauth_owner_heads.py:12-16), so this command also runs Stripe's downgrade and deletes public_mcp_apps.app_id='stripe'. Please either linearize the owner revision after 20260818_seed_stripe_mcp_app and remove the no-op merge, or target 20260818_seed_stripe_mcp_app and expect that head; add SQLite/PostgreSQL coverage preserving an edited Stripe row.
There was a problem hiding this comment.
We are not making a separate change for finding PRRT_kwDORQ4Kr86bWmEw because it duplicates review-body finding PRR_kwDORQ4Kr88AAAABKfhWRA:N1, which requested the same Stripe-preserving rollback outcome (canonical review-body finding).
The referenced merge traversal no longer exists: the current owner revision follows 20260818_seed_stripe_mcp_app directly (current migration graph).
| user_id = payload.get("user_id") | ||
| user_id_claim = payload.get("user_id") | ||
| try: | ||
| user_id = int(user_id_claim) if user_id_claim is not None else None |
There was a problem hiding this comment.
Minor [new] — int(user_id_claim) accepts True and floats such as 7.9 and can raise OverflowError for infinity, while the outer handler starts later. Require type(user_id_claim) is int and user_id_claim > 0 (with an explicit legacy None policy), reject/catch overflow before provider exchange, and add bool/float/infinity tests asserting invalid-state response, no provider call, and no database mutation.
There was a problem hiding this comment.
We are not making a separate change for finding PRRT_kwDORQ4Kr86bWmFS because it requests the same exact pre-exchange user-ID validation and no-side-effect coverage as canonical finding PRR_kwDORQ4Kr88AAAABKfhWRA:N2 (canonical review-body finding).
This inline comment is the second occurrence of that same requested outcome (inline finding), so it does not require an independent resolution.
|
|
||
| On SQLite the migration rejects globally colliding owner-index names before rebuilding the table in batch mode. Stop every worker before this rebuild and keep SQLite quiesced until the migration completes. Take and verify a database backup before the rebuild: under the driver's legacy transaction mode, SQLite DDL can commit independently of Alembic's outer transaction. | ||
|
|
||
| If the SQLite migration process exits after the rebuild starts, keep every worker stopped and retry `alembic upgrade head` once with the same release. The migration automatically completes only an unambiguous interrupted index-installation state: `resource_owner_key` has its expected nullable `VARCHAR(512)` definition, `uq_user_provider_account` is absent, and zero or one existing owner-aware index has the exact expected definition. It validates every existing owner index and creates only the missing definitions. Do not start workers until both owner-aware indexes pass the verification below. |
There was a problem hiding this comment.
Minor [new] — the runbook says recovery handles only zero or one owner index, but the migration safely accepts both exact replacement indexes when DDL completed before the revision stamp. Document zero/one/both, explain that both-present is a validated no-op, and add (ORDINARY_INDEX, ACTOR_INDEX) to the recovery parameterization in tests/alembic/test_20260818_add_user_oauth_resource_owner.py:258-264.
There was a problem hiding this comment.
We are not making a separate change for finding PRRT_kwDORQ4Kr86bWmGT because it requests the same both-index recovery documentation and parameterized test as canonical finding PRR_kwDORQ4Kr88AAAABKfhWRA:N4 (canonical review-body finding).
This inline comment is the second occurrence of that same requested outcome (inline finding), so it does not require an independent resolution.
|
I reviewed PR #1588 at head Major — blocking
Minor and nit findings
Prior status highlightsP1, P2, and P5 were REFACTORED to the Gmail lifecycle stack in bsbds/xagent#82. P3, P4, P15, and P24–P29 were FIXED after current-code verification. P6–P14, P16–P21, P23, P30, and P31 were DROPPED as safe, non-actionable, or tracked; P21 remains tracked in issue #1534. P22 is NOT FIXED: the PR description still names No local tests were run under review policy. The supplied CI preflight recorded 15/15 successful checks, and tests were statically inspected. The Simplification Lens was unavailable due to Blocking: yes |
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
This is a history-update verification for PR #1588, not a replacement for the existing review. The PR establishes the owner-aware UserOAuth schema and service boundary, ordinary-only consumers, and migration/recovery support while actor writers remain staged for the downstream stack. Late review body 4998991825 arrived after the first history export and the first consolidated review; the late claims were independently checked against the current and base code.
Late-history verification
The independent checks retain only one new finding. The interrupted SQLite owner-index retry path has a real, minor collision-diagnostics defect; the late M2, M4, and downgrade-temp N2 claims do not add blocking issues, and M3 is a downstream rollout prerequisite rather than a new #1588 finding.
| Late root | Status | Independent result | Source IDs |
|---|---|---|---|
| M2 / late P3 | FIXED (late blocking reopening dropped) | Bare empty Alembic initialization is explicitly unsupported and now fails closed when users is absent; supported paths require the users table and the ON DELETE CASCADE invariant. |
4998991825; related late occurrences 3835129384, 3835129387, 3835129392 |
| M3 | REFACTORED (out of scope for #1588) | src/xagent/web/services/triggers.py:603 is a real latent owner-blind lookup, but that file is unchanged here and this PR has no actor-row writer. PR #82 fixes the ordinary-owner lookup and must land and be deployed before PR #78 enables actor-owned writers; treat that ordering as a hard rollout prerequisite. |
4998991825 |
| M4 | DROPPED (duplicate of fixed P29) | The callback commits after the bulk delete, and SQLAlchemy commit expiration makes the later db.get() reload SQL state. The test therefore does not rely on a permanently cached actor object; no duplicate finding is warranted. |
4998991825, 3835129397 |
| Late N1 | CONFIRMED — minor, new | On the interrupted-state retry, only missing owner indexes are computed before _create_owner_indexes; global SQLite table/index/view names are not preflighted. This is distinct from fixed P4/P24 and can produce an uncontextualized sqlite3.OperationalError. |
4998991825, 3835129402 |
| Late N2 (downgrade-temp) | DROPPED | The documented rollback procedure explicitly forbids retrying a changed database after an interrupted SQLite downgrade and requires restoring the verified backup first, so the missing retry guard is not an actionable supported-path defect. | 4998991825, 3835129406 |
Minor — interrupted SQLite retry misses global relation collisions [new]
At src/xagent/migrations/versions/20260818_add_user_oauth_resource_owner.py:257, the interrupted-state retry calls _missing_owner_index_definitions(dialect) and passes the missing definitions directly to _create_owner_indexes. It does not check the global SQLite relation namespace first. If an unrelated table, index, or view already uses the name of a missing owner index, the retry raises a raw sqlite3.OperationalError instead of the guided collision error used by the fresh legacy-schema path; the migration remains recoverable, so this is minor. Intersect _sqlite_global_owner_relation_names() with only the missing owner-index names before _create_owner_indexes (valid owner indexes already present must not count as collisions), and add regression tests for unrelated TABLE, INDEX, and VIEW collisions while retaining an existing valid owner index. This is a distinct root from P4/P24.
The existing major Stripe rollback finding at docs/deployment.md:179 remains confirmed and blocking. The prior REQUEST_CHANGES review and its decision are unchanged; this follow-up does not re-report that root.
No local tests, builds, linters, or formatters were run under the review constraints. The supplied CI preflight was 15/15 successful, and the late paths and relevant tests were statically inspected.
Blocking status & recommended decision
Blocking: yes — carried forward solely by the existing major Stripe rollback finding at docs/deployment.md:179.
Recommended event for this follow-up: COMMENT. The existing REQUEST_CHANGES decision remains unchanged; the newly confirmed retry-collision issue is minor and non-blocking.
| # the first replacement index is created. Existing indexes were | ||
| # validated above, so creating only the missing definitions safely | ||
| # completes that exact interrupted state without accepting drift. | ||
| _create_owner_indexes(missing_indexes) |
There was a problem hiding this comment.
Minor [new] — this interrupted-state retry computes only missing owner indexes and calls _create_owner_indexes without preflighting SQLite's global table/index/view namespace. If an unrelated relation already uses a missing owner-index name, retry raises a raw sqlite3.OperationalError instead of the guided collision error. Before _create_owner_indexes, intersect _sqlite_global_owner_relation_names() with only the missing owner-index names (do not treat already-present valid owner indexes as collisions), and add regression tests for unrelated TABLE, INDEX, and VIEW collisions.
There was a problem hiding this comment.
We are not making a separate change for finding PRRT_kwDORQ4Kr86bWtWZ because it duplicates review-body finding PRR_kwDORQ4Kr88AAAABKflIJA:Late-N1, which requested the same interrupted-repair collision preflight (canonical follow-up review finding).
The current retry path already filters SQLite's global relation namespace by only the missing owner-index names before creating indexes (current interrupted-repair logic).
|
Follow-up history verification for PR #1588: late review body New minor findingAt Downstream rollout prerequisiteLate M3 ( Late claims independently dropped or retained
No local tests were run. The supplied CI preflight was 15/15 successful. A follow-up COMMENT review carries the one new minor inline finding; the existing REQUEST_CHANGES decision remains the blocking decision. |
Finalization SummaryPush
Resolved
Rejected
UnresolvedNone. Totals
|
Finalization SummaryPush
Resolved
Rejected
UnresolvedNone. Totals
|
Final concern dispositionThis summary applies to commit Review contract and scope boundariesThese dispositions depend on the following accepted contracts:
A disagreement with one of these contracts is a product or deployment decision, not an unresolved implementation defect in this PR. 1. PostgreSQL migration safety — HandledThe owner migration follows the Stripe seed and keeps one Alembic head. PostgreSQL runs the revision in one transaction. The migration adds the nullable owner column and the required cascade foreign key. It creates both partial unique indexes before it removes the old constraint. A failure rolls back the complete revision. The migration does not backfill or rewrite credential rows. Downgrade stops before schema changes when actor-owned rows exist. This prevents namespace collapse and credential loss. The deployment guide requires a controlled OAuth migration window. It also describes lock impact, relation-name collisions, rollback behavior, and post-migration checks. 2. Unsupported bare-empty Alembic initialization — HandledBare Alembic is not a supported initializer for an empty application database. Normal startup stamps an empty database first. It then creates the complete metadata-owned schema. A bare-empty run fails closed when the owner revision finds The deployment guide tells operators not to create 3. Nullable owner representation — Handled
A null owner identifies an ordinary credential. A non-null owner identifies an actor credential. Two disjoint partial unique indexes enforce these namespaces. Legacy behavior for a null 4. Filtered ORM relationship and database cascade — Handled
An actor row cannot enter the ordinary delete-orphan collection through normal relationship assignment. Actor-row cleanup depends on the database foreign key instead. The model declares The tests cover ordinary collection filtering and deletion of ordinary and actor rows on PostgreSQL and SQLite. 5. Legacy callback
|
3 participants
Summary
UserOAuth.resource_owner_keyVerification
20260818_user_oauth_resource_ownermainRollout
This PR does not contain a production writer for actor-owned
UserOAuthrows. Do not enable one until Gmail PR #82 has landed. Followdocs/deployment.mdfor database-specific migration, retry, verification, and rollback procedures.