feat(connectors): let team members edit a team-shared Custom API

[AlexLiu190625]

What

A Custom API's shared configuration can be edited only by the user whose user_custom_apis row carries can_edit. An application that installs the connector team hooks can now supply a second permission source, so a Custom API a team owns becomes editable by the members that application recognises.

GET and PUT /api/custom-apis/{id} widen their first gate, PUT's edit check gains a second source, and the verdict is re-established once the definition row is locked.

Standalone deployments are unaffected in the answers both routes give: with no hook installed, GET/PUT behave exactly as they do today. That said, a caller whose personal row cannot be found now costs one additional SELECT on the definition row even in that case, and no test pins the number of queries this route issues, so that cost is not covered.

This widening is deliberately scoped to editing. DELETE /api/custom-apis/{id} is untouched: it gates on the caller's own user_custom_apis row first and answers 404 when there is none, before delete_team_connector is ever called. So a team member this change now lets edit a shared Custom API still gets a 404 from the delete route on the same connector. Two reasons keep it that way. First, DELETE does have a team contract of its own — delete_team_connector returns a ConnectorDeleteDecision — but the route reaches it only after that personal-row gate, so widening DELETE means moving the gate, not reading a new field; and ConnectorAccess, the contract this change reads, cannot supply the answer either, since it carries team_owned and can_edit and nothing about deletion. Second, deleting a Custom API's definition row cascades to every other user's association row pointing at it, a blast radius an edit never has — authorizing that from a verdict that only ever meant "may edit" would be reading it for a question it does not answer. The 404 a team member with no personal row gets from DELETE matches the 404 the same caller gets from GET/PUT when the hook grants no verdict either, so this is the existing not-found answer, not a new one.

Why

The seam that answers the question already exists on main: resolve_connector_access asks an installed application whether the caller's team links a given connector and whether it may edit it. Nothing on the Custom API routes consults it, so a team that owns a Custom API still sees every member except the one owner refused.

Three things follow, each a separate hazard the code has to handle.

A caller with no personal row must be able to reach the API at all. Both routes read the definition row through the custom_api relationship on the caller's own user_custom_apis row and answer 404 when there is none. A team member who never connected the API personally has none. The routes now fall back to a bare lookup of the definition row plus the caller's team verdict, and use the existing _TeamOwnedUserApi stand-in in place of the missing association.

A payload carrying is_active from such a caller must be refused, not ignored. is_active lives on the personal association row. Written onto the stand-in it would set a shadowing instance attribute that persists nothing, and the response would read that shadow back and report a change that never happened — a 200 for a write that did not occur.

A verdict resolved before a lock wait can be stale by the time the write happens. PUT takes a FOR UPDATE on the definition row, and that wait has no bound. The application that answers the verdict can revoke the team's link at any moment by writing its own tables, which this lock does not cover.

How

The gate

_resolve_custom_api_for_request is the one place both routes resolve three things together: the caller's association (real row or stand-in), the definition row, and the caller's team verdict. It runs the same personal-link query both routes have always run, and only when that finds nothing — no row, or a row whose relationship is unexpectedly empty — does it look the definition row up on its own and consult the seam. A caller with neither a personal row nor a verdict still gets the same 404 they have always got.

skip_resolution_when lets each route declare when its own working personal row already decides the answer, so no hook call is made:

• GET passes a predicate that is always true. It never reads the verdict at all — the response it builds is the same for an owner and a non-owner — so a personal row of any kind already decides everything it returns.
• PUT passes one that checks can_edit. Only a personal row that already grants edit decides the answer on its own; a row without it does not, because a granting team verdict can still widen it.

PUT's edit check becomes: the caller's own row grants edit, or the team verdict does. There is no platform-administrator bypass on this route and none is added.

A seam failure is translated locally. This module has no function-wide try the way the MCP edit route does, so each arm maps through one helper, _http_from_connector_runtime. There are four such arms: GET's resolution, PUT's pre-lock resolution, PUT's post-lock re-check, and the rename hook. The rename hook's arm is not new behaviour being introduced here — that call has been made from this route since #904 with no typed handling on either connector family; what this change closes is the asymmetry of adding arms for the new call sites and leaving that one bare, for ConnectorRuntimeError specifically. A hook that raises some other exception on the rename arm still reaches the generic handler as an unclassified 500, exactly as it did before this change.

Refusing rather than silently ignoring

A caller with no personal row sending a payload that carries is_active gets 400 naming the reason — but only after the edit-right gate: a caller who may not edit this Custom API at all is refused with 403 first, and the is_active-specific 400 applies only to a caller who has already cleared that gate. The more general question — can this caller touch this connector at all — is answered before the narrower one about a single field, so a caller with neither right sees the reason that applies to every payload rather than one specific to this one. The same guard exists a second time, under the definition row's lock, for a caller whose personal row existed when the gate ran and is gone by the time the lock is taken.

Re-establishing authority after the lock

The route re-reads the caller's link row once it holds the definition row, and re-resolves the team verdict whenever that fresh read does not grant the edit on its own — not only when the pre-lock gate itself went through the verdict. A caller admitted through their own can_edit=True row at the gate, whose row is then deleted or downgraded while the lock is held, is now caught the same way a caller admitted through the verdict already was; the narrower condition this replaces asked only about the second case.

The re-check resolves into one of four outcomes, replacing what used to be two separate checks (a 404 on any vanished link row, a 403 on one that no longer grants edit): a fresh verdict that grants the edit is accepted; a verdict that no longer grants what the pre-lock verdict granted is refused with 403 naming the change; a caller with no surviving personal row and no granting verdict either is refused with the same 404 an unrelated caller has always gotten; and a caller whose surviving row simply does not grant the edit, with nothing to widen it, is refused with 403. Every refusal rolls back first, before any field is read or mutated and before the rename hook runs, so it has nothing to undo.

A caller with no surviving personal row is now answered through a freshly constructed stand-in rather than whatever object the gate resolved before the lock: that earlier object may be backed by a row a concurrent write has since deleted, and carrying it forward raises StaleDataError on a payload that writes the definition row, or ObjectDeletedError while the response is built on one that does not — the latter only once this request's own commit expires it. A second is_active guard exists under the lock for the same reason the pre-lock one exists, catching a caller whose personal row existed when the gate ran and is gone by the time the lock is taken.

The re-check's own hook call declares caller_holds_lock=True: it runs after the definition row is locked and before anything this request has staged is committed, so a hook that ends the transaction would release that lock without this route finding out. That contract, and the accounting that checks every declared call site against the source, live in connector_team_scope.py, merged separately as #2134; this change adds two rows for its own call sites — the pre-lock resolution, which holds nothing, and the post-lock re-check — and rewrites one now-stale docstring sentence in that module's own test file, which claimed the access slot has no call site in this repository. The re-check's hook call lives in its own function, _recheck_team_access_under_definition_lock, kept out of update_custom_api's own body so that route's hook calls stay attributable one-per-function the way #2134's accounting expects — the rename hook call already made from this route is the other one.

The re-check still runs only on the path that writes the definition row — the same condition that decides whether the lock is taken at all. A payload that writes only the caller's own link row takes no lock, so there is no unbounded wait for a revocation to slip into.

This narrows the window; it is not a fence. A fence needs the revoke path to take the same lock, and that path lives in the application that installs the hook. The re-read is also only meaningful under READ COMMITTED, PostgreSQL's default, which nothing overrides on the engine; under REPEATABLE READ or SERIALIZABLE it reuses the transaction's original snapshot and degrades to a no-op — it stops refusing rather than starting to refuse wrongly. The code says so at the site.

Keeping hook calls off the event loop

An installed hook is slow synchronous work: the seam is designed on the assumption that the installing application answers from its own tables. FastAPI runs a coroutine route on the event loop thread itself, so a slow hook call inside an async def delays every request the process is serving, not just its own; a plain def goes to the threadpool, where it occupies one worker.

get_custom_api becomes a plain def for that reason — PUT and DELETE on this module already are. The invariant is asserted rather than left to per-route judgement: the functions in this module that can reach a hook are discovered by transitive reachability from the module's own imports, the discovered set is asserted equal to a named literal so the check cannot pass by finding nothing, and every member is asserted to be a plain def.

Known limitations tracked separately

These are known and deliberately not addressed here. Each is either repo-wide work or a contract question wider than these routes.

• The seam's typed 503 does not reach the client on these routes (The connector seam's typed 503 never reaches a client on the connector management routes #2055). Where a hook failure is translated into an HTTP answer, the outer handler's shape means the client sees a generic error rather than the seam's own message. That mapping is the same on every connector management route and is being settled in one place.
• A failing hook blocks a PUT that only touches the caller's own fields (A failing connector access hook blocks a PUT that only changes the caller's own association fields #1775). The gate resolves a verdict before it knows the payload writes nothing shared, so a hook outage refuses a request the verdict has no say over.
• GET /api/custom-apis omits Custom APIs the caller can reach by id (list_custom_apis omits Custom APIs the caller can reach by id #1877). The listing reads only the caller's own association rows, so an API this change makes reachable and editable by id can still be missing from that caller's own list. Fixing the listing is a change to a different route.
• The Custom API form declares a can_edit capability but never reads it (The Custom API form declares a can_edit capability but never reads it #1702). The reported field this change makes truthful is not yet consumed by the client.
• The visibility hook's answer is never shape-checked (The connector visibility hook's answer is never shape-checked #1878), and the seam expresses "consult or skip" and "raise or degrade" five ways (The connector seam expresses "consult or skip" and "raise or degrade" five ways #1880). Both are seam-wide; this change adds call sites rather than unifying them.
• Definition-row lock waits are unbounded and held across hooks (fix(connectors): definition-row locks have no bounded wait and are held across application hooks #1946). A property of the lock this change builds on, not of the verdict it adds. The contract for what a caller holds while a hook runs, and which call sites must declare it, is now written down in connector_team_scope.py (fix(connectors): state the hook session contract and refuse a hook that ends the caller transaction #2134, merged) rather than left unstated.
• with_for_update is a no-op on SQLite (fix(db): with_for_update takes no lock on SQLite, so read-modify-write routes are unserialized on the default backend #1944) and the PostgreSQL isolation level is neither checked nor documented (fix(db): the PostgreSQL isolation level the server hands us is neither checked nor documented, so REPEATABLE READ turns concurrent edits into 500s #1945). Inherited from the lock; the re-check's dependence on READ COMMITTED is written into the code.
• The edit and delete routes each carry their own copy of the lock/re-read/re-derive sequence (refactor(connectors): the Custom API edit and delete routes each carry their own copy of the lock/re-read/re-derive sequence #2084), the edit route takes FOR UPDATE where FOR NO KEY UPDATE is the lock it needs (fix(connectors): the Custom API edit route takes FOR UPDATE where FOR NO KEY UPDATE is the lock it needs #2085), and the name uniqueness check is an unlocked check-then-act that returns 500 on a real collision (fix(custom-api): the connector name uniqueness check is an unlocked check-then-act, and a real collision returns 500 instead of 400 #2086). All three are about the lock sequence on main, which this change extends rather than reshapes.
• Test-quality gaps in the connector team access suites (Four test-quality gaps in the connector team access suites #1816). Tracked together rather than fixed piecemeal.
• Whose capability a list field describes when an administrator views another user's connectors (Whose capability do list fields describe when an admin views another user's connectors? #1703), and cleartext header values with a masking comment that claims otherwise (Connector headers values round-trip in cleartext while a masking comment claims otherwise #1701). Carried from earlier reviews, unrelated to the verdict.

Relationship to #1661

This is the Custom API half of #1661, which covered both connector families and both the seam and the locks in one change. Three parts of it have already merged separately: the access seam itself (#1912), the Custom API definition-row lock (#2059) and the MCP definition-row lock (#2060). This change is built on main with all three in place, not on that branch.

The MCP half — the same widening for GET/PUT /api/mcp/servers/{id}, plus the platform-catalog downgrade that family needs — is #2095. The two touch disjoint production files: this one changes api/custom_api.py only, that one changes api/mcp.py only. Neither depends on the other's code.

One behavioural note, stated plainly because it decides a preferred merge order rather than a dependency. The aggregate listing that feeds the connector picker lives in api/mcp.py and covers both connector families: it reports a can_edit_global for Custom API rows and decides their can_configure, and it is the MCP half that makes both reflect a team verdict.

Landing this change first means a Custom API that this route now lets a team member edit is not yet advertised as editable in that listing, so the Configure affordance stays hidden until the MCP half lands. Nothing breaks: the route permits more than the client advertises.

Landing the MCP half first inverts that, and costs more: the Configure affordance would be lit for a Custom API whose GET and PUT both still answer 404 for that caller, so opening it would make the connector appear to vanish. Merging this change first avoids that window entirely, which is why it is the preferred order.

Three comments in the MCP half describe these routes as they behave on main today. They stop being true once this change merges, so if this one lands second it should update them; if it lands first, the MCP half should.

One piece of coverage is owed to whichever lands second and is filed rather than carried: an assertion that the value the aggregate listing reports for a Custom API row equals what PUT /api/custom-apis/{id} actually does. That comparison needs both sides present, so it cannot be written on either branch on its own.

Relationship to #2134

The session-boundary contract this route's locked call sites rely on — what a caller must hold and declare while a hook runs, and the check that a hook ending the transaction is refused rather than silently succeeding — merged separately as #2134 and is not part of this diff. This change's own contribution to that contract is the two rows the module's call-site table gains for this route's call sites, and the caller_holds_lock=True declaration on the post-lock re-check.

#2134's own accounting attributes each declared call to the function that makes it, one call per function. This route now makes two hook calls under its lock (the post-lock re-check and the rename hook), so the re-check's call was moved into its own function, _recheck_team_access_under_definition_lock, rather than changing how that accounting groups calls. #1932, about the hook contract not saying what lock a caller holds, is closed by #2134 and is not carried in this PR's own known-limitations list.

Tests

tests/web/api/test_custom_api_team_connector_edit.py covers the gate itself: the resolution helper on GET and PUT, the stand-in's is_active 400 and its 403 on any other payload including an empty one, the wiring for a team editor's PUT, the verdict's re-validation under the definition lock and the hook-call-count contract across four payload shapes, the typed-error arm on each seam call site, an owner's immunity to a hook failure, the caller_holds_lock declaration on the post-lock re-check, and the reachable form of an is_active-only write admitted purely by the team verdict.

tests/web/api/test_custom_api_edit_lock_postgresql.py adds six cases against a real PostgreSQL server, each constructing an interleaving no SQLite-backed test can: a personal link row deleted or downgraded while the lock is held, crossed with whether the re-resolved team verdict still grants the edit, including the mixed-payload 400 and the stand-in response that a stale ORM object would instead raise ObjectDeletedError from.

tests/web/api/test_custom_api.py gains the coroutine guard described above, drops the two await keywords on get_custom_api, which is now a plain def, and gains one more entry in the pinned "functions that can reach the seam" set for _recheck_team_access_under_definition_lock.

Run locally:

Suite	Result
test_custom_api_team_connector_edit.py + test_custom_api.py	52 passed
test_custom_api_edit_lock_postgresql.py against PostgreSQL 17	17 passed
tests/web (whole suite)	9766 passed, 257 skipped in 11m57s

[gemini-code-assist]

Code Review

This pull request refactors the Custom API endpoints (get_custom_api and update_custom_api) from asynchronous to synchronous functions to prevent slow connector team hook calls from blocking the event loop. It introduces robust helper functions to resolve team-linked access and re-validate permissions under a database lock to prevent race conditions. Additionally, a comprehensive test suite is added to verify these behaviors. Feedback is provided regarding the new test file test_custom_api_team_connector_edit.py, where helper and test functions are incorrectly defined as asynchronous (async def) and use await to call the now-synchronous endpoints; these should be updated to be synchronous.

[AlexLiu190625]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the Custom API endpoints (GET and PUT /api/custom-apis/{api_id}) to support team-linked Custom APIs. It introduces a helper _resolve_custom_api_for_request to resolve user association, definition rows, and team access verdicts. To prevent blocking the FastAPI event loop during potentially slow external hook calls, get_custom_api is converted from an asynchronous to a synchronous function, and a new test is added to enforce that no seam-reaching functions are coroutines. Additionally, the team access verdict is revalidated under the database lock in update_custom_api to prevent race conditions, and robust error handling is added for ConnectorRuntimeError. Comprehensive unit tests are added to verify these behaviors. There are no review comments, so I have no feedback to provide.

[rogercloud]

PR Review: xagent#2094 — feat(connectors): let team members edit a team-shared Custom API

1. PR Summary

GET/PUT /api/custom-apis/{id} previously derived both reachability and edit authority solely from the caller's own user_custom_apis row, so a team member with no personal row on a team-owned Custom API was refused entirely — even though the resolve_connector_access seam already existed on main for exactly this purpose. This PR wires that seam into both routes via a stand-in object (_TeamOwnedUserApi) that fills in for a missing personal row, adds an is_active 400 guard so the stand-in can't lie about persisted state, and adds a post-lock re-check (under FOR UPDATE) to narrow the revocation race window. The two-source OR-gate logic and fail-closed 404 for a caller with neither a row nor a verdict are correctly reasoned, and test coverage is strong (45 tests). However, the post-lock re-check introduces a new, undocumented concurrency hazard by invoking a hook call while holding a live FOR UPDATE cursor.

Blocking: yes — recommended event: REQUEST_CHANGES

2. Round 0 Approach Verdict

Acceptable with reservations. The direction is right — this reuses an existing access seam rather than inventing a parallel one, and the core authorization logic is sound. The reservations below are about placement/consistency and one genuine new concurrency hazard, not about the overall architectural direction.

3. Re-review Note

The prior gemini-code-assist inline finding — that tests/web/api/test_custom_api_team_connector_edit.py used async def/await/@pytest.mark.asyncio inconsistently with the now-synchronous routes under test — has been independently re-verified as FIXED in commit f450d7a: a grep of the current file for async def|await |pytest.mark.asyncio returns zero matches; all 19 test functions and both _get/_put helpers are plain def.

4. Severity Findings

Finding 1 — MAJOR / BLOCKING

File: src/xagent/web/api/custom_api.py:593-599 (post-lock re-check hook call); precedent hazard documentation at custom_api.py:786-797 (delete_custom_api's own comment about this exact risk class)

At the point this hook call executes, the session has run only SELECTs (gate reads, the FOR UPDATE lock read, the link re-read) — precisely the condition release_db_connection_if_clean (src/xagent/web/models/database.py:105-137) checks before silently rolling back and releasing the connection/lock. delete_custom_api explicitly documents this exact hazard class for its own symmetric hook call; update_custom_api's new post-lock re-check introduces the identical pattern with no equivalent documentation or mitigation.

Trigger: an installed hook implementation that calls (or transitively triggers) release_db_connection_if_clean on the passed session while only SELECTs have executed.

Impact: the FOR UPDATE lock is silently released mid-request; execution continues assuming the lock is held, proceeds to mutate the shared CustomApi definition row, and commits — a TOCTOU/lost-update on the exact row this PR's re-check exists to protect. This defeats the protection this PR adds, for a hazard broader than revocation (any concurrent writer can interleave).

Blocking rationale: contingent on hook implementation behavior (not a guaranteed exploit in a hookless deployment), but the helper is one this same codebase uses and documents for this exact hazard class, and the purpose of this code path is specifically to close authorization races under concurrency — an implementation-contingent trigger with an unsafe data-integrity side effect on a code path this PR itself introduces meets the blocking bar.

Suggestion: either avoid making the hook call while holding a live FOR UPDATE cursor (re-verify the lock is still held after the call — e.g. re-SELECT ... FOR UPDATE and compare a version/updated_at, or use SELECT ... FOR UPDATE NOWAIT/advisory-lock patterns that fail loudly instead of silently), or explicitly guard against release_db_connection_if_clean-style session termination (e.g., mark the session as "has written" before the hook call so the helper's own precondition never triggers).

Finding 2 — MINOR

File: src/xagent/web/api/custom_api.py:569-615

The post-lock re-check has two fail-closed race gaps: (a) a caller admitted at the gate via their own row's can_edit=True (so the hook is never consulted, team_access stays None) who is downgraded during the lock wait gets refused 403 without the team verdict ever being (re-)resolved, even if it would still grant edit; (b) a caller whose personal row existed at gate time (without can_edit) but is deleted entirely during the lock wait gets a 404, since is_stand_in was computed once at gate time and never revisited, even though a fresh evaluation could resolve them as a stand-in and grant 200. Both fail closed (never wrongly admit a caller) and are correctness/consistency bugs under a narrow window, not security bugs; neither is covered by tests.

Suggestion: re-derive both team_access applicability and is_stand_in at the post-lock re-check point rather than reusing gate-time values, or add tests documenting this as accepted behavior if left as-is.

Finding 3 — MINOR

File: src/xagent/web/api/custom_api.py:332-336

_resolve_custom_api_for_request unconditionally issues a fallback db.query(CustomApi)... whenever the personal-row lookup misses, regardless of whether any connector hook is installed. On base, a caller with no personal row got a 404 after one query; on HEAD, the same caller (even with zero hooks installed) issues a second (cheap, indexed PK) query before the same 404. This contradicts the PR description's "issue no additional query" / "asserted rather than assumed" claims, which are not backed by a query-count test for this scenario. Not a performance regression, but a documentation-accuracy issue.

Suggestion: correct the PR description, or short-circuit the fallback query when no hook is registered.

Finding 4 — MINOR

File: src/xagent/web/api/custom_api.py:703-714

The three "access" call sites route through resolve_connector_access_or_raise, which normalizes any non-ConnectorRuntimeError exception into a typed 503 before the route sees it. The rename-hook call site (rename_team_connector) only catches ConnectorRuntimeError directly, so a hook raising a plain exception still surfaces as an untyped 500 here — pre-existing behavior, not a regression. However, the PR description frames this change as closing "the asymmetry... of leaving [the rename hook] bare," implying full parity across all four arms, which overclaims: parity is achieved only for ConnectorRuntimeError, not for hook failures generally. Only tested with a ConnectorRuntimeError-raising hook.

Suggestion: either route the rename-hook call through the same normalization helper for true parity, or adjust the PR description to scope the claim accurately.

Finding 5 — MINOR

File: src/xagent/web/api/custom_api.py:432,720; src/xagent/web/api/mcp.py:1465-1473

The runtime 400 guard rejecting is_active payloads from a stand-in is currently the sole protection against a silent no-op write onto the stand-in's plain, mutable is_active attribute — no structural/type-level protection exists (no read-only property, no __setattr__ override). Not currently bypassable, but a future refactor separating the guard's computation from the write could silently reintroduce the bug this PR fixes.

Suggestion: consider making the stand-in's fields read-only (e.g. frozen dataclass or property-based) as defense-in-depth.

Finding 6 — MINOR

File: src/xagent/web/api/custom_api.py:117-145; src/xagent/web/api/mcp.py:1465-1473

_TeamOwnedUserApi hardcodes is_active=True, is_default=False, and the response builder reads these directly with no special-casing, so a team-granted caller with no personal row sees a fabricated is_active: true not grounded in any real row. Verified this does not affect authorization/usability (team-owned visibility is independent per connector_team_scope.py), but could mislead a UI rendering an "Active" toggle for a caller who then gets a 400 on attempting to change it. Not asserted by any current test.

Suggestion: consider surfacing a distinct sentinel (e.g. null) for stand-in is_active rather than a fabricated true, or document the field's meaning for stand-ins in the API schema.

Finding 7 — MINOR (test coverage)

File: src/xagent/web/api/custom_api.py:455,715-720

Two success paths bypass the lock/re-check entirely and are untested: (a) an is_active-only payload from a caller admitted purely via a granting team verdict succeeds without taking the FOR UPDATE lock or re-verifying the verdict, since is_active isn't classified as a "definition row" field; (b) an empty payload from a granted stand-in is a no-op 200, same bypass. The writes_definition_row field partition is currently exactly correct, so this is not a live bug, but nothing pins the invariant with a test.

Suggestion: add hook-call-count/lock-assertion tests for these two paths to guard against a future field-exclusion change silently widening the un-rechecked surface.

Finding 8 — MINOR (test quality)

File: tests/web/api/test_custom_api.py:335,358 (test_get_custom_api, test_get_custom_api_not_found)

Both tests are new/touched by this PR (the get_custom_api route was converted async def → def) but only had their await removed — they remain async def with @pytest.mark.asyncio, misrepresenting the now-synchronous route under test. (Six similar pre-existing instances elsewhere in the same file predate this PR and are out of scope.)

Suggestion: convert both to plain def and drop the @pytest.mark.asyncio marker, consistent with the fix already applied to the other test file.

5. Simplification Opportunities

• L285 (definition), L377, L408 (call sites): skip_resolution_when: Callable[[UserCustomApi], bool] | None has exactly two call sites, one passing a constant lambda _user_api: True. Replace with a plain boolean flag (e.g. require_edit_check: bool) that reproduces both call sites identically, dropping the typing import and the dead None-default/docstring branch.

net: -8 lines possible

6. Additional Notes

• _TeamOwnedUserApi is defined in mcp.py (underscore-private) but is now load-bearing for custom_api.py — a backwards coupling, though safely deferred via TYPE_CHECKING/function-local import with no actual import-cycle risk today. Consider relocating it to a shared module.
• is_stand_in = not isinstance(user_api, UserCustomApi) (custom_api.py:417) re-derives a fact the resolver already computed internally — redundant but harmless; a full "push stand-in substitution to the response builder only" refactor would not work cleanly since the stand-in's default values are consumed at multiple authorization-gate decision points, not just for display.
• The sibling module xagent.web.api.mcp has the same class of live hazard the new AST-reachability coroutine-guard test (test_custom_api.py:33-88) is designed to catch — delete_mcp_server (mcp.py:3754) is an async def route calling the connector-hook seam directly. Pre-existing, non-blocking for this PR, but worth generalizing the test's module scope or filing a follow-up issue.
• The PR description states the is_active-on-stand-in 400 check "is ordered before the edit refusal," but the code does the opposite (403 edit-check first, then the 400 check) — arguably the more defensible order since it avoids leaking payload-validity info to an unauthorized caller. Please correct the description.
• The "relationship unexpectedly empty" fallback case documented in _resolve_custom_api_for_request's docstring (custom_api.py:292-296) is effectively unreachable given current FK (ON DELETE CASCADE) + ORM (delete-orphan) cascade guarantees; low priority to tighten the docstring.
• _http_from_connector_runtime (custom_api.py:267-277) forwards a hook-raised ConnectorRuntimeError.status_code verbatim with no bounds validation. Low severity since hooks are a trusted extension point, but a defense-in-depth clamp would be reasonable.

7. Blocking Status & Recommended Decision

Blocking: yes
Recommended event: REQUEST_CHANGES

Blocking issues:

• src/xagent/web/api/custom_api.py:593-599 — major — post-lock re-check hook call inside FOR UPDATE lock risks silent lock release, undermining the revocation-race protection this PR adds — [new]

[AlexLiu190625]

The hook session contract and the boundary check that the review of this PR asked for now live in their own PR, #2134, opened from main so that both this PR and #2095 can build on the same seam.

What moves there:

• the contract text in connector_team_scope (what a hook may and may not do with the caller's session, and what each declared call site holds when its hook runs);
• the root-transaction-end counter and the refusal path in _call_connector_hook_gate, opt-in per call site through caller_holds_lock=True;
• the application-level handler that answers one fixed 500 for that refusal.

What stays here: the post-lock re-authorization gaps (re-asking the team decision whenever the freshly read personal row no longer grants edit, and the stand-in object instead of the 404 branch after the lock), plus this route's own caller_holds_lock=True declaration at the post-lock access call, with their tests.

Merge order: #2134 first, then this PR, then #2095. I will rebase this branch once #2134 lands and update the description accordingly.

[AlexLiu190625]

This change fixes the two race gaps in the post-lock re-check, wires the new call site into the session-boundary contract that merged separately as #2134, and corrects the three inaccurate statements in the description. Point-by-point replies are on each thread; a few items from the Additional Notes don't have their own thread, so they're addressed here:

• The duplicated is_stand_in derivation: agreed, no action needed beyond what was already said.
• delete_mcp_server being async def calling into the seam directly: already tracked as delete_mcp_server calls the connector team seam synchronously on the event loop #1818 (open); not opening a second issue for it.
• The description's claim about which guard runs first (400 before 403): fixed — the code has always run the edit-right check (403) before the is_active-specific check (400), and the description now says so.
• _resolve_custom_api_for_request's "unreachable" fallback branch: correct on PostgreSQL, where the foreign key makes it genuinely unreachable. It is reachable in this test suite, which runs on an in-memory SQLite database that does not enforce foreign keys by default, so a test can construct the state that branch handles even though the schema in production would prevent it.
• The unchecked status-code range _http_from_connector_runtime forwards from a hook: already raised separately in The connector seam's typed 503 never reaches a client on the connector management routes #2055's thread, which is the right place for it since that issue is about centralizing this route family's error handling.

On the session-boundary gap raised on the post-lock re-check itself: the fix and its detection now live in #2134 (merged), and this change declares caller_holds_lock=True on this route's new call site with its own row in that module's call-site table.

One accounting note for anyone comparing this against that table: this route now has two hook calls under the same lock (the post-lock re-check and the rename hook that already existed). Rather than changing how the table's own check attributes calls to a function, the re-check's call was moved into its own function, _recheck_team_access_under_definition_lock, so each of this route's hook calls still maps to a distinct function the way the check already expects. That check, and its test, are untouched by this PR.

[rogercloud]

Major

src/xagent/web/api/custom_api.py:559-625 (PUT re-check) vs :931-986 (DELETE re-check) vs src/xagent/web/api/mcp.py:4081-4185 (MCP PUT re-check) — three call sites all "lock row → re-read personal link → re-authorize", but with three different models: Custom API PUT now allows personal-OR-team verdict (this PR), Custom API DELETE checks personal row only and never consults the team verdict, MCP PUT allows personal-OR-platform-admin. Net effect: a team member this PR newly lets edit a team-owned Custom API still gets a bare 404 from DELETE on the same connector, an inconsistency this PR introduces without stating it as a deliberate scoping decision. Please either extend DELETE to the same team-verdict check or explicitly document PUT/DELETE as intentionally asymmetric.

Minor

src/xagent/web/api/mcp.py:2018-2026 — the stand-in class (can_edit=False, is_active=True, is_default=False) still has plain mutable class attributes with no structural protection (no frozen dataclass, __slots__, or __setattr__ guard) against a future refactor accidentally writing to is_active and creating a silently-shadowing instance attribute. Not addressed by issue #2123 (that issue only covers what is_active/is_default mean, not making the fields read-only). Please add a lightweight guard (frozen dataclass or __slots__), or open a dedicated tracking issue for it.

src/xagent/web/api/custom_api.py:498 and :717 — both guards check api_data.is_active is not None, but the documented contract ("a payload carrying is_active must be refused") requires "is_active" in api_data.model_fields_set, matching the convention already used at line ~521. An explicit {"is_active": null} payload slips past both guards silently instead of getting the promised 400 (harmless today only because the actual write is gated by the same is not None check). Use model_fields_set in both guards to match the stated contract.

src/xagent/web/api/mcp.py:2018-2026 / src/xagent/web/api/custom_api.py:484-490 — the edit-gate check now also governs the caller's own personal is_active field, so a team verdict about the shared connector can authorize toggling the caller's own flag (confirmed by test_a_can_edit_false_personal_row_widened_by_the_team_writes_is_active_in_one_call). Not incorrect, but conflates two different authorities in one gate; consider separating "edit shared resource" from "edit my own association" checks.

tests/web/api/test_custom_api_team_connector_edit.py:794-819 — test_no_hook_installed_pays_zero_calls_regardless_of_payload constructs a hook-counting object but never wires it into set_connector_team_hooks() (called without access=), so assert len(hook.calls) == 0 cannot fail regardless of route behavior. Wire the counter in or drop the assertion's implied claim.

Blocking: no — recommended event: APPROVE

[AlexLiu190625]

Major — PUT/DELETE asymmetry. Documented rather than extended, and the description now says why in full. DELETE /api/custom-apis/{id} is untouched: it gates on the caller's own user_custom_apis row first and answers 404 when there is none, before delete_team_connector is ever called — so DELETE does have a team contract of its own (ConnectorDeleteDecision), but the route only reaches it after that personal-row gate. Widening DELETE therefore means moving that gate, not reading a new field, and ConnectorAccess, the contract this change reads, cannot supply the answer either: it carries team_owned and can_edit and nothing about deletion. Deleting a Custom API's definition row also cascades to every other user's association row pointing at it, a blast radius an edit never has — authorizing that from a verdict that only ever meant "may edit" would be reading it for a question it does not answer. The 404 a team member with no personal row gets from DELETE is the same 404 the same caller gets from GET/PUT when the hook grants no verdict, so this is the existing not-found answer carried into a route this PR otherwise does not touch.

Minor — stand-in structural protection. Answered in the inline thread on the pre-lock is_active guard in update_custom_api — added __slots__ to both stand-in classes in mcp.py, with a new test asserting the write raises.

Minor — is_active guards using is not None instead of model_fields_set. Fixed. Both guards in update_custom_api (the pre-lock one and the post-lock one) now check "is_active" in api_data.model_fields_set, matching the writes_definition_row convention a few lines below and closing the {"is_active": null} gap. Added test_explicit_null_is_active_from_a_stand_in_caller_is_also_400, which sends an explicit null and asserts the 400 that was previously silently skipped.

Minor — the edit gate also governs the caller's own is_active. Left as-is. is_active only ever writes the caller's own association row; nothing about it is visible to or affects any other user, so widening its gate alongside the shared-resource edit has no cross-user effect. The more precise fix — letting a caller toggle their own is_active regardless of edit rights, since it was arguably wrong that a can_edit=False personal row couldn't do that before this PR either — changes what "edit right" means for a field that isn't part of the shared resource at all, which is a product decision beyond this PR's scope.

Minor — test_no_hook_installed_pays_zero_calls_regardless_of_payload. Fixed by removing the unwired hook object and the assertion it could never fail; the test now only asserts the 404 it actually exercises, renamed to test_no_hook_installed_answers_404_for_a_stand_in_caller. A real "zero hook calls" assertion needs a spy at the seam layer itself rather than an object nothing installs — tracked in #1816 with the rest of this suite's test-quality gaps rather than built here.

Commits 06e44d583 (the is_active guards and the stand-in __slots__) and 974281383 (the test cleanup).