feat(gateway): Agent Tool Interface — discovery, pricing, MCP, metrics, and master keys#201
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🗃️ Database Migration Detected This PR includes changes to Prisma schema files. Please ensure:
Requesting review from the core team: @livepeer/core |
github-actions
Bot
added
has-migration
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds gateway features: idempotency caching for mutating requests, master-key auth and management, catalog/pricing/metrics endpoints (with cron aggregation), new DB models, catalog/MCP adapters, admin APIs, frontend master-keys UI, validation updates, and related tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway as "Gateway Route"
participant Cache as "Idempotency Cache"
participant Upstream as "Upstream Service"
Client->>Gateway: Mutating request (POST/PUT/PATCH/DELETE) w/ Idempotency-Key
Gateway->>Cache: checkIdempotency(team, connector, path, key, method)
alt Cache Hit
Cache-->>Gateway: cached {status, body, headers}
Gateway->>Gateway: add header X-Idempotent-Replayed: true
Gateway-->>Client: Return cached response
else Cache Miss
Cache-->>Gateway: null
Gateway->>Upstream: Forward request to upstream
Upstream-->>Gateway: Response {status, body, headers}
Gateway->>Cache: storeIdempotency(team, connector, path, key, method, response)
Cache-->>Gateway: stored
Gateway-->>Client: Return upstream response
end
sequenceDiagram
participant Client
participant Auth as "Auth System"
participant RateLimit as "Rate Limiter"
participant DB as "Prisma"
Client->>Auth: Request with Authorization: Bearer gwm_xxx
Auth->>RateLimit: Check/consume token by client IP
alt Rate limited
RateLimit-->>Auth: limit exceeded
Auth-->>Client: 429 Too Many Requests
else Allowed
Auth->>DB: find GatewayMasterKey by keyHash
alt Key valid & active
DB-->>Auth: MasterKey record
Auth->>DB: update lastUsedAt (fire-and-forget)
Auth-->>Client: AuthResult (callerType: masterKey, teamId, isMasterKey: true)
else invalid/expired/revoked
DB-->>Auth: null
Auth->>RateLimit: consume extra token
Auth-->>Client: 401 Unauthorized
end
end
sequenceDiagram
participant Cron
participant Metrics as "Metrics Route"
participant DB as "Prisma"
Cron->>Metrics: GET /api/v1/gw/admin/metrics/aggregate (CRON_SECRET)
Metrics->>DB: list published connectors
loop per connector
Metrics->>DB: fetch usage & health rows for period
Metrics->>Metrics: compute rates, latencies, percentiles, availability
Metrics->>DB: upsert hourly ConnectorMetrics
Metrics->>DB: aggregate and upsert daily rollup
end
Metrics-->>Cron: { aggregated: N, periodStart, periodEnd, summaries[] }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds an “Agent Tool Interface” to the Service Gateway so AI agents (and the admin UI) can discover connectors, inspect pricing/metrics/rankings, invoke tools via MCP, and use master keys + idempotency + structured error recovery.
Changes:
- Introduces catalog, pricing, rankings, discovery, and MCP endpoints under
/api/v1/gw/*. - Adds new Prisma models for pricing, master keys, metrics, and capability rankings (+ connector agent metadata fields).
- Adds frontend UI for master keys, pricing, performance, and agent metadata; plus idempotency + error-recovery support in the proxy.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| vercel.json | CORS header update, rewrite for discovery, metrics aggregation cron |
| plugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx | New master keys management UI |
| plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx | Adds Pricing/Performance tabs + agent metadata section |
| plugins/service-gateway/frontend/src/tests/MasterKeysPage.test.tsx | Tests for MasterKeysPage |
| plugins/service-gateway/frontend/src/App.tsx | Adds /master-keys route |
| packages/database/prisma/schema.prisma | Adds pricing/master-key/metrics/ranking models + connector metadata fields |
| apps/web-next/src/lib/gateway/types.ts | Adds masterKey caller type + flag |
| apps/web-next/src/lib/gateway/respond.ts | Adds agent-friendly error recovery metadata + Retry-After |
| apps/web-next/src/lib/gateway/pricing.ts | Pricing formatting + cost estimation logic |
| apps/web-next/src/lib/gateway/metrics.ts | Metrics retrieval/aggregation helpers |
| apps/web-next/src/lib/gateway/mcp-adapter.ts | MCP/OpenAI adapters + JSON-RPC handler |
| apps/web-next/src/lib/gateway/idempotency.ts | Redis-backed idempotency caching helpers |
| apps/web-next/src/lib/gateway/catalog.ts | Tool catalog builder (connectors + endpoints + pricing/metrics/rankings) |
| apps/web-next/src/lib/gateway/authorize.ts | Adds master-key authorization path |
| apps/web-next/src/lib/gateway/admin/validation.ts | Adds reserved slugs + agent metadata + endpoint examples validation |
| apps/web-next/src/lib/gateway/admin/audit.ts | Adds audit action types for master keys |
| apps/web-next/src/lib/gateway/tests/pricing.test.ts | Tests for pricing formatter + estimator |
| apps/web-next/src/lib/gateway/tests/metrics.test.ts | Tests for metrics summarization |
| apps/web-next/src/lib/gateway/tests/mcp-adapter.test.ts | Tests for MCP/OpenAI adapters + JSON-RPC handling |
| apps/web-next/src/lib/gateway/tests/idempotency.test.ts | Tests for idempotency cache get/set |
| apps/web-next/src/lib/gateway/tests/error-recovery.test.ts | Tests for new error recovery fields + Retry-After |
| apps/web-next/src/lib/gateway/tests/catalog.test.ts | Tests for tool descriptor mapping |
| apps/web-next/src/lib/gateway/tests/authorize.test.ts | Tests for master-key auth path |
| apps/web-next/src/app/api/v1/gw/rankings/route.ts | Rankings API by category |
| apps/web-next/src/app/api/v1/gw/pricing/route.ts | Pricing API (all connectors) |
| apps/web-next/src/app/api/v1/gw/pricing/[slug]/route.ts | Pricing API (single connector) |
| apps/web-next/src/app/api/v1/gw/pricing/[slug]/estimate/route.ts | Cost estimate endpoint |
| apps/web-next/src/app/api/v1/gw/mcp/route.ts | MCP JSON-RPC endpoint |
| apps/web-next/src/app/api/v1/gw/discovery/route.ts | Public discovery document |
| apps/web-next/src/app/api/v1/gw/catalog/route.ts | Catalog API (paginated + format negotiation) |
| apps/web-next/src/app/api/v1/gw/catalog/[slug]/route.ts | Single tool descriptor endpoint |
| apps/web-next/src/app/api/v1/gw/catalog/[slug]/rankings/route.ts | Rankings for a connector |
| apps/web-next/src/app/api/v1/gw/catalog/[slug]/metrics/route.ts | Metrics endpoint (summary + history) |
| apps/web-next/src/app/api/v1/gw/admin/metrics/aggregate/route.ts | Cron aggregation into ConnectorMetrics |
| apps/web-next/src/app/api/v1/gw/admin/master-keys/route.ts | Master key list/create |
| apps/web-next/src/app/api/v1/gw/admin/master-keys/[id]/route.ts | Master key detail/revoke |
| apps/web-next/src/app/api/v1/gw/admin/master-keys/[id]/rotate/route.ts | Master key rotation |
| apps/web-next/src/app/api/v1/gw/admin/connectors/[id]/rankings/route.ts | Admin upsert/list rankings |
| apps/web-next/src/app/api/v1/gw/admin/connectors/[id]/pricing/route.ts | Admin upsert/get pricing config |
| apps/web-next/src/app/api/v1/gw/[connector]/[...path]/route.ts | Adds idempotency to proxy pipeline |
| apps/web-next/next.config.js | Adds rewrite + expands CORS allowed headers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+102
to
105
| { | ||
| "source": "/api/v1/gw/.well-known/gateway.json", | ||
| "destination": "/api/v1/gw/discovery" | ||
| } |
There was a problem hiding this comment.
The PR description mentions serving the discovery document at .well-known/gateway.json, but the rewrite here is for /api/v1/gw/.well-known/gateway.json. If the intent is the standard root well-known location, add a rewrite for /.well-known/gateway.json (or update the description to match the actual path).
Comment on lines
+48
to
+59
| for (const connector of connectors) { | ||
| const records = await prisma.gatewayUsageRecord.findMany({ | ||
| where: { | ||
| connectorId: connector.id, | ||
| timestamp: { | ||
| gte: periodStart, | ||
| lt: periodEnd, | ||
| }, | ||
| }, | ||
| select: { statusCode: true, latencyMs: true, upstreamLatencyMs: true }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
The cron aggregates per connector in a sequential for loop and immediately hits the DB (gatewayUsageRecord.findMany) per connector. This N+1 pattern will scale poorly as connector count grows and increases the chance of cron timeouts. Consider batching/parallelizing with a concurrency limit and/or moving aggregation into a DB-side groupBy query.
Suggested change
| for (const connector of connectors) { | |
| const records = await prisma.gatewayUsageRecord.findMany({ | |
| where: { | |
| connectorId: connector.id, | |
| timestamp: { | |
| gte: periodStart, | |
| lt: periodEnd, | |
| }, | |
| }, | |
| select: { statusCode: true, latencyMs: true, upstreamLatencyMs: true }, | |
| }); | |
| const allRecords = await prisma.gatewayUsageRecord.findMany({ | |
| where: { | |
| connectorId: { in: connectors.map((c) => c.id) }, | |
| timestamp: { | |
| gte: periodStart, | |
| lt: periodEnd, | |
| }, | |
| }, | |
| select: { | |
| connectorId: true, | |
| statusCode: true, | |
| latencyMs: true, | |
| upstreamLatencyMs: true, | |
| }, | |
| }); | |
| for (const connector of connectors) { | |
| const records = allRecords.filter((r) => r.connectorId === connector.id); |
Comment on lines
+1226
to
+1230
| useEffect(() => { | ||
| setLoaded(false); | ||
| api.get(`/catalog/${connectorSlug}/metrics?window=${timeWindow}`) | ||
| .then((data) => { | ||
| setMetrics(data.metrics || null); |
There was a problem hiding this comment.
useGatewayApi() prefixes all requests with /api/v1/gw/admin, but this tab calls api.get(/catalog/${connectorSlug}/metrics?...), which will hit /api/v1/gw/admin/catalog/... (no such route) instead of /api/v1/gw/catalog/:slug/metrics. Use a non-admin gateway API client for catalog/metrics requests (or add matching admin routes).
| endpoints: { where: { enabled: true }, orderBy: { createdAt: 'asc' } }, | ||
| pricing: true, | ||
| healthChecks: { orderBy: { checkedAt: 'desc' }, take: 1 }, | ||
| metrics: { where: { period: 'daily' }, orderBy: { periodStart: 'desc' }, take: 1 }, |
There was a problem hiding this comment.
Catalog building includes metrics: { where: { period: 'daily' } ... }, but the only aggregation job added in this PR writes period: 'hourly' metrics. As a result, performance will always be null in the catalog unless daily rows are produced elsewhere. Either also compute/store daily metrics, or fetch/aggregate hourly rows when building descriptors.
Suggested change
| metrics: { where: { period: 'daily' }, orderBy: { periodStart: 'desc' }, take: 1 }, | |
| metrics: { where: { period: 'hourly' }, orderBy: { periodStart: 'desc' }, take: 1 }, |
Comment on lines
+28
to
+32
| const period = window === '1h' ? 'hourly' : 'daily'; | ||
| const row = await prisma.connectorMetrics.findFirst({ | ||
| where: { connectorId, period }, | ||
| orderBy: { periodStart: 'desc' }, | ||
| }); |
There was a problem hiding this comment.
For window='24h', this helper selects period = 'daily' (and the 7d path also queries daily rows), but the cron aggregation added in this PR writes period: 'hourly' metrics only. That makes 24h/7d summaries come back null even when hourly data exists. Either add daily aggregation or compute these windows by aggregating recent hourly rows.
Comment on lines
+35
to
+36
| const connector = await prisma.serviceConnector.findFirst({ | ||
| where: { slug, status: 'published' }, |
There was a problem hiding this comment.
The connector query is global (where: { slug, status: 'published' }) and doesn’t enforce the caller’s scope or connector visibility. This allows authenticated users to fetch descriptors for other teams’ private connectors by guessing slugs. Filter to connectors owned by the caller’s scope OR visibility='public' (and published).
Suggested change
| const connector = await prisma.serviceConnector.findFirst({ | |
| where: { slug, status: 'published' }, | |
| // Enforce access control: only allow published connectors that are either | |
| // public or owned by the caller's scope. | |
| const authScopeId = (auth as any).scopeId; | |
| const connectorWhere: any = | |
| authScopeId | |
| ? { | |
| slug, | |
| status: 'published', | |
| OR: [ | |
| { visibility: 'public' }, | |
| { scopeId: authScopeId }, | |
| ], | |
| } | |
| : { | |
| slug, | |
| status: 'published', | |
| visibility: 'public', | |
| }; | |
| const connector = await prisma.serviceConnector.findFirst({ | |
| where: connectorWhere, |
Comment on lines
+27
to
+28
| const connectors = await prisma.serviceConnector.findMany({ | ||
| where: { status: 'published' }, |
There was a problem hiding this comment.
This connector lookup is not scope-aware (where: { status: 'published' }), so it will return pricing for connectors owned by other teams (including private/team visibility) to any authenticated caller. Restrict to connectors accessible from the caller’s scope (owned by team/user OR public).
Suggested change
| const connectors = await prisma.serviceConnector.findMany({ | |
| where: { status: 'published' }, | |
| // Build scope-aware filters so we only return connectors visible to this caller | |
| const scopeFilters: any[] = [ | |
| // Always allow public connectors | |
| { visibility: 'public' }, | |
| ]; | |
| // If the auth object carries a user identifier, include connectors owned by this user | |
| if ((auth as any).userId) { | |
| scopeFilters.push({ ownerId: (auth as any).userId }); | |
| } | |
| // If the auth object carries a team identifier, include connectors owned by this team | |
| if ((auth as any).teamId) { | |
| scopeFilters.push({ teamId: (auth as any).teamId }); | |
| } | |
| const connectors = await prisma.serviceConnector.findMany({ | |
| where: { | |
| status: 'published', | |
| OR: scopeFilters, | |
| }, |
Comment on lines
+55
to
+56
| const connector = await prisma.serviceConnector.findFirst({ | ||
| where: { slug, status: 'published' }, |
There was a problem hiding this comment.
The estimate route also fetches connectors by { slug, status: 'published' } with no scope/visibility filtering. That leaks existence and pricing config for private connectors across teams. Apply the same scope rules as the proxy resolver (owned by caller scope OR public).
Suggested change
| const connector = await prisma.serviceConnector.findFirst({ | |
| where: { slug, status: 'published' }, | |
| const scopeConditions: Array<Record<string, unknown>> = [ | |
| { visibility: 'public' }, | |
| ]; | |
| if ((auth as any).scope?.id) { | |
| scopeConditions.push({ scopeId: (auth as any).scope.id }); | |
| } | |
| const connector = await prisma.serviceConnector.findFirst({ | |
| where: { | |
| slug, | |
| status: 'published', | |
| OR: scopeConditions, | |
| }, |
|
|
||
| export interface CachedResponse { | ||
| status: number; | ||
| body: string; |
There was a problem hiding this comment.
Idempotency caching stores body as a string. The gateway already supports binary request/response bodies (and caches GET responses as ArrayBuffers), so replaying a cached mutating response may corrupt non-text payloads and can be memory-heavy for large responses. Store the cached body as an ArrayBuffer (or base64 + content-type) and replay it accordingly.
Suggested change
| body: string; | |
| body: string | ArrayBuffer; |
Comment on lines
108
to
111
| "crons": [ | ||
| { "path": "/api/v1/gw/admin/health/check", "schedule": "*/5 * * * *" } | ||
| { "path": "/api/v1/gw/admin/health/check", "schedule": "*/5 * * * *" }, | ||
| { "path": "/api/v1/gw/admin/metrics/aggregate", "schedule": "5 * * * *" } | ||
| ] |
There was a problem hiding this comment.
Adding the hourly /api/v1/gw/admin/metrics/aggregate cron may require increasing the Vercel function timeout: this route exports maxDuration = 120, but vercel.json’s functions config for app/api/v1/gw/** is set to 60s earlier in the file. If the Vercel config overrides the route export, the cron will likely time out as connector count grows—consider adding a more specific functions rule with a higher maxDuration for this endpoint (and/or optimizing aggregation).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (11)
apps/web-next/src/app/api/v1/gw/rankings/route.ts (1)
30-38: Consider adding a limit to prevent unbounded result sets.The query lacks a
takeclause, meaning it could return an arbitrarily large number of rows if a category has many connectors. This could impact response time and memory usage. Given the PR objectives mention "pagination caps" as a fix, this endpoint may have been overlooked.♻️ Suggested fix
const rows = await prisma.connectorCapabilityRanking.findMany({ where: { category }, orderBy: { qualityRank: 'asc' }, + take: 100, // Cap results to prevent unbounded queries include: { connector: { select: { slug: true, displayName: true }, }, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/api/v1/gw/rankings/route.ts` around lines 30 - 38, The query using prisma.connectorCapabilityRanking.findMany can return an unbounded result set for a given category; add pagination limits by supplying a take parameter (and optionally skip or cursor-based pagination) to cap returned rows and prevent memory/latency issues. Update the code around prisma.connectorCapabilityRanking.findMany (the rows variable and the category filter) to accept a page size (e.g., maxPageSize) and use take: maxPageSize (and implement skip or cursor if you need subsequent pages) so the endpoint enforces pagination caps.apps/web-next/src/app/api/v1/gw/pricing/route.ts (2)
19-25: Minor: Consolidate header extraction.The
requestIdandtraceIdare extracted twice — once in the auth failure branch (lines 19-20) and again after (lines 24-25). Consider extracting them once at the start:♻️ Suggested refactor
export async function GET(request: NextRequest) { + const requestId = request.headers.get('x-request-id'); + const traceId = request.headers.get('x-trace-id'); + const auth = await authorize(request); if (!auth) { - const requestId = request.headers.get('x-request-id'); - const traceId = request.headers.get('x-trace-id'); return buildErrorResponse('UNAUTHORIZED', 'Authentication required', 401, requestId, traceId); } - - const requestId = request.headers.get('x-request-id'); - const traceId = request.headers.get('x-trace-id');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/api/v1/gw/pricing/route.ts` around lines 19 - 25, Extract requestId and traceId once before the authentication check instead of twice; move the two header reads (request.headers.get('x-request-id') and request.headers.get('x-trace-id')) to the top of the route handler (the function that calls buildErrorResponse) and remove the duplicate reads inside the auth-failure branch so both the unauthorized branch and the normal flow reuse the same requestId and traceId variables when calling buildErrorResponse.
27-31: Consider adding pagination for scalability.The endpoint returns all published connectors without pagination. If the connector count grows significantly, this could impact response times and memory usage. Consider adding
cursor/limitparameters consistent with the catalog endpoint mentioned in the PR objectives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/api/v1/gw/pricing/route.ts` around lines 27 - 31, The endpoint currently loads all published connectors via prisma.serviceConnector.findMany; make it paginated by reading optional query parameters (e.g., limit and cursor) from the incoming request and passing them to prisma.serviceConnector.findMany using take and cursor (or skip) with an orderBy on displayName to ensure stable ordering; implement cursor-based pagination semantics (fetch limit+1 to detect next page, return items and nextCursor) and update the route handler response shape to include pagination metadata so clients can request subsequent pages.apps/web-next/src/lib/gateway/__tests__/metrics.test.ts (1)
3-10: Consider removing unused mock.The
prisma.connectorMetrics.findFirst/findManymocks appear unused sincesummarizeMetricsForDescriptortakes a row object directly rather than querying the database. This mock setup could be removed to simplify the test file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/lib/gateway/__tests__/metrics.test.ts` around lines 3 - 10, Remove the unused test DB mock: delete or disable the vi.mock block that stubs prisma.connectorMetrics.findFirst and findMany since summarizeMetricsForDescriptor is tested by passing a row object directly and never calls the DB; locate the vi.mock(...) that returns prisma.connectorMetrics in the test file and remove it so the test file only defines mocks actually exercised by summarizeMetricsForDescriptor.apps/web-next/src/app/api/v1/gw/pricing/[slug]/estimate/route.ts (1)
22-32: Minor: Consolidate header extraction.Same pattern as the other pricing route — consider extracting
requestIdandtraceIdonce at the start of the function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/api/v1/gw/pricing/`[slug]/estimate/route.ts around lines 22 - 32, Extract requestId and traceId once at the top of the route handler (before any auth checks or early returns) instead of re-declaring them later; replace the two duplicate declarations of requestId and traceId in route.ts with a single pair of const requestId = request.headers.get('x-request-id') and const traceId = request.headers.get('x-trace-id') near the start of the handler so other blocks (e.g., the UNAUTHORIZED return) reuse the same variables and avoid shadowing.apps/web-next/src/app/api/v1/gw/discovery/route.ts (1)
36-42: Redundant Content-Type header.
Response.json()automatically setsContent-Type: application/json, so manually setting it in the headers object is unnecessary.♻️ Suggested simplification
const headers: Record<string, string> = { - 'Content-Type': 'application/json', }; if (requestId) headers['x-request-id'] = requestId; if (traceId) headers['x-trace-id'] = traceId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/api/v1/gw/discovery/route.ts` around lines 36 - 42, The headers object currently includes a redundant 'Content-Type' entry when returning the JSON response; remove the 'Content-Type' key from the headers declaration and only set the conditional x-request-id and x-trace-id values (i.e., update the headers variable used when calling Response.json(DISCOVERY, { headers }) so RequestId/TraceId are preserved but Content-Type is not manually provided).packages/database/prisma/schema.prisma (1)
1933-1935: Redundant index on ConnectorMetrics.The
@@index([connectorId, period, periodStart])at line 1934 duplicates the index that Prisma automatically creates for the@@unique([connectorId, period, periodStart])constraint at line 1933. You can safely remove the explicit index.♻️ Remove redundant index
@@unique([connectorId, period, periodStart]) - @@index([connectorId, period, periodStart]) @@schema("plugin_service_gateway")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/database/prisma/schema.prisma` around lines 1933 - 1935, The schema defines both @@unique([connectorId, period, periodStart]) and an explicit @@index([connectorId, period, periodStart]) (redundant) on the same model — remove the explicit @@index line so only the @@unique constraint remains; locate the block with @@unique([connectorId, period, periodStart]) and delete the subsequent @@index([connectorId, period, periodStart]) entry in the Prisma schema.plugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx (2)
203-209: Consider confirmation for destructive actions.The Rotate and Revoke buttons trigger immediately on click without confirmation. For master keys that "grant access to all connectors in scope," accidental revocation could be disruptive.
Consider adding a confirmation dialog or at minimum a double-click pattern for the Revoke action, as it's irreversible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx` around lines 203 - 209, The Rotate and Revoke buttons currently call handleRotate(key.id) and handleRevoke(key.id) immediately; add a user confirmation step before executing these destructive actions by wiring the buttons to a confirmation flow (e.g., browser confirm() or the app's Modal/Confirm component) and only call handleRotate or handleRevoke if the user explicitly confirms; for revoke consider a stronger guard such as a modal with an explicit "Revoke" button or a double-confirmation input (typing "REVOKE" or a second click) to prevent accidental irreversible revocations—update the onClick handlers in MasterKeysPage (the Rotate/Revoke button handlers) to open the confirmation and proceed based on the user's confirmation result.
151-157: Clipboard API may fail without feedback.
navigator.clipboard.writeText()can fail in non-secure contexts (HTTP) or if permissions are denied. Currently there's no user feedback on success/failure of the copy operation.♻️ Add copy feedback and error handling
+const [copied, setCopied] = useState(false); + +const handleCopy = async () => { + try { + await navigator.clipboard.writeText(createdKey!); + setCopied(true); + setTimeout(() => setCopied(false), 2000); + } catch { + setActionError('Failed to copy to clipboard'); + } +}; <button - onClick={() => navigator.clipboard.writeText(createdKey)} + onClick={handleCopy} className="px-3 py-2 bg-gray-700 hover:bg-gray-600 text-gray-200 text-xs rounded shrink-0" > - Copy + {copied ? 'Copied!' : 'Copy'} </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx` around lines 151 - 157, The current onClick inline handler uses navigator.clipboard.writeText(createdKey) with no error handling or feedback; replace it with an async handler (e.g., handleCopyCreatedKey) that awaits navigator.clipboard.writeText(createdKey) inside try/catch, sets local state (like copySuccess or copyError) to show UI feedback (toast, temporary label change on the "Copy" button, or an error message), and in the catch block provide a fallback (select a hidden textarea and document.execCommand('copy')) or display the permission/secure-context error via the same feedback state; update the button's onClick to call this handler and ensure createdKey and the feedback state are used to render success/failure UI.apps/web-next/src/lib/gateway/__tests__/catalog.test.ts (1)
119-125: Test name is misleading.The test description says "falls back bodySchema to inputSchema" but the actual behavior (and assertion) shows that when
inputSchemais not set on the endpoint, it falls back to usingbodySchema. Consider renaming for clarity:📝 Suggested rename
- it('falls back bodySchema to inputSchema when inputSchema not set', () => { + it('uses bodySchema as inputSchema when endpoint.inputSchema is not set', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/lib/gateway/__tests__/catalog.test.ts` around lines 119 - 125, The test title is misleading: change the `it` description for the test that calls `buildToolDescriptor(makeConnector(), '/api/v1/gw')` so it accurately describes the behavior that when an endpoint's `inputSchema` is not set it falls back to `bodySchema` (e.g., rename from "falls back bodySchema to inputSchema when inputSchema not set" to "falls back inputSchema to bodySchema when inputSchema not set" or similar); the assertion and test logic using `buildToolDescriptor` and `makeConnector` should remain unchanged.apps/web-next/src/app/api/v1/gw/catalog/[slug]/route.ts (1)
35-51: Relative baseUrl differs from catalog/route.ts which uses absolute URL.The
baseUrlhere is hardcoded as a relative path'/api/v1/gw'(line 50), while/api/v1/gw/catalogconstructs an absolute URL using the host header. This inconsistency may cause confusion for API consumers depending on which endpoint they call.Consider aligning the approach across both catalog endpoints.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/api/v1/gw/catalog/`[slug]/route.ts around lines 35 - 51, The baseUrl is currently set to a relative path ('/api/v1/gw') which differs from the catalog endpoint that builds an absolute URL; change baseUrl to an absolute origin-based URL built from the incoming request's host/protocol (e.g., derive origin from request.headers like host and x-forwarded-proto or use request.url/origin) and set baseUrl to `${origin}/api/v1/gw` before calling buildToolDescriptor(connector, baseUrl) so both catalog endpoints use a consistent absolute base URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web-next/src/app/api/v1/gw/`[connector]/[...path]/route.ts:
- Around line 325-336: The idempotency block around idempotencyKey is converting
binary responses via response.clone().text(), which corrupts binary endpoints;
update the logic in that block (response.clone(), idempotencyBody, and
storeIdempotency calls) to detect binary responses (e.g.,
config.endpoint.bodyTransform === 'binary' or Content-Type with binary) and
either skip idempotency storage for those methods or serialize the body using
clonedForIdempotency.arrayBuffer() and encode to base64 before passing into
storeIdempotency; also ensure CachedResponse.body is treated/typed as a base64
string when storing and that replaying logic decodes base64 back to binary.
In `@apps/web-next/src/app/api/v1/gw/admin/master-keys/route.ts`:
- Around line 17-24: The current isValidIPOrCIDR uses loose regexes (IP_V4,
CIDR_V4, IP_V6, CIDR_V6) that accept invalid addresses; replace regex-only
checks with real IP/CIDR parsing: use Node's net.isIP (or a small ip parsing
lib) to validate addresses in isValidIPOrCIDR, and for CIDRs split on '/' to
validate the base address with net.isIP and ensure the prefix length is an
integer within the correct range (0–32 for IPv4, 0–128 for IPv6). Update the
function and remove/replace the regex constants (IP_V4, CIDR_V4, IP_V6, CIDR_V6)
so both plain addresses and CIDRs are strictly validated (use the function name
isValidIPOrCIDR to locate and change the logic).
- Around line 113-137: The code currently waits for logAudit(...) after
prisma.gatewayMasterKey.create(...) so if logAudit fails the HTTP response
errors and the caller loses rawKey; change the flow so the created masterKey and
rawKey are returned to the caller immediately and make audit logging
best-effort: after creating the row (prisma.gatewayMasterKey.create) compute
safeKey and return success({...safeKey, rawKey}) first, then call logAudit(...)
in a detached try/catch (or schedule it asynchronously) so any logAudit error is
caught and logged but does not affect the response; alternatively, perform the
create + audit inside a DB transaction if you need atomicity, but do not block
returning rawKey on logAudit.
In `@apps/web-next/src/app/api/v1/gw/admin/metrics/aggregate/route.ts`:
- Around line 19-25: The percentile function (percentile) calculates the wrong
index for percentiles by using a floor-based formula that biases towards the
upper element for small arrays (e.g., p50 picks the second of two samples);
change the index calculation to a proper zero-based nearest-rank formula:
compute rank = ceil((p/100) * sorted.length) and then use index = max(0,
min(rank - 1, sorted.length - 1)); replace the existing Math.floor-based index
logic in percentile with this nearest-rank adjustment (or implement an explicit
interpolation strategy) so p50/p95/p99 return correct values.
- Around line 95-121: The upsert in connectorMetrics (connectorMetrics.upsert in
route.ts) currently writes only period: 'hourly' while readers in
lib/gateway/metrics.ts and lib/gateway/catalog.ts expect period: 'daily', so add
a daily materialization step: either (A) update the aggregation handler in
apps/web-next/src/app/api/v1/gw/admin/metrics/aggregate/route.ts to also upsert
a period: 'daily' row (compute daily periodStart, sum/aggregate the same metrics
fields like totalRequests, errorCount, latencyMeanMs, latencyP50Ms/P95Ms/P99Ms,
throughputRpm, availabilityPercent, healthCheckCount, healthChecksPassed from
the hourly values) after the hourly upsert, or (B) create a nightly rollup job
that reads hourly connectorMetrics rows for the day (using connectorId and
periodStart ranges) and writes a single daily connectorMetrics upsert per
connector with aggregated values; locate and reuse the same field names
(totalRequests, errorCount, errorRate, successRate, latencyMeanMs, latencyP50Ms,
latencyP95Ms, latencyP99Ms, upstreamLatencyMeanMs, gatewayOverheadMs,
availabilityPercent, healthCheckCount, healthChecksPassed, throughputRpm) and
ensure the daily upsert uses period: 'daily' and a daily periodStart baseline so
metrics.ts and catalog.ts queries return data.
In `@apps/web-next/src/app/api/v1/gw/mcp/route.ts`:
- Around line 25-29: The route currently returns non-JSON-RPC error bodies on
auth and internal fetch failures; change the failure responses in this route
handler so every error is wrapped in a JSON-RPC 2.0 envelope. Specifically, when
authorize(request) returns falsy replace the buildErrorResponse call with a
JSON-RPC response object with jsonrpc: "2.0", id: null, and an error object
describing the auth failure; likewise catch internal errors (including failures
from the internal fetch() used for tool dispatch) and respond with a JSON-RPC
envelope using error.code = -32603 (Internal error) and id: null. Update both
the early auth branch (the block using authorize) and the other failure branch
around the internal fetch (lines referenced 117-128) to return these JSON-RPC
envelopes instead of using buildErrorResponse so MCP clients always receive
JSON-RPC formatted errors.
In `@apps/web-next/src/lib/gateway/admin/validation.ts`:
- Around line 55-56: Replace the Zod v3 single-argument record usage with the
v4-compatible form: update occurrences where inputSchema and outputSchema are
defined (and the similar definitions at the other location) that currently use
z.record(z.unknown()) to use z.record(z.string(), z.unknown()) so the record key
type is explicit and future-proof for Zod v4.
In `@apps/web-next/src/lib/gateway/catalog.ts`:
- Around line 193-195: The catalog currently applies connector-level schemas
before endpoint-level ones; in the map that builds endpoints
(connector.endpoints.map) swap the precedence so endpoint-specific schemas are
used first: set inputSchema to use ep.bodySchema before connector.inputSchema,
and likewise prefer any endpoint-specific output schema (e.g., ep.responseSchema
or the per-endpoint output field) before falling back to connector.outputSchema
when computing outputSchema for the EndpointDescriptor.
In `@apps/web-next/src/lib/gateway/idempotency.ts`:
- Around line 49-56: buildKey currently concatenates teamId, connectorSlug,
endpointPath and idempotencyKey with ":" which allows collisions and omits HTTP
method and request fingerprint; change buildKey to accept the HTTP method and a
normalized payload/query fingerprint (e.g., canonicalize JSON body and sort
query params, then hash that string) and include those in the key, and stop
using raw ":" concatenation by building a structured, collision-free key (for
example prefixing segments and/or hashing the composed identifier) so the
function buildKey(teamId, connectorSlug, endpointPath, idempotencyKey) becomes
buildKey(teamId, connectorSlug, endpointPath, method, payloadHash) and returns a
single hashed or clearly delimited value that includes method and the normalized
body/query hash.
- Around line 25-27: The current return of cacheGet(...) in
getIdempotentResponse returns the unresolved Promise so rejections bypass the
surrounding try/catch; update getIdempotentResponse to await the cache read by
using await cacheGet<CachedResponse>(key, { prefix: PREFIX }) before returning,
mirroring how storeIdempotency awaits cacheSet; locate the call to cacheGet in
getIdempotentResponse (and references to buildKey and PREFIX) and change it to
await the result so failures are caught and the function can fail-open as
intended.
In `@apps/web-next/src/lib/gateway/mcp-adapter.ts`:
- Around line 45-52: The MCP tool-name mapping is not reversible because
buildMcpToolName collapses '-' into '_' while mcpToolNameToRoute maps '_' back
to '-', causing collisions; update the mapping to use a reversible encoding (for
example URL-safe base64 or percent-encoding) instead of simple character
replacement, and update mcpToolNameToRoute to decode the chosen encoding.
Concretely: change buildMcpToolName to produce an encoded token for the
concatenation of tool.name and ep.name (used in catalogToMcpTools), change
mcpToolNameToRoute to decode that token back to the original names, and ensure
all call sites that build or parse MCP names (including catalogToMcpTools and
any code at the locations noted) use the new encoder/decoder pair so names like
"foo-bar" and "foo_bar" remain distinct.
In `@apps/web-next/src/lib/gateway/metrics.ts`:
- Around line 100-116: The current weightedAvg helper is being used to produce
7d percentiles (latencyP50Ms, latencyP95Ms, latencyP99Ms) but percentiles are
not composable by averaging; remove the percentile fields from the 7d aggregate
(or replace them by computing true percentiles from raw latency
distributions/histograms if those raw histograms are available on entries)
instead of calling weightedAvg. Specifically, stop calling
weightedAvg('latencyP50Ms'|'latencyP95Ms'|'latencyP99Ms') in the returned object
and either (a) compute the percentiles by merging/combining the raw histogram
data from each item in typed (if a histogram field like latencyHistogram exists)
and then derive p50/p95/p99, or (b) omit/return null for latencyP50Ms,
latencyP95Ms, latencyP99Ms to avoid exposing incorrect values.
In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx`:
- Around line 1322-1356: The fetch in the useEffect that populates pricing calls
setLoaded(true) even on error which lets the UI become editable with empty
defaults; add a separate loadError state (e.g., const [loadError, setLoadError]
= useState(false)), set loadError=true in the catch and only setLoaded(true) on
successful response (or setLoaded(false) on error), and update handleSave (and
the other identical fetch block) to prevent saves when loadError is true by
early-returning or by gating the Save button with loaded && !loadError; update
references: the useEffect that GETs `/connectors/${connectorId}/pricing`, the
setForm/setLoaded calls, and the handleSave/setSaving logic to honor loadError.
- Around line 1226-1234: The useEffect is calling
api.get(`/catalog/${connectorSlug}/metrics?...`) where api is the admin-scoped
gateway client (from useGatewayApi) so the request 404s; replace the
admin-scoped client with a non-admin gateway API client (or the generic useApi)
and call the same path `/catalog/${connectorSlug}/metrics?window=${timeWindow}`
using that client (i.e., update the api variable used in the useEffect or swap
to the correct hook) so the request hits /api/v1/gw/catalog/... instead of
/api/v1/gw/admin/...
---
Nitpick comments:
In `@apps/web-next/src/app/api/v1/gw/catalog/`[slug]/route.ts:
- Around line 35-51: The baseUrl is currently set to a relative path
('/api/v1/gw') which differs from the catalog endpoint that builds an absolute
URL; change baseUrl to an absolute origin-based URL built from the incoming
request's host/protocol (e.g., derive origin from request.headers like host and
x-forwarded-proto or use request.url/origin) and set baseUrl to
`${origin}/api/v1/gw` before calling buildToolDescriptor(connector, baseUrl) so
both catalog endpoints use a consistent absolute base URL.
In `@apps/web-next/src/app/api/v1/gw/discovery/route.ts`:
- Around line 36-42: The headers object currently includes a redundant
'Content-Type' entry when returning the JSON response; remove the 'Content-Type'
key from the headers declaration and only set the conditional x-request-id and
x-trace-id values (i.e., update the headers variable used when calling
Response.json(DISCOVERY, { headers }) so RequestId/TraceId are preserved but
Content-Type is not manually provided).
In `@apps/web-next/src/app/api/v1/gw/pricing/`[slug]/estimate/route.ts:
- Around line 22-32: Extract requestId and traceId once at the top of the route
handler (before any auth checks or early returns) instead of re-declaring them
later; replace the two duplicate declarations of requestId and traceId in
route.ts with a single pair of const requestId =
request.headers.get('x-request-id') and const traceId =
request.headers.get('x-trace-id') near the start of the handler so other blocks
(e.g., the UNAUTHORIZED return) reuse the same variables and avoid shadowing.
In `@apps/web-next/src/app/api/v1/gw/pricing/route.ts`:
- Around line 19-25: Extract requestId and traceId once before the
authentication check instead of twice; move the two header reads
(request.headers.get('x-request-id') and request.headers.get('x-trace-id')) to
the top of the route handler (the function that calls buildErrorResponse) and
remove the duplicate reads inside the auth-failure branch so both the
unauthorized branch and the normal flow reuse the same requestId and traceId
variables when calling buildErrorResponse.
- Around line 27-31: The endpoint currently loads all published connectors via
prisma.serviceConnector.findMany; make it paginated by reading optional query
parameters (e.g., limit and cursor) from the incoming request and passing them
to prisma.serviceConnector.findMany using take and cursor (or skip) with an
orderBy on displayName to ensure stable ordering; implement cursor-based
pagination semantics (fetch limit+1 to detect next page, return items and
nextCursor) and update the route handler response shape to include pagination
metadata so clients can request subsequent pages.
In `@apps/web-next/src/app/api/v1/gw/rankings/route.ts`:
- Around line 30-38: The query using prisma.connectorCapabilityRanking.findMany
can return an unbounded result set for a given category; add pagination limits
by supplying a take parameter (and optionally skip or cursor-based pagination)
to cap returned rows and prevent memory/latency issues. Update the code around
prisma.connectorCapabilityRanking.findMany (the rows variable and the category
filter) to accept a page size (e.g., maxPageSize) and use take: maxPageSize (and
implement skip or cursor if you need subsequent pages) so the endpoint enforces
pagination caps.
In `@apps/web-next/src/lib/gateway/__tests__/catalog.test.ts`:
- Around line 119-125: The test title is misleading: change the `it` description
for the test that calls `buildToolDescriptor(makeConnector(), '/api/v1/gw')` so
it accurately describes the behavior that when an endpoint's `inputSchema` is
not set it falls back to `bodySchema` (e.g., rename from "falls back bodySchema
to inputSchema when inputSchema not set" to "falls back inputSchema to
bodySchema when inputSchema not set" or similar); the assertion and test logic
using `buildToolDescriptor` and `makeConnector` should remain unchanged.
In `@apps/web-next/src/lib/gateway/__tests__/metrics.test.ts`:
- Around line 3-10: Remove the unused test DB mock: delete or disable the
vi.mock block that stubs prisma.connectorMetrics.findFirst and findMany since
summarizeMetricsForDescriptor is tested by passing a row object directly and
never calls the DB; locate the vi.mock(...) that returns prisma.connectorMetrics
in the test file and remove it so the test file only defines mocks actually
exercised by summarizeMetricsForDescriptor.
In `@packages/database/prisma/schema.prisma`:
- Around line 1933-1935: The schema defines both @@unique([connectorId, period,
periodStart]) and an explicit @@index([connectorId, period, periodStart])
(redundant) on the same model — remove the explicit @@index line so only the
@@unique constraint remains; locate the block with @@unique([connectorId,
period, periodStart]) and delete the subsequent @@index([connectorId, period,
periodStart]) entry in the Prisma schema.
In `@plugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx`:
- Around line 203-209: The Rotate and Revoke buttons currently call
handleRotate(key.id) and handleRevoke(key.id) immediately; add a user
confirmation step before executing these destructive actions by wiring the
buttons to a confirmation flow (e.g., browser confirm() or the app's
Modal/Confirm component) and only call handleRotate or handleRevoke if the user
explicitly confirms; for revoke consider a stronger guard such as a modal with
an explicit "Revoke" button or a double-confirmation input (typing "REVOKE" or a
second click) to prevent accidental irreversible revocations—update the onClick
handlers in MasterKeysPage (the Rotate/Revoke button handlers) to open the
confirmation and proceed based on the user's confirmation result.
- Around line 151-157: The current onClick inline handler uses
navigator.clipboard.writeText(createdKey) with no error handling or feedback;
replace it with an async handler (e.g., handleCopyCreatedKey) that awaits
navigator.clipboard.writeText(createdKey) inside try/catch, sets local state
(like copySuccess or copyError) to show UI feedback (toast, temporary label
change on the "Copy" button, or an error message), and in the catch block
provide a fallback (select a hidden textarea and document.execCommand('copy'))
or display the permission/secure-context error via the same feedback state;
update the button's onClick to call this handler and ensure createdKey and the
feedback state are used to render success/failure UI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf9b18dc-8cbe-4d30-b68d-79242bd43ad5
📒 Files selected for processing (41)
apps/web-next/next.config.jsapps/web-next/src/app/api/v1/gw/[connector]/[...path]/route.tsapps/web-next/src/app/api/v1/gw/admin/connectors/[id]/pricing/route.tsapps/web-next/src/app/api/v1/gw/admin/connectors/[id]/rankings/route.tsapps/web-next/src/app/api/v1/gw/admin/master-keys/[id]/rotate/route.tsapps/web-next/src/app/api/v1/gw/admin/master-keys/[id]/route.tsapps/web-next/src/app/api/v1/gw/admin/master-keys/route.tsapps/web-next/src/app/api/v1/gw/admin/metrics/aggregate/route.tsapps/web-next/src/app/api/v1/gw/catalog/[slug]/metrics/route.tsapps/web-next/src/app/api/v1/gw/catalog/[slug]/rankings/route.tsapps/web-next/src/app/api/v1/gw/catalog/[slug]/route.tsapps/web-next/src/app/api/v1/gw/catalog/route.tsapps/web-next/src/app/api/v1/gw/discovery/route.tsapps/web-next/src/app/api/v1/gw/mcp/route.tsapps/web-next/src/app/api/v1/gw/pricing/[slug]/estimate/route.tsapps/web-next/src/app/api/v1/gw/pricing/[slug]/route.tsapps/web-next/src/app/api/v1/gw/pricing/route.tsapps/web-next/src/app/api/v1/gw/rankings/route.tsapps/web-next/src/lib/gateway/__tests__/authorize.test.tsapps/web-next/src/lib/gateway/__tests__/catalog.test.tsapps/web-next/src/lib/gateway/__tests__/error-recovery.test.tsapps/web-next/src/lib/gateway/__tests__/idempotency.test.tsapps/web-next/src/lib/gateway/__tests__/mcp-adapter.test.tsapps/web-next/src/lib/gateway/__tests__/metrics.test.tsapps/web-next/src/lib/gateway/__tests__/pricing.test.tsapps/web-next/src/lib/gateway/admin/audit.tsapps/web-next/src/lib/gateway/admin/validation.tsapps/web-next/src/lib/gateway/authorize.tsapps/web-next/src/lib/gateway/catalog.tsapps/web-next/src/lib/gateway/idempotency.tsapps/web-next/src/lib/gateway/mcp-adapter.tsapps/web-next/src/lib/gateway/metrics.tsapps/web-next/src/lib/gateway/pricing.tsapps/web-next/src/lib/gateway/respond.tsapps/web-next/src/lib/gateway/types.tspackages/database/prisma/schema.prismaplugins/service-gateway/frontend/src/App.tsxplugins/service-gateway/frontend/src/__tests__/MasterKeysPage.test.tsxplugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsxplugins/service-gateway/frontend/src/pages/MasterKeysPage.tsxvercel.json
Comment on lines
+49
to
+56
| function buildKey( | ||
| teamId: string, | ||
| connectorSlug: string, | ||
| endpointPath: string, | ||
| idempotencyKey: string | ||
| ): string { | ||
| return `${teamId}:${connectorSlug}:${endpointPath}:${idempotencyKey}`; | ||
| } |
There was a problem hiding this comment.
Bind idempotency entries to the full request fingerprint.
The key currently ignores HTTP method and request payload, so two different mutations to the same path with the same Idempotency-Key will share one cached response. That can replay a stale result for a different operation. Please include method + a normalized body/query hash in the key, and avoid raw : concatenation so user-controlled keys/paths cannot collide.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/lib/gateway/idempotency.ts` around lines 49 - 56, buildKey
currently concatenates teamId, connectorSlug, endpointPath and idempotencyKey
with ":" which allows collisions and omits HTTP method and request fingerprint;
change buildKey to accept the HTTP method and a normalized payload/query
fingerprint (e.g., canonicalize JSON body and sort query params, then hash that
string) and include those in the key, and stop using raw ":" concatenation by
building a structured, collision-free key (for example prefixing segments and/or
hashing the composed identifier) so the function buildKey(teamId, connectorSlug,
endpointPath, idempotencyKey) becomes buildKey(teamId, connectorSlug,
endpointPath, method, payloadHash) and returns a single hashed or clearly
delimited value that includes method and the normalized body/query hash.
Comment on lines
+45
to
+52
| export function catalogToMcpTools(catalog: ToolDescriptor[]): McpTool[] { | ||
| const tools: McpTool[] = []; | ||
| for (const tool of catalog) { | ||
| for (const ep of tool.endpoints) { | ||
| tools.push({ | ||
| name: buildMcpToolName(tool.name, ep.name), | ||
| description: buildToolDescription(tool, ep), | ||
| inputSchema: (ep.inputSchema as object) || { type: 'object', properties: {} }, |
There was a problem hiding this comment.
Make the MCP tool-name mapping reversible.
buildMcpToolName() collapses - into _, while mcpToolNameToRoute() turns every _ back into -. Because endpoint names are not slug-restricted, names like foo-bar and foo_bar collide, so tools/list can expose duplicate names and tools/call can dispatch to the wrong endpoint. Either validate endpoint names to a single safe charset before publish time or switch this helper to a reversible encoding.
Also applies to: 66-71, 82-90, 138-145, 174-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/lib/gateway/mcp-adapter.ts` around lines 45 - 52, The MCP
tool-name mapping is not reversible because buildMcpToolName collapses '-' into
'_' while mcpToolNameToRoute maps '_' back to '-', causing collisions; update
the mapping to use a reversible encoding (for example URL-safe base64 or
percent-encoding) instead of simple character replacement, and update
mcpToolNameToRoute to decode the chosen encoding. Concretely: change
buildMcpToolName to produce an encoded token for the concatenation of tool.name
and ep.name (used in catalogToMcpTools), change mcpToolNameToRoute to decode
that token back to the original names, and ensure all call sites that build or
parse MCP names (including catalogToMcpTools and any code at the locations
noted) use the new encoder/decoder pair so names like "foo-bar" and "foo_bar"
remain distinct.
Comment on lines
+1322
to
+1356
| useEffect(() => { | ||
| api.get(`/connectors/${connectorId}/pricing`).then((res: { success: boolean; data: Record<string, unknown> | null }) => { | ||
| if (res.data) { | ||
| const p = res.data; | ||
| setForm({ | ||
| upstreamCostPerUnit: p.upstreamCostPerUnit != null ? String(p.upstreamCostPerUnit) : '', | ||
| upstreamUnit: (p.upstreamUnit as string) || '', | ||
| upstreamNotes: (p.upstreamNotes as string) || '', | ||
| costPerUnit: String(p.costPerUnit ?? 0), | ||
| unit: (p.unit as string) || 'request', | ||
| currency: (p.currency as string) || 'USD', | ||
| billingModel: (p.billingModel as string) || 'per-unit', | ||
| freeQuota: p.freeQuota != null ? String(p.freeQuota) : '', | ||
| }); | ||
| } | ||
| setLoaded(true); | ||
| }).catch(() => setLoaded(true)); | ||
| }, [api, connectorId]); | ||
|
|
||
| const handleSave = async () => { | ||
| setSaving(true); | ||
| try { | ||
| await api.put(`/connectors/${connectorId}/pricing`, { | ||
| upstreamCostPerUnit: form.upstreamCostPerUnit ? parseFloat(form.upstreamCostPerUnit) : undefined, | ||
| upstreamUnit: form.upstreamUnit || undefined, | ||
| upstreamNotes: form.upstreamNotes || undefined, | ||
| costPerUnit: parseFloat(form.costPerUnit) || 0, | ||
| unit: form.unit, | ||
| currency: form.currency, | ||
| billingModel: form.billingModel, | ||
| freeQuota: form.freeQuota ? parseInt(form.freeQuota) : undefined, | ||
| }); | ||
| } finally { | ||
| setSaving(false); | ||
| } |
There was a problem hiding this comment.
Don't turn a failed bootstrap fetch into an editable empty form.
Both editors flip loaded to true on GET failure and keep saving enabled, so a transient 403/500 renders blank/default values and lets the user overwrite existing pricing or agent metadata with empties. Please track a load error separately and block saves until the initial fetch succeeds.
Also applies to: 1437-1472
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around
lines 1322 - 1356, The fetch in the useEffect that populates pricing calls
setLoaded(true) even on error which lets the UI become editable with empty
defaults; add a separate loadError state (e.g., const [loadError, setLoadError]
= useState(false)), set loadError=true in the catch and only setLoaded(true) on
successful response (or setLoaded(false) on error), and update handleSave (and
the other identical fetch block) to prevent saves when loadError is true by
early-returning or by gating the Save button with loaded && !loadError; update
references: the useEffect that GETs `/connectors/${connectorId}/pricing`, the
setForm/setLoaded calls, and the handleSave/setSaving logic to honor loadError.
seanhanca
added a commit
that referenced
this pull request
Mar 10, 2026
…bbit Security: - Add scope/visibility filtering to catalog, pricing, metrics, and rankings endpoints to prevent leaking private/team connectors - Wrap MCP auth/internal errors in JSON-RPC 2.0 envelope - Forward x-team-id, x-request-id, x-trace-id headers in MCP tools/call Correctness: - Fix metrics period mismatch: change catalog/metrics queries from 'daily' to 'hourly' and add daily rollup in aggregation cron - Fix percentile calculation using ceil-based nearest-rank formula - Set 7d percentiles to null (not composable via weighted average) - Fix inputSchema precedence: endpoint bodySchema now takes priority over connector inputSchema - Include HTTP method in idempotency cache key to prevent cross-method collisions - Use arrayBuffer+base64 for idempotency storage to avoid binary corruption - Await cacheGet in getIdempotentResponse so rejections are caught Improvements: - Fix Performance tab using wrong API client (admin vs public catalog) - Add copy feedback and revoke confirmation to MasterKeysPage - Update z.record() calls for Zod v4 forward compatibility - Consolidate duplicate requestId/traceId extraction in pricing routes - Remove redundant Content-Type header in discovery route - Remove redundant @@index from ConnectorMetrics schema - Add take:100 cap to rankings endpoint - Rename misleading test description Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx (3)
1509-1518:⚠️ Potential issue | 🟠 MajorSame issue: failed fetch allows saving empty metadata.
Similar to PricingTab, if the connector fetch fails, the form shows empty values and remains editable. Track load errors and prevent saves until the initial data loads successfully.
🛡️ Proposed fix
const [saving, setSaving] = useState(false); const [loaded, setLoaded] = useState(false); + const [loadError, setLoadError] = useState(false); useEffect(() => { + setLoadError(false); api.get(`/connectors/${connectorId}`).then((res: { success: boolean; data: Record<string, unknown> }) => { const d = res.data; setAgentDescription((d.agentDescription as string) || ''); setAgentNotFor((d.agentNotFor as string) || ''); setInputSchemaStr(d.inputSchema ? JSON.stringify(d.inputSchema, null, 2) : ''); setOutputSchemaStr(d.outputSchema ? JSON.stringify(d.outputSchema, null, 2) : ''); setLoaded(true); - }).catch(() => setLoaded(true)); + }).catch(() => { + setLoadError(true); + setLoaded(true); + }); }, [api, connectorId]); - if (!loaded) return null; + if (!loaded) return null; + if (loadError) { + return ( + <div className="bg-red-500/10 border border-red-500/30 rounded-lg p-4 text-red-400 text-sm"> + Failed to load agent metadata. Please refresh. + </div> + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around lines 1509 - 1518, The useEffect fetching connector data (api.get in ConnectorDetailPage) currently sets setLoaded(true) on both success and catch, which lets the form remain editable with empty values after a fetch failure; add a load error state (e.g., loadError / setLoadError) and set it true in the catch branch (leave setLoaded false or set a separate loaded flag only on success), then update the save handler (e.g., handleSave / onSave) and form controls to disable saving and editing when loadError is true (or when loaded is false) and show an appropriate error state so saves are blocked until the initial data loads successfully.
1298-1306:⚠️ Potential issue | 🔴 CriticalFix the endpoint path: the metrics API is not under the admin namespace.
The
useGatewayApihook prepends/api/v1/gw/adminto all paths, but the catalog metrics endpoint exists at/api/v1/gw/catalog/[slug]/metrics. This mismatch will cause a 404 and metrics will never load.Either use a direct fetch to the correct URL or create a separate non-admin API helper.
🐛 Proposed fix using direct fetch
useEffect(() => { setLoaded(false); - api.get(`/catalog/${connectorSlug}/metrics?window=${timeWindow}`) - .then((data) => { - setMetrics(data.metrics || null); + fetch(`/api/v1/gw/catalog/${connectorSlug}/metrics?window=${timeWindow}`) + .then((res) => { + if (!res.ok) throw new Error('Failed to fetch metrics'); + return res.json(); + }) + .then((data) => { + setMetrics(data.data?.metrics || null); setLoaded(true); }) .catch(() => setLoaded(true)); }, [connectorSlug, timeWindow, api]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around lines 1298 - 1306, The code in ConnectorDetailPage.tsx is calling api.get with `/catalog/${connectorSlug}/metrics` but useGatewayApi prepends the admin namespace, causing a 404; update the useEffect to call the correct non-admin endpoint by either using a direct fetch to `/api/v1/gw/catalog/${connectorSlug}/metrics` (or your app’s equivalent full path) or by using a non-admin API helper instead of the admin `api` instance; keep the existing state updates (setMetrics, setLoaded) and parameters (connectorSlug, timeWindow) intact so the logic in that useEffect remains the same.
1394-1411:⚠️ Potential issue | 🟠 MajorA failed initial fetch leaves the form editable with empty defaults, risking data loss.
If the GET request fails (e.g., 403, 500, network error), the catch block sets
loaded=truewithout distinguishing success from failure. The user sees an empty form and can save, potentially overwriting existing pricing with default values.Track load errors separately and block saves until the initial fetch succeeds.
🛡️ Proposed fix to track load errors
const [saving, setSaving] = useState(false); const [loaded, setLoaded] = useState(false); + const [loadError, setLoadError] = useState(false); useEffect(() => { + setLoadError(false); api.get(`/connectors/${connectorId}/pricing`).then((res: { success: boolean; data: Record<string, unknown> | null }) => { if (res.data) { // ... existing form population } setLoaded(true); - }).catch(() => setLoaded(true)); + }).catch(() => { + setLoadError(true); + setLoaded(true); + }); }, [api, connectorId]); const handleSave = async () => { + if (loadError) return; setSaving(true); // ... rest of save logic }; - if (!loaded) return <div className="text-gray-500 text-sm">Loading pricing...</div>; + if (!loaded) return <div className="text-gray-500 text-sm">Loading pricing...</div>; + if (loadError) return <div className="text-red-400 text-sm">Failed to load pricing. Please refresh.</div>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around lines 1394 - 1411, The GET in useEffect currently sets setLoaded(true) on both success and error, which lets the empty default form be edited and saved; introduce a separate load state (e.g., loadError or isInitialLoadSuccessful) and set it to true only on successful fetch inside api.get(...) after calling setForm, and set it to false (or an error object) in the catch block instead of just setLoaded(true); update the pricing save handler (e.g., the function that submits the form for connectorId) to check this new load state and disable/abort saves (and show an error/toast) when the initial load failed, ensuring setLoaded remains for UI spinner while the new load state controls saveability.
🧹 Nitpick comments (3)
plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx (3)
1149-1152: Consider defensive handling forid.The
id!non-null assertions work because the component returns early when!connector, which impliesidexists. However, for explicit safety, you could addidto the early return check.♻️ Optional: explicit id check
- if (loading || !connector) { + if (loading || !connector || !id) { return ( <div className="p-6 max-w-5xl mx-auto"> // loading skeleton </div> ); }Then you can safely use
idwithout assertions:- <PricingTab connectorId={id!} api={api} /> + <PricingTab connectorId={id} api={api} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around lines 1149 - 1152, The rendering uses a non-null assertion id! when rendering <PricingTab>, which is brittle; update the early-return in ConnectorDetailPage to also check for id (e.g., if (!connector || !id) return ...) so you can remove the non-null assertion and pass id directly to PricingTab (replace connectorId={id!} with connectorId={id}); ensure any other places in this component that used id! are similarly guarded.
277-290: Unnecessary destructuring inside handler.The
const { get: apiGet } = api;destructuring on line 283 is redundant sinceapiis already in scope and stable. Consider simplifying.♻️ Proposed simplification
const handleRecover = async () => { if (!id || recovering) return; setRecovering(true); setActionError(null); try { await api.put(`/connectors/${id}`, { status: 'draft' }); - const { get: apiGet } = api; - loadConnector(() => apiGet(`/connectors/${id}`)); + loadConnector(() => api.get(`/connectors/${id}`)); } catch (err: unknown) { setActionError(getSafeErrorMessage(err)); } finally { setRecovering(false); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around lines 277 - 290, In handleRecover remove the unnecessary destructuring of api (const { get: apiGet } = api) and call api.get directly when passing the reload function to loadConnector; ensure you still await api.put(`/connectors/${id}`, { status: 'draft' }) and then call loadConnector(() => api.get(`/connectors/${id}`)) so behavior is unchanged (keep setRecovering, setActionError handling and try/catch/finally intact).
1413-1429: Save failures are silent—users receive no feedback.The
handleSavefunction catches implicitly viafinallybut doesn't surface errors to the user. If the PUT fails, the user sees "Saving..." flip back to "Save Pricing" with no indication of failure.♻️ Add error feedback
+ const [saveError, setSaveError] = useState(''); const handleSave = async () => { + setSaveError(''); setSaving(true); try { await api.put(`/connectors/${connectorId}/pricing`, { // ... payload }); + } catch { + setSaveError('Failed to save pricing'); } finally { setSaving(false); } }; // In JSX, after the save button: + {saveError && <p className="text-red-400 text-xs mt-2">{saveError}</p>}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around lines 1413 - 1429, The handleSave function currently only uses finally so PUT failures are silent; update handleSave to add an explicit catch block around the api.put call (inside the existing try) that captures the error and surfaces user feedback (e.g., call the app's notification/alert method such as enqueueSnackbar, setError/setAlert state, or other global toast) with a clear message and the error detail, then keep the existing finally setSaving(false) behavior; refer to the handleSave function and the api.put(`/connectors/${connectorId}/pricing`) call to locate where to add the catch and notification logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web-next/src/app/api/v1/gw/admin/connectors/`[id]/route.ts:
- Around line 98-107: The purge branch forgets to invalidate the
resolver/catalog cache after deleting the connector—locate the cache-clear call
used in the archive branch (the call that clears cached resolver state around
the archive flow) and invoke the same invalidation before/after logAudit in the
purge branch; ensure you pass the same identifying info (id and/or existing.slug
and ctx/request) and then return success({ id, purged: true }) as before.
In `@apps/web-next/src/app/api/v1/gw/mcp/route.ts`:
- Around line 67-68: The code currently only checks truthiness of toolName so
non-string values like numbers pass; update the validation around params.name
(the toolName assignment and subsequent use in mcpToolNameToRoute) to enforce it
is a non-empty string (e.g., typeof params.name === 'string' &&
params.name.trim() !== '') before casting to string and calling
mcpToolNameToRoute; if validation fails, return the appropriate 4xx response or
throw the same error path you use for missing names so invalid types are
rejected consistently.
In `@apps/web-next/src/lib/gateway/admin/validation.ts`:
- Around line 59-61: The generic update schema updateConnectorSchema currently
allows status: 'published', which lets the generic PUT handler write publish
state directly; change the enum in updateConnectorSchema (the
createConnectorSchema.partial().omit({ slug: true }).extend(...) line) to
exclude 'published' — restrict it to only 'draft' (e.g.,
z.enum(['draft']).optional()) so publish/archive transitions remain handled by
their dedicated endpoints and cannot be set via the generic update path.
In `@plugins/service-gateway/frontend/src/pages/ConnectorWizardPage.tsx`:
- Around line 277-299: The edit path for ConnectorWizardPage.tsx currently calls
api.put(`/connectors/${editId}`, ...) without including secretRefs, so edits
that change auth/secret values leave secret metadata stale; update the PUT
payload in the edit branch (the api.put call when editId is truthy) to include
secretRefs (same key used in the create POST) or instead call the dedicated
secret update endpoint (e.g., updateConnectorSecrets or similar) after the PUT
to persist secretRefs/secret metadata—ensure you reference the editId,
secretRefs variable, and the api.put call when making this change.
- Around line 763-779: The "Skip — Create from Scratch" onClick handler only
clears selectedTemplateId but leaves other template-derived state (slug,
upstream URL, secret refs, prefilled endpoints) intact; update the onClick
callback in ConnectorWizardPage to also clear all template-related state — e.g.
reset selectedTemplateSlug, upstreamUrl, secretRefs, endpoints and any
prefilling/form state (or call the existing resetForm/resetConnectorState helper
if available) before calling setStep(1) so the wizard truly starts from scratch.
- Around line 233-238: handleAuthTypeChange currently avoids updating secretRefs
when selectedTemplateId is truthy, which leaves template-derived secretRefs
frozen and causes wrong secrets to be saved if the user manually changes auth
type; update handleAuthTypeChange so it always calls
setSecretRefs(DEFAULT_SECRET_REFS[newAuthType] || []) when auth type changes
(i.e., remove the selectedTemplateId guard) so secretRefs reflect the newly
chosen authType; reference the handleAuthTypeChange function, setSecretRefs
call, and DEFAULT_SECRET_REFS constant when making the change.
- Around line 138-149: The current hydration in ConnectorWizardPage (the eps ->
setEndpoints mapping) throws away endpoint identity causing every save to POST
and breaking edit-mode diffs; change the hydration to preserve unique
identifiers (e.g., id, uuid, or name key present on ep) into the endpoint
objects used in state (keep the original id field on each item when mapping in
setEndpoints and also in the other mapping at lines ~315-323), then implement a
diff-on-save in the save handler that compares existingConnector.endpoints to
the current endpoints state and issues create (POST) for new items, update
(PUT/PATCH) for items whose id exists but changed, and delete (DELETE) for ids
removed. Ensure the save logic references the same identifier field you
preserved so unchanged rows result in update/no-op rather than new creates, and
consider making the endpoints step read-only if identifier-preservation cannot
be guaranteed.
---
Duplicate comments:
In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx`:
- Around line 1509-1518: The useEffect fetching connector data (api.get in
ConnectorDetailPage) currently sets setLoaded(true) on both success and catch,
which lets the form remain editable with empty values after a fetch failure; add
a load error state (e.g., loadError / setLoadError) and set it true in the catch
branch (leave setLoaded false or set a separate loaded flag only on success),
then update the save handler (e.g., handleSave / onSave) and form controls to
disable saving and editing when loadError is true (or when loaded is false) and
show an appropriate error state so saves are blocked until the initial data
loads successfully.
- Around line 1298-1306: The code in ConnectorDetailPage.tsx is calling api.get
with `/catalog/${connectorSlug}/metrics` but useGatewayApi prepends the admin
namespace, causing a 404; update the useEffect to call the correct non-admin
endpoint by either using a direct fetch to
`/api/v1/gw/catalog/${connectorSlug}/metrics` (or your app’s equivalent full
path) or by using a non-admin API helper instead of the admin `api` instance;
keep the existing state updates (setMetrics, setLoaded) and parameters
(connectorSlug, timeWindow) intact so the logic in that useEffect remains the
same.
- Around line 1394-1411: The GET in useEffect currently sets setLoaded(true) on
both success and error, which lets the empty default form be edited and saved;
introduce a separate load state (e.g., loadError or isInitialLoadSuccessful) and
set it to true only on successful fetch inside api.get(...) after calling
setForm, and set it to false (or an error object) in the catch block instead of
just setLoaded(true); update the pricing save handler (e.g., the function that
submits the form for connectorId) to check this new load state and disable/abort
saves (and show an error/toast) when the initial load failed, ensuring setLoaded
remains for UI spinner while the new load state controls saveability.
---
Nitpick comments:
In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx`:
- Around line 1149-1152: The rendering uses a non-null assertion id! when
rendering <PricingTab>, which is brittle; update the early-return in
ConnectorDetailPage to also check for id (e.g., if (!connector || !id) return
...) so you can remove the non-null assertion and pass id directly to PricingTab
(replace connectorId={id!} with connectorId={id}); ensure any other places in
this component that used id! are similarly guarded.
- Around line 277-290: In handleRecover remove the unnecessary destructuring of
api (const { get: apiGet } = api) and call api.get directly when passing the
reload function to loadConnector; ensure you still await
api.put(`/connectors/${id}`, { status: 'draft' }) and then call loadConnector(()
=> api.get(`/connectors/${id}`)) so behavior is unchanged (keep setRecovering,
setActionError handling and try/catch/finally intact).
- Around line 1413-1429: The handleSave function currently only uses finally so
PUT failures are silent; update handleSave to add an explicit catch block around
the api.put call (inside the existing try) that captures the error and surfaces
user feedback (e.g., call the app's notification/alert method such as
enqueueSnackbar, setError/setAlert state, or other global toast) with a clear
message and the error detail, then keep the existing finally setSaving(false)
behavior; refer to the handleSave function and the
api.put(`/connectors/${connectorId}/pricing`) call to locate where to add the
catch and notification logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c31b9ab1-f8fb-4df7-b6b4-ef8381246e16
📒 Files selected for processing (7)
apps/web-next/src/app/api/v1/gw/admin/connectors/[id]/route.tsapps/web-next/src/app/api/v1/gw/mcp/route.tsapps/web-next/src/lib/gateway/admin/audit.tsapps/web-next/src/lib/gateway/admin/validation.tsplugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsxplugins/service-gateway/frontend/src/pages/ConnectorListPage.tsxplugins/service-gateway/frontend/src/pages/ConnectorWizardPage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web-next/src/lib/gateway/admin/audit.ts
Comment on lines
+98
to
+107
| if (purge) { | ||
| if (existing.status !== 'archived') { | ||
| return errors.badRequest('Only archived connectors can be purged. Archive it first.'); | ||
| } | ||
| await prisma.$transaction([ | ||
| prisma.gatewayUsageRecord.deleteMany({ where: { connectorId: id } }), | ||
| prisma.serviceConnector.delete({ where: { id } }), | ||
| ]); | ||
| await logAudit(ctx, { action: 'connector.purge', resourceId: id, details: { slug: existing.slug }, request }); | ||
| return success({ id, purged: true }); |
There was a problem hiding this comment.
Invalidate the connector cache on purge too.
The archive path clears cached resolver state on Line 115, but the purge branch returns without doing that. If this connector was previously cached, catalog/discovery lookups can keep serving a deleted record until the cache expires.
🛠️ Suggested change
if (purge) {
if (existing.status !== 'archived') {
return errors.badRequest('Only archived connectors can be purged. Archive it first.');
}
await prisma.$transaction([
prisma.gatewayUsageRecord.deleteMany({ where: { connectorId: id } }),
prisma.serviceConnector.delete({ where: { id } }),
]);
+ invalidateConnectorCache(ctx.teamId, existing.slug);
await logAudit(ctx, { action: 'connector.purge', resourceId: id, details: { slug: existing.slug }, request });
return success({ id, purged: true });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/app/api/v1/gw/admin/connectors/`[id]/route.ts around lines
98 - 107, The purge branch forgets to invalidate the resolver/catalog cache
after deleting the connector—locate the cache-clear call used in the archive
branch (the call that clears cached resolver state around the archive flow) and
invoke the same invalidation before/after logAudit in the purge branch; ensure
you pass the same identifying info (id and/or existing.slug and ctx/request) and
then return success({ id, purged: true }) as before.
Comment on lines
+67
to
+68
| const toolName = params.name as string; | ||
| const args = (params.arguments || {}) as Record<string, unknown>; |
There was a problem hiding this comment.
Add type validation for params.name.
The check !toolName catches null/undefined but not incorrect types (e.g., number, object). If a client sends { "name": 123 }, this passes through and could cause unexpected behavior in mcpToolNameToRoute.
Suggested fix
const params = (body.params || {}) as Record<string, unknown>;
- const toolName = params.name as string;
+ const toolName = typeof params.name === 'string' ? params.name : null;
const args = (params.arguments || {}) as Record<string, unknown>;
- if (!toolName) {
+ if (!toolName || typeof toolName !== 'string') {
return Response.json({
jsonrpc: '2.0',
id: body.id,
error: { code: -32602, message: 'Missing required param: name' },
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const toolName = params.name as string; | |
| const args = (params.arguments || {}) as Record<string, unknown>; | |
| const params = (body.params || {}) as Record<string, unknown>; | |
| const toolName = typeof params.name === 'string' ? params.name : null; | |
| const args = (params.arguments || {}) as Record<string, unknown>; | |
| if (!toolName || typeof toolName !== 'string') { | |
| return Response.json({ | |
| jsonrpc: '2.0', | |
| id: body.id, | |
| error: { code: -32602, message: 'Missing required param: name' }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/app/api/v1/gw/mcp/route.ts` around lines 67 - 68, The code
currently only checks truthiness of toolName so non-string values like numbers
pass; update the validation around params.name (the toolName assignment and
subsequent use in mcpToolNameToRoute) to enforce it is a non-empty string (e.g.,
typeof params.name === 'string' && params.name.trim() !== '') before casting to
string and calling mcpToolNameToRoute; if validation fails, return the
appropriate 4xx response or throw the same error path you use for missing names
so invalid types are rejected consistently.
Comment on lines
+59
to
+61
| export const updateConnectorSchema = createConnectorSchema.partial().omit({ slug: true }).extend({ | ||
| status: z.enum(['draft', 'published', 'archived']).optional(), | ||
| }); |
There was a problem hiding this comment.
Keep published out of the generic update schema.
Allowing status: 'published' here lets PUT /api/v1/gw/admin/connectors/:id publish a connector via the generic update path, because that handler spreads parsed.data straight into prisma.serviceConnector.update(). That bypasses the dedicated publish workflow and can leave publish-only fields/invariants, like publishedAt, out of sync. If this field is only for the recover flow, narrow it to draft here and keep publish/archive transitions on their dedicated endpoints.
🛠️ Suggested change
export const updateConnectorSchema = createConnectorSchema.partial().omit({ slug: true }).extend({
- status: z.enum(['draft', 'published', 'archived']).optional(),
+ status: z.enum(['draft']).optional(),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const updateConnectorSchema = createConnectorSchema.partial().omit({ slug: true }).extend({ | |
| status: z.enum(['draft', 'published', 'archived']).optional(), | |
| }); | |
| export const updateConnectorSchema = createConnectorSchema.partial().omit({ slug: true }).extend({ | |
| status: z.enum(['draft']).optional(), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/lib/gateway/admin/validation.ts` around lines 59 - 61, The
generic update schema updateConnectorSchema currently allows status:
'published', which lets the generic PUT handler write publish state directly;
change the enum in updateConnectorSchema (the
createConnectorSchema.partial().omit({ slug: true }).extend(...) line) to
exclude 'published' — restrict it to only 'draft' (e.g.,
z.enum(['draft']).optional()) so publish/archive transitions remain handled by
their dedicated endpoints and cannot be set via the generic update path.
Comment on lines
+138
to
+149
| const eps = c.endpoints as Array<Record<string, string>> | undefined; | ||
| if (eps && eps.length > 0) { | ||
| setEndpoints( | ||
| eps.map((ep: Record<string, string>) => ({ | ||
| name: ep.name || '', | ||
| method: ep.method || 'GET', | ||
| path: ep.path || '/', | ||
| upstreamPath: ep.upstreamPath || '/', | ||
| upstreamContentType: ep.upstreamContentType || 'application/json', | ||
| bodyTransform: ep.bodyTransform || 'passthrough', | ||
| })), | ||
| ); |
There was a problem hiding this comment.
Edit mode never reconciles existing endpoints.
You drop endpoint identity when hydrating the form and then always POST every row on save. In edit mode that means remove/rename changes can't be applied correctly: unchanged rows become ignored 409s, edited rows create additional endpoints, and deleted rows remain on the connector. Please diff existing endpoints and issue create/update/delete calls here, or make the endpoints step read-only until that path exists.
Also applies to: 315-323
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/frontend/src/pages/ConnectorWizardPage.tsx` around
lines 138 - 149, The current hydration in ConnectorWizardPage (the eps ->
setEndpoints mapping) throws away endpoint identity causing every save to POST
and breaking edit-mode diffs; change the hydration to preserve unique
identifiers (e.g., id, uuid, or name key present on ep) into the endpoint
objects used in state (keep the original id field on each item when mapping in
setEndpoints and also in the other mapping at lines ~315-323), then implement a
diff-on-save in the save handler that compares existingConnector.endpoints to
the current endpoints state and issues create (POST) for new items, update
(PUT/PATCH) for items whose id exists but changed, and delete (DELETE) for ids
removed. Ensure the save logic references the same identifier field you
preserved so unchanged rows result in update/no-op rather than new creates, and
consider making the endpoints step read-only if identifier-preservation cannot
be guaranteed.
Comment on lines
233
to
+238
| const handleAuthTypeChange = useCallback((newAuthType: string) => { | ||
| setAuthType(newAuthType); | ||
| if (selectedTemplateIds.size === 0) { | ||
| if (!selectedTemplateId) { | ||
| setSecretRefs(DEFAULT_SECRET_REFS[newAuthType] || []); | ||
| } | ||
| }, [selectedTemplateIds]); | ||
| }, [selectedTemplateId]); |
There was a problem hiding this comment.
Template-derived auth can save the wrong secrets.
Because selectedTemplateId stays truthy after step 0, this guard freezes secretRefs even after the user manually changes authType. A simple repro is: choose a bearer template, switch to basic auth, then save — the wizard still submits ['token'], so the username/password inputs never make it into /secrets.
🛠️ Suggested fix
const handleAuthTypeChange = useCallback((newAuthType: string) => {
setAuthType(newAuthType);
- if (!selectedTemplateId) {
- setSecretRefs(DEFAULT_SECRET_REFS[newAuthType] || []);
- }
-}, [selectedTemplateId]);
+ setSecretRefs(DEFAULT_SECRET_REFS[newAuthType] || []);
+}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/frontend/src/pages/ConnectorWizardPage.tsx` around
lines 233 - 238, handleAuthTypeChange currently avoids updating secretRefs when
selectedTemplateId is truthy, which leaves template-derived secretRefs frozen
and causes wrong secrets to be saved if the user manually changes auth type;
update handleAuthTypeChange so it always calls
setSecretRefs(DEFAULT_SECRET_REFS[newAuthType] || []) when auth type changes
(i.e., remove the selectedTemplateId guard) so secretRefs reflect the newly
chosen authType; reference the handleAuthTypeChange function, setSecretRefs
call, and DEFAULT_SECRET_REFS constant when making the change.
Comment on lines
+277
to
+299
| if (editId) { | ||
| await api.put(`/connectors/${editId}`, { | ||
| displayName, | ||
| description, | ||
| visibility, | ||
| upstreamBaseUrl, | ||
| authType, | ||
| healthCheckPath: healthCheckPath || undefined, | ||
| streamingEnabled, | ||
| }); | ||
| connectorId = editId; | ||
| } else { | ||
| const connRes = await api.post<{ success: boolean; data: { id: string } }>('/connectors', { | ||
| slug, | ||
| displayName, | ||
| description, | ||
| visibility, | ||
| upstreamBaseUrl, | ||
| authType, | ||
| healthCheckPath: healthCheckPath || undefined, | ||
| streamingEnabled, | ||
| secretRefs, | ||
| }); |
There was a problem hiding this comment.
Edit mode drops secretRefs updates.
The create payload persists secretRefs, but the edit PUT omits them while the rest of the save flow still uses the current secretRefs state. If someone changes auth while editing, the connector can end up with the new authType and new secret values, but stale required-secret metadata. Please persist secretRefs on the edit path too, or route this through a dedicated secret-settings update.
Also applies to: 304-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/frontend/src/pages/ConnectorWizardPage.tsx` around
lines 277 - 299, The edit path for ConnectorWizardPage.tsx currently calls
api.put(`/connectors/${editId}`, ...) without including secretRefs, so edits
that change auth/secret values leave secret metadata stale; update the PUT
payload in the edit branch (the api.put call when editId is truthy) to include
secretRefs (same key used in the create POST) or instead call the dedicated
secret update endpoint (e.g., updateConnectorSecrets or similar) after the PUT
to persist secretRefs/secret metadata—ensure you reference the editId,
secretRefs variable, and the api.put call when making this change.
Comment on lines
+763
to
779
| onClick={() => { | ||
| const minStep = editId ? 1 : 0; | ||
| if (step > minStep) setStep(step - 1); | ||
| else if (editId) navigate(`/connectors/${editId}`); | ||
| else navigate('/'); | ||
| }} | ||
| className="px-4 py-2 text-gray-400 hover:text-gray-200 text-sm transition-colors" | ||
| > | ||
| {step > 0 ? '← Back' : '← Cancel'} | ||
| {(editId ? step > 1 : step > 0) ? '← Back' : '← Cancel'} | ||
| </button> | ||
| <div className="flex gap-3"> | ||
| {step === 0 && ( | ||
| <button | ||
| onClick={() => { setSelectedTemplateIds(new Set()); setStep(1); }} | ||
| onClick={() => { setSelectedTemplateId(null); setStep(1); }} | ||
| className="px-4 py-2 text-gray-400 hover:text-gray-200 text-sm transition-colors" | ||
| > | ||
| Skip — Create from Scratch |
There was a problem hiding this comment.
"Create from Scratch" doesn't reset an already applied template.
After a user applies a template, goes back to step 0, and clicks this action, only selectedTemplateId is cleared. The wizard still carries the template's slug, upstream URL, secret refs, and prefilled endpoints forward, so this path is not actually starting from scratch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/frontend/src/pages/ConnectorWizardPage.tsx` around
lines 763 - 779, The "Skip — Create from Scratch" onClick handler only clears
selectedTemplateId but leaves other template-derived state (slug, upstream URL,
secret refs, prefilled endpoints) intact; update the onClick callback in
ConnectorWizardPage to also clear all template-related state — e.g. reset
selectedTemplateSlug, upstreamUrl, secretRefs, endpoints and any prefilling/form
state (or call the existing resetForm/resetConnectorState helper if available)
before calling setStep(1) so the wizard truly starts from scratch.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| const { category, page = 1, pageSize = 50 } = options || {}; | ||
| const skip = (page - 1) * pageSize; | ||
|
|
||
| const where: Record<string, unknown> = { |
Contributor
There was a problem hiding this comment.
The buildToolCatalog function's visibility fix only handles team scopes, not personal scopes - users with personal scopes cannot see their own connectors in the catalog.
| const format = searchParams.get('format') || 'native'; | ||
| const requestId = request.headers.get('x-request-id'); | ||
| const traceId = request.headers.get('x-trace-id'); | ||
|
|
Contributor
There was a problem hiding this comment.
Single catalog endpoint uses { teamId: auth.teamId } filter which fails for personal-scope users whose connectors use ownerUserId instead of teamId
|
|
||
| const rows = await prisma.connectorCapabilityRanking.findMany({ | ||
| where: { | ||
| category, |
Contributor
There was a problem hiding this comment.
Rankings endpoint ownership filter doesn't properly handle personal scope users - uses teamId instead of ownerUserId for personal connectors
|
|
||
| const connectors = await prisma.serviceConnector.findMany({ | ||
| where: { status: 'published' }, | ||
| include: { pricing: true }, |
Contributor
There was a problem hiding this comment.
Users in personal scope cannot see their own private connectors' pricing because { teamId: auth.teamId } query doesn't match personal connectors which use ownerUserId field.
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (2)
apps/web-next/src/lib/gateway/idempotency.ts (1)
51-59:⚠️ Potential issue | 🟠 MajorBind idempotency entries to the full request fingerprint.
buildKey()now includes the method, but two same-key mutations to the same path with different bodies or query strings still share one cache entry. The second request will replay the first result even though it represents a different operation. Include a canonical body/query hash, or store and compare the fingerprint alongside the cached response before returning a hit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/lib/gateway/idempotency.ts` around lines 51 - 59, buildKey currently distinguishes by teamId, connectorSlug, method, endpointPath and idempotencyKey but not by request fingerprint (body/query), causing distinct requests to collide; modify buildKey in idempotency.ts (function buildKey) to incorporate a canonical request fingerprint (e.g., a hash of normalized body + sorted query string) into the returned key, or alternatively store the fingerprint alongside the cached response and, in the lookup path that uses buildKey, compare the stored fingerprint to the incoming request fingerprint before returning a cached hit; ensure the fingerprint is computed deterministically (normalize JSON bodies, sort query params) and referenced wherever buildKey and the idempotency lookup/restore logic run.plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx (1)
1395-1429:⚠️ Potential issue | 🟠 MajorDon't unlock these editors after a failed bootstrap fetch.
Both editors call
setLoaded(true)incatch, so a transient 403/500 renders empty/default fields while save stays enabled. From there,handleSave()can overwrite existing pricing or agent metadata with blanks. Track a separate load-error state and keep saving disabled until the initial GET succeeds.Also applies to: 1510-1546
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around lines 1395 - 1429, The effect currently sets setLoaded(true) in the catch block so a failed GET makes the UI think loading succeeded and enables saving; create a separate load-success flag (e.g., loadedSuccess or loadError boolean) and only set it true on successful api.get response inside the useEffect (keep setLoaded for the spinner if you want), do NOT set loadedSuccess when the fetch fails, and change handleSave (and any save button disabled logic that references loaded) to check loadedSuccess (or !loadError) before proceeding so saves are disabled until the initial GET for /connectors/${connectorId}/pricing succeeds; update references to setLoaded, handleSave, api.get and connectorId accordingly.
🧹 Nitpick comments (1)
apps/web-next/src/lib/gateway/__tests__/catalog.test.ts (1)
135-137: Add one populated performance-mapping case.The suite only verifies the
nullfallback forperformance. Since catalog performance metrics are a first-class part of this PR, one non-null assertion here would protect the latency/availability/throughput mapping from regressing silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/lib/gateway/__tests__/catalog.test.ts` around lines 135 - 137, Add a test that covers a non-null performance mapping by creating a connector with explicit performance metrics, calling buildToolDescriptor(makeConnectorWithMetrics(...), '/api/v1/gw'), and asserting result.performance is not null and that its latency/availability/throughput fields equal the expected mapped values; update or add a helper (e.g., makeConnector or makeConnectorWithMetrics) to inject sample metrics and use result.performance.latency, result.performance.availability and result.performance.throughput in the expectations to guard the mapping in buildToolDescriptor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web-next/src/app/api/v1/gw/`[connector]/[...path]/route.ts:
- Around line 184-195: The current logic only reads completed cached responses
via checkIdempotency and writes the cache after proxyToUpstream, which allows
two concurrent requests with the same idempotency-key to both miss and hit
upstream; change this to perform an atomic reserve-before-proxy: call a new or
extended function (e.g., reserveIdempotencyKey or checkAndReserveIdempotency)
that atomically checks for an existing completed response and, if absent,
inserts a pending/in-flight marker tied to the idempotencyKey, scopeId, slug,
consumerPath and method; only the request that successfully reserves proceeds to
call proxyToUpstream, and on completion call finalizeIdempotency(cacheEntry,
response) to replace the pending marker with the completed cached response (or
clear it on error); also modify the post-response fire-and-forget write logic
used elsewhere (the other idempotency block around proxyToUpstream) to use the
same reserve/finalize helpers so concurrent retries are serialized.
In `@apps/web-next/src/app/api/v1/gw/catalog/`[slug]/metrics/route.ts:
- Around line 30-33: The current prisma.serviceConnector.findFirst call mixes
owner/team/public scopes and can pick the wrong row; replace it with a
scoped-precedence lookup: first query for a published connector with slug and
ownerUserId === auth.userId, if not found query for a published connector with
slug and teamId === auth.teamId, and finally if still not found query for a
published connector with slug and visibility === 'public'; update the code paths
around prisma.serviceConnector.findFirst to use these three ordered lookups
(using auth.userId, auth.teamId, visibility and ownerUserId) so
owner/team/public precedence matches the catalog lookup.
In `@apps/web-next/src/app/api/v1/gw/catalog/`[slug]/rankings/route.ts:
- Around line 27-30: The connector lookup using
prisma.serviceConnector.findFirst is nondeterministic and leaks visibility;
replace it with a scope-aware resolution that enforces explicit precedence
(owner-scoped > same-team > public) before loading rankings: perform ordered
checks (e.g., first query for connector where slug matches and ownerId ==
auth.userId and status == 'published', then if not found query where slug
matches and teamId == auth.teamId and status == 'published', then finally where
slug matches and visibility == 'public' and status == 'published'), select the
same fields (id, slug, displayName) and use the first successful result as
connector; ensure owner-scoped connectors are included and that duplicate slugs
across scopes resolve deterministically before proceeding to read rankings.
- Around line 38-54: The rankings query returns full ConnectorCapabilityRanking
rows and leaks internal fields; update the
prisma.connectorCapabilityRanking.findMany call (and the rankings variable) to
only select consumer-facing fields (e.g., qualityRank and the public capability
identifiers/names you expose) instead of all columns, omitting rankedBy, notes,
connectorId, createdAt/updatedAt, etc., and return that trimmed shape in
Response.json so only intended fields are exposed.
In `@apps/web-next/src/lib/gateway/metrics.ts`:
- Around line 18-35: The code treats non-7d windows as hourly, so
/metrics?window=24h fetches an hourly snapshot; update the logic for the 24h
case so it returns a true 24-hour rollup: either query the daily rollup via
prisma.connectorMetrics.findFirst/ findMany with period: 'daily' (or the DB’s
daily period string) and pass that row into summarizeMetricsForDescriptor, or
fetch the last 24 rows with period: 'hourly' (where window === '24h'), orderBy
periodStart desc, take 24, and then call aggregateMetrics(rows, '24h') before
returning; adjust the conditional branching around window, the prisma queries
(connectorMetrics.findFirst/findMany) and the callers aggregateMetrics and
summarizeMetricsForDescriptor accordingly.
In `@packages/database/prisma/schema.prisma`:
- Around line 1889-1891: authorizeMasterKey currently only verifies
hash/status/expiry so master keys with prefix "gwm_" bypass scope and IP
restrictions; update the authorizeMasterKey implementation to enforce the
`scopes` and `allowedIPs` fields on the loaded masterKey: (1) if
masterKey.scopes is non-empty, verify the requested scope (e.g., requestedScope
or operation name passed into authorizeMasterKey) is present in masterKey.scopes
and reject if not; (2) if masterKey.allowedIPs is non-empty, verify the client's
IP (clientIp or request.ip passed into authorizeMasterKey) matches at least one
allowed entry (support exact IP and CIDR/mask checking using an IP utility) and
reject if not; ensure failures return the same unauthorized/error path as other
checks so the key is denied when restrictions are violated.
- Around line 1944-1961: The uniqueness rule is leaky because modelName is
nullable but used in @@unique([connectorId, category, modelName]); fix by
normalizing "no model name" to a non-null sentinel or making modelName required:
either change modelName from String? to String with a default sentinel (e.g.,
set modelName String `@default`("") and update application write paths to convert
null/undefined -> ""), or make it required (modelName String) and ensure all
inserts/upserts supply a value; keep the @@unique([connectorId, category,
modelName]) constraint and update any places that currently write null modelName
to use the chosen sentinel/value.
In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx`:
- Around line 1387-1426: The PricingTab form is dropping volumeTiers and
featurePricing because the GET populates only scalar fields and the PUT payload
omits those arrays (api.put in handleSave), causing the server to default them
to []; update the useEffect that loads `/connectors/${connectorId}/pricing` to
capture p.volumeTiers and p.featurePricing into component state (extend
PricingFormData to include volumeTiers and featurePricing) and then include
those same arrays in the api.put payload in handleSave when sending the update
(preserve existing arrays unless the UI explicitly edits them); ensure the field
names match the backend schema (volumeTiers, featurePricing) and that
parsing/typing is handled when setting and submitting the form.
- Around line 1279-1291: The PerformanceMetrics interface in
ConnectorDetailPage.tsx currently types latencyP50Ms, latencyP95Ms and
latencyP99Ms as number which forces null 7d percentiles to render as 0ms; change
those fields to number | null (and any other percentile fields that can be null)
and update any rendering logic that uses latencyP50Ms/latencyP95Ms/latencyP99Ms
(and similar percentile usages elsewhere in this file) to treat null as
"unavailable" instead of formatting/rounding to 0ms (e.g., branch before
formatting or use a helper that returns a placeholder when value === null).
In `@plugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx`:
- Around line 48-56: The input validation in handleCreate (and the similar
create path around lines 118-123) uses raw newKeyName which treats
whitespace-only strings as valid; trim newKeyName before checking/submitting by
replacing uses of newKeyName with newKeyName.trim() for the empty-check and when
sending the payload, and ensure you update any state or error messages
accordingly (e.g., in handleCreate and the secondary create flow referenced) so
that a name of only spaces is rejected client-side before the API call.
- Around line 69-90: handleRotate and handleRevoke lack an in-flight guard so
multiple clicks trigger duplicate requests; implement a per-key in-flight guard
(e.g., a React state Set/Map like processingKeys) and in both
handleRotate(keyId) and handleRevoke(keyId) check the guard at the start, return
early if already processing, then mark processingKeys.add(keyId) before the API
call and remove it in finally; keep existing behavior (call loadKeys,
setCreatedKey, setActionError) but ensure the UI row action buttons are disabled
when processingKeys has the keyId (also apply the same guard to the handlers
referenced around lines 208-233).
- Around line 31-46: The page currently hides fetch failures by letting keys
collapse to []; update loadKeys (which calls execute) to catch and surface
errors: call execute(() => api.get('/master-keys')).catch(err =>
setActionError(String(err?.message || err))) and ensure you clear actionError on
successful fetch; also change any downstream derivation of keys (where you use
data?.data ?? []) to preserve a null/undefined state (e.g., use data?.data ??
null) and render an explicit error state when actionError is set instead of
showing "No master keys found." This same pattern should be applied to the other
fetch usage referenced (the block around lines 184-193) so failures set
actionError and do not silently render an empty list.
---
Duplicate comments:
In `@apps/web-next/src/lib/gateway/idempotency.ts`:
- Around line 51-59: buildKey currently distinguishes by teamId, connectorSlug,
method, endpointPath and idempotencyKey but not by request fingerprint
(body/query), causing distinct requests to collide; modify buildKey in
idempotency.ts (function buildKey) to incorporate a canonical request
fingerprint (e.g., a hash of normalized body + sorted query string) into the
returned key, or alternatively store the fingerprint alongside the cached
response and, in the lookup path that uses buildKey, compare the stored
fingerprint to the incoming request fingerprint before returning a cached hit;
ensure the fingerprint is computed deterministically (normalize JSON bodies,
sort query params) and referenced wherever buildKey and the idempotency
lookup/restore logic run.
In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx`:
- Around line 1395-1429: The effect currently sets setLoaded(true) in the catch
block so a failed GET makes the UI think loading succeeded and enables saving;
create a separate load-success flag (e.g., loadedSuccess or loadError boolean)
and only set it true on successful api.get response inside the useEffect (keep
setLoaded for the spinner if you want), do NOT set loadedSuccess when the fetch
fails, and change handleSave (and any save button disabled logic that references
loaded) to check loadedSuccess (or !loadError) before proceeding so saves are
disabled until the initial GET for /connectors/${connectorId}/pricing succeeds;
update references to setLoaded, handleSave, api.get and connectorId accordingly.
---
Nitpick comments:
In `@apps/web-next/src/lib/gateway/__tests__/catalog.test.ts`:
- Around line 135-137: Add a test that covers a non-null performance mapping by
creating a connector with explicit performance metrics, calling
buildToolDescriptor(makeConnectorWithMetrics(...), '/api/v1/gw'), and asserting
result.performance is not null and that its latency/availability/throughput
fields equal the expected mapped values; update or add a helper (e.g.,
makeConnector or makeConnectorWithMetrics) to inject sample metrics and use
result.performance.latency, result.performance.availability and
result.performance.throughput in the expectations to guard the mapping in
buildToolDescriptor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: abf58aea-b603-4e5a-89b7-6f76b5e00362
📒 Files selected for processing (19)
apps/web-next/src/app/api/v1/gw/[connector]/[...path]/route.tsapps/web-next/src/app/api/v1/gw/admin/metrics/aggregate/route.tsapps/web-next/src/app/api/v1/gw/catalog/[slug]/metrics/route.tsapps/web-next/src/app/api/v1/gw/catalog/[slug]/rankings/route.tsapps/web-next/src/app/api/v1/gw/discovery/route.tsapps/web-next/src/app/api/v1/gw/mcp/route.tsapps/web-next/src/app/api/v1/gw/pricing/[slug]/estimate/route.tsapps/web-next/src/app/api/v1/gw/pricing/[slug]/route.tsapps/web-next/src/app/api/v1/gw/pricing/route.tsapps/web-next/src/app/api/v1/gw/rankings/route.tsapps/web-next/src/lib/gateway/__tests__/catalog.test.tsapps/web-next/src/lib/gateway/__tests__/idempotency.test.tsapps/web-next/src/lib/gateway/admin/validation.tsapps/web-next/src/lib/gateway/catalog.tsapps/web-next/src/lib/gateway/idempotency.tsapps/web-next/src/lib/gateway/metrics.tspackages/database/prisma/schema.prismaplugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsxplugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/web-next/src/app/api/v1/gw/pricing/[slug]/route.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/web-next/src/app/api/v1/gw/admin/metrics/aggregate/route.ts
- apps/web-next/src/lib/gateway/catalog.ts
- apps/web-next/src/app/api/v1/gw/rankings/route.ts
- apps/web-next/src/app/api/v1/gw/discovery/route.ts
- apps/web-next/src/app/api/v1/gw/pricing/route.ts
- apps/web-next/src/app/api/v1/gw/pricing/[slug]/estimate/route.ts
- apps/web-next/src/lib/gateway/admin/validation.ts
- apps/web-next/src/app/api/v1/gw/mcp/route.ts
Comment on lines
+184
to
+195
| // ── 9b. Idempotency Check (mutating methods only) ── | ||
| const idempotencyKey = request.headers.get('idempotency-key'); | ||
| if (idempotencyKey && ['POST', 'PUT', 'PATCH', 'DELETE'].includes(method)) { | ||
| const cached = await checkIdempotency(scopeId, slug, consumerPath, idempotencyKey, method); | ||
| if (cached) { | ||
| const buf = Buffer.from(cached.body, 'base64'); | ||
| return new Response(buf, { | ||
| status: cached.status, | ||
| headers: { ...cached.headers, 'X-Idempotent-Replayed': 'true' }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is replay caching, not safe idempotency for concurrent retries.
The key is only checked against completed responses; nothing reserves it before proxyToUpstream(). Two same-key mutations arriving close together will both miss the cache and both execute upstream, and the post-response fire-and-forget write leaves an extra race for fast retries. Reserve the key atomically before proxying (pending/in-flight state), then publish the cached response only when the first request finishes.
Also applies to: 326-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/app/api/v1/gw/`[connector]/[...path]/route.ts around lines
184 - 195, The current logic only reads completed cached responses via
checkIdempotency and writes the cache after proxyToUpstream, which allows two
concurrent requests with the same idempotency-key to both miss and hit upstream;
change this to perform an atomic reserve-before-proxy: call a new or extended
function (e.g., reserveIdempotencyKey or checkAndReserveIdempotency) that
atomically checks for an existing completed response and, if absent, inserts a
pending/in-flight marker tied to the idempotencyKey, scopeId, slug, consumerPath
and method; only the request that successfully reserves proceeds to call
proxyToUpstream, and on completion call finalizeIdempotency(cacheEntry,
response) to replace the pending marker with the completed cached response (or
clear it on error); also modify the post-response fire-and-forget write logic
used elsewhere (the other idempotency block around proxyToUpstream) to use the
same reserve/finalize helpers so concurrent retries are serialized.
Comment on lines
+30
to
+33
| const connector = await prisma.serviceConnector.findFirst({ | ||
| where: { slug, status: 'published', OR: [{ visibility: 'public' }, { teamId: auth.teamId }] }, | ||
| select: { id: true, slug: true, displayName: true }, | ||
| }); |
There was a problem hiding this comment.
Mirror the full visibility/scope lookup here.
This predicate still treats every same-team connector as visible, even if it is marked private, and it cannot match owner-scoped connectors because it never checks ownerUserId. Since ServiceConnector.slug is only unique within owner/team scope, this unordered findFirst can also pick the wrong published row when a public connector shares the slug. Use the same owner/team/public precedence as the catalog lookup instead of findFirst over mixed scopes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/app/api/v1/gw/catalog/`[slug]/metrics/route.ts around lines
30 - 33, The current prisma.serviceConnector.findFirst call mixes
owner/team/public scopes and can pick the wrong row; replace it with a
scoped-precedence lookup: first query for a published connector with slug and
ownerUserId === auth.userId, if not found query for a published connector with
slug and teamId === auth.teamId, and finally if still not found query for a
published connector with slug and visibility === 'public'; update the code paths
around prisma.serviceConnector.findFirst to use these three ordered lookups
(using auth.userId, auth.teamId, visibility and ownerUserId) so
owner/team/public precedence matches the catalog lookup.
Comment on lines
+27
to
+30
| const connector = await prisma.serviceConnector.findFirst({ | ||
| where: { slug, status: 'published', OR: [{ visibility: 'public' }, { teamId: auth.teamId }] }, | ||
| select: { id: true, slug: true, displayName: true }, | ||
| }); |
There was a problem hiding this comment.
Use a scope-aware connector lookup before reading rankings.
This findFirst has the same visibility leak as the metrics route: same-team callers can match published connectors that are still private, owner-scoped connectors are excluded, and duplicate slugs across public/team scopes make the result nondeterministic. Resolve the connector with explicit owner/team/public precedence before loading rankings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/app/api/v1/gw/catalog/`[slug]/rankings/route.ts around
lines 27 - 30, The connector lookup using prisma.serviceConnector.findFirst is
nondeterministic and leaks visibility; replace it with a scope-aware resolution
that enforces explicit precedence (owner-scoped > same-team > public) before
loading rankings: perform ordered checks (e.g., first query for connector where
slug matches and ownerId == auth.userId and status == 'published', then if not
found query where slug matches and teamId == auth.teamId and status ==
'published', then finally where slug matches and visibility == 'public' and
status == 'published'), select the same fields (id, slug, displayName) and use
the first successful result as connector; ensure owner-scoped connectors are
included and that duplicate slugs across scopes resolve deterministically before
proceeding to read rankings.
Comment on lines
+38
to
+54
| const rankings = await prisma.connectorCapabilityRanking.findMany({ | ||
| where: { connectorId: connector.id }, | ||
| orderBy: { qualityRank: 'asc' }, | ||
| }); | ||
|
|
||
| const headers: Record<string, string> = {}; | ||
| const requestId = request.headers.get('x-request-id'); | ||
| const traceId = request.headers.get('x-trace-id'); | ||
| if (requestId) headers['x-request-id'] = requestId; | ||
| if (traceId) headers['x-trace-id'] = traceId; | ||
|
|
||
| return Response.json( | ||
| { | ||
| connector: { id: connector.id, slug: connector.slug, displayName: connector.displayName }, | ||
| rankings, | ||
| }, | ||
| { headers } |
There was a problem hiding this comment.
Trim the rankings payload to consumer-facing fields.
findMany() without a select returns the entire ConnectorCapabilityRanking row, which includes internal fields like rankedBy, notes, connectorId, and bookkeeping timestamps. That leaks more than the catalog API needs to expose.
✂️ Suggested payload shape
const rankings = await prisma.connectorCapabilityRanking.findMany({
where: { connectorId: connector.id },
orderBy: { qualityRank: 'asc' },
+ select: {
+ category: true,
+ modelName: true,
+ qualityRank: true,
+ qualityScore: true,
+ speedRank: true,
+ costEfficiencyRank: true,
+ totalRanked: true,
+ benchmarkSource: true,
+ benchmarkScore: true,
+ benchmarkUrl: true,
+ capabilityTags: true,
+ lastRankedAt: true,
+ },
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/app/api/v1/gw/catalog/`[slug]/rankings/route.ts around
lines 38 - 54, The rankings query returns full ConnectorCapabilityRanking rows
and leaks internal fields; update the prisma.connectorCapabilityRanking.findMany
call (and the rankings variable) to only select consumer-facing fields (e.g.,
qualityRank and the public capability identifiers/names you expose) instead of
all columns, omitting rankedBy, notes, connectorId, createdAt/updatedAt, etc.,
and return that trimmed shape in Response.json so only intended fields are
exposed.
Comment on lines
+1387
to
+1426
| function PricingTab({ connectorId, api }: { connectorId: string; api: ReturnType<typeof useGatewayApi> }) { | ||
| const [form, setForm] = useState<PricingFormData>({ | ||
| upstreamCostPerUnit: '', upstreamUnit: '', upstreamNotes: '', | ||
| costPerUnit: '0', unit: 'request', currency: 'USD', billingModel: 'per-unit', freeQuota: '', | ||
| }); | ||
| const [saving, setSaving] = useState(false); | ||
| const [loaded, setLoaded] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| api.get(`/connectors/${connectorId}/pricing`).then((res: { success: boolean; data: Record<string, unknown> | null }) => { | ||
| if (res.data) { | ||
| const p = res.data; | ||
| setForm({ | ||
| upstreamCostPerUnit: p.upstreamCostPerUnit != null ? String(p.upstreamCostPerUnit) : '', | ||
| upstreamUnit: (p.upstreamUnit as string) || '', | ||
| upstreamNotes: (p.upstreamNotes as string) || '', | ||
| costPerUnit: String(p.costPerUnit ?? 0), | ||
| unit: (p.unit as string) || 'request', | ||
| currency: (p.currency as string) || 'USD', | ||
| billingModel: (p.billingModel as string) || 'per-unit', | ||
| freeQuota: p.freeQuota != null ? String(p.freeQuota) : '', | ||
| }); | ||
| } | ||
| setLoaded(true); | ||
| }).catch(() => setLoaded(true)); | ||
| }, [api, connectorId]); | ||
|
|
||
| const handleSave = async () => { | ||
| setSaving(true); | ||
| try { | ||
| await api.put(`/connectors/${connectorId}/pricing`, { | ||
| upstreamCostPerUnit: form.upstreamCostPerUnit ? parseFloat(form.upstreamCostPerUnit) : undefined, | ||
| upstreamUnit: form.upstreamUnit || undefined, | ||
| upstreamNotes: form.upstreamNotes || undefined, | ||
| costPerUnit: parseFloat(form.costPerUnit) || 0, | ||
| unit: form.unit, | ||
| currency: form.currency, | ||
| billingModel: form.billingModel, | ||
| freeQuota: form.freeQuota ? parseInt(form.freeQuota) : undefined, | ||
| }); |
There was a problem hiding this comment.
Saving this editor wipes tiered and feature pricing.
The admin pricing PUT schema defaults volumeTiers and featurePricing to [], but this form never loads or resubmits either field. Editing any scalar value here will silently erase existing tier definitions and feature-specific prices. Either expose those fields in the UI or round-trip the existing arrays unchanged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around
lines 1387 - 1426, The PricingTab form is dropping volumeTiers and
featurePricing because the GET populates only scalar fields and the PUT payload
omits those arrays (api.put in handleSave), causing the server to default them
to []; update the useEffect that loads `/connectors/${connectorId}/pricing` to
capture p.volumeTiers and p.featurePricing into component state (extend
PricingFormData to include volumeTiers and featurePricing) and then include
those same arrays in the api.put payload in handleSave when sending the update
(preserve existing arrays unless the UI explicitly edits them); ensure the field
names match the backend schema (volumeTiers, featurePricing) and that
parsing/typing is handled when setting and submitting the form.
Comment on lines
+31
to
+46
| const { data, loading, execute } = useAsync<{ success: boolean; data: MasterKey[] }>(); | ||
| const [newKeyName, setNewKeyName] = useState(''); | ||
| const [selectedScopes, setSelectedScopes] = useState<string[]>(['proxy']); | ||
| const [createdKey, setCreatedKey] = useState<string | null>(null); | ||
| const [creating, setCreating] = useState(false); | ||
| const [actionError, setActionError] = useState(''); | ||
| const [copied, setCopied] = useState(false); | ||
| const [confirmRevokeId, setConfirmRevokeId] = useState<string | null>(null); | ||
|
|
||
| const loadKeys = useCallback(() => { | ||
| return execute(() => api.get('/master-keys')); | ||
| }, [execute, api]); | ||
|
|
||
| useEffect(() => { | ||
| loadKeys(); | ||
| }, [loadKeys]); |
There was a problem hiding this comment.
Surface load failures instead of falling back to "No master keys found."
This page never renders an initial-load error state, and keys collapses to [] when the fetch result is absent. On a 403/network failure, users can be shown an empty catalog instead of a failure, which is especially risky here because it makes missing keys look real and can lead to accidental duplicate creation.
Also applies to: 184-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx` around lines
31 - 46, The page currently hides fetch failures by letting keys collapse to [];
update loadKeys (which calls execute) to catch and surface errors: call
execute(() => api.get('/master-keys')).catch(err =>
setActionError(String(err?.message || err))) and ensure you clear actionError on
successful fetch; also change any downstream derivation of keys (where you use
data?.data ?? []) to preserve a null/undefined state (e.g., use data?.data ??
null) and render an explicit error state when actionError is set instead of
showing "No master keys found." This same pattern should be applied to the other
fetch usage referenced (the block around lines 184-193) so failures set
actionError and do not silently render an empty list.
Comment on lines
+48
to
+56
| const handleCreate = async () => { | ||
| if (!newKeyName || selectedScopes.length === 0) return; | ||
| setCreating(true); | ||
| setActionError(''); | ||
| try { | ||
| const res = await api.post<{ success: boolean; data: { rawKey: string } }>('/master-keys', { | ||
| name: newKeyName, | ||
| scopes: selectedScopes, | ||
| }); |
There was a problem hiding this comment.
Trim the key name before validating/submitting it.
!newKeyName treats " " as valid, so the UI can send a create request with a whitespace-only name and then fall back to a generic error if the backend rejects it.
Suggested diff
const handleCreate = async () => {
- if (!newKeyName || selectedScopes.length === 0) return;
+ const trimmedName = newKeyName.trim();
+ if (!trimmedName || selectedScopes.length === 0) return;
setCreating(true);
setActionError('');
try {
const res = await api.post<{ success: boolean; data: { rawKey: string } }>('/master-keys', {
- name: newKeyName,
+ name: trimmedName,
scopes: selectedScopes,
});
@@
- disabled={!newKeyName || selectedScopes.length === 0 || creating}
+ disabled={!newKeyName.trim() || selectedScopes.length === 0 || creating}Also applies to: 118-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx` around lines
48 - 56, The input validation in handleCreate (and the similar create path
around lines 118-123) uses raw newKeyName which treats whitespace-only strings
as valid; trim newKeyName before checking/submitting by replacing uses of
newKeyName with newKeyName.trim() for the empty-check and when sending the
payload, and ensure you update any state or error messages accordingly (e.g., in
handleCreate and the secondary create flow referenced) so that a name of only
spaces is rejected client-side before the API call.
Comment on lines
+69
to
+90
| const handleRevoke = async (keyId: string) => { | ||
| setActionError(''); | ||
| try { | ||
| await api.del(`/master-keys/${keyId}`); | ||
| loadKeys(); | ||
| } catch { | ||
| setActionError('Failed to revoke master key'); | ||
| } | ||
| }; | ||
|
|
||
| const handleRotate = async (keyId: string) => { | ||
| setActionError(''); | ||
| try { | ||
| const res = await api.post<{ success: boolean; data: { rawKey: string } }>(`/master-keys/${keyId}/rotate`); | ||
| if (res.success) { | ||
| setCreatedKey(res.data.rawKey); | ||
| loadKeys(); | ||
| } | ||
| } catch { | ||
| setActionError('Failed to rotate master key'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Prevent duplicate rotate/revoke submissions.
handleRotate and handleRevoke have no in-flight guard, and the row actions stay clickable while the request is pending. Double-clicking rotate can issue multiple rotations and leave the user copying a key that was immediately superseded by the second response.
[suggested fix]
Suggested diff
const [creating, setCreating] = useState(false);
+ const [pendingActionKeyId, setPendingActionKeyId] = useState<string | null>(null);
const [actionError, setActionError] = useState('');
@@
const handleRevoke = async (keyId: string) => {
+ if (pendingActionKeyId) return;
+ setPendingActionKeyId(keyId);
setActionError('');
try {
await api.del(`/master-keys/${keyId}`);
- loadKeys();
+ await loadKeys();
} catch {
setActionError('Failed to revoke master key');
+ } finally {
+ setPendingActionKeyId(null);
}
};
@@
const handleRotate = async (keyId: string) => {
+ if (pendingActionKeyId) return;
+ setPendingActionKeyId(keyId);
setActionError('');
try {
const res = await api.post<{ success: boolean; data: { rawKey: string } }>(`/master-keys/${keyId}/rotate`);
if (res.success) {
setCreatedKey(res.data.rawKey);
- loadKeys();
+ await loadKeys();
}
} catch {
setActionError('Failed to rotate master key');
+ } finally {
+ setPendingActionKeyId(null);
}
};
@@
- <button onClick={() => handleRotate(key.id)} className="text-xs text-blue-400 hover:text-blue-300">
+ <button
+ onClick={() => handleRotate(key.id)}
+ disabled={pendingActionKeyId === key.id}
+ className="text-xs text-blue-400 hover:text-blue-300 disabled:opacity-50"
+ >
Rotate
</button>
@@
- <button
+ <button
onClick={() => { handleRevoke(key.id); setConfirmRevokeId(null); }}
+ disabled={pendingActionKeyId === key.id}
className="text-xs text-red-400 hover:text-red-300 font-medium"
>Also applies to: 208-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx` around lines
69 - 90, handleRotate and handleRevoke lack an in-flight guard so multiple
clicks trigger duplicate requests; implement a per-key in-flight guard (e.g., a
React state Set/Map like processingKeys) and in both handleRotate(keyId) and
handleRevoke(keyId) check the guard at the start, return early if already
processing, then mark processingKeys.add(keyId) before the API call and remove
it in finally; keep existing behavior (call loadKeys, setCreatedKey,
setActionError) but ensure the UI row action buttons are disabled when
processingKeys has the keyId (also apply the same guard to the handlers
referenced around lines 208-233).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx (4)
1400-1412:⚠️ Potential issue | 🟠 MajorRound-trip
volumeTiersandfeaturePricing.
packages/database/prisma/schema.prismadefines both fields onConnectorPricing, but this form only loads and saves the scalar fields. Editing any value here will drop existing tiered or feature-specific pricing unless the server preserves omitted fields. Keep those arrays in component state and resubmit them unchanged until the UI can edit them explicitly.Also applies to: 1421-1430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around lines 1400 - 1412, The form currently replaces ConnectorPricing with only scalar fields and will drop array fields volumeTiers and featurePricing; update the load and save logic in ConnectorDetailPage (where setForm is called and the form state is prepared) to preserve those arrays by reading res.data.volumeTiers and res.data.featurePricing into component state (e.g., include them on the form state alongside upstreamCostPerUnit/etc.) and ensure the submit payload re-includes volumeTiers and featurePricing unchanged if the UI doesn't edit them (also apply the same preservation to the adjacent loading block around lines referenced 1421-1430).
1283-1289:⚠️ Potential issue | 🟠 MajorRender missing percentiles as unavailable.
The 7d metrics payload can return
nullpercentiles, but this local type forces them tonumberand the UI rounds them to0ms. That turns “not computable” into bogus latency data.Suggested fix
interface PerformanceMetrics { errorRate: number; successRate: number; latencyMeanMs: number; - latencyP50Ms: number; - latencyP95Ms: number; - latencyP99Ms: number; + latencyP50Ms: number | null; + latencyP95Ms: number | null; + latencyP99Ms: number | null; upstreamLatencyMeanMs: number; gatewayOverheadMs: number; availabilityPercent: number; throughputRpm: number; sampleSize: number; } + +const formatLatency = (value: number | null) => + value == null ? '—' : `${Math.round(value)}ms`; @@ - <div className="text-gray-200 font-mono">{Math.round(metrics.latencyP50Ms)}ms</div> + <div className="text-gray-200 font-mono">{formatLatency(metrics.latencyP50Ms)}</div> @@ - <div className="text-gray-200 font-mono">{Math.round(metrics.latencyP95Ms)}ms</div> + <div className="text-gray-200 font-mono">{formatLatency(metrics.latencyP95Ms)}</div> @@ - <div className="text-gray-200 font-mono">{Math.round(metrics.latencyP99Ms)}ms</div> + <div className="text-gray-200 font-mono">{formatLatency(metrics.latencyP99Ms)}</div>Also applies to: 1354-1365
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around lines 1283 - 1289, The PerformanceMetrics interface currently types percentile fields (latencyP50Ms, latencyP95Ms, latencyP99Ms, and latencyMeanMs if applicable) as number which forces null payloads to become 0ms in the UI; update the interface (PerformanceMetrics) to allow those fields to be number | null, then update any render/formatting logic that uses those symbols (e.g., where latencyP50Ms, latencyP95Ms, latencyP99Ms are rounded or displayed) to check for null and render an "unavailable"/placeholder string instead of formatting 0ms. Ensure the same change is applied to the duplicate definitions referenced around the other block (lines showing the other PerformanceMetrics usage).
1399-1416:⚠️ Potential issue | 🟠 MajorDon't unlock Pricing after a failed bootstrap fetch.
The catch path marks the form as loaded and leaves save enabled with default values. A transient 403/500 here lets users overwrite existing pricing with blanks/zeros on the next PUT. Track load failure separately and keep saving disabled until the initial GET succeeds.
Also applies to: 1418-1434, 1497-1499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around lines 1399 - 1416, The GET in the useEffect currently calls setLoaded(true) in both success and catch, which enables the form/save after a failed bootstrap; change the logic to track load success separately (e.g., add a boolean state like loadSucceeded or loadError) and only call setForm and mark load success when the API returns data in the then branch; in the catch branch set the new failure flag (don't setLoaded(true) or set it to false) so the Save control remains disabled until loadSucceeded is true; update any save-enabling checks (wherever save button/submit is gated) to require loadSucceeded (and setLoaded if you still need a loading spinner) rather than just setLoaded.
1514-1523:⚠️ Potential issue | 🟠 MajorBlock agent-metadata saves until the initial load succeeds.
This has the same bootstrap problem as
PricingTab:.catch(() => setLoaded(true))renders an empty form after a failed GET, and save can then clearagentDescription,agentNotFor, or the schemas. Keep a separate load-error state and disable save when the initial fetch did not succeed.Also applies to: 1527-1549, 1577-1579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx` around lines 1514 - 1523, The initial GET useEffect sets loaded=true in the catch path which lets the form render empty and allows saves to overwrite agent metadata; add a separate loadError (e.g., const [loadError, setLoadError] = useState<boolean>(false)) and change the GET handler in the useEffect that calls api.get(`/connectors/${connectorId}`) so that on success you set the agent fields via setAgentDescription/setAgentNotFor/setInputSchemaStr/setOutputSchemaStr and setLoaded(true), and on failure you setLoadError(true) (and do not setLoaded(true)). Update any save handler (e.g., handleSave/onSave) and UI save buttons to disable saving when !loaded || loadError, and apply the same pattern to the other identical useEffect blocks referenced (around lines 1527-1549 and 1577-1579) so initial load failures prevent accidental clears.
🧹 Nitpick comments (1)
plugins/service-gateway/frontend/src/components/GatewayNav.tsx (1)
21-44: Consider simplifying ARIA semantics for navigation links.The current implementation uses the tabs ARIA pattern (
role="tablist",role="tab",aria-selected) on what are effectively navigation links that trigger route changes. The tabs pattern technically expects associatedtabpanelelements witharia-controls/aria-labelledbyrelationships.Since these buttons navigate to different pages rather than switching in-place content panels, the implicit navigation role from
<nav>witharia-labelmay be sufficient. You could usearia-current="page"alone (which you already have) and omit the tabs-specific roles.That said, this is a common pattern in modern UIs and screen readers generally handle it well.
💡 Optional: Simplify to pure navigation semantics
- <nav aria-label="Service Gateway" className="flex border-b border-gray-800 px-6 bg-gray-900/50 shrink-0" role="tablist"> + <nav aria-label="Service Gateway" className="flex border-b border-gray-800 px-6 bg-gray-900/50 shrink-0"> {NAV_ITEMS.map((item) => { const active = item.match(location.pathname); return ( <button key={item.label} - role="tab" - aria-selected={active} aria-current={active ? 'page' : undefined} onClick={() => navigate(item.path)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/components/GatewayNav.tsx` around lines 21 - 44, The component GatewayNav.tsx currently applies tabs ARIA roles (role="tablist" on the <nav> and role="tab" plus aria-selected on the item buttons) even though NAV_ITEMS triggers route navigation; remove the tabs-specific attributes: delete role="tablist" from the <nav> and remove role="tab" and aria-selected from the item elements, keep the existing aria-label on the <nav> and preserve aria-current={active ? 'page' : undefined} on each item (and retain the onClick/navigate logic or convert to anchors if you prefer) so the markup uses standard navigation semantics instead of the tab pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/service-gateway/frontend/src/components/GatewayNav.tsx`:
- Around line 9-14: NAV_ITEMS in GatewayNav.tsx is missing an entry for the API
Keys page, so add a new item to the NAV_ITEMS array: include { label: 'API
Keys', path: '/keys', match: (p: string) => p === '/keys' } alongside the
existing entries (e.g., the ones for 'Connectors', 'Dashboard', 'Master Keys',
'Plans') so the tab bar shows and routes to the /keys (ApiKeysPage) route.
---
Duplicate comments:
In `@plugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx`:
- Around line 1400-1412: The form currently replaces ConnectorPricing with only
scalar fields and will drop array fields volumeTiers and featurePricing; update
the load and save logic in ConnectorDetailPage (where setForm is called and the
form state is prepared) to preserve those arrays by reading res.data.volumeTiers
and res.data.featurePricing into component state (e.g., include them on the form
state alongside upstreamCostPerUnit/etc.) and ensure the submit payload
re-includes volumeTiers and featurePricing unchanged if the UI doesn't edit them
(also apply the same preservation to the adjacent loading block around lines
referenced 1421-1430).
- Around line 1283-1289: The PerformanceMetrics interface currently types
percentile fields (latencyP50Ms, latencyP95Ms, latencyP99Ms, and latencyMeanMs
if applicable) as number which forces null payloads to become 0ms in the UI;
update the interface (PerformanceMetrics) to allow those fields to be number |
null, then update any render/formatting logic that uses those symbols (e.g.,
where latencyP50Ms, latencyP95Ms, latencyP99Ms are rounded or displayed) to
check for null and render an "unavailable"/placeholder string instead of
formatting 0ms. Ensure the same change is applied to the duplicate definitions
referenced around the other block (lines showing the other PerformanceMetrics
usage).
- Around line 1399-1416: The GET in the useEffect currently calls
setLoaded(true) in both success and catch, which enables the form/save after a
failed bootstrap; change the logic to track load success separately (e.g., add a
boolean state like loadSucceeded or loadError) and only call setForm and mark
load success when the API returns data in the then branch; in the catch branch
set the new failure flag (don't setLoaded(true) or set it to false) so the Save
control remains disabled until loadSucceeded is true; update any save-enabling
checks (wherever save button/submit is gated) to require loadSucceeded (and
setLoaded if you still need a loading spinner) rather than just setLoaded.
- Around line 1514-1523: The initial GET useEffect sets loaded=true in the catch
path which lets the form render empty and allows saves to overwrite agent
metadata; add a separate loadError (e.g., const [loadError, setLoadError] =
useState<boolean>(false)) and change the GET handler in the useEffect that calls
api.get(`/connectors/${connectorId}`) so that on success you set the agent
fields via
setAgentDescription/setAgentNotFor/setInputSchemaStr/setOutputSchemaStr and
setLoaded(true), and on failure you setLoadError(true) (and do not
setLoaded(true)). Update any save handler (e.g., handleSave/onSave) and UI save
buttons to disable saving when !loaded || loadError, and apply the same pattern
to the other identical useEffect blocks referenced (around lines 1527-1549 and
1577-1579) so initial load failures prevent accidental clears.
---
Nitpick comments:
In `@plugins/service-gateway/frontend/src/components/GatewayNav.tsx`:
- Around line 21-44: The component GatewayNav.tsx currently applies tabs ARIA
roles (role="tablist" on the <nav> and role="tab" plus aria-selected on the item
buttons) even though NAV_ITEMS triggers route navigation; remove the
tabs-specific attributes: delete role="tablist" from the <nav> and remove
role="tab" and aria-selected from the item elements, keep the existing
aria-label on the <nav> and preserve aria-current={active ? 'page' : undefined}
on each item (and retain the onClick/navigate logic or convert to anchors if you
prefer) so the markup uses standard navigation semantics instead of the tab
pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1781912-6a11-4997-bc49-e2da27f017f3
📒 Files selected for processing (3)
plugins/service-gateway/frontend/src/App.tsxplugins/service-gateway/frontend/src/components/GatewayNav.tsxplugins/service-gateway/frontend/src/pages/ConnectorDetailPage.tsx
Comment on lines
+9
to
+14
| const NAV_ITEMS = [ | ||
| { label: 'Connectors', path: '/', match: (p: string) => p === '/' || p.startsWith('/new') || p.startsWith('/connectors') }, | ||
| { label: 'Dashboard', path: '/dashboard', match: (p: string) => p === '/dashboard' }, | ||
| { label: 'Master Keys', path: '/master-keys', match: (p: string) => p === '/master-keys' }, | ||
| { label: 'Plans', path: '/plans', match: (p: string) => p === '/plans' }, | ||
| ] as const; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how ApiKeysPage is accessed - look for navigation links to /keys
rg -n --type=ts --type=tsx -C2 '"/keys"' plugins/service-gateway/frontend/src/Repository: livepeer/naap
Length of output: 84
🏁 Script executed:
# Search for "/keys" route references in TypeScript files
rg -n "/keys" plugins/service-gateway/frontend/src/ -A 2 -B 2Repository: livepeer/naap
Length of output: 4049
🏁 Script executed:
# Check ApiKeysPage import and usage
rg -n "ApiKeysPage" plugins/service-gateway/frontend/src/ -A 2 -B 2Repository: livepeer/naap
Length of output: 7390
🏁 Script executed:
# View the complete App.tsx routing to verify all routes
cat plugins/service-gateway/frontend/src/App.tsxRepository: livepeer/naap
Length of output: 2003
Add missing navigation entry for API Keys page.
The /keys route (ApiKeysPage) is implemented as a full page component for viewing "all team API keys" but has no corresponding entry in the NAV_ITEMS array. Users currently have no way to navigate to this page via the tab bar.
Add an API Keys tab to NAV_ITEMS:
{ label: 'API Keys', path: '/keys', match: (p: string) => p === '/keys' }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/frontend/src/components/GatewayNav.tsx` around lines
9 - 14, NAV_ITEMS in GatewayNav.tsx is missing an entry for the API Keys page,
so add a new item to the NAV_ITEMS array: include { label: 'API Keys', path:
'/keys', match: (p: string) => p === '/keys' } alongside the existing entries
(e.g., the ones for 'Connectors', 'Dashboard', 'Master Keys', 'Plans') so the
tab bar shows and routes to the /keys (ApiKeysPage) route.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/service-gateway/frontend/src/pages/ConnectorListPage.tsx (1)
69-69: Default filter change may mislead users with draft-only connectors.Defaulting to
'published'is a sensible UX choice for most users. However, combined with the empty state message at line 219 ("Create your first connector"), users who only have draft connectors will see the empty state prompt, which could be confusing.Consider either:
- Adjusting the empty state to account for active filters (e.g., "No connectors match your filters" vs "Create your first connector"), or
- Keeping this as-is if the expected user flow is that draft connectors are transient.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/service-gateway/frontend/src/pages/ConnectorListPage.tsx` at line 69, The default statusFilter is set to 'published' which can make users with only draft connectors see the "Create your first connector" empty state; update the UI logic in ConnectorListPage to detect when filters are active (statusFilter !== 'all' or a non-empty search) and show a different empty message like "No connectors match your filters" (or include a CTA to clear filters) instead of "Create your first connector", or alternatively change the initial state of statusFilter (setStatusFilter) from 'published' to 'all' so drafts are included by default; update the conditional that renders the empty state (the component/function that uses statusFilter) accordingly and ensure the message reflects whether the list is empty due to filters versus truly no connectors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/service-gateway/frontend/src/App.tsx`:
- Line 32: The Master Keys page and nav item are currently accessible to all
authenticated users; update the frontend to perform role-based/guard checks
before rendering them: wrap the Route element for path="/master-keys" (the Route
that renders <MasterKeysPage />) with the existing TeamGuard or a new AdminGuard
component and add a conditional render for the navigation item so it only shows
for users with admin permission/role (use the same user/role selector/hooks used
elsewhere). Ensure you reference the Route/element (MasterKeysPage) and the
TeamGuard/admin-check utilities so unauthorized users neither see the nav entry
nor can reach the /master-keys route in the UI.
---
Nitpick comments:
In `@plugins/service-gateway/frontend/src/pages/ConnectorListPage.tsx`:
- Line 69: The default statusFilter is set to 'published' which can make users
with only draft connectors see the "Create your first connector" empty state;
update the UI logic in ConnectorListPage to detect when filters are active
(statusFilter !== 'all' or a non-empty search) and show a different empty
message like "No connectors match your filters" (or include a CTA to clear
filters) instead of "Create your first connector", or alternatively change the
initial state of statusFilter (setStatusFilter) from 'published' to 'all' so
drafts are included by default; update the conditional that renders the empty
state (the component/function that uses statusFilter) accordingly and ensure the
message reflects whether the list is empty due to filters versus truly no
connectors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 145c537e-82da-40c5-902f-3d6c8bd178e2
📒 Files selected for processing (4)
plugins/service-gateway/frontend/src/App.tsxplugins/service-gateway/frontend/src/components/GatewayNav.tsxplugins/service-gateway/frontend/src/pages/ConnectorListPage.tsxplugins/service-gateway/frontend/src/pages/DashboardPage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/service-gateway/frontend/src/components/GatewayNav.tsx
| <Route path="/connectors/:id" element={<ConnectorDetailPage />} /> | ||
| <Route path="/connectors/:id/edit" element={<ConnectorWizardPage />} /> | ||
| <Route path="/keys" element={<ApiKeysPage />} /> | ||
| <Route path="/master-keys" element={<MasterKeysPage />} /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify backend authorization for master-keys API endpoints.
# Expected: The route handlers should check for admin/owner role before processing requests.
echo "=== Checking master-keys route handlers for authorization ==="
rg -n -A 10 'master-keys' --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts' | head -80
echo ""
echo "=== Searching for admin/auth middleware on master-keys routes ==="
ast-grep --pattern 'app.$_("/master-keys", $$$)'Repository: livepeer/naap
Length of output: 3857
🏁 Script executed:
#!/bin/bash
# Check the getAdminContext implementation and how it's used
echo "=== Reading team-guard file for authorization logic ==="
fd -t f team-guard\\.ts$ | head -5
echo ""
echo "=== Checking first backend route handler for authorization usage ==="
cat -n apps/web-next/src/app/api/v1/gw/admin/master-keys/route.ts | head -50
echo ""
echo "=== Searching for frontend authorization guards ==="
fd -t f -path '*/frontend/src/*' GatewayNav\\.tsx MasterKeysPage\\.tsx 2>/dev/null | head -10Repository: livepeer/naap
Length of output: 2354
🏁 Script executed:
#!/bin/bash
# Read the team-guard authorization logic
echo "=== Reading team-guard.ts for getAdminContext implementation ==="
cat -n apps/web-next/src/lib/gateway/admin/team-guard.ts | head -100
echo ""
echo "=== Find the App.tsx file being reviewed ==="
fd -t f App\\.tsx | grep -i frontend
echo ""
echo "=== List files in the frontend source directory ==="
find plugins/service-gateway/frontend/src -type f -name '*.tsx' -o -name '*.ts' 2>/dev/null | head -20Repository: livepeer/naap
Length of output: 6109
🏁 Script executed:
#!/bin/bash
# Check the App.tsx file and the frontend authorization components
echo "=== Reading App.tsx with context around line 32 ==="
cat -n plugins/service-gateway/frontend/src/App.tsx | head -50
echo ""
echo "=== Reading GatewayNav.tsx for authorization checks ==="
wc -l plugins/service-gateway/frontend/src/components/GatewayNav.tsx
head -80 plugins/service-gateway/frontend/src/components/GatewayNav.tsx
echo ""
echo "=== Finding MasterKeysPage component ==="
fd -t f MasterKeysPage\\.tsx plugins/service-gateway/frontend/srcRepository: livepeer/naap
Length of output: 5692
🏁 Script executed:
#!/bin/bash
# Check the MasterKeysPage component and TeamGuard usage
echo "=== Reading MasterKeysPage.tsx ==="
wc -l plugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx
cat -n plugins/service-gateway/frontend/src/pages/MasterKeysPage.tsx | head -100
echo ""
echo "=== Check if TeamGuard is used in the plugin ==="
rg -n TeamGuard plugins/service-gateway/frontend/src
echo ""
echo "=== Checking useGatewayApi hook to see how it handles backend errors ==="
cat plugins/service-gateway/frontend/src/hooks/useGatewayApi.tsRepository: livepeer/naap
Length of output: 7029
Implement frontend authorization checks for the Master Keys route.
The /master-keys route and navigation item are currently exposed to all authenticated users without role-based conditional rendering. While the backend properly enforces authorization via getAdminContext() on all POST /api/v1/gw/admin/master-keys/* endpoints (validating token and team membership), the frontend should implement defense-in-depth by:
- Conditionally rendering the Master Keys nav item based on user role/permissions
- Conditionally rendering or guarding the route itself to prevent unauthorized users from accessing the page and receiving a backend error
Consider using the existing TeamGuard component pattern or implementing a role-based check to hide these controls from non-admin users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/service-gateway/frontend/src/App.tsx` at line 32, The Master Keys
page and nav item are currently accessible to all authenticated users; update
the frontend to perform role-based/guard checks before rendering them: wrap the
Route element for path="/master-keys" (the Route that renders <MasterKeysPage
/>) with the existing TeamGuard or a new AdminGuard component and add a
conditional render for the navigation item so it only shows for users with admin
permission/role (use the same user/role selector/hooks used elsewhere). Ensure
you reference the Route/element (MasterKeysPage) and the TeamGuard/admin-check
utilities so unauthorized users neither see the nav entry nor can reach the
/master-keys route in the UI.
Contributor
|
Does this PR aggregate capabilities across all orchestrators or only a single one? |
Contributor
|
This feature should be reworked so that it is sending jobs to the real Livepeer network. |
|
This PR has conflicts with the base branch. Please rebase to resolve them: git fetch origin
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThe |
… and metrics Implement the complete agent tool layer for the Service Gateway, enabling AI agent frameworks (MCP, OpenAI, LangChain) to discover, price, and invoke tools through standardized protocols. Schema: - ConnectorPricing, GatewayMasterKey, ConnectorMetrics, ConnectorCapabilityRanking - Agent metadata fields on ServiceConnector (agentDescription, agentNotFor, inputSchema, outputSchema) - Examples field on ConnectorEndpoint Auth: - Master key (gwm_ prefix) auth path in authorize.ts - Rate-limited, SHA-256 hashed, supports team + personal scope APIs (public): - GET /gw/catalog (native, OpenAI, MCP formats) - GET /gw/pricing, /gw/pricing/:slug/estimate - POST /gw/mcp (JSON-RPC 2.0 tools/list + tools/call) - GET /gw/discovery (no auth, standard entry point) - GET /gw/rankings?category=... (cross-connector rankings) - GET /gw/catalog/:slug/metrics, /gw/catalog/:slug/rankings APIs (admin): - Master key CRUD (create, list, revoke, rotate) - Connector pricing CRUD - Connector rankings CRUD - Metrics aggregation cron job (hourly) Reliability: - Idempotency-Key support in proxy route via @naap/cache - Agent-friendly error recovery (retryable, suggestedAction, Retry-After) - Reserved slug validation to prevent route collisions Frontend: - MasterKeysPage (create, revoke, rotate gwm_ keys) - Pricing tab on ConnectorDetailPage - Agent Metadata section in Settings tab - Performance tab with latency distribution, error rate, availability Config: - Idempotency-Key added to CORS headers (vercel.json + next.config.js) - .well-known/gateway.json rewrite to /gw/discovery - Metrics aggregation cron (5 * * * *) Tests: 35 files, 354 tests (baseline: 29 files, 294 tests), 0 regressions Made-with: Cursor
…nd error handling - Fix CRITICAL auth bypass in metrics aggregate endpoint when CRON_SECRET is unset - Fix SSRF vulnerability in MCP route by using env-based self-origin instead of Host header - Fix MCP tools/call silently dropping args for GET endpoints — now converts to query params - Fix pricing logic where volume tiers incorrectly overrode feature-specific pricing - Fix PerformanceTab bypassing auth by using raw fetch — now uses api hook - Fix variable shadowing of global `window` in PerformanceTab (renamed to timeWindow) - Fix AgentMetadata silently returning on invalid JSON — now shows error message - Fix MasterKeysPage missing error handling on create/revoke/rotate - Fix rankings batch upsert lacking transaction — now uses $transaction - Fix discovery document missing rankings endpoint - Fix NaN pagination and add pageSize cap (max 200) in catalog route - Fix mcp-adapter discarding agentDescription content in tool descriptions - Fix calculateCost returning empty connector field — now accepts connectorSlug param - Add createdAt to ConnectorPricing and ConnectorCapabilityRanking models - Add missing ownerUserId index to GatewayMasterKey - Prevent creating master keys with zero scopes - Add tests for feature-pricing precedence and connector slug passthrough Made-with: Cursor
Use catalog-validated connector slug instead of user-provided value in the self-fetch URL. Apply encodeURIComponent to prevent path traversal. This ensures the URL path only references connectors that exist in the published catalog. Made-with: Cursor
…ult filter, and archive actions - Fix template click from empty state navigating to template list instead of selected template - Fix draft connector edit opening template step instead of connect step with pre-filled data - Enforce single template selection (radio) instead of multi-select (checkbox) - Default connector listing to show published connectors instead of all statuses - Add recover-to-draft and permanent purge actions for archived connectors - Add connector.purge audit action and backend purge endpoint with ?purge=true - Add status field to updateConnectorSchema for recover workflow Made-with: Cursor
…bbit Security: - Add scope/visibility filtering to catalog, pricing, metrics, and rankings endpoints to prevent leaking private/team connectors - Wrap MCP auth/internal errors in JSON-RPC 2.0 envelope - Forward x-team-id, x-request-id, x-trace-id headers in MCP tools/call Correctness: - Fix metrics period mismatch: change catalog/metrics queries from 'daily' to 'hourly' and add daily rollup in aggregation cron - Fix percentile calculation using ceil-based nearest-rank formula - Set 7d percentiles to null (not composable via weighted average) - Fix inputSchema precedence: endpoint bodySchema now takes priority over connector inputSchema - Include HTTP method in idempotency cache key to prevent cross-method collisions - Use arrayBuffer+base64 for idempotency storage to avoid binary corruption - Await cacheGet in getIdempotentResponse so rejections are caught Improvements: - Fix Performance tab using wrong API client (admin vs public catalog) - Add copy feedback and revoke confirmation to MasterKeysPage - Update z.record() calls for Zod v4 forward compatibility - Consolidate duplicate requestId/traceId extraction in pricing routes - Remove redundant Content-Type header in discovery route - Remove redundant @@index from ConnectorMetrics schema - Add take:100 cap to rankings endpoint - Rename misleading test description Made-with: Cursor
- Add GatewayNav horizontal sub-navigation (Connectors, Dashboard, Master Keys, Plans) - Render GatewayNav above Routes inside MemoryRouter for persistent navigation - Promote AgentMetadataSection from embedded in Settings to independent Agent tab - Add ARIA accessibility attributes (role=tablist, role=tab, aria-selected) - Add overflow-x-auto and whitespace-nowrap for tab bar on narrow viewports - Add loading state to Agent tab instead of rendering nothing - Guard against undefined res.data in AgentMetadataSection - Update file header to reflect current 9-tab layout Made-with: Cursor
…e UX - Replace horizontal tab nav with compact left sidebar (208px) featuring plugin title, subtitle, and icon-labeled nav items - Fix DashboardPage infinite re-render loop caused by Date.now() in useCallback dependency array (moved computation inside callback) - Simplify ConnectorListPage header to avoid title duplication with sidebar - Layout changed from flex-col to flex for side-by-side nav + content Made-with: Cursor
- Add scope/visibility filtering to catalog/[slug], pricing list, and pricing estimate routes (OR public/teamId) - Enforce master key scopes: return scopes and allowedIPs from authorizeMasterKey, check 'proxy' scope on the engine route - Add masterKeyScopes field to AuthResult type - Decouple rawKey delivery from audit log success in master key creation (fire-and-forget audit) - Eliminate N+1 queries in metrics aggregation cron by batch-fetching all usage records and health checks upfront - Change catalog/[slug] metrics query from 'daily' to 'hourly' period Made-with: Cursor
- Add plan dropdown to API Keys tab creation form - Fetch available plans when API Keys tab is active - Pass planId to POST /keys when a plan is selected - Display plan name in the keys table (new Plan column) - Add plan field to ApiKey interface Made-with: Cursor
Every scope now has an auto-created "default" plan (100 req/min, 10K daily, 100K monthly) that applies to API keys with no explicit plan. The default plan is fully editable but cannot be deleted. - Add getOrCreateDefaultPlan utility with upsert for race safety - Integrate into authorize.ts so planless API keys inherit limits - Auto-create default plan on Plans list GET - Prevent deletion of the default plan in DELETE handler - Fix PUT handler to work with personal scopes (not just teamId) - Update PlansPage: default badge, inline editing, sort default first - Show "Default" label for planless keys in ConnectorDetailPage Made-with: Cursor
The Play tab was sending API keys via x-api-key header, but the gateway proxy expects Authorization: Bearer. Also adds credentials: 'include' so session cookies are sent for JWT auth, and x-team-id header for scope resolution. PerformanceTab fetch also updated with credentials and team header. Made-with: Cursor
…erence client Replace the old ffmpeg/trickle-based media bridge with WebSocket JPEG frame streaming that matches the upstream Livepeer lightweight gateway protocol. Frontend: new App.tsx with camera capture, dual canvas (input/output), real-time stats, and log panel. HTTP calls (start-job, stop-job) route through the livepeer-gateway Service Gateway connector. WebSocket connects directly to the backend proxy for frame exchange. Backend: rewritten as a simple Express + ws server that proxies bidirectional WebSocket connections between the browser and the upstream gateway's /ws/stream endpoint. Removed ffmpeg, trickle, and node-fetch dependencies. Added plugin.json manifest, vite.config.ts, and mount.tsx scaffolding. Removed obsolete VideoPlayer.tsx and useMediaBridge.ts. Made-with: Cursor
- replicate.py: Full inference + training, uses model-specific
endpoint (/v1/models/{owner}/{name}/predictions) for named models
- runpod.py: Full inference + training via serverless API
- Both support train, train_submit, train_status methods
- Adapter: multi-capability registration, runtime cap add/remove API
- Synced all container code with deployed VM versions
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t to upstream - Replace direct gateway calls with NaaP Service Gateway connector routing - Add CORS support (Access-Control-Allow-Origin: *) for gateway consumer routes - Add OPTIONS preflight handler to gateway engine route - Demo page fields: NaaP URL, connector slug, API key, upstream WS URL Made-with: Cursor
Made-with: Cursor
… MCP Security: - Enforce master key IP allowlist in authorizeMasterKey using matchIPAllowlist - Replace regex-based IP/CIDR validation with net.isIP() + prefix-length checks Idempotency: - Store content-type in cached responses for correct replay - Preserve content-type header when replaying idempotent responses Metrics: - Fix percentile calculation (Math.floor zero-based nearest-rank) - Add daily rollup aggregation from hourly metrics - Fix 24h window to query hourly period with proper aggregation - Batch connector processing with Promise.allSettled (concurrency=5) MCP: - Harden SSRF: use NEXT_PUBLIC_APP_URL instead of NEXTAUTH_URL/PORT - Use JSON-RPC -32000 error code for auth failures Config: - Add /.well-known/gateway.json rewrite for standard discovery location - Add specific function timeout (120s) for metrics aggregate cron route Made-with: Cursor
seanhanca
force-pushed
the
feat/gateway-agent-tool-interface
branch
from
8dea839 to
9de6823
Compare
This was referenced Mar 25, 2026
eliteprox
added a commit
that referenced
this pull request
Mar 25, 2026
) Regenerate package-lock.json so it matches package.json after #201 (optional platform-specific dev deps: @napi-rs/wasm-runtime, @nx/nx-*, @oxc-resolver/binding-*). Update bin/vercel-build.sh for CI and local parity with Vercel: - Enable Corepack before the Next.js build so packageManager (npm@10.9.4) is honored and preinstalled Yarn 1.x does not break next build. - Run sync-plugin-registry only when VERCEL_ENV is production or preview, matching the prisma db push guard; CI uses DATABASE_URL without a running Postgres.
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
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.
Summary
Add a complete Agent Tool Interface to the Service Gateway, enabling AI agents to discover, evaluate, and invoke gateway connectors programmatically.
What's included
/api/v1/gw/catalog) — paginated catalog of all published connectors with endpoints, pricing, health, performance metrics, and rankings. Supports native, OpenAI function-calling, and MCP output formats./api/v1/gw/pricing) — per-connector pricing with volume tiers, feature-specific pricing, and cost estimation./api/v1/gw/mcp) — standardtools/listandtools/callmethods for MCP-compatible agents./api/v1/gw/discovery,.well-known/gateway.json) — public endpoint listing all available gateway APIs, auth methods, and formats./api/v1/gw/rankings) — admin-curated quality/speed/cost rankings per category for agent tool selection.gwm_prefix) — scope-wide API keys for AI agent integrations, with CRUD, rotation, and revocation.ConnectorMetrics, served via catalog and dedicated endpoints.Idempotency-Keyheader on mutating proxy requests with cached response replay.retryable,suggestedAction, andRetry-Afterheaders.Database changes
4 new models in the
plugin_service_gatewayschema:ConnectorPricing— per-unit cost, volume tiers, feature pricing, upstream costGatewayMasterKey— SHA-256 hashed master keys with scopes and IP allowlistsConnectorMetrics— rolling hourly performance aggregatesConnectorCapabilityRanking— admin-curated quality rankingsExtended
ServiceConnectorwithagentDescription,agentNotFor,inputSchema,outputSchemafields.Extended
ConnectorEndpointwithexamplesfield.Frontend changes
Code review fixes (second commit)
Addressed all findings from thorough code review:
CRON_SECRETis unsettools/callsilently dropping args for GET endpoints — now converts to query paramsfetchinstead of theapihookwindow(renamed totimeWindow)$transaction— partial failures now roll backcreatedAttoConnectorPricingandConnectorCapabilityRanking, added missing indexesTest results
Test plan
GET /api/v1/gw/discoveryreturns all endpointsGET /api/v1/gw/catalog(requires auth)tools/listandtools/callviaPOST /api/v1/gw/mcpGET /api/v1/gw/pricing/:slug/estimate?units=1000Idempotency-Keyheader twice, confirm replayed responseGET /api/v1/gw/admin/metrics/aggregateMade with Cursor
Summary by CodeRabbit
New Features
Infrastructure