🛍️ fix: Discover Managed Agents in Marketplace Search - #16324
Conversation
|
Review handoff for head |
|
Review handoff for head |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23bc3b246c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (typeof requiredPermissions !== 'number' || requiredPermissions < 1) { | ||
| throw new Error('requiredPermissions must be a positive number'); | ||
| } | ||
| if (await hasManageAgentsCapability(user, deps)) { |
There was a problem hiding this comment.
For every authenticated non-manager load, this await finishes the database-backed capability check before the two agent ACL lookups at lines 25–30 can even start. This endpoint is on the initial client path (Root.tsx mounts useAgentsMap, which requests the VIEW list), so the change adds a serialized database round trip to ordinary startup and marketplace loads; start the capability and scoped ACL reads together, then discard the ACL result when the capability succeeds.
AGENTS.md reference: AGENTS.md:L49-L54
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 174195f1a392bfe4170141b941ae1971938283d3: capability and both ACL reads now start concurrently. The ACL results are ignored for managers, and their errors cannot block a successful capability bypass.
| user: IUser, | ||
| requiredPermissions: PermissionBits, | ||
| deps: Pick<AgentManagementReadDeps, 'hasCapability' | 'findAccessibleResources'>, | ||
| ): Promise<{ accessibleIds: Types.ObjectId[] | null; editableIds: Types.ObjectId[] | null }> { |
There was a problem hiding this comment.
This newly exported packages/api function exposes Types.ObjectId in its return contract, making every consumer and substitute implementation depend on Mongoose even though this layer only needs plain agent identifiers. Define the exported dependency/result contract with plain IDs and leave ObjectId conversion behind the data-schemas method instead of widening the existing storage-engine leak.
AGENTS.md reference: AGENTS.md:L70-L75
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 174195f1a392bfe4170141b941ae1971938283d3: the exported resolver now returns plain string IDs and has no Mongoose type in its public signature; data-schemas accepts those IDs in its existing filtered query.
| @@ -1456,7 +1457,7 @@ export function createAgentMethods( | |||
|
|
|||
There was a problem hiding this comment.
When a manage:agents capability holder has no tenantId and TENANT_ISOLATION_STRICT is left at its documented default of false, tenantContextMiddleware deliberately continues without a tenant scope (packages/api/src/middleware/tenant.ts:162-168) and the Mongoose plugin consequently injects no predicate. Removing _id here then turns Agent.find(baseQuery) into a cross-tenant query, exposing—and during avatar refresh potentially updating—agents belonging to every tenant; the unrestricted path must add an explicit caller tenant/legacy-tenant predicate or reject the bypass when no tenant can be established.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 174195f1a392bfe4170141b941ae1971938283d3: unrestricted list queries always add an explicit authenticated tenant predicate; missing tenant context is restricted to legacy tenantId null/missing records. Covered by Mongo-backed tests without ambient request context.
| if (await hasManageAgentsCapability(user, deps)) { | ||
| return { accessibleIds: null, editableIds: null }; |
There was a problem hiding this comment.
For a manager with a cold per-user avatar cache, this resolver returns accessibleIds: null, which is then reused by resolveAvatarRefresh before the filtered list query. A search for one agent therefore selects up to MAX_AVATAR_REFRESH_AGENTS (1000) S3-backed agents from the entire tenant and blocks the response while refreshListAvatars performs an S3 refresh plus an updateAgent write for each changed URL; large tenants can turn the marketplace search into hundreds of unrelated network calls and database writes every 30 minutes for each manager. Keep the unrestricted scope out of this background refresh, or make the refresh tenant-shared and independent of the search response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 174195f1a392bfe4170141b941ae1971938283d3: managers skip the full-tenant prequery and only presign S3 avatars in the returned page. No agent writes occur after the cursor snapshot; URLs are cached per user and tenant. Paginated browser test proves later pages are not lost.
|
Review handoff for exact pushed head |
|
Review handoff for exact pushed head |
|
@codex review the latest head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e485d1d179
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const cache = getLogStores(CacheKeys.S3_EXPIRY_INTERVAL); | ||
| const refreshKey = `${userId}:agents_avatar_refresh`; | ||
| const refreshKey = `${userId}:${req.user.tenantId ?? ''}:agents_avatar_refresh`; |
There was a problem hiding this comment.
When a user has already populated this cache and then replaces an S3 agent avatar, the upload handler still deletes the old key ${req.user.id}:agents_avatar_refresh at line 2014 rather than this new tenant-scoped key. Because the upload also deletes the old avatar object, subsequent list responses can keep substituting its cached signed URL for up to 30 minutes, leaving the newly uploaded avatar broken or stale; update the invalidation path to construct the same tenant-aware key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in af339f1eb2de23ae90b4f4973cc0d4e8633b7179: both the list writer and upload invalidator call one typed cache-key builder with the authenticated user and tenant. A successful upload regression checks the deleted tenant key, and the list test checks its matching read key. Manager page caches also compare the stored avatar path before reusing a URL, so a different user's upload cannot leave a stale signed URL.
|
Review handoff for exact pushed head |
|
@codex review the latest head |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
A user with the
manage:agentssystem capability can open an unshared agent directly, but the browser-facingGET /api/agentslist and marketplace search omit it because the handler filters only by explicit agent ACL entries. A manager who does not know the agent ID cannot discover it.The browser list now honors the same management capability. Managers can find agents within their authenticated tenant, or only legacy untenanted agents when their request has no tenant; non-managers continue to see only ACL-accessible agents. Search, category, promoted, pagination, public status and VIEW skill-scope filtering are unchanged. Capability-check failures fall back to ACL filtering.
Fixes #16317
How it works
The browser controller wires the typed access and avatar helpers in
packages/apito the tenant-filtered query inpackages/data-schemas. The capability check is shared with the machine-authenticated management list. In the non-manager startup path, capability and ACL reads overlap instead of adding a serial round trip.Type of change
Testing
Tested environments/configuration: MongoDB 8.2.1 in memory with an actual role-scoped
manage:agentsgrant; authenticated tenant IDs and legacy requests without a tenant; S3-avatar list pagination. Managers can discover unshared agents by search without crossing tenant boundaries.Automated tests:
api/server/controllers/agents/v1.spec.jsandapi/server/controllers/avatar.contentFilter.spec.js: 49 passed across list security, S3 avatar refresh and upload content-filter coverage, including tenant-aware list cache access and successful upload invalidation.packages/data-schemas/src/methods/agent.spec.tsaccess-list security: 20 passed, including explicit tenant/legacy separation without ambient tenant context and ACL string-ID casting.packages/api/src/agents/reads.spec.tsandlistingAvatars.spec.ts: 24 passed, including parallel ACL/capability reads, bounded manager avatar refresh, cache expiry, shared cache keys and cross-user avatar replacement.tsc --noEmitinpackages/apiandpackages/data-schemas: passed.npm run build --workspace @librechat/data-schemasandnpm run build --workspace @librechat/api: passed.npm run static-checkson the staged diff: passed.Screenshots / recordings
No layout or styling changes; this changes which existing marketplace cards appear. No before/after screenshot was captured because local Chromium cannot start without the sandbox's missing
libatksystem library. The running-list behavior is covered by Mongo-backed controller tests; CI runs the Lighthouse browser lane.Risk / compatibility
An unrestricted ID filter is only used after a successful
manage:agentscheck. Even without request tenant context, the query explicitly restricts to the authenticated tenant, or to legacy records wheretenantIdis null/missing. Empty/omitted ACL lists still match nothing, and invalid permission masks reject before any bypass. Ordinary users' ACL reads are issued in parallel with the capability check; managers incur speculative ACL reads but never depend on their outcome. For managers, presigned S3 URLs are cached per user and tenant, bounded at the existing 1000-agent budget and expiring no later than the first signed page; they are not persisted, avoiding full-tenant avatar work and cursor mutation. Page-only cache entries cannot suppress the old full ACL refresh after capability downgrade. Avatar uploads invalidate the same user/tenant key that listing reads; if another user uploads, the cached URL is re-signed when the stored avatar path changes. Ordinary users keep their existing pre-query avatar refresh.Checklist