feat(web): let a team edit the shared configuration of a connector it owns

[AlexLiu190625]

What changes

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

Standalone deployments are unaffected: with no hook installed, every route in this diff behaves exactly as it does today, and that equivalence is asserted rather than assumed.

Behaviour changes

• New hook slot. set_connector_team_hooks gains an access keyword. The hook is asked (db, user_id, refs) — refs a collection of (connector_type, connector_id) pairs — once per request no matter how many connectors are involved, and answers a dict mapping each linked ref to a ConnectorAccess(team_owned, can_edit). A ref missing from the answer means "the caller's team does not link this connector" — the only way that fact is ever expressed; there is no None-shaped verdict for an individual ref. A malformed answer — a non-dict, a key that is not exactly a (str, int) ref, a verdict for a ref that was never asked about, a team_owned that is not exactly True, or a can_edit that is not exactly True or False — is rejected at the boundary rather than normalised. The key check is exact and runs before the membership check, because Python's ordinary tuple equality treats ("mcp", True), ("mcp", 1.0) and ("mcp", Decimal("1")) as equal to ("mcp", 1), and a key that merely aliases one of the refs asked about is not an answer to the question.

• GET and PUT on /api/mcp/servers/{id} and /api/custom-apis/{id} widen their first gate. Each answers 404 without a personal association row for the caller; they now also admit a caller for whom the access hook returns an answer. A caller with neither still gets 404.

• A platform catalog app's shared row never becomes editable because of a team access verdict. xagent provisions one shared MCPServer row per catalog app and attaches every user who connects that app to that same row; a key-based app's row may additionally hold the administrator's platform fallback key in its env. That configuration is the platform's, not any one team's. An installing application may still answer can_edit=True for such a ref — the seam does not police what a hook says — so this module downgrades that answer against the fact that the row is the catalog's own, and the downgraded verdict is the single object both the gate and every reported field read. Nothing can therefore advertise an edit the PUT would refuse. Only can_edit is cleared: team_owned stays as the application answered it, so the connector remains reachable and readable by the team, where dropping the verdict entirely would 404 a connector the team genuinely links.

  The rule is written once, in _team_access_for_shared_row, and the verdict passes through it at every place one is produced — the GET/PUT resolution, both loops of the /servers listing, the connect response, the post-lock re-check, and the toggle response, six call sites across five routes. That is two gates and four reported fields: gating alone would leave a Configure button lit whose save returns 403. Three places do not call it, each for a structural reason rather than for lack of need: the apps listing already skips a catalog app's stored row entirely, using this same predicate, before any verdict is read; Custom APIs have no catalog concept at all, no app id and no catalog association on the model; and the delete branch of the permission predicate never reads the verdict.

  The predicate is _server_catalog_keys against the catalog's own keys — the one the apps listing already uses, so this module holds a single definition of "catalog-managed". Two neighbours were measured and rejected for it. _is_reserved_catalog_name answers "may a new row take this name" and reads the name alone, so it no longer recognises a builtin-OAuth catalog row an administrator renamed, even though that row still carries its app_id in auth — and builtin OAuth is 21 of the 28 built-in catalog apps. _catalog_server_has_platform_key answers "catalog row that also holds the platform key", which is False for every keyless and mcp_oauth row and for every key-based row whose key each user supplies, all of which are still platform-owned configuration.

  The check runs only for a verdict that already grants edit, the one case where it can change an answer. A deployment with no access hook installed therefore issues no additional query at all. A listing whose verdicts do grant edit builds the catalog key set once per request and shares it across every row: one extra SELECT, not one per row. Two existing query-budget constants moved by exactly that one statement — the /servers hook-call budget from 7 to 8, and the healthy-listing count for servers from 5 to 6 — both measured, both asserted identical across the two population sizes those tests parametrise over. The failing-hook constants are unchanged, because a failing hook returns no verdicts and the check never fires.

  One boundary is resolved deliberately rather than correctly, and is written into the function as such: a connector someone built themselves under a name a catalog app later took. The catalog claims that name, so the row is treated as catalog-managed and the team edit is withheld. Its creator keeps their own edit right in full — is_owner decides the edit branch before any verdict is read — so what is withheld is only a teammate editing that connector on the owner's behalf. Telling such a row apart from a real catalog row needs a stored "who created this definition" fact the schema does not carry today.

  Two related facts worth stating, because both are easy to assume otherwise. The connecting user is not always left without ownership on a catalog app: the builtin-OAuth provisioning path in auth.py writes is_owner=True, which is the path 21 of the 28 built-in apps take, while the key-based, keyless and mcp_oauth connect paths write is_owner=False. And the application that installs the hook is expected to exclude catalog rows on its own side too; that exclusion is tracked on the application side and is a merge precondition for that hook's installation. Keeping both is duplication on purpose: the application's answer is a snapshot taken when the connector was shared, while this one is derived per request, and both a row's name and its transport are mutable through this very PUT.

• can_edit_global becomes one computation per connector kind. For MCP servers the shared predicate absorbs the access verdict, so the value reported by the list, by GET, by PUT's response and by the toggle response is a single calculation. Custom APIs keep their own gate's formula, because that gate has no platform-admin bypass and the MCP predicate does — sharing one predicate across both kinds would make the list report an editable connector whose PUT returns 403.

• Personal fields are refused rather than ignored for a caller without an association row. A PUT carrying user_env or is_active from such a caller returns 400 naming the reason, instead of a 200 reporting a write that did not happen. On the Custom API side such a write would otherwise land on a shadowing attribute and be reported as successful. A caller whose team links the connector but whose verdict denies edit now gets a 403 for any other payload as well, rather than a 200 that wrote nothing: that caller's writable field set is empty — the personal fields have no row to land on, the tamper check refuses every shared field it can compare, and the secrets it deliberately cannot compare are emptied out of the payload — so every 200 it could have received was a success report for a request that changed nothing. The 403 is ordered after the personal-field 400, so a personal-only payload still gets the more precise answer. Callers who do hold a personal row are unaffected: they can still write their own row's fields.

• PUT on both connector kinds takes a row lock on the definition, and so does the Custom API DELETE. A single-table SELECT ... FOR UPDATE with populate_existing() precedes the tamper check and the config build on MCP servers, and the same lock precedes the rename-ordering read and the field mutations on Custom APIs, so two concurrent edits cannot interleave into a partially applied row. Every value the tamper check and the rename propagation depend on is read after the lock. A definition removed between the gate's read and the lock yields the existing 404, not an error from the write path. The Custom API DELETE takes that same definition lock before it touches the association row, so this pair of tables has exactly one lock order everywhere — parent then child. Without it, PUT (definition first, association second) and last-association delete (association first, definition second) could take the same two rows in opposite orders and deadlock. Both Custom API write routes are plain def rather than async def, so FastAPI runs them in its threadpool: a FOR UPDATE wait can block indefinitely on a concurrent writer, and inside a coroutine route that wait would occupy the event loop thread and stall every other request the process is serving. The MCP PUT was already synchronous.

• Nothing that can reach an installed hook runs on the event loop thread. The reason above is not confined to routes that take a lock, so this diff states it as an invariant instead of a per-route decision: a hook call is slow synchronous work in its own right — the seam is designed on the assumption that the installing application answers from its own tables — and a slow call inside a coroutine route occupies the process's single event loop thread rather than one threadpool worker, delaying every request the process is serving rather than only this one. Thirteen functions across the two API modules can reach a hook. The set is discovered by transitive reachability from those modules' own imports rather than written out by hand, is asserted equal to a named literal set so the check cannot pass by finding nothing, and every member is asserted to be a plain def. One member is exempt and named rather than left implicit: delete_mcp_server awaits an external OAuth revocation call and cannot be converted without changing how that call is made. The exemption must itself carry an await that is not the seam call — a coroutine whose only await is the seam call is a coroutine of the seam's own making, convertible by making that call synchronous, so "contains some await" would wave it through. The remaining gap is tracked in delete_mcp_server calls the connector team seam synchronously on the event loop #1818. Other async def routes in these two modules are untouched — the same discovery shows none of them reaches the seam.

• The team-scope seam's failures are typed where the verdict gates, and degrade where it decorates. PUT on both connector kinds always treats the verdict as the gate, and so does a single-item GET for a caller with no personal row at all: a hook that raises, or answers with a malformed shape, produces the seam's bounded error and the route translates it explicitly rather than letting it reach the generic handler — degrading that case would answer "does not exist" for a connector the call merely failed to ask about. Everywhere else the verdict only decorates an answer the caller can already reach some other way — MCP's single-item GET for a caller who already holds a personal row, the whole /servers and /apps listings (a listing row's presence is already established by the visibility hook, independent of the access verdict), the connect and toggle responses after their write has committed, and the per-row can_configure in the apps listing — a resolution failure degrades that field to False (the value reported before the verdict existed) with a logged warning, so a reporting dependency can never fail or invert a durable write, and one bad row, or one failed batch, cannot blank a listing. A single door inside the seam invokes every installed hook and checks every answer it returns, and it restores the shared session on both failure shapes — a hook that raises, and a hook that returns an answer the seam rejects — so neither leaves the request's transaction unusable for whatever the route does next. The restore sits on the invocation rather than on the callers that wrap one, so a hook slot added to this module later inherits it; before this, two of the five slots were covered. One shape is deliberately not covered: a hook that poisons the session, swallows its own failure and still returns a well-formed answer raises nothing anywhere, so nothing triggers a restore. A caller whose own personal row already decides the answer — an owner on either connector kind, or a Custom API GET, which never reads the verdict at all — never triggers a hook call.

• A PUT that writes on the verdict's authority re-resolves it under the definition row lock. The verdict is resolved before the lock exists, and the application that answers it can revoke the team's link at any moment by writing its own tables, which this lock does not cover. Both PUT routes therefore ask again once the transaction holds the definition row, and refuse with 403 if the fresh answer no longer grants edit; the re-check sits before any field is read or mutated and before rename propagation runs, so a refusal has nothing to undo. 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, not here. The re-read is also only meaningful under READ COMMITTED — PostgreSQL's default, which nothing here overrides on the engine. Both routes now say so in a comment: under REPEATABLE READ or SERIALIZABLE the re-read reuses the transaction's original snapshot and the re-check degrades to a no-op, which stops it refusing rather than making it refuse wrongly. The comment above the Custom API DELETE's lock says what the ordering statement covers, too: this repository's own two tables and nothing else. A hook writes the installing application's tables, which this lock does not cover, and the two routes reach their hooks in opposite positions relative to it — the PUT locks and then renames, this route calls the delete hook and then locks — so an application whose hooks take locks of their own can still deadlock against a concurrent edit/delete pair, and only that application can order its own locks compatibly. The two routes trigger on different conditions because their gates differ: MCP skips the re-check for a platform admin (whose write authority never came from the verdict) and for a payload touching only the caller's own association fields (which the verdict makes no decision about), while the Custom API route has neither exemption — it has no admin bypass at all, and its gate requires the edit right for every payload it admits. Where the re-check runs, the 200 reports the verdict it produced, because that is the answer the write was authorised on. Where it is skipped, the pre-lock verdict is reported unchanged: reporting a post-lock answer there would mean calling the hook on exactly the paths the exemptions removed it from, which is the per-request cost those exemptions exist to avoid.

• snapshot_connector_team_hooks is added, matching the equivalents in mcp_runtime and knowledge_base_team_scope, with a coverage test that discovers this module's hook globals by name so a future slot cannot be added without being covered. The seam's own unit tests use it too, so no suite in this repository resets these hooks by clearing every slot — a bare reset drops whatever the process had installed before the file ran, not only what the test set.

Sharing, unsharing, per-user environment overrides, the per-user OAuth routes and the toggle route's gate are all unchanged. Deletion is almost unchanged: the Custom API DELETE now takes the definition row lock before the association row and runs as a synchronous route, but its authorisation semantics and its outcome are identical — what changed is the lock order and the execution model, nothing about who may delete what or what gets deleted.

Why the answer is a verdict rather than a role

The seam stays free of tenancy vocabulary. It asks an access question about one identified connector and receives an access answer; it learns nothing about how the installing application decides. This follows knowledge_base_team_scope's access hook, which answers the same shape of question for knowledge bases.

The containment terms an installing application is expected to apply — that the connector's definition genuinely belongs to the answering scope, and that no second scope holds it — are the application's to enforce, because only it can see its own link table. The seam's validator enforces internal coherence only: an answer granting can_edit must also assert team_owned.

The two hooks are separate slots and nothing cross-checks them: list membership comes from visible_team_connector_ids, direct-id reachability and edit authority from resolve_connector_access. An installing application must derive both from one and the same link query. Both entry points' docstrings now state that requirement, and state what happens when the two answers disagree — each question is answered by the hook that owns it, with no reconciliation.

Reading this diff

The change is lopsided on purpose: about a fifth is production code and the rest is tests.

	files	lines
production	4	+1491 / −190
tests	9	+6346 / −171
CI registration	1	+33

Where to spend review attention. The production change is concentrated in five places, and they are the whole behavioural surface:

1. connector_team_scope.py — the new access seam: one dataclass, one hook slot, an answer validator, two resolvers, a snapshot primitive.
2. mcp.py — one gate helper, one fallback branch inside the existing permission predicate, and the wiring of GET/PUT plus the response builders.
3. custom_api.py — the same wiring for the other connector kind. Larger than it looks: these two routes reach the definition row through a relationship on the association, which a caller without an association does not have, and this module has no function-wide try the way update_mcp_server does, so each seam failure needs its own local arm. There are four of them now, all mapping through one helper. The fourth is the rename hook: that call has been made from this route since feat(connectors,agents,knowledge-base): opt-in team ownership via application hooks #904, six weeks before this branch, with no typed handling on either connector kind — what this diff briefly introduced was the asymmetry of giving the MCP side an arm and not this one, and that is now closed.
4. _local_mcp_can_configure — one predicate widened from a one-source to a two-source question.
5. _team_access_for_shared_row — the catalog downgrade, plus the one-query helper it reads the catalog through. One function holding one rule, called from every place a verdict is produced.

Why tests outweigh production four to one. Four reasons, none incidental:

• The reported-permission field is emitted from nine call sites. Proving they agree means a parametrised matrix over caller populations across two connector kinds, not one assertion. The matrix omits one population that is not constructible, and includes the platform admin, which is the only population where the two kinds legitimately differ.
• Refusal paths assert zero side effects: for each, that the definition row is unchanged after a rollback and re-query, and that no association row was created. That is three assertions per refusal rather than a status-code check. The comparison is against literal values captured before the call, not against the in-session object, which the rollback would otherwise make compare equal to itself.
• One file is a real-PostgreSQL concurrency suite. FOR UPDATE is a no-op on SQLite, which every other suite here runs on, so a lock statement that silently does nothing is indistinguishable from a working one. Two-connection, barrier-synchronised tests with disposable databases are verbose per assertion.
• The new mechanisms' costs are measured rather than assumed. The post-lock re-check's extra hook round trip is pinned per population — who pays it and who does not — and the degraded listing path's query count is pinned as a formula in the number of rows.

What this diff does not contain. No new table, column or migration. No change to sharing, unsharing, per-user environment overrides, the per-user OAuth routes, or the toggle route's gate; deletion changes only in lock order and execution model, as described above. No file outside the four production files, their tests, and one CI registration.

Verification

• Standalone parity: with no hook installed, every route in this diff is asserted equivalent to current behaviour, for both connector kinds on every leg — the aggregate listing, the local apps listing, detail, and the write contracts.
• Permission agreement: can_edit_global is asserted equal across the list, GET, PUT and toggle for the same caller and connector, parametrised over every constructible caller population, for both connector kinds. That includes the population this change exists for — a caller who already holds a personal association row that does not grant edit, widened by a granting team verdict — which is covered on both kinds in the agreement matrix, in each kind's durability and no-second-row checks, and, on the MCP side, in the re-check's personal-field exemption. The Custom API assertion compares the reported field against whether PUT actually succeeds, so it observes the gate rather than a fixed expectation. One population is the exception, and asserts a 403 on the write surface instead: a stand-in whose verdict denies edit no longer receives a PUT response to compare.
• Zero side effects on refusal, on each of the 404, 400, 403 and typed-error paths.
• Durability rather than staging: successful-edit assertions roll back and re-query rather than reading back through the writing session.
• Mutation checks. Each of these turns a specific named test red: reverting the rename propagation; the post-lock name capture; the configure-hint widening; the row lock; the Custom API verdict term; the DELETE path's definition lock; the access answer's key-type checks (three separate mutations, which isolate the bool half from the int half and the tuple-shape check from both); the denying stand-in's 403; the post-lock re-check (three mutations — the whole block, its refusal branch alone, and its personal-payload exemption alone); and the session restore after a hook failure. Added with the later revisions: each of the two route conversions that keep the seam off the event loop, whose failure message names the offending function, plus the reachability discovery that finds them, pinned separately so the check cannot pass by finding nothing, plus removing the exempted route's await, which invalidates its exemption, and making the seam call the only await it has, which invalidates it too; the Custom API rename hook's typed-error arm; the session restore at the seam's single hook door, and again with the answer check moved back outside that door; the access answer's verdict-value type check, where the duck-typed case is the one that fails by not raising at all; the personal-field exemption in the re-check's trigger; and the hook-reset scope, where restoring the clear-everything form fails the pin that asserts a pre-installed hook is put back. One item's mutation record is partial and the test says so: setting the re-check's reported verdict to None turns its class red, but deleting the reassignment outright leaves it green, because both verdicts in that test grant edit.
• The catalog downgrade is pinned as twelve cases across four catalog row shapes — a key-based row holding the platform key, a key-based row without one, an mcp_oauth row, and a builtin-OAuth row an administrator renamed — on the gate and on each reported field, plus a self-built row that stays team-editable and a catalog row whose own owner still edits it. Its cost is pinned separately across two population sizes: no additional statement at all with no hook installed, exactly one more with a granting hook, and the same number for two rows as for six. Mutations: removing either listing loop's downgrade independently, and swapping the predicate for the name-only one, which the renamed builtin-OAuth case is there to catch. That last mutation fails on the request reaching config building and being rejected by the existing transport="oauth" schema validation, so it demonstrates that the predicate discriminates rather than that this particular row shape carried a reachable write.
• The row-lock, rename-ordering and lock-order tests run against PostgreSQL. FOR UPDATE is a no-op on SQLite, so the SQLite run is explicitly not evidence for them; the files are registered in the workflow that provisions a database, both in its path filter and as their own steps. The PUT-versus-DELETE lock-order test was added to the existing Custom API lock file rather than a new one — no new *_postgresql.py file is introduced by this work, so neither of that workflow's two hand-maintained path lists needed a line changed. The SQLite side of the same property is covered separately as a statement-order test, which asserts a count rather than a presence, because the route's own not-found guard already lazy-loads the definition row before any delete. In the session-fault suite, two route-level tests no longer perform a rollback of their own between the poisoning hook and their verification query, so that query is the statement that reaches the aborted transaction: those two are independently sensitive to the production restore now, and only under PostgreSQL. The suite's other two route-level tests issue no statement after the route call at all, so they remain a correctness check rather than a pin, and the module docstring says which is which.

Both production files these locks live in — src/xagent/web/api/mcp.py and src/xagent/web/api/custom_api.py — are registered in that workflow's path filter now, alongside connector_team_scope.py and both lock test files, in both the push trigger's path list and its pull-request mirror. A later production-only change to either lock, even one that never touches a test file, now re-triggers the suite that can tell PostgreSQL row locking apart from a SQLite no-op.

Relationship to other open work

• fix(mcp): offer a team-shared catalog connector in the connector picker #1403 also edits src/xagent/web/api/mcp.py and src/xagent/web/services/connector_team_scope.py. This change adds a keyword to set_connector_team_hooks and a dataclass to the seam; whichever lands second absorbs the conflict. The changes are textually adjacent but semantically independent — this one does not touch the connector picker, list_mcp_apps, or any attachability predicate.
• Ownership badge hidden for a resolver-credentialed mcp_oauth connector: is_connected disagrees with can_attach #1623 concerns _mcp_oauth_server_is_actually_connected and _local_mcp_can_attach, immediate siblings of _local_mcp_can_configure. This change touches only _local_mcp_can_configure and alters no connection-state predicate.
• Other users' OAuth grants are cascade-deleted without external revocation when an MCP server definition is removed #1637 is on the deletion path. This change touches that path only for the Custom API lock order described above, and alters none of its authorisation or outcome.

Filed rather than carried here, so the gaps are tracked instead of implied:

• delete_mcp_server calls the connector team seam synchronously on the event loop #1818 — delete_mcp_server reaches the seam and stays a coroutine, because it awaits an external revocation call.
• Refactor update_mcp_server: resolve the team verdict once, after the lock #1815 — resolve the MCP PUT's verdict once, after the lock, and drop the personal-only exemption's separate existence.
• Four test-quality gaps in the connector team access suites #1816 — four test-quality gaps: a lock-order test that cannot fail for the reason it states, assertions that restate a payload's own omission, negative timing windows with no upper bound, and query-cost parity with no hook installed.
• PUT /api/mcp/servers/{id} rewrites managed to "external" on every update #1817 — managed is rewritten on every MCP PUT, in code this diff does not touch.
• load_team_mcp_env never restores the shared session after a failing hook #1837 — load_team_mcp_env never restores the shared session after a failing hook; the same seam shape, on a path outside this diff.
• list_custom_apis omits Custom APIs the caller can reach by id #1877 — GET /api/custom-apis lists only the caller's own association rows, so a Custom API they can reach and edit by id may be missing from their own list.
• The connector visibility hook's answer is never shape-checked #1878 — the connector visibility hook's answer is the one hook answer never shape-checked, so a malformed one surfaces as a raw KeyError rather than the seam's typed error.
• list_mcp_apps treats its two hook failures differently #1879 — list_mcp_apps lets one of its two hook failures reach the generic handler while the other degrades.
• The connector seam expresses "consult or skip" and "raise or degrade" five ways #1880 — the seam expresses "consult or skip" and "raise or degrade" in five ad-hoc ways across two spellings, and its contract names no point in time a pair of hook answers is supposed to describe.
• A failing connector access hook blocks a PUT that only changes the caller's own association fields #1775, Connector headers values round-trip in cleartext while a masking comment claims otherwise #1701, The Custom API form declares a can_edit capability but never reads it #1702, Whose capability do list fields describe when an admin views another user's connectors? #1703 — carried from earlier reviews, unchanged here.

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[AlexLiu190625]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a team access hook (ConnectorAccessHook) to resolve team-level permissions for Custom APIs and MCP servers when a user lacks a personal association row. It updates the retrieval, modification, and listing endpoints to integrate this fallback mechanism, backed by comprehensive new test suites. The review feedback suggests enhancing runtime safety by replacing type casts with explicit conditional checks and raising ValueError exceptions when validating stand-in connectors during updates.

[rogercloud]

This PR adds an optional application-provided ConnectorAccess hook so team members without UserMCPServer/UserCustomApi rows can reach shared MCP and Custom API definitions, while preserving the no-hook standalone path. It threads that verdict through item gates, aggregate permission fields, personal-field refusal, degradation handling, and MCP row-lock/rename behavior, with extensive route and PostgreSQL coverage. Blocking: yes — recommended event: REQUEST_CHANGES

Re-review update

No commits were pushed after the reviewed HEAD. The only substantive follow-up was the author's reply to the two prior cast comments; this pass independently checked the complete current diff, all supplied PR history and linked-issue discussions, and every occurrence of each normalized root rather than treating those replies as fixes.

Approach verdict

acceptable-with-reservations. The central direction is sound: an application-owned resource verdict avoids embedding team/role vocabulary in xagent, removes the assumption that a personal association is the only authorization source, and leaves deletion, OAuth, toggle gates, and no-hook behavior outside the requested expansion. The reservations are at the boundaries: the verdict validator is not actually fail-closed, read visibility is conflated with secret-bearing configuration access, the MCP and Custom API write paths have different concurrency guarantees, and the singular hook plus UI capability hint leave scaling and end-to-end contracts implicit.

Findings

Major

1. ConnectorAccess validation can fail open across the tenant boundary

• Location: src/xagent/web/services/connector_team_scope.py:239-265; consumed by src/xagent/web/api/mcp.py:1404-1406,1553-1565 and src/xagent/web/api/custom_api.py:331-346,402-405
• Severity: major
• Impact: _validate_connector_access_answer accepts ConnectorAccess(False, False) even though None is documented as the only “not linked” answer. It also accepts truthy strings/integers because it checks truthiness rather than exact booleans. Both resolvers treat any non-None object as a link, and both edit predicates call bool(...), so a malformed can_edit such as "false" or 1 can grant shared writes while the false/false form can bypass the 404 gate and expose a definition.
• Fix: Make the seam reject malformed answers before they reach routes: require type(team_owned) is bool, type(can_edit) is bool, and team_owned is True for every non-None answer (or remove the redundant team_owned field and use None as the sole no-link value). Add route-level tests proving false/false and non-boolean answers produce the typed 503/404-safe behavior and cannot edit.

2. Custom API rename propagation is not serialized for the new team-writer population

• Location: src/xagent/web/api/custom_api.py:424-438,502-510
• Severity: major
• Impact: Custom PUT captures old_name before any row lock and invokes the selector-rewrite hook without FOR UPDATE. Two team editors can both read A; after the first commits A→B, the second still calls the hook with A→C, so selectors rewritten to B are not found and the definition ends at C while agents still select B.
• Fix: Mirror the MCP ordering: refresh the CustomApi row with populate_existing().with_for_update(), capture old_name only after the lock, and keep mutation, rename_team_connector, and commit in that transaction. Add a two-session PostgreSQL test that asserts the second hook receives the first committed name.

3. View-only team access reaches secret-bearing GET responses

• Location: src/xagent/web/api/custom_api.py:128-145,337-373; src/xagent/web/api/mcp.py:1559-1607,1635-1674
• Severity: major
• Impact: The PR-new direct GET fallback admits any non-None verdict, including a valid team_owned=True, can_edit=False answer. Custom responses copy headers unchanged, and MCP responses mask env/auth fields but leave static config.headers unchanged; Authorization or X-API-Key stored there can therefore be copied by a member who is explicitly not allowed to edit. The aggregate visibility path already had some of this projection, but the new access-only/visibility-mismatched direct-ID population is newly reachable.
• Fix: Separate “may view metadata,” “may view configuration,” and “may edit” in the access contract, or require an appropriate view/edit capability for the full response and return a secret-safe projection otherwise. Independently mask or omit sensitive static header values in both connector response builders, with tests for view-only Custom API and MCP headers.

4. MCP PUT uses a stale team grant after revoke/unshare can commit

• Location: src/xagent/web/api/mcp.py:3472-3515,3530-3648; access contract at src/xagent/web/services/connector_team_scope.py:268-285
• Severity: major
• Impact: The access hook runs before the later MCPServer FOR UPDATE, and the lock refreshes only the definition. If an application revokes the team link after the hook returns but before the definition lock/write, the request retains can_edit_global=True and can commit shared command, URL, header, auth, description, or name changes after revocation. The new lock serializes definition writers, not the external authorization state.
• Fix: Define one authorization linearization point and lock order: for example, lock the definition first, then revalidate/lock the application-owned grant in the same session through a lock-aware hook, and require revoke paths to use the same order. If that cannot be transactional, return a versioned verdict and revalidate before commit; add a two-connection revoke-vs-edit PostgreSQL test with no write after revoke.

5. Catching a hook failure without recovering the Session breaks the advertised degradation paths

• Location: src/xagent/web/api/mcp.py:2482-2495,2598-2611,3247-3261,3947-3961; wrapper at src/xagent/web/services/connector_team_scope.py:350-367
• Severity: major
• Impact: The hook receives the endpoint's live SQLAlchemy Session. A DB-backed hook failure can leave its transaction inactive, but each degradation catch only logs and continues. The MCP apps loop then issues more SQL, while connect/toggle serialize an expired server after their durable commit; those operations can raise PendingRollbackError/equivalent and return 500 even though the write landed. The Custom API loop shares the same poisoned-session invariant even when it happens to be the last SQL-producing loop.
• Fix: Use an isolated read-only session/transaction for decoration, or explicitly rollback and reload all ORM state before continuing/serializing. Add tests whose hook executes a failing database operation (not only ValueError) and verify list, connect, and toggle remain usable with durable writes.

Minor

6. Optional access-hook failure unnecessarily blocks personal non-owner operations and /servers

• Location: src/xagent/web/api/mcp.py:1549-1557,2699-2713,2735-2742,2795-2800
• Severity: minor
• Impact: A personal non-owner row already establishes existence and can update its own user_env/is_active, but GET and PUT still force an access resolution before reaching those paths; a hook failure becomes 503. The aggregate /api/mcp/servers loop also calls the hook without a per-row catch, so one decoration failure blanks the entire list. The /api/mcp/apps loops already demonstrate the intended per-row degradation, making this inconsistency in scope.
• Fix: Keep fail-closed resolution where a no-personal-row request needs the verdict as its gate, but resolve best-effort for existing personal rows and personal-only PUTs, and catch decoration failures per /servers row with can_edit_global=False while continuing the list.

7. Singular access hook calls make list authorization N+1 by contract

• Location: src/xagent/web/api/mcp.py:2485-2487,2600-2603,2707-2712,2739-2741,2762-2784; hook signature at src/xagent/web/services/connector_team_scope.py:63,268-285
• Severity: minor
• Impact: The new singular callback is invoked once for every applicable personal non-owner or team-only MCP/Custom API row, including both aggregate lists and local apps. A normal host implementation that checks its link/membership tables performs O(N) extra database round trips; the current budget test uses a no-op callback and therefore does not measure that cost.
• Fix: Add a request-scoped/batch access resolver returning an id-to-verdict map (or an explicit per-request cache/preload contract), while retaining singleton resolution for item GET/PUT. Extend the query-budget coverage to both connector kinds, /apps, and a DB-backed hook.

8. The PostgreSQL lock test is not triggered by production mcp.py changes

• Location: .github/workflows/test-migrations.yml:52,138,442-447
• Severity: minor
• Impact: Both the push path filter and RELEVANT_PATHS include the new lock test but omit src/xagent/web/api/mcp.py. A later PR that changes only the production lock/ordering causes the detector to skip the PostgreSQL step (and a main push may skip the workflow entirely), so the only test that distinguishes real PostgreSQL locking from SQLite behavior is not a durable regression gate.
• Fix: Add src/xagent/web/api/mcp.py to both mirrored path lists; add custom_api.py as well if the Custom lock regression is introduced.

9. can_configure opens a non-actionable Save form for view-only team users

• Location: src/xagent/web/api/mcp.py:2164-2207,2475-2529,2595-2643
• Severity: minor
• Impact: The PR makes can_configure=True for any non-None team verdict, including a no-personal-row view-only user. The picker interprets that field as an actionable Configure affordance and mounts Save-capable forms; MCP personal user_env then returns 400 for the stand-in, and Custom API writes return 403. The backend refusal is safe, but the new user population is presented with a dead-end editing workflow.
• Fix: Either make can_configure mean actionable edit capability and require the appropriate can_edit/personal capability, or expose separate view/edit fields and propagate has_personal_association/editability to render a genuinely read-only form with Save and stand-in-only fields disabled.

10. Admin target inspection reports a Custom API capability for the wrong subject

• Location: src/xagent/web/api/mcp.py:2736-2744,1669-1670; direct gates at src/xagent/web/api/custom_api.py:349-409
• Severity: minor
• Impact: For /api/mcp/servers?user_id=target, the new Custom API projection resolves team access for the inspected target and can report can_edit_global=True, but the actual Custom API GET/PUT resolves the current admin's own association/access and has no platform-admin bypass. An admin can therefore see an apparently editable target row and receive 404/403 when acting as self. The analogous MCP mismatch is pre-existing and is not reported here.
• Fix: Decide whether the field describes the inspected target or the acting principal. Compute it from the same subject as the route clients will call, or expose separate target capability and actor capability fields; add an admin-inspects-other regression with different verdicts.

11. Standalone parity coverage omits changed routes and populations

• Location: tests/web/api/test_mcp_reported_edit_permission.py:616-695; changed no-hook paths include src/xagent/web/api/mcp.py:2415,2703-2744,3237-3265
• Severity: minor
• Impact: The new parity test covers an owner and an unrelated stranger, but not a personal non-owner, /api/mcp/apps, connect_mcp_app, or non-owner aggregate rows. The no-hook resolver currently returns None, so this is a coverage gap rather than a demonstrated production regression; the test nevertheless cannot support the PR's claim that every changed route is unchanged standalone.
• Fix: Add no-hook owner/non-owner/no-row cases for MCP and Custom API across detail, PUT, toggle, aggregate list, local apps, and connect, asserting the relevant response and refusal contracts against the base behavior.

Prior finding status: DROPPED / VERIFIED-SAFE

The two earlier cast comments are one canonical root, not two current findings: custom_api.py:515 / review comment 3850984405 and mcp.py:3644 / review comment 3850984420. Current code has explicit pre-write stand-in guards at custom_api.py:418-422 and mcp.py:3492-3501, covering the only personal-field writes before shared mutation; there is no executable assert to replace, and a write-site ValueError would incorrectly become a 500. The author's replies (3851132199 and 3851132669) are technically correct. Status is DROPPED/VERIFIED-SAFE, not “fixed,” and the root is not re-reported.

The native /api/custom-apis collection asymmetry is pre-existing and outside this PR's stated item-gate/permission scope; the suspected private stand-in import runtime hazard is not reachable in the current module graph; and the repository-level documentation omission is not actionable under the established source-only convention for application-owned hooks. These are dropped scope decisions, not findings.

Review limitations

No local tests, builds, linters, or formatters were run under the review default. CI was green at preflight. The Simplification Lens was unavailable because its agent hit the usage limit, so no Simplification opportunities section is included.

Blocking status & recommended decision

Blocking: yes

• src/xagent/web/services/connector_team_scope.py:260 — major — malformed/false ConnectorAccess answers can bypass the no-link gate or grant shared edits. [new] Recommended event: REQUEST_CHANGES
• src/xagent/web/api/custom_api.py:438 — major — concurrent team Custom API renames can leave selector names behind the committed definition. [new] Recommended event: REQUEST_CHANGES
• src/xagent/web/api/custom_api.py:337 — major — view-only access can retrieve raw static credential headers through the new direct GET path. [new] Recommended event: REQUEST_CHANGES
• src/xagent/web/api/mcp.py:3503 — major — a revoke can commit after the hook verdict and before the locked MCP write, leaving a stale edit grant. [new] Recommended event: REQUEST_CHANGES
• src/xagent/web/api/mcp.py:2488 — major — a DB-backed hook failure poisons the request Session and can turn degraded/list or post-commit responses into 500s. [new] Recommended event: REQUEST_CHANGES

[rogercloud]

This PR adds an optional application-owned ConnectorAccess batch hook so team members without personal association rows can discover, read, and (when granted) edit shared MCP and Custom API definitions while retaining standalone no-hook behavior. It threads verdicts through list/detail/write paths, adds stand-in personal-field guards, definition-row locks and rename handling, session-failure recovery, PostgreSQL lock coverage, and extensive parity/permission tests. The current head contains ten follow-up commits after a1af5f40 that improve batching, lock ordering, degradation, CI triggers, and test coverage, but authorization, concurrency, and false-success paths remain.
Blocking: yes — recommended event: REQUEST_CHANGES

Approach verdict

acceptable-with-reservations. The central approach is sound: an application-owned, batched access verdict avoids fabricating one association row per team member and keeps tenancy policy outside xagent. The validator boundary, fail-closed gate/decorate distinction, list batching, and definition-row locking are coherent extensions of the existing routes. The reservations are at the integration boundaries: the access verdict is not fenced against an application-side revoke, the new synchronous lock runs inside an async route, the Custom API lock order conflicts with deletion, and the widened team-only population is not represented precisely enough in the UI and no-hook test contract. This design verdict is independent of the number of findings that remain open.

Update summary — ten commits after a1af5f40

• bf5ca765 batches and types the connector-scope access answer.
• 8d54722c pins the apps-listing hook/query budget.
• f093c86 restores the shared session after a failed team hook.
• 96639310 degrades verdict failures on read paths while keeping writes fail-closed.
• bf4cd778 locks the Custom API definition before rename propagation.
• 5e7cad68 registers connector lock suites in the migration workflow triggers.
• 1ee7a488 expands the standalone matrix to both constructible populations.
• a9508828 pins the reported subject for admin inspection.
• d7eb382e strengthens the admin-subject test against a leaked bypass.
• 25f15430 prevents the apps-listing degradation log from reading a poisoned ORM session.

Prior-finding status checklist

• cast-guards (3850984405, 3850984420, replies 3851132199, 3851132669) — DROPPED / VERIFIED-SAFE; current pre-write HTTPException(400) guards cover the personal-field writes, and there is no executable assertion to replace.
• access-validator (3851854486, 3859250258) — FIXED; exact team_owned/can_edit checks and typed wrappers now reject the previously reported shapes.
• custom-rename-lock (3851854491, 3859250894) — FIXED; the Custom API definition is locked before old_name is read and rename propagation is committed in the same transaction.
• header-secrets (3851854493, 3859268318) — DROPPED / TRACKED in #1701; the cleartext header projection predates this PR, although the widened reachability makes the follow-up important.
• stale-team-grant (3851854501, 3859268765) — NOT FIXED, major; reported below as [prior].
• session-recovery (3851854506, 3859269181) — FIXED; both typed and generic failure arms roll back, and current degradation call sites use the wrappers.
• list-degradation (3851854512, 3859269705) — PARTIAL, minor; /servers and GET degradation were added, but a personal-only MCP PUT still resolves the hook before its personal-field path and can return 503.
• hook-batching (3851854517, 3859270172) — REFACTORED / FIXED; list paths use one batch call and tests pin a constant call/query budget.
• ci-lock-trigger (3851854522, 3859270662) — FIXED; production and test paths are mirrored in the workflow filters and dedicated PostgreSQL steps.
• configure-affordance (3851854525, 3859273763) — NOT FIXED, minor; a linked but non-editable stand-in still receives an actionable Configure/Save affordance.
• admin-subject (3851854537, 3859280152) — DROPPED / TRACKED in #1703; the target/actor split predates this PR and the new regression test pins the current behavior.
• standalone-parity (3851854546, 3859284376) — PARTIAL, minor; the no-hook matrix still asserts only MCP rows even though Custom API projections changed.

The separate private stand-in coupling note (custom_api.py:26-28,310-311) is not repeated: the consolidated verification found it style-only and runtime-safe.

Confirmed findings

Major

1. [prior] A team grant can be revoked after the verdict and still authorize the write

• Location: src/xagent/web/api/mcp.py:3579-3588,3614-3633,3761
• Severity: major
• Blocking: yes
• Impact: _resolve_mcp_server_for_request captures team_access before update_mcp_server takes the MCPServer ... FOR UPDATE lock. The captured can_edit value is then used through the shared-field mutation and commit without a grant version, lock-aware recheck, or fence. If the installing application revokes/unshares the connector after the hook returns, this request can still commit a shared command, URL, description, auth, or name change after the caller is no longer authorized.
• Fix: Make the access answer versioned or lock-aware and revalidate it after the definition row is locked, with the application-side revoke path using the same fence/lock order; add a PostgreSQL revoke-vs-edit interleaving test that proves no write commits after revocation.

2. [new] The Custom API FOR UPDATE wait blocks the event loop

• Location: src/xagent/web/api/custom_api.py:376-382,431-436
• Severity: major
• Blocking: yes
• Impact: update_custom_api remains async def, but the new .with_for_update().first() executes synchronously on the injected SQLAlchemy Session. A concurrent writer holding the row lock makes this synchronous wait block the FastAPI event loop, stalling unrelated async requests and creating a material availability regression under normal concurrent edits.
• Fix: Convert this route to a synchronous def so FastAPI runs the blocking Session work in its threadpool, or migrate the whole path to AsyncSession and an async driver. Add an ASGI responsiveness test while a competing transaction holds the lock.

3. [new] Custom API PUT and last-association delete acquire locks in opposite order

• Location: src/xagent/web/api/custom_api.py:431-436,538-542,594-604
• Severity: major
• Blocking: yes
• Impact: The new PUT path locks the CustomApi definition first and later updates UserCustomApi. The team-owned delete path flushes/deletes UserCustomApi first and then deletes the parent definition when it was the last association. An is_active-only/team edit concurrent with last-association deletion can therefore hold one row lock while waiting for the other, producing PostgreSQL 40P01 and a 500.
• Fix: Establish one lock order for both routes—prefer locking the definition before the association mutation in delete, or consistently lock the association before the definition in PUT—and add a barrier-synchronized PostgreSQL edit/delete interleaving test.

4. [new] Python tuple aliases let a malformed access key reach the grant map

• Location: src/xagent/web/services/connector_team_scope.py:279-306
• Severity: major
• Blocking: yes
• Impact: resolve_connector_access normalizes requested IDs with int(), then _validate_connector_access_answer checks key not in requested using ordinary tuple equality. A hook key such as ('mcp', True) or ('mcp', 1.0) compares equal to ('mcp', 1), survives validation under its malformed key, and is found by downstream .get(('mcp', 1)); the answer can therefore grant shared writes or bypass the no-link 404 with a non-canonical reference.
• Fix: Validate the key shape and types before membership—especially reject bool and non-int IDs—and require the key to equal a canonical (ConnectorType, int) tuple only after exact-type validation. Add tests for boolean and float aliases for both connector kinds.

5. [new] A denying team-only MCP PUT can return a false-success 200

• Location: src/xagent/web/api/mcp.py:3582-3588,3640-3646,3761-3774
• Severity: major
• Blocking: yes
• Impact: A valid stand-in with can_edit=False passes the route gate. If the payload is empty, unchanged, or changes a secret field that _global_config_tampered deliberately cannot compare, the route empties incoming_config, commits, and returns 200 even though this caller has no personal association and no writable shared capability. The existing TestDenyingVerdictIsFalseEverywhere explicitly obtains this successful empty PUT response, so clients can observe a successful write that did nothing.
• Fix: After the existing stand-in personal-field 400 guard, return 403 immediately for is_stand_in and not can_edit_global. Keep the prior mixed global/personal behavior for real personal rows, and assert 403 plus no side effects for a denying stand-in.

Minor

6. [prior] Hook failure still blocks personal-only MCP updates

• Location: src/xagent/web/api/mcp.py:3579-3581,3590-3606,3735-3753
• Severity: minor
• Blocking: no
• Impact: A personal non-owner already has the association required to update user_env or is_active, but the route resolves the team hook before reaching those branches. A transient DB-backed hook failure therefore returns 503 for a personal-only update even though the team verdict is only decoration for that operation.
• Fix: Resolve the personal row first and invoke the access hook only when stand-in reachability or shared-config authorization is needed; otherwise degrade the decoration and continue the personal-only update. Preserve fail-closed behavior for team-only/shared writes and add a poisoned-hook personal-only regression test.

7. [prior] can_configure still opens a dead-end form for view-only stand-ins

• Location: src/xagent/web/api/mcp.py:2196-2239,2607-2615,2687-2696
• Severity: minor
• Blocking: no
• Impact: _local_mcp_can_configure returns true for any non-None team verdict, including can_edit=False with no personal association. The unchanged picker opens Configure and Save-capable forms; MCP stand-in personal fields are rejected with 400, while a Custom API save is rejected with 403 (and the new MCP denying path can otherwise false-success as described above).
• Fix: I saw the reply that can_configure intentionally means route reachability and that the form-side issue was filed as #1702. That does not remove the new hook-only population's dead-end UX: either make this field mean an actually writable capability, or return separate view/edit and has_personal_association capabilities and render a genuinely read-only form. Add a view-only stand-in UI/API contract test.

8. [prior] No-hook parity still omits the changed Custom API projections

• Location: tests/web/api/test_mcp_reported_edit_permission.py:820-823,1025-1044
• Severity: minor
• Blocking: no
• Impact: The new matrix's aggregate assertion selects only the MCP row, and the apps parity fixture also creates/asserts only an MCP row, even though this PR changes Custom API projection and team-overlay wiring. A no-hook regression in the Custom API aggregate or local-app path can therefore pass while the PR claims every changed route remains equivalent.
• Fix: I saw the reply that the owner/non-owner matrix and apps/connect legs were expanded in 1ee7a4889; those additions cover MCP but still leave the changed Custom API projections unpinned. Add Custom API rows and assertions for owner, personal non-owner, and no-row/no-hook cases across aggregate list, local apps, detail, and write contracts, or narrow the standalone-parity claim.

9. [new] Explicit null personal fields are treated as omitted for stand-ins

• Location: src/xagent/web/api/mcp.py:3597-3606; src/xagent/web/api/custom_api.py:418-422
• Severity: minor
• Blocking: no
• Impact: Pydantic records an explicit JSON null in model_fields_set, but both guards test only value is not None. A stand-in can therefore submit user_env: null or is_active: null, bypass the 400 personal-field guard, commit, and receive 200 as though it had supplied no personal field.
• Fix: Use model_fields_set together with the value check and reject a present personal field whose value is null for stand-ins; mirror the rule in MCP and Custom API tests.

10. [new] Session rollback causes query storms in degraded listings

• Location: src/xagent/web/services/connector_team_scope.py:361; callers src/xagent/web/api/mcp.py:2544,2814,2557-2704,2826-2885
• Severity: minor
• Blocking: no
• Impact: _restore_session_after_hook_failure correctly rolls back the shared Session, but that rollback expires the already-loaded ORM identity-map state. The degraded /apps and /servers loops then lazily refresh rows one by one; the current targeted repro observed roughly 7 + 2N SELECTs for /apps and 5 + 2N + 1 for /servers after a failed hook, while both responses still return 200.
• Fix: Run hook resolution in an isolated session/transaction, or fully materialize/refresh the response rows before rolling back the route Session. Add failure-path query-budget assertions so recovery does not become an N-dependent database load.

11. [new/design] Visibility and access hooks can disagree about the same connector

• Location: src/xagent/web/api/mcp.py:1504-1599,2764-2816; src/xagent/web/services/connector_team_scope.py:138-175,310-336
• Severity: minor
• Blocking: no
• Design-only: this is a cross-hook contract concern, so it has no inline anchor.
• Impact: List membership comes from visible_team_connector_ids, while direct detail/PUT reachability and edit authority come from ConnectorAccess. An application can consequently make a connector visible but direct-ID-inaccessible, or direct-ID-editable but absent from lists; the two answers have no shared identity/version invariant.
• Fix: Document that both hooks must derive from one application-level link query and add an integration test for list/detail/PUT agreement, or replace the split with one richer batch resolver returning visibility and capabilities together.

Review limitations

No local tests, builds, linters, formatters, or installs were run under the review default. CI preflight reported zero failing checks and all visible checks succeeded. The Simplification Lens and automated history extractor both exhausted usage-limit retries; no simplification findings are asserted, and the prior history statuses above use the manually reconstructed records and the consolidated evidence instead.

Blocking status & recommended decision

Blocking: yes
Recommended event: REQUEST_CHANGES

Blocking issues:

• src/xagent/web/api/mcp.py:3579 — major — a team edit can commit after the application revokes the grant because the pre-lock verdict is never fenced or revalidated. [prior]
• src/xagent/web/api/custom_api.py:436 — major — a synchronous FOR UPDATE wait inside the async PUT route can stall the event loop and unrelated requests. [new]
• src/xagent/web/api/custom_api.py:431 — major — PUT/delete lock inversion can deadlock PostgreSQL during concurrent edit and last-association deletion. [new]
• src/xagent/web/services/connector_team_scope.py:280 — major — Python equality aliases can turn a malformed hook key into an authorized connector grant. [new]
• src/xagent/web/api/mcp.py:3583 — major — a denying stand-in can receive a false-success 200 for a request that performs no mutation. [new]

The recommended event is REQUEST_CHANGES because the author is not qinxuye and each listed issue has a concrete supported-path trigger with product impact.

[rogercloud]

Minor I saw your reply in 3859269705 that PUT should remain fail-closed because the team verdict is an authorization gate. That is correct for stand-in/shared-config writes, but not for a personal non-owner PUT whose payload only changes association-owned user_env or is_active: this resolver call happens before those branches, so a transient hook failure still returns 503 instead of preserving the personal operation. Resolve the personal row first and invoke/degrade the hook only when stand-in reachability or shared-config authorization is needed; keep fail-closed behavior for team-only writes and add a poisoned-hook personal-only regression test.

[AlexLiu190625]

You are right, and I need to correct what I wrote earlier in this thread before anything else.

Correcting my earlier reply. I wrote:

PUT's verdict is a gate, not a report; keeping it fail-closed is the same distinction the rest of this PR's failure-typing is built on, not an inconsistency to reconcile.

That is true for a shared-config write. It is not true for the payload you singled out, and stating it without that qualification was wrong. For a payload that only carries user_env or is_active, the verdict is not a gate on this route — it is a reported field and nothing else.

Here is the check, on the three places update_mcp_server uses the verdict (line numbers at 1f7101858):

use	reachable for a user_env/is_active-only payload?
the tamper 403: if not can_edit_global: if _global_config_tampered(server_data, server): 403	No. Every branch of _global_config_tampered compares a field only when that field is present in the payload — name, transport, description, the comparable global config keys, the runtime declaration fields, the non-secret auth metadata. With none of them present it returns False unconditionally, so the 403 is unreachable.
the rename branch, if can_edit_global and server_data.name and ...	No. Unreachable without name in the payload.
team_access=team_access passed to the response builder	Yes — this is the reported can_edit_global field.

And the write itself does not consult it. mcp.py:3811 writes server_data.user_env onto the caller's own association row and mcp.py:3827 writes server_data.is_active onto the same row; neither reads can_edit_global or team_access. So for this payload class the verdict decorates one response field, and a hook outage turning that into a 503 blocks a write the caller was entitled to make on the strength of their own association row.

Measured at 1f7101858, not reasoned about. Same caller (a personal non-owner), same payload ({"is_active": false}), only the hook's health differs:

healthy hook             -> 200 ''                                 member_is_active=False
raising hook             -> 503 'Connector access is unavailable.' member_is_active=True

Identical request, identical entitlement, different answer depending on an optional component's health.

I am not fixing it in this PR, and I want to give you the real reason rather than a scope wave. The natural fix — resolve the caller's personal row first and invoke the verdict only when stand-in reachability or shared-config authorisation actually needs it — splits _resolve_mcp_server_for_request, a helper both GET and PUT go through. This branch has already restructured that helper twice (the degrade-versus-raise flag, and the batched access contract); a third structural change to it, landing alongside five other behaviour changes in the same review round, is where a contract change stops being reviewable. There is also a genuine question underneath it that a patch would settle silently: this seam's own failure-policy table says a raising hook is a typed 503 for any payload on this route, while the later and more general rule — fail closed where the verdict gates, degrade where it only decorates — says this payload class should degrade. Those two rules disagree about this exact cell, the second was written after the first, and nobody re-swept the first against it. Choosing which one governs is the actual decision, and it should be made in the open rather than inside a fix commit.

Filed as 1775, with the reproduce steps, the three-use analysis above, and the rule conflict written out.

One thing to be precise about for the other connector kind: PUT /api/custom-apis/{id} is not affected the same way. Its gate at custom_api.py:411-415 requires the edit right for every payload it admits, including an is_active-only one, so on that route the verdict genuinely is the gate and fail-closed is the right answer there.

Leaving this thread open — the finding stands, and the decision it needs is in the linked issue rather than in this branch.

[rogercloud]

Confirmed still PARTIAL at current HEAD (1f71018): GET/listing now degrade gracefully, but this PUT path still resolves the team verdict unconditionally before checking whether the payload is personal-only, and can 503 on hook failure for a write the verdict never gates. Acknowledged as deferred to #1775 -- not blocking, see the full re-review body for details.

[rogercloud]

Minor I saw your reply in 3859273763 that can_configure intentionally means route reachability and that the form-side gap was filed as #1702. That explanation does not remove this PR's new hook-only population: returning true for a can_edit=False stand-in advertises Configure/Save even though MCP personal fields return 400 and Custom API saves return 403 (and the denying MCP path can otherwise false-success). Either make this field represent an actually writable capability, or return explicit view/edit and has_personal_association capabilities and render a read-only form; add a view-only stand-in contract test.

[AlexLiu190625]

You are right that my earlier answer did not address what you actually raised, and I want to separate the two claims in it, because one holds and one does not.

What holds: the field's meaning is unchanged. _local_mcp_can_configure has always answered "can this caller reach the configure route at all", never "can this caller write something". It has never been a permission — the routes gate independently.

What does not hold: my reply implied nothing changed. Something did. This PR added the second term:

$ git blame -L 2243,2243 1f7101858 -- src/xagent/web/api/mcp.py
aabdac492d (Alexliu 2026-08-24) return association is not None or team_access is not None

Before that term, the population seeing can_configure: true was "callers with at least one personal association row". After it, it also includes callers with no association row at all. The semantics stayed put; the set of people the field says true to got wider, and the widened part is precisely the population for whom the form is a dead end. That is your finding, and it stands.

What changed on this branch since that reply: the MCP PUT now refuses a stand-in whose verdict denies edit with a 403 rather than returning a 200 that wrote nothing (54770643120661a86095c897061a03c46d7ecc47, replied to in the sibling thread). So for this population the picture is now: press Configure, fill the form, press Save, get a clear refusal. On the Custom API side that was already the behaviour. The false-success half of what you described is gone; the dead-end half is not.

Where the remaining gap actually is. The API already reports the distinction the form needs. can_edit_global is emitted for every row on the listing, on GET, and on the write response, and it is false for exactly this population. The form does not read it — it opens editable fields and a Save button regardless. That gap is filed as #1702, which is where the fix belongs: it is a rendering decision in the client, not a change to what this predicate answers.

I have deliberately not made can_configure mean "actually writable" here. It is the input to the four routes' first gate, and those routes admit this caller — a stand-in can open the connector and read it. Making the field mean writability would make it disagree with the reachability its own call sites test for, and would also hide a connector the caller is genuinely allowed to view. Splitting it into explicit view/edit plus has_personal_association capabilities is a defensible API shape, but it is a response-contract change with client work attached, and it should be decided in #1702 alongside the form work rather than added as a sixth behaviour change to this branch.

Leaving this thread open rather than resolving it — the API-shape question is yours to close, and I would rather you weigh in than have me declare it settled.

[rogercloud]

Confirmed still NOT FIXED at current HEAD (1f71018): can_configure still returns true for a denying stand-in. The false-success/silent-no-op half of the impact is gone now that PUT correctly 403s (see the new stand-in guard), so this is purely a dead-end-UX issue, not a security concern. Acknowledged as deferred to #1702 -- not blocking, see the full re-review body for details.

[rogercloud]

Minor This guard checks only values, so an explicit JSON null in user_env or is_active is absent from the condition even though Pydantic records the field in model_fields_set; the stand-in can commit and receive 200 as if the personal field were omitted. Use model_fields_set to reject a present null personal field for stand-ins, and mirror the same fix in custom_api.py:418 with tests for both routes.

[AlexLiu190625]

Your description of the mechanism is exactly right — Pydantic does record an explicit JSON null in model_fields_set, and this guard tests values only, so a null sails past it. I am not changing the guard, and here is the measurement that convinced me it makes no difference.

The write sites use the same is not None test. At 1f7101858:

mcp.py:3811          if server_data.user_env is not None:
mcp.py:3827          if server_data.is_active is not None:
custom_api.py:574    if api_data.is_active is not None:

So "user_env": null and "no user_env key at all" are byte-for-byte synonymous on both routes: neither reaches the guard, and neither reaches a write. Nothing is committed, because there is nothing to commit.

Measured at 1f7101858, driving the route functions directly (member_rows is the number of association rows this caller owns afterwards, owner_is_active is read back after expiry):

MCP  granting stand-in, both nulls     fields_set=['is_active', 'user_env'] -> 200 ''    | member_rows=0 owner_is_active=True
MCP  granting stand-in, is_active=False fields_set=['is_active']            -> 400 'No personal connection exists to configure u' | member_rows=0 owner_is_active=True
MCP  denying stand-in, both nulls      fields_set=['is_active', 'user_env'] -> 403 'You do not have permission to edit this MCP ' | member_rows=0 owner_is_active=True
CAPI granting stand-in, is_active=null  fields_set=['is_active']            -> 200 ''    | member_rows=0 owner_is_active=True
CAPI granting stand-in, is_active=False fields_set=['is_active']            -> 400 'No personal connection exists to configure i' | member_rows=0 owner_is_active=True

Reading those five rows:

• The null payloads return 200 with zero rows created and the definition untouched — the 200 is for a request that asked for nothing, not for a personal write that leaked through.
• The control rows (a real false) are 400 on both kinds, so the guard does fire on the payloads it exists for.
• The third row is new since your review: a stand-in whose verdict denies edit now gets a 403 before this even matters, from the guard added in 54770643120661a86095c897061a03c46d7ecc47.

On switching to model_fields_set, one wider consequence. is not None is how this route spells "not supplied", for every caller population and both personal fields, not just for stand-ins. Changing only the stand-in branch to a presence test would make {"is_active": null} mean "omitted" for one population and "supplied" for another on the same route — a distinction with no behavioural difference behind it, since nothing writes on a null either way.

The same function does use model_fields_set for three other fields (runtime_input_schema, runtime_bindings, allow_delegated_authorization, at mcp.py:3803-3807), and that is not an inconsistency: for those three, null is a meaningful value — it clears the field — so presence and value genuinely differ. For user_env and is_active, null is not a value the model can store; there is nothing for it to mean.

If the goal is a clearer error for a client that sends explicit nulls, I would rather do that as an input-validation rule on the payload model, applied uniformly to every caller, than as a population-specific branch inside the write path. Happy to take that as a follow-up if you want it — I just do not think it belongs in this branch's guard, since no request outcome changes.

Leaving this thread open for you rather than resolving it, since I am declining rather than fixing.

[rogercloud]

Independently re-verified at current HEAD (1f71018): the write sites use the identical is not None check as this guard, so an explicit null is confirmed behaviorally inert (0 rows written either way), and no other side effect is unlocked by reaching past the guard. Your original explanation holds -- accepting as documented, not blocking. See the full re-review body for details.

[AlexLiu190625]

Thanks for this — the test-quality pass in particular found things no previous read had.

Everything with its own inline thread is answered there. This covers the items that only appear in the review body. Head is 14c52c461; ten commits since 1f710185.

---

5 — session restore covers 2 of ~5 hook call sites

Fixed in a721c8498, extended in 45c74f1d1, documented in 14c52c461.

There was no single invocation point to move the restore to — the fix had to build one. Before this, _restore_session_after_hook_failure had four call sites, all inside the two *_or_raise wrappers. That covers the access hook and the team-visibility hook; the visibility, delete and rename slots had nothing. Adding the same guard at three more callers would have been the wrong shape, so the restore now lives on the invocation itself:

# src/xagent/web/services/connector_team_scope.py:448-454 @ 14c52c461
def _call_connector_hook_gate(
    db: Any,
    hook: "Callable[..., _HookResult]",
    *args: Any,
    validate: "Callable[[Any], _HookResult] | None" = None,
    **kwargs: Any,
) -> _HookResult:

All five slots are called through it. The four restore calls inside the wrappers were deleted, so the repository has one mechanism rather than two. Classifying and translating the failure stays with the wrappers, which still own the typed-error contract; the door re-raises unchanged. A slot added to this module later inherits the restore without its author having to know it exists.

Test: test_every_hook_door_restores_the_session_when_the_hook_fails, parametrized over the three slots that previously had no restore at all. Mutation: deleting the door's except/restore/raise turns all three red with PendingRollbackError.

Sweeping this by class rather than by call site turned up a second axis the finding did not name. The door covered the hook call, but not the answer check that runs immediately after it for the two slots that have one. A hook that runs a failing statement, catches that itself, and answers with a shape the seam rejects therefore still left the session unusable, even with the door in place — the rejection is this module's own exception, raised outside the door. 45c74f1d1 moves validation inside the same try, passed in by the two call sites that have a validator; the three slots that validate nothing pass nothing, which is now what says at the call site that this seam checks nothing about those answers.

Test: test_a_hook_that_swallows_its_failure_and_answers_malformed_restores_too, parametrized over both validating slots. Mutation — restoring the pre-45c74f1d1 production file and re-running: both cases red, and red on the right line. The traceback lands on

    assert db_session.query(User).count() == 1

with PendingRollbackError carrying the original UNIQUE constraint failed: users.id — the hook's own swallowed insert. Not on the status-code assertion above it, which is a guard rather than the evidence. A second mutation, deleting both validate= arguments, produces 39 failures across the two suites: 37 pre-existing malformed-answer tests plus these 2.

One shape stays uncovered, and I would rather name it than let the invariant read as total. A hook that poisons the session, swallows its own failure, and still returns a well-formed answer raises nothing anywhere, so nothing triggers a restore. Closing it would mean probing the session's health after every hook call, which is a different design from a failure path. It is stated in the door's docstring (connector_team_scope.py:483-489) and in the PR description.

Related, and deliberately not in this PR: load_team_mcp_env has the same seam shape with no session restore at all. It is a pre-existing path this diff does not touch, filed as #1837.

---

6 — O(N) query cost in degraded listings

Position unchanged: the cost stays, and the query-budget tests pin it as a formula in the row count rather than removing it — 7 + 2N for the apps listing, 5 + 2N + 1 for the servers listing, against a constant 7 and 5 on the healthy path.

On the tracking: #1711 records the underlying library behaviour — a Session.rollback() expires every loaded attribute, so the next read of any of them reloads the whole row. That behaviour is what this cost is made of, so rather than open a second issue restating it, I added the measurement from this change there and said how the two relate, including that these two routes are plain def handlers so the cost lands on the threadpool and on database load rather than on the event loop: #1711 (comment)

That comment also carries a correction to the issue body: this repository does not pin SQLAlchemy to 2.0.48 — pyproject.toml declares sqlalchemy>=2.0.0, and the behaviour reproduces on 2.0.47.

---

10 and 12 — a caller with both a personal row and a granting verdict

Fixed in b460f3b07. You are right that this is the feature's actual reason for existing and that it was reachable only through a mock. It was missing from five places, and all five are filled:

where	what was added
MCP report-consistency parametrization	a personal_row_and_granting_verdict population
Custom API report-consistency parametrization	the same population
MCP TestPutWiringForATeamEditor	test_a_member_with_a_personal_row_edits_the_shared_config_durably
Custom API TestPutWiringForATeamEditor	the same test for the other connector kind
MCP re-check cost	test_a_member_with_a_personal_row_pays_one_call_on_a_real_personal_field

The last one is your finding 12: the exemption's only previous coverage was an empty payload, and a denying verdict, which short-circuits one clause earlier. Neither reaches the exemption with a payload that carries a real personal field and a caller it can land on. Custom API has no personal-only exemption of its own — its trigger has only two clauses — so that fifth case is MCP-only by design.

Mutations:

• _check_mcp_permission's team-access fallback returning False: 11 cases red, including the new population and both new durability tests. personal_non_owner_no_team_link stays green, which is the point — that population never depended on the verdict.
• Deleting the Custom API can_edit or clause: 8 cases red, including the new population and the new durability test.
• Deleting and not payload_is_personal_only from the re-check trigger: 2 cases red, the new personal-field test and the existing empty-payload one, both assert 2 == 1 — two hook calls where the exemption should have kept it at one.

One correction to my own expectation while running these: I expected the first mutation to redden the new personal-field cost test too. It does not, and should not — that test's payload is is_active only, which takes the personal-field exemption path and never reaches _check_mcp_permission. The third mutation is what covers it.

The design's named tests for this class are still not implemented under their own names; what changed is that the population they exist for is now covered on both connector kinds.

---

11 — the isinstance(verdict, ConnectorAccess) arm

Fixed in c6df5bab4: five parametrized cases, each a well-formed (connector_type, id) key paired with a value that is not a ConnectorAccess — a plain dict, a duck-typed object exposing the same two attributes, a ConnectorDeleteDecision, None, and a bare True.

Mutation, deleting the isinstance branch: all five red. The duck-typed case fails with DID NOT RAISE — the object is accepted and satisfies every attribute read below it, which is precisely what the check exists to stop. The other four fail with AttributeError, i.e. they were already being caught by accident downstream. That difference is why the duck-typed case is the one worth having.

---

13 — the session-fault suite's mutation sensitivity, and the dead assertion

Fixed in 85ea5b3d6. Both db.rollback() calls the toggle and connect tests made between the poisoning hook and their own verification query are removed; that query is now the statement that reaches the aborted transaction, so removing the production restore makes it fail rather than pass. Both call sites carry a comment saying why the rollback is absent, so it does not get re-added as a tidy-up.

The dead assertion is replaced with one that can fail: .one() already raises when nothing matches, so what the line now asserts is the route's own decision — connecting never grants ownership, so assoc.is_owner is False, and that decision survived the poisoned hook.

The module docstring is corrected rather than left claiming more than it did. Your reading of the four route calls was right and is preserved: none of the four routes issues a new statement on the poisoned connection while building its own response, because each response builder reads the row's attributes before the hook runs. What changed is the tests — toggle and connect are now independently sensitive through their own post-call query. The apps-listing and servers-listing tests stay non-sensitive, because neither issues any statement after the route call at all, and the docstring now says so.

Honest limit: PostgreSQL is not available in this working environment, so that suite skips locally and runs only in CI. The mutation was not executed against PostgreSQL here. What was executed is the SQLite twin, TestSessionRecoveryAfterHookFailure, which carried the same two masking rollbacks: with them removed the class passes against the current code, and removing the production restore turns four of its cases red (the two that stay green are the raw-statement parameters, which SQLite does not poison the same way). Those two rollbacks are removed for the same reason — one file should not carry both the production restore and a hand-written one.

---

16 — two hook-reset conventions in one PR

Fixed in 201116869, and your framing was the right one: the warning text this PR added on one side and the pattern this file still used on the other could not both be right.

The autouse fixture now wraps the whole test body in snapshot_connector_team_hooks, pulled out into a module-level _reset_hooks_scope() so a test can exercise the scope directly. The 32 per-test try/finally blocks that called the clear-everything setter in their finally clause are gone, since the fixture covers every path — pass, fail or raise — and leaving them would be the same two-mechanisms problem one level down. Two of the 32 were not mechanical: one reset a second, unrelated hook that the fixture already covers, and one was not teardown at all — it was the mid-test clear its own final assertion depends on, so it stays as a plain statement.

New pin: test_the_reset_scope_restores_a_pre_installed_hook_rather_than_clearing_it. Mutation, restoring the clear-everything fixture: that test fails on assert None is <sentinel> while the other 69 cases in the file stay green — which is the honest result, since inside this file the two behave identically. What the clear-everything form breaks is anything the process installed before this file ran, which is exactly what the pin asserts.

---

14, 15, 17, 18 — accepted, tracked in #1816

These four are real and none of them is a code defect, so I have filed them together rather than growing this PR further:

• 14 — the delete-vs-edit lock-order test cannot fail for the reason its docstring gives, since the DELETE statement blocks under MVCC whether or not the declared lock is taken. Making it distinguish the two needs a different mechanism than the one it uses.
• 15 — assertions that restate the payload's own omission instead of pinning an effect. The load-bearing assertions in those tests are sound; these lines are padding that should be replaced with something that can fail.
• 17 — the fixed 1.0s negative-window assertions have no pytest.mark.timeout, so CI slowness can satisfy them for the wrong reason.
• 18 — standalone parity is asserted on response values only; query cost with no hook installed is not asserted against a pre-change baseline.

If you would rather any of them land here instead of in a follow-up, say which and I will move it.

---

FYI — managed overwritten on every MCP PUT

Confirmed pre-existing and unrelated to this diff, by the same check you ran. Filed as #1817 with the mechanism; not attributed to this PR and not changed here.

---

Design note 6 — update_mcp_server's branch count

Agreed, and it matches what I concluded from the other direction, which is why the response-reporting fix in the sibling thread is one line rather than a restructure. Resolving the verdict once after the lock and dropping the personal-only exemption's separate existence is filed as #1815.

Design notes 1, 3 and 4 I read as accurate scope statements rather than requests, and note 5 as agreement on the seam boundary — the PR description states the unwidened routes as a deliberate boundary. Say the word if any of them should be treated as a change request.

[rogercloud]

PR summary

This PR lets a connector's (MCP server / Custom API) shared configuration be read and edited by a team member who holds no personal ownership row, via a new application-supplied access hook slot (set_connector_team_hooks(access=...)) that returns a ConnectorAccess(team_owned, can_edit) verdict keyed by (connector_type, connector_id). It widens the GET/PUT gates on /api/mcp/servers/{id} and /api/custom-apis/{id}, adds SELECT ... FOR UPDATE row locking with a post-lock verdict recheck (narrowing, not eliminating, a TOCTOU window — documented as such), and distinguishes hook failures that must gate the request (typed error, fail closed) from ones that only decorate a reported field (degrade to False + log). Standalone (no-hook) behavior is asserted unchanged throughout.

Update summary (since the 2026-08-27T04:02:39Z review)

Since the third round, the author landed commits that: convert get_custom_api and toggle_mcp_server to synchronous def (closing the event-loop-blocking finding that was the sole blocker from round 3), add ConnectorRuntimeError handling around the Custom API rename-hook call, and unify session-restore-after-hook-failure across all five hook call sites (visibility, team-visibility, access, delete, rename). The author also self-disclosed, via a reply and tracking issue #1818, a residual gap: delete_mcp_server still calls the connector-team seam synchronously on the event loop — the same bug class fixed everywhere else in this PR — left unaddressed and never explicitly accepted by a reviewer.

Round 0 — Design verdict: acceptable-with-reservations

The direction is sound: an application-owned access verdict avoids teaching xagent about team/role vocabulary and mirrors the existing knowledge_base_team_scope.py pattern. _call_connector_hook_gate consolidating "call the hook + validate the answer + restore the session on failure" into one door across all five hook slots is the strongest structural improvement in the PR — it retroactively fixes an inconsistency that predated this change.

Design-level notes (non-blocking except where a line-level finding below says otherwise):

• D1 — Two authorization sources, never cross-checked. The visibility hook and the new access hook both answer "does this team link this connector," independently. This is now explicitly documented in both hooks' docstrings (both disagreement directions: visible-but-inaccessible, accessible-but-invisible) — this is exactly the fix round 3 asked for. Accepted seam boundary, non-blocking.
• D2 — Undocumented isolation-level assumption. The post-lock recheck (mcp.py ~3649-3706, custom_api.py ~449-476) only observes a concurrent revocation under READ COMMITTED; under REPEATABLE READ/SERIALIZABLE it would silently become decorative. Production runs default READ COMMITTED and nothing overrides it, so there's no live risk today — but this codebase has an established convention of stating this assumption explicitly in analogous code (task_interaction_staging.py, task_interaction_schema.py, services/workforce_creator.py), and the new recheck code doesn't. A one-line comment would close the gap. Non-blocking.
• D3 — The hook contract never asks for a real linearization point. The hook runs inside the route's own locked transaction, so an installing application could close the TOCTOU window completely by taking its own lock on its link row during the recheck — but nothing in the ConnectorAccessHook type or docs says this is possible or expected, so no hook implementation will do it. Worth naming as a scope boundary for a future iteration; non-blocking.
• D4 — "When to consult the hook" is five ad-hoc policies expressed two different ways (a Callable predicate on the Custom API side, a Literal["raise","degrade"] on the MCP side, for the same underlying concept). Partially addressed by the simplification opportunity below; the broader inconsistency (which routes skip resolution for owners/admins and which don't) remains — see F8/F9 below.

Findings

Blocking

F1 — No catalog/platform-key exclusion on the widened MCP edit gate

• Location: src/xagent/web/api/mcp.py:1377-1406 (_check_mcp_permission), consumed by update_mcp_server (mcp.py:3570-3871)
• Severity: major — Blocking: yes
• Trigger: an installed ConnectorAccessHook — the exact mechanism this PR ships — grants can_edit=True on a catalog-backed connector ref to a team member with no personal ownership row (reachable via the _TeamOwnedUserMCP stand-in this PR introduces).
• Impact: _check_mcp_permission's edit branch is if is_owner: return True then return bool(team_access is not None and team_access.can_edit) — no check anywhere for a catalog-managed row or one holding a platform fallback key before that stand-in can rewrite command/args/url/env/auth. _catalog_server_has_platform_key (mcp.py:3874) is not an authorization gate — it only runs post-authorization in delete_mcp_server (mcp.py:4067) to decide retain-vs-hard-delete, with no analog on the edit path. This is genuinely new: at the base commit, the edit branch was return is_owner with no team_access at all, and every catalog-connect path hardcodes is_owner=False for the connecting user — so pre-PR, a catalog-backed server's shared config was structurally uneditable by any non-admin. This PR introduces a brand-new, independent route to edit authorization with zero awareness of "catalog." A team editor can therefore rewrite a shared, possibly platform-credentialed catalog server that other team members' agents execute unknowingly — a concrete authorization-boundary violation with real blast radius (arbitrary command/URL/auth substitution using platform-held credentials, affecting users who never granted this). This was never raised in any of the three prior review rounds.
• Fix: gate the team_access branch of _check_mcp_permission for require="edit" (or the write path in update_mcp_server) behind the same catalog/managed exclusion that _catalog_server_has_platform_key/_is_reserved_catalog_name already express for delete/rename. Add a test: a team stand-in with can_edit=True on a catalog-backed server must not be able to mutate its config.

Non-blocking (must still be resolved or explicitly accepted before merge)

F2 — delete_mcp_server still runs the connector-team seam synchronously on the event loop

• Location: mcp.py:3902 (still async def), mcp.py:3941 (synchronous delete_team_connector call)
• Severity: major — Blocking: no (pre-existing at the base commit, not introduced by this PR; same bug class already fixed in this PR for 5 sibling routes; self-disclosed by the author and tracked in #1818)
• Impact: an installed, DB-backed hook that's slow will stall the FastAPI event loop for every concurrent request during a delete — identical risk to the routes this PR already converted to sync def. The new AST guard test (tests/web/api/test_custom_api.py:606-651) explicitly exempts this function, and the exemption only checks "contains some await" (satisfied by an unrelated OAuth-revocation await), not that the seam call itself is off the loop.
• This deferral was disclosed by the author only in a reply after the round-3 review and has not been explicitly accepted by a reviewer in a formal round. Please confirm explicitly whether this ships as-is (tracked in #1818) or gets fixed in this PR — this is the same risk class treated as blocking everywhere else in this PR, so it shouldn't be waved through silently.

F3 — PUT/DELETE lock ordering is only proven consistent for xagent's own tables, not the hook's

• Location: custom_api.py:436-442 vs :572 (PUT: lock then rename-hook) vs custom_api.py:627 vs :652-658 (DELETE: delete-hook then lock) — genuinely opposite order on the Custom API side. MCP: lock at mcp.py:3636-3642 then rename hook at :3839 (PUT) vs delete hook at :3941 with no lock at all on mcp_servers (DELETE).
• Severity: minor/informational — Blocking: no. This depends on an assumption about an external hook implementation taking its own conflicting lock, which no hook in this codebase does (no production hook is installed anywhere; set_connector_team_hooks is only called from tests). Worth a code comment caveat, not a blocking defect.

F4 — GET /api/custom-apis has no team-overlay/visibility integration at all

• Location: custom_api.py:174-188 (list_custom_apis)
• Severity: minor — Blocking: no. Unlike get_mcp_servers, this list endpoint queries only the caller's own UserCustomApi rows. A Custom API a caller can edit via direct-id PUT (through a team grant) may not appear in their own list — a completeness/UX gap, not an authorization bypass (PUT-by-id re-resolves independently). Not previously raised.

F5 — The visibility hook's answer is completely unvalidated, unlike its siblings

• Location: connector_team_scope.py:191-193 (no validate= passed to _call_connector_hook_gate), vs :253-259 and :404-411 (both validated)
• Severity: minor — Blocking: no. A malformed visibility-hook answer raises a raw KeyError deep in mcp.py instead of the seam's typed error. Real and reachable, but a gap on an already-existing hook signature, not new.

F6 — Inconsistent hook-failure handling within list_mcp_apps

• Location: mcp.py:2451 (visibility hook, no try/except) vs mcp.py:2547-2558 (access hook, degrades gracefully)
• Severity: minor — Blocking: no. Both ultimately reach only the app's generic 500 handler either way, so this is a UX/consistency gap, not a crash.

F7 — Session-rollback-after-hook-failure can surface ObjectDeletedError on a lazy refresh in listing routes

• Location: connector_team_scope.py (_restore_session_after_hook_failure), callers in list_mcp_apps/get_mcp_servers
• Severity: minor — Blocking: no. Requires a hook failure and a genuinely concurrent delete of the same row in a narrow window; the seam's own docstring already admits this shape stays uncovered deliberately.

F8 — toggle_mcp_server calls the access hook even for owners/admins, whose reported field it can never affect

• Location: mcp.py:4093-4165 (unconditional hook call at :4147-4150), vs _check_mcp_permission (:1396-1406) short-circuiting True on is_owner/is_admin before reading team_access
• Severity: minor, perf-only — Blocking: no.

F9 — get_mcp_servers resolves access verdicts for the inspected user (?user_id=target) but the admin-viewing response path discards them

• Location: mcp.py:2803 (resolves against effective_user_id) vs mcp.py:2828 (is_admin reflects the viewing admin, short-circuits can_edit_global before team_access is read)
• Severity: minor, perf-only — Blocking: no.

F10 — The delete-authorization decision is never rechecked under the definition-row lock, unlike the edit verdict

• Location: delete_custom_api (custom_api.py:627 decision before lock at :652-658); delete_mcp_server (no lock on mcp_servers at all)
• Severity: minor — Blocking: no. A narrow TOCTOU window, but delete is a single short transaction with no external round-trip after the decision, making exploitation implausible in practice.

F11 — Name-uniqueness conflict-row check is unlocked in both PUT routes (pre-existing, not new)

• Location: mcp.py:3742-3746, custom_api.py:492-496
• Severity: minor, FYI only — Blocking: no. Verified identical at the base commit; this PR added row-locking for the edited row but didn't extend it to the conflict-check query. Not a new risk.

F12 — _resolve_custom_api_for_request has unreachable defensive code reclassifying a real personal row as a stand-in

• Location: custom_api.py:330-333
• Severity: minor, FYI only — Blocking: no. UserCustomApi.custom_api_id is a non-nullable FK with ondelete="CASCADE" plus ORM cascade="all, delete-orphan", so the .custom_api is None branch is unreachable under the current schema. Optional cleanup, not required.

F13 — Personal-only MCP PUT still resolves the team verdict unconditionally at initial resolution (only the post-lock recheck is skipped for personal-only payloads)

• Location: mcp.py:3583 (initial resolution) vs :3674 (payload_is_personal_only, computed too late to affect the initial call)
• Matches the still-open, already-tracked #1775 exactly — no worse behavior found than what's documented there. Status: STILL OPEN, non-blocking (already accepted as deferred).

Prior findings checklist (since round 3)

• Round 3's sole blocking item — get_custom_api/toggle_mcp_server calling the access hook synchronously inside async def — FIXED. Both are now plain def (verified).
• Round 3 minor — update_custom_api missing ConnectorRuntimeError handling around the rename-hook call — FIXED. except ConnectorRuntimeError as exc: now present at custom_api.py:580.
• Round 3 minor — update_mcp_server's response reporting the pre-recheck (stale) verdict — FIXED, with zero observable gap. For the one scenario where the recheck is skipped because team_access.can_edit was already False (a personal-row-authorized caller), _check_mcp_permission computes that caller's can_edit_global from is_owner, never from team_access — so staleness there is unobservable. The fix commit's own message already scoped and disclosed this precisely.
• Round 3 minor — session-restore-after-hook-failure covering only 2 of ~5 hook call sites — FIXED. All five hook slots (visibility, team-visibility, access, delete, rename) now route through the same _call_connector_hook_gate door.
• Round 3 minor — personal-only MCP PUT resolving the team verdict unconditionally, tracked in #1775 — STILL OPEN, unchanged, non-blocking (see F13).
• Round 3 minors tracked in #1711 (query-storm in degraded listings) and #1702 (can_configure dead-end UX for view-only stand-ins) — unchanged, still out of scope for this PR, non-blocking; nothing in this pass suggests regression.
• Delete-hook-drop event-loop gap in delete_mcp_server — NOT FIXED, self-disclosed by the author after round 3 and tracked in #1818, but never formally reviewed/accepted — see F2.
• Visibility-hook/access-hook disagreement (round 3 finding #11) — WAIVED/accepted, confirmed now documented accurately in both hooks' docstrings (both disagreement directions stated), exactly as round 3 asked.
• Simplification suggestion to drop ConnectorAccess.team_owned (restated this round) — WAIVED. The author already rebutted the identical suggestion (dropping the field while keeping both fields defaulted) with a reproduced failure case: a bare ConnectorAccess() must remain rejected by the validator, and removing team_owned while keeping defaults would let it silently normalize into (False, False), a meaning nobody chose. That rebuttal is technically correct against the suggestion as stated; not re-reporting it.

Simplification opportunities

• custom_api.py:281: yagni: skip_resolution_when in _resolve_custom_api_for_request is a Callable predicate with exactly 2 fixed call sites (:375 constant-True, :402 bool(ua.can_edit)). Replace with a plain skip_resolution: bool computed by each caller.
• mcp.py:1509: yagni: on_resolution_failure: Literal["raise","degrade"] in _resolve_mcp_server_for_request has exactly 2 call sites (:2925 degrade, :3583 raise-default). Replace with degrade_on_failure: bool = False — this also unifies the parameterization with the Custom API side above, which expresses the identical "should we consult the hook" concept as a different type today.

net: -0 lines but removes a needless Callable/Literal split across the two nearly-identical resolution helpers.

Note: a simplification suggestion to drop the bool/float/Decimal alias check in _validate_connector_access_answer (connector_team_scope.py ~326) was considered and explicitly rejected — that check validates the access hook's return value, which is untrusted, externally-supplied data, not an internally-constructed key. Removing it would reintroduce the exact authorization-bypass vulnerability this PR's own earlier rounds found and fixed (key aliasing via True == 1, 1.0 == 1, Decimal("1") == 1). Keep it.

Test quality (non-blocking observations)

• tests/web/api/test_mcp_reported_edit_permission.py:1532 — assert assoc is not None after a preceding .one() call is a no-op assertion (.one() already raises if the row is missing).
• tests/web/api/test_mcp_team_connector_edit.py:821-844 — the test's own docstring admits it cannot distinguish "reports the recheck" from "reports the pre-lock answer"; the underlying behavior it's trying to pin is confirmed fine (see the round-3 stale-verdict item above), but the test itself doesn't prove it.
• tests/web/api/test_custom_api_edit_lock_postgresql.py:316-395 — claims to prove DELETE takes the same lock order as PUT, but would still pass with the DELETE route's with_for_update() removed, since the subsequent DELETE FROM custom_apis statement would itself block on the editor's held lock regardless.
• mcp.py:1495 (_TeamOwnedUserApi.is_owner) — confirmed dead attribute; no test references it, no production code reads .is_owner off a stand-in instance.

Review limitations

No local tests, builds, linters, or formatters were run under the review default — CI is green at preflight (all checks passing). This review is a re-review (4th round); Round 0/1 were performed as an independent, blank-slate pass before reading any prior review history, per process.

Blocking status & recommended decision

Blocking: yes
Recommended event: REQUEST_CHANGES

Blocking issues:

• src/xagent/web/api/mcp.py:1404 — major — the widened team-edit gate has no catalog/platform-key exclusion, letting a team stand-in with a granted can_edit verdict rewrite command/url/env/auth on a shared, possibly credentialed catalog-managed MCP server. [new]

[rogercloud]

Blocking (major). This branch grants shared-config edit rights purely on team_access.can_edit, with no catalog/platform-key exclusion anywhere before update_mcp_server applies the write. _catalog_server_has_platform_key (mcp.py:3874) only runs in delete_mcp_server post-authorization to decide retain-vs-hard-delete — it's not an ACL gate and has no analog here.

At the base commit this branch was return is_owner with no team_access parameter at all, and every catalog-connect path hardcodes is_owner=False for the connecting user — so pre-PR, a catalog-backed server's shared config was structurally uneditable by any non-admin. This PR introduces team_access.can_edit as a brand-new, independent route to edit authorization with zero awareness of "catalog," reachable via the _TeamOwnedUserMCP stand-in with no personal ownership row at all.

Trigger: an installed ConnectorAccessHook grants can_edit=True on a catalog-backed connector ref to such a stand-in. That caller can then rewrite command/args/url/env/auth on a shared, possibly platform-credentialed catalog server that other team members' agents execute — a concrete authorization-boundary violation, not raised in any of the three prior review rounds.

Suggested fix: gate this branch (or the write path in update_mcp_server) behind the same catalog/managed exclusion _catalog_server_has_platform_key/_is_reserved_catalog_name already express for delete/rename.

[AlexLiu190625]

Fixed in 10c113d46, on the current head 6064285d4.

The invariant the fix states, rather than the check it adds:

A platform catalog app's shared row never becomes editable because of a team access verdict. An installing application may answer can_edit=True for such a ref; this module downgrades that answer against the fact that the row is the catalog's own, and the downgraded verdict is the single object both the gate and every reported field read — so no surface can advertise an edit the PUT would refuse.

Where the rule lives

Two new functions in mcp.py, and the rule itself is written exactly once, in the second:

• _catalog_reserved_keys(db) (mcp.py:1829) — every normalized key the catalog claims, in one SELECT.
• _team_access_for_shared_row(db, server, access, *, reserved_keys=None) (mcp.py:1841) — returns the verdict as this module's routes may act on it:

    if access is None or not access.can_edit:
        return access
    keys = _catalog_reserved_keys(db) if reserved_keys is None else reserved_keys
    if not keys.intersection(_server_catalog_keys(server)):
        return access
    return replace(access, can_edit=False)

Only can_edit is cleared. team_owned is left as the application answered it, so the connector stays reachable and readable by the team — returning None here would 404 a connector the team genuinely links.

Which predicate, and why neither of the two you named

Your suggestion was _catalog_server_has_platform_key / _is_reserved_catalog_name. Both were measured against seven row shapes, each built the way its own provisioning helper builds it:

row shape	_server_catalog_keys ∩ catalog	_is_reserved_catalog_name	_catalog_server_has_platform_key	get_app_for_mcp_server
api_key catalog row (name = app id)	True	True	False	None
keyless catalog row	True	True	False	None
mcp_oauth catalog row	True	True	False	None
builtin_oauth catalog row (name = display name)	True	True	False	gmail
builtin_oauth catalog row renamed by an administrator	True	False	False	gmail
self-built row, no name collision	False	False	False	None
self-built row squatting a catalog id	True	True	False	None

• _is_reserved_catalog_name answers a different question — "may a new row take this name" — and reads the name alone. A builtin-OAuth catalog row an administrator renamed still carries its app_id in auth and is still the platform's row, and that function no longer recognizes it. That miss is not a corner: get_builtin_public_mcp_app_rows() classified through classify_app_auth gives {'builtin_oauth': 21, 'api_key': 4, 'mcp_oauth': 2, 'keyless': 1} — 21 of the 28 built-in catalog apps are the shape it misses.
• _catalog_server_has_platform_key answers "catalog row that also carries the platform key". Every keyless and mcp_oauth row reads False there, and so does every key-based row whose key each user supplies themselves — while all of them are still platform-owned configuration.

The predicate used instead is _server_catalog_keys against the catalog's own keys — the same predicate list_mcp_apps already uses at mcp.py:2656-2658 to decide that a stored row is some catalog app's shared row. So this module holds one definition of "catalog-managed", not a second one introduced by this change.

Every place a verdict is produced, not just the gate

Gating alone would leave the button lit and the press 403. Six call sites across five routes:

site	what it feeds	line
_resolve_mcp_server_for_request	the GET/PUT gate and the reported field	mcp.py:1615
get_mcp_servers, personal-row loop	the listing's reported field	mcp.py:2945
get_mcp_servers, stand-in loop	the listing's reported field	mcp.py:2980
connect_mcp_app	the connect response's reported field	mcp.py:3491
update_mcp_server, post-lock re-check	the answer the write is authorised on	mcp.py:3825
toggle_mcp_server	the toggle response's reported field	mcp.py:4301

(Two gates and four reported fields.) The consumers — _check_mcp_permission, _db_server_to_response, _custom_api_to_mcp_response — are untouched: they already receive the normalized object, so the gate and the reported field cannot disagree.

Three places are exempt, each for a structural reason rather than for lack of need:

• list_mcp_apps — mcp.py:2656-2658 skips the whole row with library_keys.intersection(_server_catalog_keys(server)) before any verdict is read, using the same predicate the downgrade uses.
• Custom APIs — the model carries no app id and no catalog association at all; _server_catalog_keys and _is_reserved_catalog_name exist only in mcp.py.
• The require="delete" branch — mcp.py:1408 returns is_owner or can_delete and never reads team_access.

Cost

The downgrade runs only for a verdict that already grants edit, which is the only case where it can change an answer.

• A deployment with no access hook installed resolves None for every row and issues no additional query at all.
• A listing whose verdicts do grant edit builds the key set once per request (mcp.py:2937) and shares it across every row — one extra SELECT public_mcp_apps.*, which does not grow with the number of rows.

Two existing query-budget constants moved, from a measured run rather than adjusted to pass:

• test_mcp_reported_edit_permission.py:244 — assert len(queries) == 7 → == 8.
• Same file :383 — HEALTHY = {"apps": 7, "servers": 5} → {"apps": 7, "servers": 6}. BASE and EXTRA are unchanged: a failing hook returns no verdicts, the downgrade's first line short-circuits, and nothing extra is issued.

Both of those existing budget tests run at num_rows 2 and 6 and assert a single constant, so "one per request, not one per row" is what the constant means — the apps listing's number is unchanged because that endpoint is the exempt one.

A rename-then-edit bypass does not exist

update_mcp_server runs in this order:

_resolve_mcp_server_for_request(...)     ← verdict produced from the pre-write row
_check_mcp_permission(...)               ← gate decided
SELECT ... FOR UPDATE                    ← lock taken
_team_access_for_shared_row(db, locked_server, ...)   ← re-derived under the lock
... MCPServer.name == server_data.name ...            ← rename conflict check

The gate always reads the name and auth the row has before this request writes anything, and renaming is itself a write that must pass that gate first — which refuses it under the current name. The post-lock re-derivation uses the locked row, so a concurrent rename lands in the same window this change already documents as narrowed rather than fenced.

Tests

TestCatalogRowsAreNeverTeamEditable in tests/web/api/test_mcp_team_connector_edit.py — 12 cases covering four catalog shapes (api_key with a platform key, api_key without one, mcp_oauth, renamed builtin_oauth), the GET, listing (both loops), connect and toggle reported fields, a self-built row that stays team-editable, and the catalog row's own owner still editing it. TestCatalogCheckQueryBudget in the same file pins the two cost properties above across the same two population sizes. Each turns red under a named mutation; the two most load-bearing are removing either listing loop's downgrade independently, and swapping the predicate for _is_reserved_catalog_name.

---

Three things worth stating outright

1. A boundary this resolves deliberately rather than correctly. If someone built their own connector under a name a catalog app later took, the catalog claims that name, so this function treats the row as catalog-managed and withholds the team edit. The row's own creator keeps their edit right in full — is_owner decides the edit branch in _check_mcp_permission (mcp.py:1409) before any verdict is read. What is withheld is only a teammate editing that connector on the owner's behalf. Telling such a row apart from a real catalog row needs a stored "who created this definition" fact the schema does not carry today. Until it does, the ambiguity is resolved on this side on purpose; it is written into the function's docstring as a declared boundary and pinned by a test.

2. This rule now exists in two places, on purpose. The application that installs the hook is expected to exclude catalog rows on its own side too, and that exclusion is tracked on the application side as a merge precondition for that hook's installation. Keeping both is duplication, and it is worth it because the two are not the same check: the application's answer is a snapshot taken when the connector was shared, while this one is derived per request — and both the row's transport and its name are mutable through this very PUT. This does move part of the exclusion upstream of where the split between the two sides originally put it; that is a deliberate second layer, not a layering mistake.

3. One premise in the finding needs correcting — and the finding still stands. The finding says every catalog-connect path hardcodes is_owner=False for the connecting user. That holds for the api_key, keyless and mcp_oauth shapes (mcp.py:3427 and mcp.py:3533), but not for builtin OAuth: _ensure_user_mcp_server in auth.py writes is_owner=True (auth.py:1670 on this branch, auth.py:2152 on upstream/main — the same line), and that is the path 21 of the 28 built-in catalog apps take. So "a catalog-backed server's shared config was structurally uneditable by any non-admin before this change" was true for three of the four shapes, not all four.

That correction does not weaken the conclusion. The population it removes is one where a connecting user already had edit rights; the population the finding is actually about — a team member with no personal row at all, granted edit by a verdict on a shared row that may carry the administrator's platform fallback key — is exactly the api_key shape, which is the one where the pre-change gate really was closed and where the blast radius is largest. The finding was correct and the fix is in.

Two notes on how to read the evidence

The renamed-builtin-OAuth test proves the predicate, not an exploit. With the correct predicate that test gets a 403 from the stand-in gate. With the name-only predicate substituted it goes red — but it fails with a 400 from the existing transport="oauth" schema validation in the config build, not with a successful write. So that mutation demonstrates that the predicate discriminates (the request got past both authorisation gates and reached config building). It is not evidence that this particular row shape carried a reachable attack. The shape that did is the api_key catalog row holding the platform fallback key, which is the first case in the new class.

connect_mcp_app still pays one hook call whose answer is now fixed. Every row that route returns is a catalog app's shared row, so the downgrade makes its reported can_edit_global False regardless of what the hook said. The resolution is kept anyway, and the reason is in the code at mcp.py:3456-3462: the reported field must come from the same object the PUT gate would read, rather than taking its own shortcut to False. The price is one hook round trip per connect — a user-initiated action, not a hot path — and the call is not entirely without effect: a raising hook still logs the degrade warning, which an existing test asserts. If you would rather this route skipped the call outright, that is a simplification we can take, and it belongs with the "when do we consult a hook" cleanup in #1880 rather than here.

[AlexLiu190625]

Head is now 6064285d4. Eight commits since the reviewed state:

commit	what it does
10c113d46	never let a team verdict grant edit on a platform-catalog row
b810cefc6	name the isolation level the post-lock verdict re-check assumes
38756c4cd	say what the global lock order does not cover
e2737cb98	assert something that can fail after a .one() lookup
41e2a49d2	the coroutine exemption must carry an await that is not the seam call
d4a59e302	cover the personal-row downgrade in the get_mcp_servers listing
6f6a0dc71	fix a stale cost description in the _server_catalog_keys docstring
6064285d4	scope three comments to what the code actually guarantees

The catalog exclusion is answered in its own thread. Everything else follows.

Changed here

delete_mcp_server on the event loop — answering your question directly: it ships as it is, tracked in #1818. Two facts that were not in the earlier disclosure: the async def on that route is 1e361f494 (2026-08-01) and the seam call inside it is 41987742bc (2026-07-17). Both predate this branch, and this branch does not touch either. Converting the route means changing how its external OAuth revocation call is made, which is a change to a path whose behaviour is otherwise untouched here.

Your point about the guard is correct and is fixed. The exemption used to be satisfied by "contains some await", which a coroutine whose only await is the seam call would also satisfy — and that shape is convertible, so it should never have qualified. Tightened in 41e2a49d2: the exemption now requires an await that is not itself a call to a name imported from connector_team_scope in the same function body. The distinguishing mutation: make the seam call awaited, and the new assertion goes red while the old one stayed green. delete_mcp_server qualifies on its await _revoke_mcp_oauth_grant_externally(...), which is not a seam call.

Lock ordering versus the hook's own tables — comment added in 38756c4cd. It does not claim to fix anything; it records what the ordering statement above the delete route's lock does and does not cover. The four facts it names: the MCP PUT takes the row lock and calls the rename hook afterwards; the MCP DELETE calls the delete hook and takes no lock on mcp_servers at all; the Custom API PUT takes the lock and then calls the rename hook; the Custom API DELETE calls the delete hook before taking the lock. So an installing application whose hooks lock rows of their own can still deadlock against a concurrent edit/delete pair, and no ordering statement inside this repository can prevent it — only the application can order its own locks compatibly.

The isolation level the post-lock re-check assumes — stated in b810cefc6, on both PUT routes, matching the disclosure this codebase already makes in task_interaction_staging.py, task_interaction_schema.py and services/workforce_creator.py. It names READ COMMITTED, notes that nothing sets an isolation_level on the engine, and says what happens under stricter levels: the re-read reuses the transaction's original snapshot and the re-check degrades to a no-op — it stops refusing, it does not start refusing wrongly.

The no-op assertion after .one() — this one is ours, and the earlier fix was incomplete. An earlier reply on this pull request said the assertion had been "replaced with one that can fail", but only the line that had been named was changed; the class of assertion was never swept. It has been now: a repo-wide AST scan for the pattern (an assignment through .one()/.scalar_one() followed by an is not None or truthy assert on that same name) found nine candidates. Eight assert on a column value read off the result rather than on the row object itself, and can genuinely fail. One was a real dead assertion — the one you named. Fixed in e2737cb98; it now asserts the shape connect actually writes, (assoc.is_owner, assoc.is_active) == (False, True). Mutation: hardcode is_active=False at the connect write site, and the new assertion goes red where the old one stayed green.

One coverage gap we found ourselves and closed (d4a59e302). The listing endpoint runs two independent loops — one for callers holding their own non-owning association row, one for stand-ins — and only the stand-in loop's downgrade was pinned. Removing the personal-row loop's downgrade left the whole related suite green. There is now a test for it, and it fails on exactly that removal.

Not changed here, with the reason

GET /api/custom-apis has no team overlay — filed as #1877. That endpoint queries only the caller's own UserCustomApi rows (custom_api.py:174-188, from 693730db06, 2026-04-17); this branch does not touch it. Fixing it means giving a list endpoint a team overlay it never had, which is a behaviour change on code outside this diff.

The visibility hook's answer is never shape-checked — filed as #1878. You are right that it is the one hook slot reaching _call_connector_hook_gate without a validate=, while both siblings pass one. The change itself is one line, but it changes what an existing hook slot does on a malformed answer, and it needs a test that the malformed answer produces the seam's typed error instead of a bare KeyError. The issue says both.

list_mcp_apps handles its two hook failures differently — filed as #1879. The visibility call has no try (pre-existing, 5eaf31bd32, 2026-08-13) while the access call degrades. Making the visibility call degrade too changes an existing route's failure semantics from a 500 into silently listing fewer rows — a product decision, not a consistency edit.

No linearization point in the hook contract, and "consult or skip" spelled two ways — filed as #1880, together with both simplification suggestions. They are one issue rather than three because all three are the same surface: two near-identical resolver helpers expressing the same two decisions differently. Collapsing on_resolution_failure to a bool on its own has a negative net: fifteen lines of its docstring explain the semantic difference between the two values, so the explanation has to be rewritten rather than deleted. Done as one deliberate pass over how this seam expresses consultation and failure, it pays for itself.

toggle_mcp_server consulting the hook for owners and admins, and get_mcp_servers resolving for the inspected user while the admin-viewing path discards the result. Neither is changed here. The toggle path has already changed twice on this branch and now carries one more line from the catalog downgrade; a third change to it, for a cost-only reason, is two separate concerns landing on one site. The "whose capability does this field describe when an admin views another user's list" half of the second one is #1703. The cost half of both — a route consulting the hook where the answer cannot change its own — is the same surface #1880 is about, but #1880 does not enumerate these two call sites today. Say the word and they go on it.

ObjectDeletedError on a lazy refresh after a hook failure — confirmed, no code change. The seam's own docstring (14c52c461) states this shape as deliberately uncovered, which is what you read.

The unlocked name-uniqueness conflict check in both PUT routes — confirmed, unchanged. mcp.py's is ^dc32c0011 (2026-02-15) and custom_api.py's is 693730db06 (2026-04-17); this branch added row locking for the edited row and did not extend it to the conflict query.

Personal-only MCP PUT still resolving the verdict unconditionally — confirmed, unchanged, #1775.

The delete decision is not re-checked under the definition-row lock. Not doing it here. Adding a post-lock re-check to the delete path means introducing a new mechanism on a path whose authorisation semantics this change does not alter — the same argument for not extending the exclusion sideways. If you want the window tracked rather than argued, say so and it gets an issue.

The defensive branch in _resolve_custom_api_for_request that looks unreachable — keeping it, on two facts. SQLite does not enforce foreign keys unless PRAGMA foreign_keys is switched on, and every suite in this repository except the PostgreSQL ones runs on SQLite, so the schema argument for unreachability does not hold where these tests run. And the identical defensive shape already exists a few lines above, at custom_api.py:185 (if user_api.custom_api:), verbatim at the base commit. Removing one of the two would leave two conventions for the same situation in one file.

_TeamOwnedUserApi.is_owner — keeping it. Its sibling _TeamOwnedUserMCP.is_owner is read, through getattr(user_mcp, "is_owner", False) in _check_mcp_permission. Deleting the attribute on one stand-in and not the other makes two same-family classes structurally different, and the next getattr(x, "is_owner", ...) written against them would be depending on the getattr default rather than on a declared class attribute.

The lock-order test that would still pass with with_for_update() removed — #1816, whose first section is that exact observation.

The test whose own docstring admits it cannot distinguish "reports the re-check" from "reports the pre-lock answer" — left as it is. Its self-description is accurate about what it pins, and you have confirmed the underlying behaviour is fine.

Three comments narrowed after re-reading them

6064285d4 scopes three pieces of prose down to what the code actually guarantees, all found by re-reading rather than by review:

• The ordering note on the catalog downgrade read as though placing it before the 404 test would break reachability. It would not — the downgrade never turns a verdict into None. The comment now says the ordering is belt-and-braces today and names the change that would make it load-bearing.
• _catalog_app_keys lists its callers by name and had not grown the one this work added.
• The new GET test's comment claimed to pin "a downgrade, not an erasure", which past the 404 test it cannot distinguish. It now says what it does pin: reachable and readable, with no edit right.

The pull request description now carries the invariant, every place the verdict is produced, the three exemptions and the per-request cost.

[AlexLiu190625]

Two notes to keep this review pointed at the right things.

First, a correction: my last status comment said the head was 6064285d4. One more commit landed after it (df5a20460, docs(mcp): align the downgrade docstring with its call site's ordering note), so df5a20460 is the head to read this PR against — that is what CI ran on, and nothing has been pushed since.

Second, scope after the split: #1912 (the connector access seam) and #1913 (the definition-row lock) were split out of this PR and are under review separately. Both have evolved during their own reviews, so the copies of those files in this branch are split-time snapshots; once the two PRs merge, this branch will be reconciled to the merged versions rather than the other way around. What remains to review here is the team-edit behavior itself: the gating changes in mcp.py — where all five open threads already are — and their Custom API counterparts.

[AlexLiu190625]

Closing this in favour of two smaller pull requests, one per connector family. Nothing here is abandoned. This branch carried four separable parts; three of them are already merged as changes of their own, and the remaining two are the pull requests below.

What has already landed

• The connector access seam — the new access hook slot, its answer validator, the single hook-invocation door and the session restore behind it — merged as feat(connectors): add an optional team access hook to the connector seam #1912.
• The Custom API definition-row lock, its skip-lock partition and its post-lock re-authorisation — merged as fix(connectors): lock the Custom API definition row on edit and delete #2059.
• The MCP server definition-row lock, same shape — merged as fix(connectors): lock the MCP server definition row on edit #2060.

main now carries all three, and each went through further review after being split off, so main's version of services/connector_team_scope.py, api/custom_api.py and api/mcp.py is strictly newer than the snapshot on this branch. The two new pull requests are built on main, not on this branch, so none of that later work is undone.

What the two new pull requests carry

• feat(connectors): let team members edit a team-shared MCP connector #2095 — MCP servers: GET/PUT /api/mcp/servers/{id} admit a caller whose team links the server, the platform-catalog downgrade, and the can_edit_global the listings, the connect response and the toggle response report.
• feat(connectors): let team members edit a team-shared Custom API #2094 — Custom APIs: the same widening for GET/PUT /api/custom-apis/{id}, its stand-in guards and its post-lock re-check.

Each is written to stand on its own: neither depends on the other's code, and neither PR body assumes a reader has seen this one.

Review comments on this branch

Every point that was accepted on this branch is either implemented in the code the two new pull requests carry, or filed as an issue of its own. Those issues are #1701, #1702, #1703, #1775, #1815, #1816, #1817, #1818, #1837, #1877, #1878, #1879, #1880, #1917, #1918, #1932, #1944, #1945 and #1946; all are open and assigned. The ones that bear directly on the routes each new pull request touches are also listed in that pull request's own "Known limitations tracked separately" section.

Splitting by connector family is what makes the remaining work reviewable in one sitting each.