fix(app): support cross-owner agent invites - #2326
Conversation
DavdGao
left a comment
There was a problem hiding this comment.
Checked this out locally — the related tests pass and pre-commit is clean on the changed files. The core fix and the owner_id widening look right, and existing records need no migration since owner_id == team owner == agent owner for them. Left notes inline; the first one (team detail no longer re-checking the policy) is the only thing I'd call a real regression, the rest are follow-ups or cleanups.
| ":attr:`TeamRecord.user_id` on purpose — the surrounding " | ||
| "team already carries the team owner in context, so calling " | ||
| "this field ``user_id`` too would be ambiguous." | ||
| "Owner of the member's agent definition. For a cross-owner " |
There was a problem hiding this comment.
Widening this to the definition owner means every reader that feeds it into get_agent now crosses into another user's namespace. _router/_session.py:98 does exactly that with no policy check — I confirmed that after the grant is revoked, team detail still returns the shared agent's full AgentData including system_prompt. Chat itself fails closed via resolve_agent, but this read path doesn't, so it should go through ResourceAccessService too.
Same pattern at _team_say.py:237 and _agent_create.py:415, though those only surface the agent name into a routing directory.
There was a problem hiding this comment.
Fixed the sensitive team-detail path. _build_team_detail now resolves each member through ResourceAccessService.try_resolve_agent and verifies the resolved owner before returning agent data. I also added a regression test covering grant revocation. I left the name-only routing lookups unchanged in this focused fix.
| else: # invited | ||
| await self.delete_session( | ||
| member.owner_id, | ||
| user_id, |
There was a problem hiding this comment.
This covers team deletion, but not the reverse direction: delete_agent scrubs team back-references only within list_teams(user_id), so when the owner deletes a shared agent the borrower's session and its TeamMember both survive. TeamSay then silently drops the member (get_agent → None → continue) and opening the borrowed session 404s.
Partly pre-existing — the sessions router already lets a viewer create a session on a shared agent — but this PR is what puts such a session into a team roster. Probably worth a note or a follow-up issue rather than fixing here.
There was a problem hiding this comment.
Agreed. I kept the reverse owner-deletion cascade out of this PR as suggested, so this change remains focused on cross-owner invitation and the sensitive read-path regression.
| # Re-fetch fresh — both the invite settings and a cross-owner | ||
| # access grant may have changed since the toolkit snapshot was | ||
| # assembled. | ||
| fresh = await self._resolve_fresh_agent(invited) |
There was a problem hiding this comment.
Once this can return a cross-owner record, the comment at :353 stops being accurate: it says the invited agent's own primary session is preferred because it already has MCP / skills / cache set up, but :360 looks up list_sessions(self._user_id, ...), which for a cross-owner invite never finds the owner's session and always takes the fresh-workspace branch.
The behaviour is right and the new test locks the isolation in — the comment just needs to say so. Worth being explicit too that a borrowed cross-owner agent therefore runs with the leader's chat model and none of the owner's MCP / skills (#2042).
There was a problem hiding this comment.
Updated the comments to describe the cross-owner behavior explicitly: a borrowed agent uses a fresh leader-owned workspace and the leader chat model, without reusing the owner session, MCP configuration, skills, cache, or permissions.
| ) -> "AgentRecord | None": | ||
| """Resolve a snapshot entry without losing its owner namespace.""" | ||
| if self._resource_access_service is None: | ||
| return await self._storage.get_agent(invited.user_id, invited.id) |
There was a problem hiding this comment.
This moved from get_agent(self._user_id, ...) to get_agent(invited.user_id, ...), which drops the implicit "the pool may only hold the caller's agents" guard — pool correctness is now entirely the constructor's responsibility. Fine for the direct-construction path, but worth a line in the docstring.
There was a problem hiding this comment.
Updated the constructor documentation to state that, without a resource access service, pool correctness is the caller’s responsibility.
| # Keep the web-framework exception dependency local to the app-only | ||
| # access-service path. A revoked share is a normal stale-snapshot | ||
| # outcome for this tool, not an unhandled tool failure. | ||
| from fastapi import HTTPException |
There was a problem hiding this comment.
This doesn't need to be function-local. The import that has to stay deferred is .._service._access (circular via _toolkit → .._tool); fastapi isn't circular, and the whole app package already hard-depends on the service extra.
There was a problem hiding this comment.
Removed the function-local FastAPI import together with the web-framework exception handling from the tool layer.
| except HTTPException as exc: | ||
| if exc.status_code == 404: | ||
| return None | ||
| raise |
There was a problem hiding this comment.
Unreachable — resolve_agent only ever raises 404, via _not_found. A non-raising resolve variant on ResourceAccessService (returning None) would collapse _resolve_fresh_agent to a couple of lines and keep the web-framework exception out of the tool layer entirely.
There was a problem hiding this comment.
Added the non-raising ResourceAccessService.try_resolve_agent variant and simplified _resolve_fresh_agent to use it directly.
f899e46 to
4839e05
Compare
Summary
ResourceAccessServiceso cross-owner access is checked again at invocation timeTests
pre-commit run --files <changed files>pytest tests/service_team_tools_test.py tests/storage_redis_test.py -qpytest tests/storage_sql_test.py -qpytest tests/service_toolkit_test.py tests/resource_access_policy_test.py -qCloses #2319