feat(mcp-gateway): team MCP gateway UI replacing the marketplace behind the mcp-gateway flag - #3643
Merged
Merged
Conversation
|
😎 Merged successfully - details. |
|
React Doctor found 8 issues in 6 files · 8 warnings. 8 warnings
Reviewed by React Doctor for commit |
Contributor
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
cvolzer3
marked this pull request as ready for review
July 30, 2026 17:46
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+258
to
+259
| // per-tool defaults when sharing a server with an agent. | ||
| const DESTRUCTIVE_TOOL_RE = |
Contributor
There was a problem hiding this comment.
When a server exposes a sensitive tool whose name does not match this narrow regex, defaultAgentGrantPolicy marks it as approved and the sharing dialog submits that default without requiring an explicit selection, allowing the agent to invoke the tool unrestricted. How this was verified: The sharing dialog applies this helper to untouched tools and sends every resulting policy with the service-account access request.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/mcp-gateway/gatewayServers.ts
Line: 258-259
Comment:
**Agent grants auto-approve unmatched tools**
When a server exposes a sensitive tool whose name does not match this narrow regex, `defaultAgentGrantPolicy` marks it as approved and the sharing dialog submits that default without requiring an explicit selection, allowing the agent to invoke the tool unrestricted. **How this was verified:** The sharing dialog applies this helper to untouched tools and sends every resulting policy with the service-account access request.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+179
to
+184
| {policy.decided_by === "rule" ? ( | ||
| <Badge color="gray" variant="soft" size="1"> | ||
| Blocked by team policy | ||
| </Badge> | ||
| ) : ( | ||
| <ToolPolicyToggle |
Contributor
There was a problem hiding this comment.
When a blocked team ceiling returns locked: true with a decided_by value other than rule, this branch renders an editable toggle and includes its value in the grant payload, causing the UI to offer an invalid override and the share request to be rejected or misapplied.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/mcp-gateway/components/parts/GiveAccessDialog.tsx
Line: 179-184
Comment:
**Team-ceiling locks remain editable**
When a blocked team ceiling returns `locked: true` with a `decided_by` value other than `rule`, this branch renders an editable toggle and includes its value in the grant payload, causing the UI to offer an invalid override and the share request to be rejected or misapplied.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Contributor
|
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
|
|
||
| /** Default policy offered when granting an agent access to a tool. */ | ||
| export function defaultAgentGrantPolicy(toolName: string): AgentPolicyState { | ||
| return DESTRUCTIVE_TOOL_RE.test(toolName) ? "do_not_use" : "approved"; |
There was a problem hiding this comment.
Low: Agent tools default to allow based on attacker-controlled names
An MCP server controls its tool names, so a malicious server can name a destructive tool search and have this flow submit it as approved when the user shares the server with an agent. Default tools to blocked and require the user to explicitly allow each capability.
Suggested change
| return DESTRUCTIVE_TOOL_RE.test(toolName) ? "do_not_use" : "approved"; | |
| return "do_not_use"; |
PR overviewThis pull request replaces the MCP marketplace with a team MCP gateway UI behind the Two security issues remain open. A teammate-controlled OAuth server can cause the Connect flow to launch an arbitrary registered protocol, while server-supplied tool names can cause destructive capabilities to be approved by default when shared with an agent. Neither issue has yet been addressed, so URL scheme validation and explicit tool approval are still needed. Open issues (2)
Fixed/addressed: 0 · PR risk: 6/10 |
sortafreel
approved these changes
Jul 31, 2026
Contributor
🦔 ReviewHog reviewed this pull requestFound 1 must fix, 14 should fix, 22 consider. Published 37 findings (view the review). |
…ateway flag
Replaces the per-user MCP marketplace with the team gateway surface from the
design handoff when the mcp-gateway flag is on: servers home with connection
status, server detail with per-scope tool policies and the admin access
section, team & agents roster, agent service-account detail (identity, token
rotation, shared servers, call history), member detail with per-server
revocation, team settings (custom-server gate, approval baselines, server
access, team rules), the audit log, and the gateway add-server form with
sharing options. The legacy marketplace remains the fallback while the flag
is off.
Adds hand-written /api/projects/{id}/mcp_gateway/* client methods and types
to @posthog/api-client (the endpoints are not in the generated OpenAPI client
yet), and portable helpers with tests in @posthog/core/mcp-gateway.
Generated-By: PostHog Code
Task-Id: 7ecfb6f3-39d8-4443-96e7-36e9d1ebd144
…nstead of a state-sync effect Addresses the react-doctor blocking finding (no-adjust-state-on-prop-change) on the add-server form: the mutation now re-reads the gateway registry and returns the created server, so the form navigates from the mutation result instead of chaining pendingUrl state through an effect. Also derives the role-guarded route at render in McpGatewayView rather than correcting it in an effect, and renames the audit pager's map variable so the key reads as the page number it is. Generated-By: PostHog Code Task-Id: 7ecfb6f3-39d8-4443-96e7-36e9d1ebd144
The mcp_gateway/members/ endpoint returns a DRF-paginated object, not a bare array, so the Team & agents page crashed with "filteredMembers.slice is not a function". Parse results and pass limit=500 like the sibling gateway list endpoints. Generated-By: PostHog Code Task-Id: e58ce040-9955-42aa-8a4a-40bb4c98ad6a
The backend removed the team-shared credential concept (PostHog/posthog#72409): no auth_mode, shared_credential, allow_personal_connections, or your_connection.scope. Every credential is personal to the member who connected it, and agents reach it through explicit grants instead. These types are hand-mirrored rather than generated, so the removed fields failed silently instead of at compile time: the rail filtered on `auth_mode === "individual"`, which no server satisfies once the field is gone, so "Your connections" rendered its empty state even for a server that had just connected successfully. The two rail sections collapse into one connected-servers list, and the shared-credential panel, its rotate button, the personal-connections toggle and the pre-authorized member card are gone. The per-connection self-disable switch stays, now offered for any connection rather than only members on shared servers - it is the only surface that reveals a self-disabled installation.
…and catalog The registry is sparse now: a gateway row exists only for servers the team uses or an admin explicitly configured. The home screen merges real rows with recommended catalog templates (connect-only cards), team settings toggles untouched templates via set_template_enabled, and the enable/disable-all switch drives the new default_servers_enabled posture so it also covers catalog servers published later. Generated-By: PostHog Code Task-Id: 1e260db0-3ca7-420b-b2b1-61af484afaa2
Radix Tooltip stamps its own data-state (closed/delayed-open) onto its child. With a Radix Themes Switch as that child it overwrote the switch's own checked/unchecked state, and the track's colour is driven by selectors matching those two values — so neither matched, background position fell back to 0% and the off switch rendered full accent yellow. Wrapping the Switch in a span gives the Tooltip something else to stamp. Hover and focus still open the tooltip, since React's onFocus delegates through focusin, which bubbles from the inner button. The same pattern was fixed in GatewayServerDetail.
There is no rotate_token endpoint behind mcp_gateway/service_accounts/<id>/rotate_token/, so the Rotate button and its confirmation dialog could only ever fail. The masked token beside it went too - it is not actionable on its own, and the full token is still shown once at creation. That leaves nothing on this page that mints a token, so the NewTokenDialog render goes with it: newToken is per-hook-instance state and rotation was the only thing setting it here. Shared servers now sort granted-first with a labelled divider before the rest, so an agent's actual reach reads without scanning switches. The row moves into a ServerAccessRow component rather than duplicating the JSX across both groups.
…hooks The Code app renders neither an approval-baseline picker nor a team-rules list, so useGatewayRules had no consumer at all and useGatewayConfig's applyPreset mutation was never called. Presets and rules are set from the PostHog web UI; nothing here needs to write them. Removes the hooks plus the client methods only they reached (apply_preset, rules list, rules patch) and the types those methods owned. Rules still surface read-only through the resolved tool policy - decided_by "rule"/"preset", rule_name, rule_description - so the lock badges and their tooltips in GatewayToolRow and GiveAccessDialog are unaffected.
Connecting stored the credential but never listed the upstream tools, so a gateway server sat at zero tools until an admin pressed the manual refresh. List them on connect, register, and reconnect, and keep an empty-catalog backstop on the detail page so older connections and failed listings recover.
cvolzer3
force-pushed
the
posthog-code/mcp-gateway-ui
branch
from
July 31, 2026 13:00
0986865 to
d607d59
Compare
Contributor
Author
|
@charlesvien @jonathanlab I'm merging a main repo PR then I'll merge this one right after |
Contributor
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ReviewHog Report
Business logic
Issues: 2 issues
Files (4)
packages/core/src/mcp-gateway/gatewayServers.tspackages/core/src/mcp-gateway/gatewayAddServer.tspackages/core/src/mcp-gateway/gatewayInstallFlow.tspackages/core/src/mcp-gateway/gatewayToolDiscovery.ts
What were the main changes
- New portable @posthog/core/mcp-gateway module: server/template filtering, connection-status resolution, policy-state ceiling/strictness math, agent-grant defaults, and audit time formatting
- gatewayToolDiscovery.ts backstops the sparse registry by auto-listing a server's tools right after a connect
- Review flagged defaultAgentGrantPolicy() in gatewayServers.ts: unmatched (attacker-named) tools default to 'approved' rather than blocked, letting a malicious server auto-grant an agent unrestricted tool access
- No dedicated test files included in this diff despite PR description citing unit test coverage — verify test files land alongside these
Frontend
Issues: 2 issues
Files (5)
packages/ui/src/features/mcp-gateway/components/McpGatewayView.tsxpackages/ui/src/features/mcp-gateway/gatewayRoute.tspackages/ui/src/features/mcp-gateway/components/parts/GatewayRail.tsxpackages/ui/src/features/mcp-servers/components/McpServersView.tsxpackages/shared/src/flags.ts
What were the main changes
- New MCP_GATEWAY_FLAG gates rendering McpGatewayView in place of the legacy marketplace at /mcp-servers
- McpGatewayView is the top-level route switch (servers/server/add/team/agent/member/settings/audit) with a role-based route guard
- GatewayRail is the persistent left-nav: connected-servers search plus admin-only links to team/settings/audit
Frontend
Issues: 4 issues
Files (3)
packages/ui/src/features/mcp-gateway/hooks/gatewayKeys.tspackages/ui/src/features/mcp-gateway/hooks/useGatewayConfig.tspackages/ui/src/features/mcp-gateway/hooks/useGatewayServers.ts
What were the main changes
- Central query-key scheme and the useGatewayServers hook (list, connect/reconnect/disconnect, self-toggle, template enable, bulk enable/disable, delete) that most gateway screens depend on
- useGatewayConfig resolves the caller's admin/agent-management role used throughout the feature for gating UI
- Connect success paths trigger gatewayToolDiscovery to auto-list tools for a freshly connected server
Frontend
Issues: 3 issues
Files (2)
packages/ui/src/features/mcp-gateway/components/parts/GatewayServersHome.tsxpackages/ui/src/features/mcp-gateway/components/parts/avatars.tsx
What were the main changes
- Servers home: search/category filtering over real gateway rows merged with connect-only 'recommended' catalog template cards (registry is sparse)
- Shared avatar primitives (UserAvatar, RobotAvatar, AvatarStack) used across the whole gateway feature for people/agent identity
Frontend
Issues: 4 issues
Files (2)
packages/ui/src/features/mcp-gateway/components/parts/GatewayAddServer.tsxpackages/ui/src/features/mcp-gateway/hooks/useRegisterGatewayServer.ts
What were the main changes
- Add-custom-server form: OAuth/API-key auth, admin-only team-enable toggle, and agent-sharing checklist gated by canManageAgentAccess
- useRegisterGatewayServer resolves the new registry row post-install and triggers tool discovery before navigating to its detail page
Frontend
Issues: 4 issues
Files (1)
packages/ui/src/features/mcp-gateway/components/parts/GatewayServerDetail.tsx
What were the main changes
- Largest single file in the PR (970 added lines): server hero, self connect/disconnect/reconnect, admin Access section (member connections + agent grants), scoped tool-policy list with bulk actions, and delete/disconnect confirmation wiring
- Kept as its own review unit given its size and centrality — genuinely one coherent screen rather than several smaller concerns
Frontend
Issues: 3 issues
Files (9)
packages/ui/src/features/mcp-gateway/components/parts/GatewayToolRow.tsxpackages/ui/src/features/mcp-gateway/components/parts/GiveAccessDialog.tsxpackages/ui/src/features/mcp-gateway/components/parts/GatewayDeleteServerDialog.tsxpackages/ui/src/features/mcp-gateway/hooks/useGatewayToolPolicies.tspackages/ui/src/features/mcp-servers/components/parts/ToolPolicyToggle.tsxpackages/ui/src/features/mcp-servers/components/parts/ToolRow.tsxpackages/ui/src/features/mcp-servers/components/parts/ToolPermissionList.tsxpackages/ui/src/features/mcp-servers/components/parts/ServerDetailView.tsxpackages/ui/src/features/mcp-servers/hooks/useMcpInstallationTools.ts
What were the main changes
- Tool-policy control surface shared between the new gateway UI and the legacy per-user marketplace: ToolPolicyToggle gains disabledStates/allowedStates for team-ceiling locks and agent-only state restriction
- GiveAccessDialog is the 'share server with an agent' flow; review flagged that a blocked team ceiling with decided_by != 'rule' still renders as an editable toggle and is submitted in the grant payload, offering an invalid override
- Legacy ToolRow/ToolPermissionList/ServerDetailView/useMcpInstallationTools updated to respect the new team_state/locked ceiling fields returned by gateway-aware backends
- useGatewayToolPolicies adds scope-aware auto-discovery and bulk-set, rejecting needs_approval for agent scope
Frontend
Issues: 6 issues
Files (3)
packages/ui/src/features/mcp-gateway/components/parts/GatewayTeamView.tsxpackages/ui/src/features/mcp-gateway/components/parts/GatewayMemberDetail.tsxpackages/ui/src/features/mcp-gateway/hooks/useGatewayMembers.ts
What were the main changes
- Team & agents roster (agents first, then searchable member list) and the per-member detail screen with per-server revocation
- useGatewayMembers optimistically patches both the members and servers caches on access-toggle since the endpoint returns no body
Frontend
Issues: 3 issues
Files (3)
packages/ui/src/features/mcp-gateway/components/parts/GatewayAgentDetail.tsxpackages/ui/src/features/mcp-gateway/hooks/useServiceAccounts.tspackages/ui/src/features/mcp-gateway/components/parts/NewTokenDialog.tsx
What were the main changes
- Agent (service account) detail page: identity, pause/resume, shared servers with per-server tool-policy shortcut, and recent audit calls
- useServiceAccounts covers create/pause/delete/grant-access mutations and surfaces a show-once token via NewTokenDialog
- NewTokenDialog does not appear to be wired into any visible creation flow in this diff — worth confirming it's actually reachable
Frontend
Issues: 1 issue
Files (1)
packages/ui/src/features/mcp-gateway/components/parts/GatewayTeamSettings.tsx
What were the main changes
- Admin team settings: custom-server gate, member agent-access gate, enable/disable-all posture control, and per-server/per-template access list with materialize-on-write for untouched catalog templates
Frontend
Issues: 3 issues
Files (2)
packages/ui/src/features/mcp-gateway/components/parts/GatewayAuditLog.tsxpackages/ui/src/features/mcp-gateway/hooks/useGatewayAudit.ts
What were the main changes
- Audit log screen: quick filters (all/agents/approvals/blocked), agent-caller filter, and paginated table with a sliding page-number window
- useGatewayAudit backs both the paginated log and the agent-detail 'recent calls' list
Documentation
Issues: 1 issue
Files (1)
docs/LOCAL-DEVELOPMENT.md
What were the main changes
- New troubleshooting sections: scheme-less VITE_POSTHOG_API_HOST silently breaking all feature-flag fetches, and OAuth invalid_scope caused by a non-empty scope ceiling on the local OAuth application
Bugfix
Issues: 1 issue
Files (1)
packages/ui/src/features/scouts/components/ScoutConfigControls.tsx
What were the main changes
- Unrelated drive-by fix: wraps the enable Switch in a span so Radix Tooltip's data-state stamping doesn't overwrite the Switch's own checked/unchecked state and stick it on the accent color
Comment on lines
+63
to
+65
| ...(values.authType === "api_key" && values.apiKey | ||
| ? { api_key: values.apiKey } | ||
| : {}), |
Contributor
There was a problem hiding this comment.
api_key value is sent untrimmed, unlike every other field in the same request builder
Why we think it's a valid issue
- Checked:
buildGatewayInstallRequestinpackages/core/src/mcp-gateway/gatewayAddServer.ts(PR head 0986865) and the sole call site plus input handling inpackages/ui/src/features/mcp-gateway/components/parts/GatewayAddServer.tsx. - Found: In the builder,
name/url/descriptionare.trim()ed, and both OAuth credentials trim in both guard and payload (values.clientId.trim() ? { client_id: values.clientId.trim() }, lines ~67-71).api_keyalone uses the raw value in both guard and payload:values.apiKey ? { api_key: values.apiKey }(lines 63-65). It is the only credential/text field not trimmed. - Found: In
GatewayAddServer.tsx:168-169the API-key input storese.target.valueverbatim — identical to the clientId/clientSecret inputs — so there is no upstream trim; the builder is the only place trimming happens, and api_key is the lone omission. No other guard prevents whitespace reaching the request. - Impact: A credential pasted from a provider dashboard with an incidental leading/trailing space or newline (a routine real-world occurrence) is sent verbatim, so upstream authentication for the registered server can silently fail with no visible cause. The single-field divergence from four trimmed siblings marks this as an oversight, not an intentional choice, and the fix mirrors the existing code exactly. Real trigger + concrete consequence + trivial fix clear the bar as a minor robustness bug; the reviewer's
considerpriority is appropriate.
Issue description
In buildGatewayInstallRequest (imported and invoked directly from GatewayAddServer.tsx line 59), name, url, description, client_id, and client_secret are all trimmed before being sent, but api_key is passed through as values.apiKey verbatim (line 64). A copy-pasted API key with incidental leading/trailing whitespace — common when copying from a provider dashboard — will be sent as-is and can silently fail upstream authentication, while a user might reasonably read the 'never exposed' credential field as safe to paste with surrounding whitespace.
Suggested fix
Trim the value consistently: ...(values.authType === "api_key" && values.apiKey.trim() ? { api_key: values.apiKey.trim() } : {}).
Prompt to fix with AI (copy-paste)
## Context
@packages/core/src/mcp-gateway/gatewayAddServer.ts#L63-65
<issue_description>
In `buildGatewayInstallRequest` (imported and invoked directly from `GatewayAddServer.tsx` line 59), `name`, `url`, `description`, `client_id`, and `client_secret` are all trimmed before being sent, but `api_key` is passed through as `values.apiKey` verbatim (line 64). A copy-pasted API key with incidental leading/trailing whitespace — common when copying from a provider dashboard — will be sent as-is and can silently fail upstream authentication, while a user might reasonably read the 'never exposed' credential field as safe to paste with surrounding whitespace.
</issue_description>
<issue_validation>
- **Checked:** `buildGatewayInstallRequest` in `packages/core/src/mcp-gateway/gatewayAddServer.ts` (PR head 0986865) and the sole call site plus input handling in `packages/ui/src/features/mcp-gateway/components/parts/GatewayAddServer.tsx`.
- **Found:** In the builder, `name`/`url`/`description` are `.trim()`ed, and both OAuth credentials trim in both guard and payload (`values.clientId.trim() ? { client_id: values.clientId.trim() }`, lines ~67-71). `api_key` alone uses the raw value in both guard and payload: `values.apiKey ? { api_key: values.apiKey }` (lines 63-65). It is the only credential/text field not trimmed.
- **Found:** In `GatewayAddServer.tsx:168-169` the API-key input stores `e.target.value` verbatim — identical to the clientId/clientSecret inputs — so there is no upstream trim; the builder is the only place trimming happens, and api_key is the lone omission. No other guard prevents whitespace reaching the request.
- **Impact:** A credential pasted from a provider dashboard with an incidental leading/trailing space or newline (a routine real-world occurrence) is sent verbatim, so upstream authentication for the registered server can silently fail with no visible cause. The single-field divergence from four trimmed siblings marks this as an oversight, not an intentional choice, and the fix mirrors the existing code exactly. Real trigger + concrete consequence + trivial fix clear the bar as a minor robustness bug; the reviewer's `consider` priority is appropriate.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Trim the value consistently: `...(values.authType === "api_key" && values.apiKey.trim() ? { api_key: values.apiKey.trim() } : {})`.
</potential_solution>
Comment on lines
+120
to
+147
| yourConnections.map((server) => { | ||
| const connection = server.your_connection; | ||
| if (!connection) return null; | ||
| const status = getGatewayConnectionStatus(connection); | ||
| const usedAgo = formatAgo(connection.last_used_at); | ||
| const sub = | ||
| status === "needs_reauth" | ||
| ? "Reconnect required" | ||
| : status === "pending_oauth" | ||
| ? "Finish connecting" | ||
| : usedAgo | ||
| ? `used ${usedAgo}` | ||
| : "Connected"; | ||
| return ( | ||
| <RailServerRow | ||
| key={server.id} | ||
| server={server} | ||
| templatesById={templatesById} | ||
| active={activeServerId === server.id} | ||
| sub={sub} | ||
| connectionStatus={status} | ||
| onClick={() => | ||
| onNavigate({ view: "server", serverId: server.id }) | ||
| } | ||
| /> | ||
| ); | ||
| }) | ||
| )} |
Contributor
There was a problem hiding this comment.
Rail connection status ignores self-disabled, team-disabled, and revoked states
Why we think it's a valid issue
- Checked:
railConnectedServersandgetGatewayConnectionStatusinpackages/core/src/mcp-gateway/gatewayServers.ts; the rail row rendering inGatewayRail.tsx:120-147; the type fields onMcpGatewayServer/McpGatewayYourConnectioninpackages/api-client/src/mcp-gateway.ts; and every production read ofis_enabled/is_team_enabled/is_revoked_for_youacross the mcp-gateway UI. - Found (status derivation):
getGatewayConnectionStatus(gatewayServers.ts:126-132) only inspectsneeds_reauthandpending_oauth, returning"connected"otherwise;railConnectedServers(L17-22) filters solely onyour_connection !== null. Neither consultsis_enabled,is_team_enabled, oris_revoked_for_you. - Found (self-disabled contradiction):
GatewayServerDetail.tsx:162derivesselfEnabled = yourConnection.is_enabledand L343-357 renders a "Disabled for you" banner when it is false — yetyour_connectionstays non-null, so the rail still lists the server with a green dot and "used Xh ago". - Found (team-disabled contradiction):
GatewayServersHome.tsx:236computesoff = !server.is_team_enabledand renders an "Off" badge, andGatewayServerDetail.tsx:246shows an "Off" badge — proving team-disabled servers are present in the sameserversarray the rail consumes; the rail shows them as green "Connected". - Found (revoked never surfaced): grep confirms
is_revoked_for_you(defined at mcp-gateway.ts:71) is read nowhere in production UI — only in.test.tsxfixtures — so a member whose access an admin revoked gets no indication anywhere, including the rail. - Impact: A persistent, always-visible left-nav element misrepresents connection state for normal, first-class product states (member self-disable via
toggleYourConnection; admin team-disable via the settings switch), directly contradicting the detail and home screens for the same server. These are reachable everyday states, not edge cases — a real logic/correctness UI-state defect.should_fixis appropriate: user-visible and confusing, but UI-only with no data/security/crash consequence. (Theis_revoked_for_yousub-claim depends on the backend keepingyour_connectionpopulated after revocation, which I could not confirm, but the self- and team-disabled cases stand independently.)
Issue description
The "Your connections" list in the rail computes status purely from getGatewayConnectionStatus(connection) (packages/core/src/mcp-gateway/gatewayServers.ts), which only inspects pending_oauth/needs_reauth on your_connection. It never checks your_connection.is_enabled (the member's own "Disabled for you" switch, toggled via gateway.toggleYourConnection and rendered as a dedicated "Disabled for you" banner in GatewayServerDetail.tsx), nor server.is_team_enabled (an admin-side master switch, surfaced as an "Off" badge for every role in GatewayServersHome.tsx's GatewayServerCard), nor server.is_revoked_for_you (which is defined on McpGatewayServer but is never read anywhere in the codebase — not in GatewayRail, not in GatewayServersHome, not in GatewayServerDetail). Because railConnectedServers only filters on your_connection !== null, a server the caller has explicitly disabled for themselves, that an admin has turned off team-wide, or where the admin has revoked the caller's access, still appears in the persistent left rail with a green "Connected" dot and a "Connected"/"used Xh ago" sub-label — actively contradicting what the server detail page (and, for team-disabled, the servers home) tells the same user about the same server.
Suggested fix
Extend GatewayConnectionStatus (or add a new derived state) to account for !server.is_team_enabled, !connection.is_enabled, and server.is_revoked_for_you, and have GatewayRail's RailServerRow render a distinct "Off"/"Disabled"/"Access revoked" indicator and sub-label for those cases instead of the green "Connected" dot — mirroring the off ? <Badge>Off</Badge> treatment already used in GatewayServersHome's GatewayServerCard.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/mcp-gateway/components/parts/GatewayRail.tsx#L120-147
<issue_description>
The "Your connections" list in the rail computes status purely from `getGatewayConnectionStatus(connection)` (packages/core/src/mcp-gateway/gatewayServers.ts), which only inspects `pending_oauth`/`needs_reauth` on `your_connection`. It never checks `your_connection.is_enabled` (the member's own "Disabled for you" switch, toggled via `gateway.toggleYourConnection` and rendered as a dedicated "Disabled for you" banner in GatewayServerDetail.tsx), nor `server.is_team_enabled` (an admin-side master switch, surfaced as an "Off" badge for every role in GatewayServersHome.tsx's `GatewayServerCard`), nor `server.is_revoked_for_you` (which is defined on `McpGatewayServer` but is never read anywhere in the codebase — not in GatewayRail, not in GatewayServersHome, not in GatewayServerDetail). Because `railConnectedServers` only filters on `your_connection !== null`, a server the caller has explicitly disabled for themselves, that an admin has turned off team-wide, or where the admin has revoked the caller's access, still appears in the persistent left rail with a green "Connected" dot and a "Connected"/"used Xh ago" sub-label — actively contradicting what the server detail page (and, for team-disabled, the servers home) tells the same user about the same server.
</issue_description>
<issue_validation>
- **Checked:** `railConnectedServers` and `getGatewayConnectionStatus` in `packages/core/src/mcp-gateway/gatewayServers.ts`; the rail row rendering in `GatewayRail.tsx:120-147`; the type fields on `McpGatewayServer`/`McpGatewayYourConnection` in `packages/api-client/src/mcp-gateway.ts`; and every production read of `is_enabled`/`is_team_enabled`/`is_revoked_for_you` across the mcp-gateway UI.
- **Found (status derivation):** `getGatewayConnectionStatus` (gatewayServers.ts:126-132) only inspects `needs_reauth` and `pending_oauth`, returning `"connected"` otherwise; `railConnectedServers` (L17-22) filters solely on `your_connection !== null`. Neither consults `is_enabled`, `is_team_enabled`, or `is_revoked_for_you`.
- **Found (self-disabled contradiction):** `GatewayServerDetail.tsx:162` derives `selfEnabled = yourConnection.is_enabled` and L343-357 renders a "Disabled for you" banner when it is false — yet `your_connection` stays non-null, so the rail still lists the server with a green dot and "used Xh ago".
- **Found (team-disabled contradiction):** `GatewayServersHome.tsx:236` computes `off = !server.is_team_enabled` and renders an "Off" badge, and `GatewayServerDetail.tsx:246` shows an "Off" badge — proving team-disabled servers are present in the same `servers` array the rail consumes; the rail shows them as green "Connected".
- **Found (revoked never surfaced):** grep confirms `is_revoked_for_you` (defined at mcp-gateway.ts:71) is read nowhere in production UI — only in `.test.tsx` fixtures — so a member whose access an admin revoked gets no indication anywhere, including the rail.
- **Impact:** A persistent, always-visible left-nav element misrepresents connection state for normal, first-class product states (member self-disable via `toggleYourConnection`; admin team-disable via the settings switch), directly contradicting the detail and home screens for the same server. These are reachable everyday states, not edge cases — a real logic/correctness UI-state defect. `should_fix` is appropriate: user-visible and confusing, but UI-only with no data/security/crash consequence. (The `is_revoked_for_you` sub-claim depends on the backend keeping `your_connection` populated after revocation, which I could not confirm, but the self- and team-disabled cases stand independently.)
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Extend `GatewayConnectionStatus` (or add a new derived state) to account for `!server.is_team_enabled`, `!connection.is_enabled`, and `server.is_revoked_for_you`, and have `GatewayRail`'s `RailServerRow` render a distinct "Off"/"Disabled"/"Access revoked" indicator and sub-label for those cases instead of the green "Connected" dot — mirroring the `off ? <Badge>Off</Badge>` treatment already used in `GatewayServersHome`'s `GatewayServerCard`.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
Fixed in ced13c0. Added getGatewayRailStatus in gatewayServers.ts which folds the team master switch, per-user revocation, and the caller's own enable toggle into the connection status (precedence: team off > revoked > self-disabled > auth states). The rail now renders a gray dot with "Off for the team" / "Access revoked" / "Disabled for you" sub-labels for those states instead of the green Connected dot. Covered by new unit tests in gatewayServers.test.ts and GatewayRail.test.tsx.
Comment on lines
+34
to
+36
| const { isAdmin, allowCustomServers, canManageAgentAccess, configLoading } = | ||
| useGatewayConfig(); | ||
| const canAddServers = isAdmin || allowCustomServers; |
Contributor
There was a problem hiding this comment.
canAddServers defaults to permissive before gateway config resolves
Why we think it's a valid issue
- Checked:
useGatewayConfig.tsfallback defaults,canAddServersderivation (McpGatewayView.tsx:36), whether any loading skeleton suppresses the first render, and everycanAddServers-gated add affordance (GatewayRail.tsx:69,GatewayServersHome.tsx:95) plus theaddbranch ofisRouteAllowed. - Found:
useGatewayConfigreturnsisAdmin: config?.is_admin ?? false(fail-closed) butallowCustomServers: config?.allow_custom_servers ?? true(fail-open), so during the first config fetchcanAddServers = false || true = true.McpGatewayViewhas noif (configLoading)gate — it renders the full layout immediately, so on initial mount the rail's "+" IconButton (GatewayRail.tsx:69) and the servers-home add affordance (GatewayServersHome.tsx:95, the default route) both render for every caller. - Impact (confirmed, reachable): A non-admin member on a team with
allow_custom_servers=falsesees the add-server button appear during the first fetch and then vanish once config resolves — a genuine, reachable fail-open flash, not a hypothetical one (unlike the admin-view loading concern, this needs no future entry point; the rail/home always render eagerly). The fix is trivial and the default is inconsistent with the deliberate fail-closedisAdminsibling two lines up. - Impact (why only
consider): Severity is low — a sub-second cosmetic flicker on the initial fetch only (cached config makesconfigLoadingfalse on later visits), no security/data consequence (custom-server creation is enforced server-side, and the route guard snapsaddback toserversthe instant config resolves). This is a real-but-minor issue that fits the reviewer'sconsiderrating; keeping it on record without over-surfacing.
Issue description
canAddServers = isAdmin || allowCustomServers is computed from useGatewayConfig(), whose loading-state fallbacks are isAdmin: config?.is_admin ?? false but allowCustomServers: config?.allow_custom_servers ?? true. While configLoading is true (the first fetch of getMcpGatewayConfig), any team whose actual allow_custom_servers is false and whose caller is not an admin will still see canAddServers evaluate to true for the brief loading window — the rail's "+" add-server button renders and the add route is reachable via isRouteAllowed's canAddServers check — before flipping to the correct, more restrictive value once the query resolves. This is the opposite of the fail-closed pattern used for isAdmin in the same hook, and doesn't match the options?.enabled ?? true convention used elsewhere in the codebase (which gates query execution, not authorization-sensitive UI).
Suggested fix
Default allowCustomServers (and allowMemberAgentAccess) to false while loading in useGatewayConfig, or gate the add-server affordances on !configLoading && canAddServers, so a member without custom-server permission never briefly sees the add-server button/route before the real setting loads.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/mcp-gateway/components/McpGatewayView.tsx#L34-36
<issue_description>
`canAddServers = isAdmin || allowCustomServers` is computed from `useGatewayConfig()`, whose loading-state fallbacks are `isAdmin: config?.is_admin ?? false` but `allowCustomServers: config?.allow_custom_servers ?? true`. While `configLoading` is true (the first fetch of `getMcpGatewayConfig`), any team whose actual `allow_custom_servers` is `false` and whose caller is not an admin will still see `canAddServers` evaluate to `true` for the brief loading window — the rail's "+" add-server button renders and the `add` route is reachable via `isRouteAllowed`'s `canAddServers` check — before flipping to the correct, more restrictive value once the query resolves. This is the opposite of the fail-closed pattern used for `isAdmin` in the same hook, and doesn't match the `options?.enabled ?? true` convention used elsewhere in the codebase (which gates query execution, not authorization-sensitive UI).
</issue_description>
<issue_validation>
- **Checked:** `useGatewayConfig.ts` fallback defaults, `canAddServers` derivation (`McpGatewayView.tsx:36`), whether any loading skeleton suppresses the first render, and every `canAddServers`-gated add affordance (`GatewayRail.tsx:69`, `GatewayServersHome.tsx:95`) plus the `add` branch of `isRouteAllowed`.
- **Found:** `useGatewayConfig` returns `isAdmin: config?.is_admin ?? false` (fail-closed) but `allowCustomServers: config?.allow_custom_servers ?? true` (fail-open), so during the first config fetch `canAddServers = false || true = true`. `McpGatewayView` has no `if (configLoading)` gate — it renders the full layout immediately, so on initial mount the rail's "+" IconButton (`GatewayRail.tsx:69`) and the servers-home add affordance (`GatewayServersHome.tsx:95`, the default route) both render for every caller.
- **Impact (confirmed, reachable):** A non-admin member on a team with `allow_custom_servers=false` sees the add-server button appear during the first fetch and then vanish once config resolves — a genuine, reachable fail-open flash, not a hypothetical one (unlike the admin-view loading concern, this needs no future entry point; the rail/home always render eagerly). The fix is trivial and the default is inconsistent with the deliberate fail-closed `isAdmin` sibling two lines up.
- **Impact (why only `consider`):** Severity is low — a sub-second cosmetic flicker on the initial fetch only (cached config makes `configLoading` false on later visits), no security/data consequence (custom-server creation is enforced server-side, and the route guard snaps `add` back to `servers` the instant config resolves). This is a real-but-minor issue that fits the reviewer's `consider` rating; keeping it on record without over-surfacing.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Default `allowCustomServers` (and `allowMemberAgentAccess`) to `false` while loading in `useGatewayConfig`, or gate the add-server affordances on `!configLoading && canAddServers`, so a member without custom-server permission never briefly sees the add-server button/route before the real setting loads.
</potential_solution>
Comment on lines
+399
to
+407
| const connections = server.connections; | ||
| if (connections.length === 0) return null; | ||
| const agentCount = server.agents.length; | ||
| const label = | ||
| connections.length === 1 | ||
| ? `${gatewayUserName(connections[0].user).split(" ")[0]} is connected` | ||
| : `${connections.length} teammates connected${ | ||
| agentCount ? ` · ${agentCount} agent${agentCount > 1 ? "s" : ""}` : "" | ||
| }`; |
Contributor
There was a problem hiding this comment.
Agent-sharing count is silently dropped when exactly one teammate is connected
Why we think it's a valid issue
- Checked:
CardPeopleRow(lines 385-416) and theMcpGatewayServershape inpackages/api-client/src/mcp-gateway.ts(connections: McpGatewayConnection[],agents: McpGatewayAgentAccess[]), plus theAvatarStackrender at line 410. - Found: The premise holds exactly. Line 400 early-returns when
connections.length === 0; theconnections.length === 1branch (line 404) emits"${firstName} is connected"with no reference toagentCount, while the else branch (lines 405-407) appends· ${agentCount} agent(s). So agent sharing is surfaced only when 2+ humans are connected. Line 410'sAvatarStackmapsconnections(humans) only, so in the single-connection case agents get no representation on the card at all — confirming this is an unintended asymmetry, not a deliberate omission (the plural branch clearly means to show it). - Found: The single-connection case is the common one (a lone owner), so the gap is hit routinely, not a rare edge.
- Impact: Real but display-only: the label stays truthful ("Alice is connected" is correct) — it is merely less complete, omitting the agent count for the most common configuration. This is an admin-only summary row, and the same sharing data is reachable on the server-detail admin Access section, so no data is lost and no functionality breaks. The one-line fix (shared
agentSuffixappended to both branches) is cheap and correct. - Priority: Lowering to
consider— no wrong data, crash, or functional/security/data impact; it is an informational-completeness gap on an admin summary row with the same info available elsewhere, milder than a functionalshould_fixdefect.
Issue description
In CardPeopleRow, agentCount (server.agents.length) is only surfaced in the label when connections.length !== 1 (the ${connections.length} teammates connected${agentCount ? ... : ""} branch). When exactly one teammate is connected, the label falls into the other branch (${gatewayUserName(connections[0].user).split(" ")[0]} is connected) which never references agentCount at all. So a server with 1 human connection and, say, 3 agents sharing it shows only "Alice is connected" with no indication any agents have access, while the same server with 2 human connections would correctly show "2 teammates connected · 3 agents". This is inconsistent handling of the same underlying data depending on an unrelated branch condition (number of human connections), and it under-informs admins — who are the only audience for this row (isAdmin && <CardPeopleRow .../>) — about agent access on the most common case (a single connected owner).
Suggested fix
Include the agent count in both branches, e.g.: const agentSuffix = agentCount ? \ · ${agentCount} agent${agentCount > 1 ? "s" : ""}` : "";` and append it to both the singular and plural label strings so agent sharing is always represented regardless of how many humans are connected.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/mcp-gateway/components/parts/GatewayServersHome.tsx#L399-407
<issue_description>
In `CardPeopleRow`, `agentCount` (`server.agents.length`) is only surfaced in the label when `connections.length !== 1` (the `${connections.length} teammates connected${agentCount ? ... : ""}` branch). When exactly one teammate is connected, the label falls into the other branch (`${gatewayUserName(connections[0].user).split(" ")[0]} is connected`) which never references `agentCount` at all. So a server with 1 human connection and, say, 3 agents sharing it shows only "Alice is connected" with no indication any agents have access, while the same server with 2 human connections would correctly show "2 teammates connected · 3 agents". This is inconsistent handling of the same underlying data depending on an unrelated branch condition (number of human connections), and it under-informs admins — who are the only audience for this row (`isAdmin && <CardPeopleRow .../>`) — about agent access on the most common case (a single connected owner).
</issue_description>
<issue_validation>
- **Checked:** `CardPeopleRow` (lines 385-416) and the `McpGatewayServer` shape in `packages/api-client/src/mcp-gateway.ts` (`connections: McpGatewayConnection[]`, `agents: McpGatewayAgentAccess[]`), plus the `AvatarStack` render at line 410.
- **Found:** The premise holds exactly. Line 400 early-returns when `connections.length === 0`; the `connections.length === 1` branch (line 404) emits `"${firstName} is connected"` with no reference to `agentCount`, while the else branch (lines 405-407) appends `· ${agentCount} agent(s)`. So agent sharing is surfaced only when 2+ humans are connected. Line 410's `AvatarStack` maps `connections` (humans) only, so in the single-connection case agents get *no* representation on the card at all — confirming this is an unintended asymmetry, not a deliberate omission (the plural branch clearly means to show it).
- **Found:** The single-connection case is the common one (a lone owner), so the gap is hit routinely, not a rare edge.
- **Impact:** Real but display-only: the label stays truthful ("Alice is connected" is correct) — it is merely less complete, omitting the agent count for the most common configuration. This is an admin-only summary row, and the same sharing data is reachable on the server-detail admin Access section, so no data is lost and no functionality breaks. The one-line fix (shared `agentSuffix` appended to both branches) is cheap and correct.
- **Priority:** Lowering to `consider` — no wrong data, crash, or functional/security/data impact; it is an informational-completeness gap on an admin summary row with the same info available elsewhere, milder than a functional `should_fix` defect.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Include the agent count in both branches, e.g.: `const agentSuffix = agentCount ? \` · ${agentCount} agent${agentCount > 1 ? "s" : ""}\` : "";` and append it to both the singular and plural label strings so agent sharing is always represented regardless of how many humans are connected.
</potential_solution>
Comment on lines
+418
to
+426
| {!isAdmin && scopeEditable && ( | ||
| <BulkTrio | ||
| label="Set all" | ||
| filtered={hasToolSearch} | ||
| disabled={tools.setAllPending || bulkEditableCount === 0} | ||
| allowNeedsApproval | ||
| onSet={setBulkPolicy} | ||
| /> | ||
| )} |
Contributor
There was a problem hiding this comment.
Top-level bulk trio duplicates the scope-aware one and wrongly allows "Needs Approval" for agent scope
Why we think it's a valid issue
- Checked: Render guards for both bulk panels (
GatewayServerDetail.tsx:418top trio!isAdmin && scopeEditable,:429scope-switcher(isAdmin || canManageAgentAccess) && scopes.length > 1),scopeEditable(:189-192),scopesconstruction (:121-142),canManageAgentAccessderivation (useGatewayConfig.ts:40-42), thesetAllmutation guard (useGatewayToolPolicies.ts:100-112), andisAgentPolicyState(gatewayServers.ts:175-178). - Found (duplication is real & reachable):
canManageAgentAccess = is_admin || allow_member_agent_access, andallow_member_agent_accessdefaults totrue, so a non-admin member iscanManageAgentAccess=trueby default. For such a member viewing a server with ≥1 granted agent,scopes.length ≥ 2(YOU + agent scopes) so the switcher block renders, AND!isAdmin && scopeEditableis simultaneously true so the topBulkTrioalso renders — two 'Set all' controls at once. In agent scope the top trio exposes the amber 'Needs Approval' button (hardcodedallowNeedsApproval,:423) while the switcher trio correctly hides it (allowNeedsApproval={!agentScope},:469). The top trio's guard never accounts for the switcher being present; the reviewer'sscopes.length <= 1fix is correct. - Found (data-harm claim is inaccurate): clicking the top trio's 'Needs Approval' in agent scope cannot push agent tools into
needs_approval.setAllMutationrejects whenscope.scopeType === "agent" && !isAgentPolicyState(state);isAgentPolicyState("needs_approval")isfalse, so the call is rejected and an error toast is shown (useGatewayToolPolicies.ts:100-104). No invalid state is persisted. - Impact: A genuine, reachable UI logic bug under default settings — duplicate/contradictory bulk controls, plus a dead-end 'Needs Approval' button in agent scope that always errors. This is a real correctness defect worth fixing, but it fails safely (guarded mutation, graceful toast) with no data corruption or security impact.
- Priority: Lowering must_fix → should_fix: the reviewer's stated severity rests on the agent tools being pushed into
needs_approval, which the mutation guard prevents; the actual harm is confusing duplicate controls and a gracefully-failing button, not an integrity breach.
Issue description
For a non-admin user with canManageAgentAccess=true who switches scope to an agent (scope.scopeType === "agent"), scopeEditable becomes true via the (scope.scopeType === "agent" && canManageAgentAccess) clause, so the plain BulkTrio at line 418 (!isAdmin && scopeEditable) renders at the same time as the scope-switcher block at line 429 ((isAdmin || canManageAgentAccess) && scopes.length > 1) — both conditions are true for exactly this role/scope combination. The top widget hardcodes allowNeedsApproval (line 423, i.e. true), always exposing the amber "Needs Approval" bulk action, while the scope-aware widget correctly sets allowNeedsApproval={!agentScope} (line 469) to hide it because agent-scoped tools have no human to approve them. The result is two bulk-action panels acting on the same agent-scoped tool list with contradictory capabilities, and the top one lets the user push an agent's tools into a needs_approval state the rest of the UI treats as invalid/unreachable for agents.
Suggested fix
Gate the top-level BulkTrio so it can't co-exist with the scope-switcher one, e.g. render it only when !isAdmin && scopeEditable && scopes.length <= 1, or apply the same allowNeedsApproval={!agentScope} guard to it.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/mcp-gateway/components/parts/GatewayServerDetail.tsx#L418-426
@packages/ui/src/features/mcp-gateway/components/parts/GatewayServerDetail.tsx#L465-471
<issue_description>
For a non-admin user with `canManageAgentAccess=true` who switches `scope` to an agent (`scope.scopeType === "agent"`), `scopeEditable` becomes true via the `(scope.scopeType === "agent" && canManageAgentAccess)` clause, so the plain `BulkTrio` at line 418 (`!isAdmin && scopeEditable`) renders at the same time as the scope-switcher block at line 429 (`(isAdmin || canManageAgentAccess) && scopes.length > 1`) — both conditions are true for exactly this role/scope combination. The top widget hardcodes `allowNeedsApproval` (line 423, i.e. `true`), always exposing the amber "Needs Approval" bulk action, while the scope-aware widget correctly sets `allowNeedsApproval={!agentScope}` (line 469) to hide it because agent-scoped tools have no human to approve them. The result is two bulk-action panels acting on the same agent-scoped tool list with contradictory capabilities, and the top one lets the user push an agent's tools into a `needs_approval` state the rest of the UI treats as invalid/unreachable for agents.
</issue_description>
<issue_validation>
- **Checked:** Render guards for both bulk panels (`GatewayServerDetail.tsx:418` top trio `!isAdmin && scopeEditable`, `:429` scope-switcher `(isAdmin || canManageAgentAccess) && scopes.length > 1`), `scopeEditable` (`:189-192`), `scopes` construction (`:121-142`), `canManageAgentAccess` derivation (`useGatewayConfig.ts:40-42`), the `setAll` mutation guard (`useGatewayToolPolicies.ts:100-112`), and `isAgentPolicyState` (`gatewayServers.ts:175-178`).
- **Found (duplication is real & reachable):** `canManageAgentAccess = is_admin || allow_member_agent_access`, and `allow_member_agent_access` defaults to `true`, so a non-admin member is `canManageAgentAccess=true` by default. For such a member viewing a server with ≥1 granted agent, `scopes.length ≥ 2` (YOU + agent scopes) so the switcher block renders, AND `!isAdmin && scopeEditable` is simultaneously true so the top `BulkTrio` also renders — two 'Set all' controls at once. In agent scope the top trio exposes the amber 'Needs Approval' button (hardcoded `allowNeedsApproval`, `:423`) while the switcher trio correctly hides it (`allowNeedsApproval={!agentScope}`, `:469`). The top trio's guard never accounts for the switcher being present; the reviewer's `scopes.length <= 1` fix is correct.
- **Found (data-harm claim is inaccurate):** clicking the top trio's 'Needs Approval' in agent scope cannot push agent tools into `needs_approval`. `setAllMutation` rejects when `scope.scopeType === "agent" && !isAgentPolicyState(state)`; `isAgentPolicyState("needs_approval")` is `false`, so the call is rejected and an error toast is shown (`useGatewayToolPolicies.ts:100-104`). No invalid state is persisted.
- **Impact:** A genuine, reachable UI logic bug under default settings — duplicate/contradictory bulk controls, plus a dead-end 'Needs Approval' button in agent scope that always errors. This is a real correctness defect worth fixing, but it fails safely (guarded mutation, graceful toast) with no data corruption or security impact.
- **Priority:** Lowering must_fix → should_fix: the reviewer's stated severity rests on the agent tools being pushed into `needs_approval`, which the mutation guard prevents; the actual harm is confusing duplicate controls and a gracefully-failing button, not an integrity breach.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Gate the top-level `BulkTrio` so it can't co-exist with the scope-switcher one, e.g. render it only when `!isAdmin && scopeEditable && scopes.length <= 1`, or apply the same `allowNeedsApproval={!agentScope}` guard to it.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
Fixed in 0fd92e1 — the top-level BulkTrio is now gated behind scopes.length <= 1 so it never renders alongside the scope switcher, and it mirrors the switcher's allowNeedsApproval={!agentScope} guard for the residual case where the initial scope is an agent that's absent from the scope list. Added a regression test covering the member + agent-scope case.
Comment on lines
+524
to
+542
| {tools.policiesLoading && tools.policies.length === 0 ? ( | ||
| <Flex align="center" justify="center" py="6"> | ||
| <Spinner size="2" /> | ||
| </Flex> | ||
| ) : tools.policies.length === 0 ? ( | ||
| <Flex | ||
| direction="column" | ||
| align="center" | ||
| gap="1" | ||
| py="6" | ||
| className="rounded border border-gray-6 border-dashed" | ||
| > | ||
| <Text className="font-medium text-sm">No tools discovered yet.</Text> | ||
| <Text color="gray" className="text-[13px]"> | ||
| {refreshInstallationId | ||
| ? "This server listed no tools. Refresh to try again." | ||
| : "Connect your account to list this server's tools."} | ||
| </Text> | ||
| </Flex> |
Contributor
There was a problem hiding this comment.
Tool-catalog fetch failures render identically to "no tools", with no retry path for regular members
Why we think it's a valid issue
- Checked:
useGatewayToolPoliciesreturn shape (useGatewayToolPolicies.ts:185-195), the query wrapper's retry config (useAuthenticatedQuery.ts:28-42), the empty/loading branch (GatewayServerDetail.tsx:524-542), the auto-discovery guard (useGatewayToolPolicies.ts:158,162-183), and the gateway-scene refetch triggers (McpGatewayView.tsx:42-55,:78key). - Found: The hook never surfaces
isError/error; on a settled fetch failure react-query'sdataisundefined, sopolicies ?? []yields[]withpoliciesLoadingfalse, dropping into the 'No tools discovered yet' state — indistinguishable from a genuinely empty catalog. Auto-discovery can't retry it because itscatalogResolved = policies !== undefinedguard is false on error. For a plain member (scopes.length === 1) the refresh toolbar (:429-490) doesn't render, so there's no explicit retry button. - Found (mitigations):
useAuthenticatedQueryapplies react-query's default 3-retry policy (no override atuseAuthenticatedQuery.ts:28-42), so transient errors self-recover; andMcpGatewayView.tsx:42-55invalidates["mcp"]on window focus/visibility while thekey={serverId}remount refetches — implicit recovery paths that mean the page is not permanently stuck. - Impact: Real error-handling gap — a persistent tool-policy fetch error is swallowed and shown as 'no tools', misleading a member into thinking a server has none. Legitimate to keep (swallowed-error reliability), but bounded.
- Priority: Lowering should_fix → consider. react-query already retries transient failures and focus/remount trigger automatic refetch, so the 'silently stuck, no retry' framing overstates it; the genuine gap is a nicer explicit error-vs-empty state, a minor UX/reliability polish rather than a functional defect.
Issue description
useGatewayToolPolicies only exposes policies/policiesLoading, never surfacing isError/error from the underlying query. If getMcpGatewayToolPolicies fails, policies settles to [] and policiesLoading becomes false, so this block falls straight into the "No tools discovered yet" empty state — indistinguishable from a server that genuinely has zero tools. For a plain member (not admin/agent-manager), scopes.length is 1, so the refresh-button toolbar at lines 429-484 never renders either, leaving no retry affordance anywhere in the UI for a page silently stuck on a swallowed fetch error.
Suggested fix
Thread isError/error out of useGatewayToolPolicies and branch on it distinctly from the empty-catalog case (e.g. an explicit error state with a retry action), instead of only distinguishing loading vs. empty.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/mcp-gateway/components/parts/GatewayServerDetail.tsx#L524-542
<issue_description>
`useGatewayToolPolicies` only exposes `policies`/`policiesLoading`, never surfacing `isError`/`error` from the underlying query. If `getMcpGatewayToolPolicies` fails, `policies` settles to `[]` and `policiesLoading` becomes `false`, so this block falls straight into the "No tools discovered yet" empty state — indistinguishable from a server that genuinely has zero tools. For a plain member (not admin/agent-manager), `scopes.length` is 1, so the refresh-button toolbar at lines 429-484 never renders either, leaving no retry affordance anywhere in the UI for a page silently stuck on a swallowed fetch error.
</issue_description>
<issue_validation>
- **Checked:** `useGatewayToolPolicies` return shape (`useGatewayToolPolicies.ts:185-195`), the query wrapper's retry config (`useAuthenticatedQuery.ts:28-42`), the empty/loading branch (`GatewayServerDetail.tsx:524-542`), the auto-discovery guard (`useGatewayToolPolicies.ts:158,162-183`), and the gateway-scene refetch triggers (`McpGatewayView.tsx:42-55`, `:78` key).
- **Found:** The hook never surfaces `isError`/`error`; on a settled fetch failure react-query's `data` is `undefined`, so `policies ?? []` yields `[]` with `policiesLoading` false, dropping into the 'No tools discovered yet' state — indistinguishable from a genuinely empty catalog. Auto-discovery can't retry it because its `catalogResolved = policies !== undefined` guard is false on error. For a plain member (`scopes.length === 1`) the refresh toolbar (`:429-490`) doesn't render, so there's no explicit retry button.
- **Found (mitigations):** `useAuthenticatedQuery` applies react-query's default 3-retry policy (no override at `useAuthenticatedQuery.ts:28-42`), so transient errors self-recover; and `McpGatewayView.tsx:42-55` invalidates `["mcp"]` on window focus/visibility while the `key={serverId}` remount refetches — implicit recovery paths that mean the page is not permanently stuck.
- **Impact:** Real error-handling gap — a persistent tool-policy fetch error is swallowed and shown as 'no tools', misleading a member into thinking a server has none. Legitimate to keep (swallowed-error reliability), but bounded.
- **Priority:** Lowering should_fix → consider. react-query already retries transient failures and focus/remount trigger automatic refetch, so the 'silently stuck, no retry' framing overstates it; the genuine gap is a nicer explicit error-vs-empty state, a minor UX/reliability polish rather than a functional defect.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Thread `isError`/`error` out of `useGatewayToolPolicies` and branch on it distinctly from the empty-catalog case (e.g. an explicit error state with a retry action), instead of only distinguishing loading vs. empty.
</potential_solution>
Comment on lines
+134
to
+139
| onSuccess: () => { | ||
| silentRefreshRef.current = false; | ||
| queryClient.invalidateQueries({ | ||
| queryKey: gatewayKeys.serverTools(serverId), | ||
| }); | ||
| queryClient.invalidateQueries({ queryKey: gatewayKeys.servers }); |
Contributor
There was a problem hiding this comment.
Manual refresh gives no success confirmation, unlike the sibling mcp-servers hook
Why we think it's a valid issue
- Checked: the gateway
refreshMutation.onSuccess(useGatewayToolPolicies.ts:134-139) vs the siblinguseMcpInstallationTools.ts:131-137, plus the manual-refresh consumerGatewayServerDetail.tsx:472-481. - Found — premise accurate: the sibling captures
silentand doesif (!silent) toast.success("Tools refreshed")before clearing the ref and invalidating; the gatewayonSuccessonly setssilentRefreshRef.current = falseand invalidatesserverTools/servers— no toast on any path. The hook already tracks the silent-vs-manual distinction (itsonErrorreadssilentRefreshRef), so omitting it on success reads as an inconsistency rather than a deliberate choice. - Found — reachable:
GatewayServerDetail.tsx:479wires a user-facing refresh button totools.refresh(...), so a manual success genuinely produces no confirmation, and when the re-list returns an unchanged catalog there is no visible list change either — feedback is truly absent in that case. - Impact: A missing success confirmation on a manual user action — a real, reproducible, but minor UX-feedback gap. The 'redundant round-trips' harm is largely neutralized (
disabled={tools.refreshPending}at:478blocks re-clicks while in flight, and a successful re-list updates the list), so it is not a performance concern. It is not noise (not speculative/defensive/taste-with-no-behavioral-difference — the behavior does differ), so it belongs on record at the lowest priority. The reviewer'sconsideris correctly calibrated.
Issue description
The analogous useMcpInstallationTools.ts refresh mutation shows toast.success("Tools refreshed") when a non-silent (manual) refresh succeeds. Here, onSuccess only resets the ref and invalidates queries — no toast is ever shown, whether the completed call was the silent auto-discovery path or a user-initiated manual refresh. A user clicking a "refresh tools" affordance gets no confirmation the action worked, which invites redundant re-clicks, each firing another live round-trip to refreshMcpInstallationTools against the upstream MCP server for no benefit.
Suggested fix
Mirror the sibling hook: check whether the completing call was silent before resetting the ref, and show a success toast for the non-silent path, e.g. if (!silentRefreshRef.current) toast.success("Tools refreshed") before clearing it.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/mcp-gateway/hooks/useGatewayToolPolicies.ts#L134-139
<issue_description>
The analogous `useMcpInstallationTools.ts` refresh mutation shows `toast.success("Tools refreshed")` when a non-silent (manual) refresh succeeds. Here, `onSuccess` only resets the ref and invalidates queries — no toast is ever shown, whether the completed call was the silent auto-discovery path or a user-initiated manual refresh. A user clicking a "refresh tools" affordance gets no confirmation the action worked, which invites redundant re-clicks, each firing another live round-trip to `refreshMcpInstallationTools` against the upstream MCP server for no benefit.
</issue_description>
<issue_validation>
- **Checked:** the gateway `refreshMutation.onSuccess` (`useGatewayToolPolicies.ts:134-139`) vs the sibling `useMcpInstallationTools.ts:131-137`, plus the manual-refresh consumer `GatewayServerDetail.tsx:472-481`.
- **Found — premise accurate:** the sibling captures `silent` and does `if (!silent) toast.success("Tools refreshed")` before clearing the ref and invalidating; the gateway `onSuccess` only sets `silentRefreshRef.current = false` and invalidates `serverTools`/`servers` — no toast on any path. The hook already tracks the silent-vs-manual distinction (its `onError` reads `silentRefreshRef`), so omitting it on success reads as an inconsistency rather than a deliberate choice.
- **Found — reachable:** `GatewayServerDetail.tsx:479` wires a user-facing refresh button to `tools.refresh(...)`, so a manual success genuinely produces no confirmation, and when the re-list returns an unchanged catalog there is no visible list change either — feedback is truly absent in that case.
- **Impact:** A missing success confirmation on a manual user action — a real, reproducible, but minor UX-feedback gap. The 'redundant round-trips' harm is largely neutralized (`disabled={tools.refreshPending}` at `:478` blocks re-clicks while in flight, and a successful re-list updates the list), so it is not a performance concern. It is not noise (not speculative/defensive/taste-with-no-behavioral-difference — the behavior does differ), so it belongs on record at the lowest priority. The reviewer's `consider` is correctly calibrated.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Mirror the sibling hook: check whether the completing call was silent before resetting the ref, and show a success toast for the non-silent path, e.g. `if (!silentRefreshRef.current) toast.success("Tools refreshed")` before clearing it.
</potential_solution>
Comment on lines
+15
to
+38
| const { data: pageData, isLoading } = useAuthenticatedQuery( | ||
| gatewayKeys.audit(options), | ||
| (client) => | ||
| client.getMcpGatewayAuditEvents({ | ||
| quickFilter: options.quickFilter, | ||
| actorServiceAccountId: options.actorServiceAccountId, | ||
| limit: AUDIT_PAGE_SIZE, | ||
| offset: options.page * AUDIT_PAGE_SIZE, | ||
| }), | ||
| { enabled: options.enabled ?? true, placeholderData: keepPreviousData }, | ||
| ); | ||
|
|
||
| const { data: counts } = useAuthenticatedQuery( | ||
| gatewayKeys.auditCounts, | ||
| (client) => client.getMcpGatewayAuditCounts(), | ||
| { enabled: options.enabled ?? true }, | ||
| ); | ||
|
|
||
| return { | ||
| events: pageData?.results ?? [], | ||
| totalCount: pageData?.count ?? 0, | ||
| auditLoading: isLoading, | ||
| counts: counts ?? null, | ||
| }; |
Contributor
There was a problem hiding this comment.
Query errors are swallowed and rendered as a false empty audit log
Why we think it's a valid issue
- Checked: Both hooks in
useGatewayAudit.ts—useGatewayAuditdestructures only{ data, isLoading }for the events query (:15) and{ data: counts }for counts (:27);useAgentRecentCallsdestructures only{ data, isLoading }(:43).useAuthenticatedQueryreturns the fullUseQueryResult(useAuthenticatedQuery.ts:24), soisError/error/refetchare available but dropped. The table branch inGatewayAuditLog.tsx:178-189isauditLoading → events.length===0 → rows, with no error branch. - Found (error truly swallowed): There is no global mitigation. The renderer
QueryClient(apps/code/src/renderer/utils/queryClient.ts:4-11) sets onlystaleTime/refetchOnWindowFocus— noQueryCache.onError, no default error toast, nothrowOnError. TheauthScopedmeta is consumed solely by an auth-invalidation predicate (authQueries.ts:46), not an error notifier. So on a failed fetch,pageDatais undefined →events = [],auditLoadingsettles false → the table renders the exact"No tool calls match these filters."empty state (:187) with no toast anywhere. - Impact: This is the "swallowed errors that hide failures / missing handling for a failure mode that will occur" case the criteria keep. The failure mode is certain over time (network/500/outage — and the PR notes the
mcp_gatewaybackend ships in an in-flight, flag-gated PR), and the consequence is concrete: a security/compliance audit trail with a dedicated "Blocked" policy-violation filter presents a failed fetch as "nothing happened," so a reviewer can wrongly conclude no blocked calls occurred. Not speculation, paranoia, or style. - Note (one supporting claim is overstated but doesn't defeat the finding): The cited cross-feature pattern is real —
AgentApplicationsListView.tsx:149-161doesisLoading → isError(error.message) → empty. But within the mcp-gateway feature the omission is consistent:useGatewayServers,useServiceAccounts,useGatewayConfig,useGatewayMembers,useGatewayToolPoliciesall expose onlyisLoadingfor their read queries (theirerror/onErrorusages are mutation toasts). So it's a feature-wide posture, not an audit-specific regression — yet nothing prevents the false-empty (no "already handled" escape applies), and the audit log is the view where a silent false-empty is most consequential, so should_fix stands.
Issue description
Both useGatewayAudit and useAgentRecentCalls destructure only isLoading from useAuthenticatedQuery and never expose isError/error, so the consumer has no way to know a request failed. In GatewayAuditLog.tsx (lines ~178-189) the table only branches on auditLoading and events.length === 0: if getMcpGatewayAuditEvents/getMcpGatewayAuditCounts fail (network error, 500, an outage on the new mcp_gateway endpoints while its backend PR is still in flight), the screen renders exactly the same as a genuinely empty result — 'No tool calls match these filters.' — with no error message and no retry affordance. For a security/compliance audit trail (it has a dedicated 'Blocked' quick filter for policy violations), silently presenting a failed fetch as 'nothing happened' is a real reliability gap. This also deviates from the established pattern elsewhere in this codebase, e.g. AgentApplicationsListView.tsx, which explicitly branches isLoading -> isError (shows the error message) -> empty state.
Suggested fix
Return isError/error (or refetch) from both hooks and add an error branch in GatewayAuditLog.tsx before the empty-state branch, mirroring the isLoading ? ... : isError ? ... : ... pattern already used in AgentApplicationsListView.tsx, so a failed fetch surfaces an actionable error instead of a misleading empty table.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/mcp-gateway/hooks/useGatewayAudit.ts#L15-38
@packages/ui/src/features/mcp-gateway/hooks/useGatewayAudit.ts#L42-55
<issue_description>
Both `useGatewayAudit` and `useAgentRecentCalls` destructure only `isLoading` from `useAuthenticatedQuery` and never expose `isError`/`error`, so the consumer has no way to know a request failed. In `GatewayAuditLog.tsx` (lines ~178-189) the table only branches on `auditLoading` and `events.length === 0`: if `getMcpGatewayAuditEvents`/`getMcpGatewayAuditCounts` fail (network error, 500, an outage on the new `mcp_gateway` endpoints while its backend PR is still in flight), the screen renders exactly the same as a genuinely empty result — 'No tool calls match these filters.' — with no error message and no retry affordance. For a security/compliance audit trail (it has a dedicated 'Blocked' quick filter for policy violations), silently presenting a failed fetch as 'nothing happened' is a real reliability gap. This also deviates from the established pattern elsewhere in this codebase, e.g. `AgentApplicationsListView.tsx`, which explicitly branches `isLoading -> isError (shows the error message) -> empty state`.
</issue_description>
<issue_validation>
- **Checked:** Both hooks in `useGatewayAudit.ts` — `useGatewayAudit` destructures only `{ data, isLoading }` for the events query (`:15`) and `{ data: counts }` for counts (`:27`); `useAgentRecentCalls` destructures only `{ data, isLoading }` (`:43`). `useAuthenticatedQuery` returns the full `UseQueryResult` (`useAuthenticatedQuery.ts:24`), so `isError`/`error`/`refetch` are available but dropped. The table branch in `GatewayAuditLog.tsx:178-189` is `auditLoading → events.length===0 → rows`, with no error branch.
- **Found (error truly swallowed):** There is no global mitigation. The renderer `QueryClient` (`apps/code/src/renderer/utils/queryClient.ts:4-11`) sets only `staleTime`/`refetchOnWindowFocus` — no `QueryCache.onError`, no default error toast, no `throwOnError`. The `authScoped` meta is consumed solely by an auth-invalidation predicate (`authQueries.ts:46`), not an error notifier. So on a failed fetch, `pageData` is undefined → `events = []`, `auditLoading` settles false → the table renders the exact `"No tool calls match these filters."` empty state (`:187`) with no toast anywhere.
- **Impact:** This is the "swallowed errors that hide failures / missing handling for a failure mode that will occur" case the criteria keep. The failure mode is certain over time (network/500/outage — and the PR notes the `mcp_gateway` backend ships in an in-flight, flag-gated PR), and the consequence is concrete: a security/compliance audit trail with a dedicated "Blocked" policy-violation filter presents a failed fetch as "nothing happened," so a reviewer can wrongly conclude no blocked calls occurred. Not speculation, paranoia, or style.
- **Note (one supporting claim is overstated but doesn't defeat the finding):** The cited cross-feature pattern is real — `AgentApplicationsListView.tsx:149-161` does `isLoading → isError(error.message) → empty`. But within the mcp-gateway feature the omission is *consistent*: `useGatewayServers`, `useServiceAccounts`, `useGatewayConfig`, `useGatewayMembers`, `useGatewayToolPolicies` all expose only `isLoading` for their read queries (their `error`/`onError` usages are mutation toasts). So it's a feature-wide posture, not an audit-specific regression — yet nothing *prevents* the false-empty (no "already handled" escape applies), and the audit log is the view where a silent false-empty is most consequential, so should_fix stands.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Return `isError`/`error` (or `refetch`) from both hooks and add an error branch in `GatewayAuditLog.tsx` before the empty-state branch, mirroring the `isLoading ? ... : isError ? ... : ...` pattern already used in `AgentApplicationsListView.tsx`, so a failed fetch surfaces an actionable error instead of a misleading empty table.
</potential_solution>
Comment on lines
+24
to
+28
| const { data: members, isLoading } = useAuthenticatedQuery( | ||
| gatewayKeys.members, | ||
| (client) => client.getMcpGatewayMembers(), | ||
| { enabled: options.enabled }, | ||
| ); |
Contributor
There was a problem hiding this comment.
getMcpGatewayMembers() hard-caps at 500 rows with no pagination path, so members past the cutoff silently vanish and read as 'Member not found'
Why we think it's a valid issue
- Checked: the full fetch chain for the limit/pagination, both claimed consequences, and how the member-detail route is actually reached.
- Found:
getMcpGatewayMembers()hard-codessearch: { limit: 500 }with no offset/cursor param (posthog-client.ts:4749-4759), anduseGatewayMemberscalls it with no args (lines 24-28) — so >500-member orgs cannot page the remainder. Accurate. - Found: Consequence (2) (a real member rendering "Member not found.") is practically unreachable. The gateway route is in-page
useState<GatewayRoute>initialized to{ view: "servers" }(McpGatewayView.tsx:30) with no URL/deep-link/arbitrary-id entry; the sole navigation toview: "member"is clicking a roster row (GatewayTeamView.tsx:134), and that roster is drawn from the same truncated array — you can only open members that are displayed. So the scarier consequence the finding leads with is moot. - Impact: The reachable effect is consequence (1): silent roster/search truncation with no "showing N of total" affordance, but only for orgs exceeding a deliberate, generous 500-member cap — a minority audience on an early flag-gated feature where a 500 v1 bound with pagination as a follow-up is defensible. Real data-completeness gap worth recording, not a broad-impact defect.
- Priority: should_fix → consider. Narrow audience (>500 members), the misleading-not-found path is unreachable via the in-page-only routing, and a lightweight mitigation (truncation affordance / raised cap) is acknowledged — so it is a real-but-minor limitation to keep on record, not merge-blocking.
Issue description
client.getMcpGatewayMembers() (packages/api-client/src/posthog-client.ts) hard-codes search: { limit: 500 } on the mcp_gateway/members/ request and the client method takes zero parameters — there is no offset/page argument anywhere in the call chain from this hook down to the API client, so a team with more than 500 members can never have the remainder fetched by this UI. This is a different failure mode than an 'unbounded payload' performance concern: it's a hard, silent, un-recoverable truncation. Two concrete consequences follow directly from files in this chunk: (1) GatewayTeamView.tsx's roster and search silently omit every member past row 500 with no indication to the admin that the list is incomplete, and (2) GatewayMemberDetail.tsx (lines 34-51) does members.find((entry) => entry.user.id === userId) against this same truncated array — for a real member beyond the cutoff this returns undefined, and since membersLoading is already false by then, the component renders the literal text 'Member not found.' for a member who does in fact exist, which is actively misleading to an admin trying to manage that person's access.
Suggested fix
Thread a page/offset (or cursor) parameter from useGatewayMembers down through getMcpGatewayMembers() to the backend request, and either paginate the roster fetch or raise the limit with an explicit 'showing first N of TOTAL' affordance so admins know when the list is incomplete instead of silently missing data.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/mcp-gateway/hooks/useGatewayMembers.ts#L24-28
<issue_description>
`client.getMcpGatewayMembers()` (packages/api-client/src/posthog-client.ts) hard-codes `search: { limit: 500 }` on the `mcp_gateway/members/` request and the client method takes zero parameters — there is no offset/page argument anywhere in the call chain from this hook down to the API client, so a team with more than 500 members can never have the remainder fetched by this UI. This is a different failure mode than an 'unbounded payload' performance concern: it's a hard, silent, un-recoverable truncation. Two concrete consequences follow directly from files in this chunk: (1) `GatewayTeamView.tsx`'s roster and search silently omit every member past row 500 with no indication to the admin that the list is incomplete, and (2) `GatewayMemberDetail.tsx` (lines 34-51) does `members.find((entry) => entry.user.id === userId)` against this same truncated array — for a real member beyond the cutoff this returns `undefined`, and since `membersLoading` is already `false` by then, the component renders the literal text 'Member not found.' for a member who does in fact exist, which is actively misleading to an admin trying to manage that person's access.
</issue_description>
<issue_validation>
- **Checked:** the full fetch chain for the limit/pagination, both claimed consequences, and how the member-detail route is actually reached.
- **Found:** `getMcpGatewayMembers()` hard-codes `search: { limit: 500 }` with no offset/cursor param (`posthog-client.ts:4749-4759`), and `useGatewayMembers` calls it with no args (lines 24-28) — so >500-member orgs cannot page the remainder. Accurate.
- **Found:** Consequence (2) (a real member rendering "Member not found.") is practically unreachable. The gateway route is in-page `useState<GatewayRoute>` initialized to `{ view: "servers" }` (`McpGatewayView.tsx:30`) with no URL/deep-link/arbitrary-id entry; the sole navigation to `view: "member"` is clicking a roster row (`GatewayTeamView.tsx:134`), and that roster is drawn from the same truncated array — you can only open members that are displayed. So the scarier consequence the finding leads with is moot.
- **Impact:** The reachable effect is consequence (1): silent roster/search truncation with no "showing N of total" affordance, but only for orgs exceeding a deliberate, generous 500-member cap — a minority audience on an early flag-gated feature where a 500 v1 bound with pagination as a follow-up is defensible. Real data-completeness gap worth recording, not a broad-impact defect.
- **Priority:** should_fix → consider. Narrow audience (>500 members), the misleading-not-found path is unreachable via the in-page-only routing, and a lightweight mitigation (truncation affordance / raised cap) is acknowledged — so it is a real-but-minor limitation to keep on record, not merge-blocking.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Thread a page/offset (or cursor) parameter from `useGatewayMembers` down through `getMcpGatewayMembers()` to the backend request, and either paginate the roster fetch or raise the limit with an explicit 'showing first N of TOTAL' affordance so admins know when the list is incomplete instead of silently missing data.
</potential_solution>
Comment on lines
+42
to
+55
| export function useAgentRecentCalls(accountId: string) { | ||
| const { data, isLoading } = useAuthenticatedQuery( | ||
| gatewayKeys.audit({ | ||
| quickFilter: "all", | ||
| actorServiceAccountId: accountId, | ||
| }), | ||
| (client) => | ||
| client.getMcpGatewayAuditEvents({ | ||
| actorServiceAccountId: accountId, | ||
| limit: 50, | ||
| }), | ||
| ); | ||
| return { events: data?.results ?? [], eventsLoading: isLoading }; | ||
| } |
Contributor
There was a problem hiding this comment.
useAgentRecentCalls shares a cache key with the audit log's paginated "agent + all" query despite requesting different data
Why we think it's a valid issue
- Checked: Key construction in
gatewayKeys.audit(gatewayKeys.ts:43-55) for both call sites; the two queryFns (useAgentRecentCallsuseGatewayAudit.ts:42-55 vsuseGatewayAuditlines 15-25); GatewayAuditLog's default filter/page state (GatewayAuditLog.tsx:49-58); and QueryClient staleTime/refetch behavior (apps/code/src/renderer/utils/queryClient.ts). - Found: Keys collide exactly —
useAgentRecentCalls(accountId)omitspageso it resolves to["mcp","gateway","audit","all",accountId,0], identical to the audit log at defaultquickFilter:"all"+ Caller=accountId + page 0. But the requests differ:{actorServiceAccountId, limit:50}(no offset/quickFilter) vs{quickFilter:"all", actorServiceAccountId, limit:10, offset:0}. WithstaleTime: 5minandrefetchOnMountonly firing when stale, navigating between the two routes within 5 min serves whichever populated the entry first with NO refetch. - Impact: Confirmed wrong-data display: agent-detail 'Recent tool calls' capped at 10 (missing up to 40 recent calls) if the audit-log page-0 fetch landed first; or the audit log rendering up to 50 rows in a 10-row page with a false 'Showing 1–10 of N' if the agent-detail fetch landed first. Reachable via a plausible admin workflow (only the Caller selection is non-default), self-healing only after 5-min staleness or a focus
["mcp"]invalidation — not during active navigation. A genuine queryKey/queryFn mismatch (a key must uniquely describe its request), distinct from the separate 50-event pagination-cap finding. Real correctness defect, simple correct fix (distinct key); bounded/self-healing so not must_fix. Keep at should_fix.
Issue description
GatewayAgentDetail.tsx:48 calls useAgentRecentCalls(accountId), which builds its query key via gatewayKeys.audit({ quickFilter: "all", actorServiceAccountId: accountId }) — since page is omitted, gatewayKeys.audit (hooks/gatewayKeys.ts:43-55) resolves it to ["mcp","gateway","audit","all",accountId,0]. That is the exact same TanStack Query key GatewayAuditLog.tsx produces when a user leaves the quick filter on "All activity" (its default state) and picks that same agent from the "Caller" dropdown at page 0 (GatewayAuditLog.tsx:49-58). React Query identifies cache entries purely by queryKey, but the two call sites invoke materially different actual requests against getMcpGatewayAuditEvents: useAgentRecentCalls sends { actorServiceAccountId, limit: 50 } (no quickFilter, no offset), while GatewayAuditLog sends { quickFilter: "all", actorServiceAccountId, limit: AUDIT_PAGE_SIZE (10), offset: 0 }. Whichever of the two mounts first populates the shared cache entry, so the other renders the wrong shape: if the audit log's 10-row page-0 fetch lands first, the agent detail page's "Recent tool calls" table (and its local "Show more" pager, which slices client-side up to events.length) is silently capped at 10 events instead of up to 50; if the agent detail page's 50-event fetch lands first, the audit log's "Showing 1–10 of N" pagination text and page-window renders up to 50 rows crammed into what is supposed to be a 10-row page. This is a genuine queryKey/queryFn mismatch — a query key must uniquely describe the request it represents — not the already-reported 50-event pagination cap (which is about useAgentRecentCalls itself never paging past 50; this is about it colliding with an unrelated screen's cache entry).
Suggested fix
Give useAgentRecentCalls its own distinct query key (e.g. add a fixed discriminator or the actual limit into the key, or a separate gatewayKeys.agentRecentCalls(accountId) entry) instead of reusing gatewayKeys.audit(...) with a quickFilter/page pairing that only coincidentally matches a real audit-log query. Alternatively, make the two call sites request identical params for identical keys (pass the true limit/offset into the key builder) so any key collision reflects an actual identical request.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/mcp-gateway/hooks/useGatewayAudit.ts#L42-55
<issue_description>
GatewayAgentDetail.tsx:48 calls `useAgentRecentCalls(accountId)`, which builds its query key via `gatewayKeys.audit({ quickFilter: "all", actorServiceAccountId: accountId })` — since `page` is omitted, `gatewayKeys.audit` (hooks/gatewayKeys.ts:43-55) resolves it to `["mcp","gateway","audit","all",accountId,0]`. That is the exact same TanStack Query key `GatewayAuditLog.tsx` produces when a user leaves the quick filter on "All activity" (its default state) and picks that same agent from the "Caller" dropdown at page 0 (GatewayAuditLog.tsx:49-58). React Query identifies cache entries purely by queryKey, but the two call sites invoke materially different actual requests against `getMcpGatewayAuditEvents`: `useAgentRecentCalls` sends `{ actorServiceAccountId, limit: 50 }` (no `quickFilter`, no `offset`), while `GatewayAuditLog` sends `{ quickFilter: "all", actorServiceAccountId, limit: AUDIT_PAGE_SIZE (10), offset: 0 }`. Whichever of the two mounts first populates the shared cache entry, so the other renders the wrong shape: if the audit log's 10-row page-0 fetch lands first, the agent detail page's "Recent tool calls" table (and its local "Show more" pager, which slices client-side up to `events.length`) is silently capped at 10 events instead of up to 50; if the agent detail page's 50-event fetch lands first, the audit log's "Showing 1–10 of N" pagination text and page-window renders up to 50 rows crammed into what is supposed to be a 10-row page. This is a genuine queryKey/queryFn mismatch — a query key must uniquely describe the request it represents — not the already-reported 50-event pagination cap (which is about `useAgentRecentCalls` itself never paging past 50; this is about it colliding with an unrelated screen's cache entry).
</issue_description>
<issue_validation>
- **Checked:** Key construction in `gatewayKeys.audit` (gatewayKeys.ts:43-55) for both call sites; the two queryFns (`useAgentRecentCalls` useGatewayAudit.ts:42-55 vs `useGatewayAudit` lines 15-25); GatewayAuditLog's default filter/page state (GatewayAuditLog.tsx:49-58); and QueryClient staleTime/refetch behavior (apps/code/src/renderer/utils/queryClient.ts).
- **Found:** Keys collide exactly — `useAgentRecentCalls(accountId)` omits `page` so it resolves to `["mcp","gateway","audit","all",accountId,0]`, identical to the audit log at default `quickFilter:"all"` + Caller=accountId + page 0. But the requests differ: `{actorServiceAccountId, limit:50}` (no offset/quickFilter) vs `{quickFilter:"all", actorServiceAccountId, limit:10, offset:0}`. With `staleTime: 5min` and `refetchOnMount` only firing when stale, navigating between the two routes within 5 min serves whichever populated the entry first with NO refetch.
- **Impact:** Confirmed wrong-data display: agent-detail 'Recent tool calls' capped at 10 (missing up to 40 recent calls) if the audit-log page-0 fetch landed first; or the audit log rendering up to 50 rows in a 10-row page with a false 'Showing 1–10 of N' if the agent-detail fetch landed first. Reachable via a plausible admin workflow (only the Caller selection is non-default), self-healing only after 5-min staleness or a focus `["mcp"]` invalidation — not during active navigation. A genuine queryKey/queryFn mismatch (a key must uniquely describe its request), distinct from the separate 50-event pagination-cap finding. Real correctness defect, simple correct fix (distinct key); bounded/self-healing so not must_fix. Keep at should_fix.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Give `useAgentRecentCalls` its own distinct query key (e.g. add a fixed discriminator or the actual `limit` into the key, or a separate `gatewayKeys.agentRecentCalls(accountId)` entry) instead of reusing `gatewayKeys.audit(...)` with a `quickFilter`/`page` pairing that only coincidentally matches a real audit-log query. Alternatively, make the two call sites request identical params for identical keys (pass the true `limit`/`offset` into the key builder) so any key collision reflects an actual identical request.
</potential_solution>
The audit row's avatar and label already branch three ways (agent, user, actor_label fallback for deleted actors), but the caller badge only branched two ways, so calls made by a since-deleted agent were badged "human". Badge the fallback state as "deleted" instead, since the null foreign keys mean the actor record is gone and its type can't be claimed. Generated-By: PostHog Code Task-Id: 950dadd8-b4aa-414e-9dbe-d1b6d621768a
The troubleshooting prose said a failed flag fetch makes isFeatureEnabled return `false`, contradicting the console snippet below it. posthog-js returns `undefined` when flags have never loaded; `false` only appears after flags load and the flag is absent. Generated-By: PostHog Code Task-Id: 8f10c212-547a-4dbe-943d-73397aa8b9ee
… between grants The dialog stays mounted while `open` toggles, so `selectedId` and `policyMap` survived a close (including the parent closing it after a successful grant) and switching the agent picker kept the previous agent's tool-policy overrides. The next grant was then seeded with the prior agent's policies instead of the per-tool defaults. Clear both on close and clear the policy draft whenever a different agent is selected. Generated-By: PostHog Code Task-Id: 88f6ba96-c074-49a5-b4cd-b961695dcb44
…cted The rail derived its dot and sub-label from the raw connection status alone, so a server the caller self-disabled, an admin turned off team-wide, or revoked the caller's access from still showed a green "Connected" dot — contradicting the detail and home screens. Fold those switches into a new getGatewayRailStatus and render gray "Off for the team" / "Access revoked" / "Disabled for you" states instead. Generated-By: PostHog Code Task-Id: 4dc7d546-7985-41d3-8a1b-bfff7920f7cf
…rs in team settings Server rows in the team-settings access list rendered ServerIcon from the URL-derived domain only, so a materialized catalog server whose curated icon_domain differs from its URL host showed the wrong icon on this screen while showing the brand icon everywhere else. Look up the originating template via templatesById, matching the other gateway components. Generated-By: PostHog Code Task-Id: d7f1a081-afed-4582-9826-1ca37e6354f3
…ites The bulk-approval filter short-circuited on teamScope, so "set all" on a shared installation sent PATCHes for rule-locked tools too — rule locks are meant to override every scope. Make the lock exclusion unconditional and keep only the ceiling check scope-conditional, matching useGatewayToolPolicies' setAllMutation, and cover the filter with hook tests. Generated-By: PostHog Code Task-Id: 50a9979d-3ad5-4663-826c-58d0fd0cc6fa
…n user getGatewayServerRemovalAction looked the caller up in server.connections, but that roster is admin-only (empty for members), so a member who registered a custom server never saw "delete for you" — and any member got it on a creator-less custom server via undefined === undefined. Pass the session user's id in (from useCurrentUser) and require a non-null creator; cover the real member-facing shape (connections: []) in the core tests plus a UI regression test for the member delete affordance. Generated-By: PostHog Code Task-Id: 2880070b-9691-450f-b566-b76d60089914
…t scope
A non-admin member with agent access viewing an agent scope rendered
two "Set all" panels at once — the top-level BulkTrio and the
scope-switcher's — and the top one exposed a "Needs Approval" bulk
action that is invalid for agent-scoped tools (the setAll mutation
rejects it). Gate the top trio behind scopes.length <= 1 so it never
co-exists with the switcher, and mirror the switcher's
allowNeedsApproval={!agentScope} guard for the residual case where the
initial scope is an agent absent from the scope list.
Fixes review finding on #3643 (discussion_r3691348600).
Generated-By: PostHog Code
Task-Id: 5266fbe7-3dbc-44c0-8299-6ab9564f1c99
…d of assuming OAuth Connecting always submitted an OAuth install, so members could never establish a correct connection to an api_key custom server (and api-key catalog templates never got the member's key). Mirror the backend: surface template_auth_type on the gateway row, connect plain OAuth templates directly, and collect credentials first for everything else — a dialog where custom-server members pick OAuth or API key (with optional client id/secret) and api-key servers take the member's key. Generated-By: PostHog Code Task-Id: a6fbcf9c-dab8-45d9-a9c5-c4a645be7c1a
… access patch setAccessMutation's no-refetch cache patch filled granted_by from the stale cache entry (null for a fresh grant), so the "shared by" attribution on the server-detail access row stayed blank until an unrelated refetch. The backend assigns granted_by to the requesting user on every enable, so mirror that by projecting the session user (useCurrentUser) into the patched agents row, falling back to the previous behavior only while the user is still loading. Generated-By: PostHog Code Task-Id: ebf7c1be-72ee-497b-8ed2-225c71d59b32
The only "Refresh tools from server" button lived inside the scope-switcher bar, which is gated to admins/agent-managers with multiple scopes. A plain connected member never saw it, yet the empty-state copy told them to "Refresh to try again." Render the refresh button in the always-visible Tools header row whenever the scope bar is hidden, so any connected member can manually retry tool discovery against their own credential. Generated-By: PostHog Code Task-Id: 4df9bfd3-de17-470b-a26d-7214a2e2af17
Contributor
Author
|
/trunk merge |
| api_key: apiKey, | ||
| }); | ||
| } | ||
| return installCustomWithOAuth(client, oauth, { |
There was a problem hiding this comment.
Medium: OAuth redirect can launch arbitrary protocols
A teammate who can register a custom OAuth MCP server can make its authorization flow return a non-web URL. This helper passes that response through openAndWaitForCallback to ElectronUrlLauncher, which calls shell.openExternal without scheme validation, so clicking Connect can invoke an attacker-chosen registered protocol handler on the victim's machine. Validate at the host boundary with isSafeExternalUrl and restrict OAuth launches to absolute HTTP(S) URLs before calling urlLauncher.launch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The MCP servers page in PostHog Code is a per-user install list: each member connects servers for themselves and approves tools for themselves, with no shared team view or control. The backend now models a team MCP gateway (PostHog/posthog#72409, behind the
mcp-gatewayflag) — a control plane for which servers a team runs, under whose credentials, with which tools allowed, and who called what. The desktop app needs the matching UI.Why: we're refactoring the existing MCP server marketplace into the gateway experience from the design handoff, so teams manage servers, credentials, agent identities, per-tool policies, and auditing in one place — implemented here per the design artifacts.
Changes
packages/ui/src/features/mcp-gatewayfeature rendered at/mcp-serverswhen themcp-gatewayflag is on; the legacy marketplace stays as the fallback while the flag is off.default_servers_enabledposture, which also covers catalog servers published later.@posthog/api-client: hand-written types + client methods for themcp_gatewayAPI family (not in the generated OpenAPI client yet); install methods accept the new sharing options.@posthog/core/mcp-gateway: portable helpers (filtering, policy counts, time formatting, add-server request building, install flow) with unit tests.mcpCallbackdeep-link round-trip.Adaptations where the backend constrains the design:
handlerather than an email.How did you test this?
pnpm typecheckacross the workspace (24 tasks green).biome lintclean on all touched packages (including zeronoRestrictedImportsinpackages/core).packages/core/src/mcp-gatewaysuites (57 tests), newpackages/ui/src/features/mcp-gatewaysuites (19 tests across 9 files), existingmcp-servers/mcp-server-managerUI suites, and the full@posthog/api-clientsuite (106 tests) all pass.node scripts/check-host-boundaries.mjs— no new violations.mcp-gatewayflag in PostHog/posthog#72409, which is still open.Automatic notifications
Created with PostHog Code