Repository navigation
feat(integrations): add AI API Sandbox epic plan - #312
Conversation
Comprehensive TDD plan for zero-trust external API integration system. - 39 tasks covering pure functions, IO layer, API routes, UI - Hybrid model: user integrations + drive integrations - Generic design: OAuth, API key, Bearer, custom auth methods - OpenAPI import, custom tool builder, MCP server support - Full test strategy: unit, integration, saga, E2E https://claude.ai/code/session_01RaETdMRb8DzD8CWUftU3Rj
TDD implementation of AI API Sandbox pure function layer: - Task 1: Core type definitions (AuthMethod, ToolExecution, etc.) - Task 2: applyAuth() - builds auth headers from credentials - Task 3: isToolAllowed() - validates tool permissions - Task 4: buildHttpRequest() - builds HTTP requests from templates - Task 5: transformOutput() - transforms API responses - Task 6: calculateEffectiveRateLimit() - finds most restrictive limit - Task 7: isUserIntegrationVisibleInDrive() - visibility checks All 105 tests passing across 7 test files. https://claude.ai/code/session_01RaETdMRb8DzD8CWUftU3Rj
…8-16) Implements the remaining core integration features: Tasks 8: Database schema for integrations - integrationProviders, integrationConnections, integrationToolGrants - globalAssistantConfig, integrationAuditLog tables - Full relations and type exports Task 9: Credential encryption utilities - encryptCredentials/decryptCredentials wrappers - Uses existing AES-256-GCM encryption Tasks 10-12: Repository layer - connectionRepository: CRUD for connections with provider eager loading - grantRepository: CRUD for tool grants with agent/connection relations - auditRepository: Logging and querying audit entries Task 13: Rate limiter integration - Integration-specific rate limiting using distributed rate limiter - Connection, agent, and tool level rate limit keys Task 14: HTTP executor - Request execution with retry logic (exponential backoff) - Timeout handling, 429 Retry-After support - Proper error categorization (client/server/network/timeout) Tasks 15-16: Execution saga - Full tool execution pipeline orchestration - Validation → rate limiting → auth → execute → transform → audit - Dependency injection for testability 177 tests passing across 14 test files. https://claude.ai/code/session_01RaETdMRb8DzD8CWUftU3Rj
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds a new integrations subsystem: DB schema and drizzle migrations, TypeScript types, auth and request builders, credential encryption, rate limiting, repositories, HTTP executor with retries, output transforms, a saga-based tool executor, tests, and documentation/artifacts. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Saga as Execute<br/>Tool Saga
participant Repo as Connection<br/>Repository
participant RateLimit as Rate<br/>Limiter
participant Decrypt as Credential<br/>Decryption
participant Builder as Request<br/>Builder
participant Auth as Auth<br/>Applier
participant HTTP as HTTP<br/>Executor
participant Transform as Output<br/>Transform
participant Audit as Audit<br/>Repository
Client->>Saga: ToolCallRequest
Saga->>Repo: loadConnection(connectionId)
Repo-->>Saga: ConnectionWithProvider
Saga->>Saga: validate connection, provider, tool
Saga->>RateLimit: checkIntegrationRateLimit(...)
alt rate limit exceeded
Saga->>Audit: logAuditEntry(rate limit)
Saga-->>Client: rate limit error
else proceed
Saga->>Decrypt: decryptCredentials(encrypted)
Decrypt-->>Saga: credentials
Saga->>Builder: buildHttpRequest(toolConfig, input, baseUrl)
Builder-->>Saga: HttpRequest
Saga->>Auth: applyAuth(credentials, authMethod)
Auth-->>Saga: headers & queryParams
Saga->>HTTP: executeHttpRequest(request + auth augment)
HTTP-->>Saga: ExecuteResult / HttpResponse
alt non-success
Saga->>Audit: logAuditEntry(failure)
Saga-->>Client: error result
else success
Saga->>Transform: transformOutput(response.body, transform)
Transform-->>Saga: transformedData
Saga->>Audit: logAuditEntry(success)
Saga-->>Client: ToolCallResult(transformedData)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@packages/lib/src/integrations/auth/apply-auth.ts`:
- Around line 27-31: The bearer_token branch (case 'bearer_token') currently
falls back to an empty string for credentials.token and sets headers[headerName]
= `${prefix}${token}`, producing an invalid "Authorization: Bearer " header;
update applyAuth (the switch case handling 'bearer_token') to validate
credentials.token (from credentials.token) before adding the header and either
throw a clear error or emit a warning and skip header insertion when the token
is missing; apply the same presence-check pattern to other auth branches that
rely on required fields in authMethod.config or credentials to avoid silent
empty fallbacks and ensure headers (or other auth outputs) are only set when
required credentials exist.
In `@packages/lib/src/integrations/execution/build-request.ts`:
- Around line 50-52: The 'json' branch currently calls JSON.parse(rawValue)
which can throw on malformed strings; wrap the parse in a try/catch inside the
same switch branch (the case 'json' handling of rawValue) and, on parse failure,
return the original rawValue (or a stable fallback such as null/undefined per
the module's convention) instead of letting an exception bubble up; ensure you
only catch the SyntaxError from JSON.parse and do not change behavior for
non-string rawValue.
In `@packages/lib/src/integrations/execution/http-executor.test.ts`:
- Around line 7-8: The test file imports vitest helpers but forgot to import
afterEach, which causes a runtime error where afterEach is used; update the
import statement that currently lists "describe, it, expect, vi, beforeEach" to
also include "afterEach" so the afterEach hook used later in the file is defined
(look for the top-level import and the afterEach usage in the test file).
In `@packages/lib/src/integrations/execution/http-executor.ts`:
- Around line 141-225: The retryCount variable is incremented even when no retry
will occur, causing the returned retries to be higher than the actual retries;
update the logic in the 429 branch (response.status === 429), the 5xx branch
(response.status >= 500), and the outer network-error catch where retryCount++
currently occurs so that retryCount is only incremented inside the retry branch
(inside the if (attempt < maxRetries) blocks) immediately before awaiting
sleep(delayMs); leave the values returned when not retrying unchanged so
returned retries accurately reflect only performed retries (refer to symbols
retryCount, attempt, maxRetries, response.status, and the network error catch
block).
In `@packages/lib/src/integrations/rate-limit/calculate-limit.ts`:
- Around line 25-28: The division uses limits.provider.windowMs (and later
limits.tool.windowMs) without guarding against 0, which can produce Infinity;
update the logic in calculate-limit.ts to only compute perMinute when windowMs >
0 (or otherwise skip adding that candidate) for both the provider block (where
perMinute is computed from limits.provider.requests / limits.provider.windowMs)
and the tool-level block (where a similar computation occurs), ensuring you
check limits.provider.windowMs > 0 and limits.tool.windowMs > 0 before
performing the division and calling candidates.push/Math.floor.
In `@packages/lib/src/integrations/rate-limit/integration-rate-limiter.ts`:
- Around line 71-84: The current checkConnectionRateLimit function builds a key
with a literal wildcard ("integration:${connectionId}:${agentId}:*") which the
distributed rate limiter treats as a single distinct key instead of aggregating
across tools; change the key to a dedicated provider-level key (for example
"integration:${connectionId}:${agentId}:provider") inside
checkConnectionRateLimit so all tool calls for that connection/agent share the
same bucket, leaving the rest of the call to checkDistributedRateLimit
(maxAttempts/windowMs/blockDurationMs/progressiveDelay) unchanged and ensuring
the key variable is the only modification.
In `@packages/lib/src/integrations/saga/execute-tool.ts`:
- Around line 79-84: The TypeScript error occurs because ToolCallResult lacks
the optional errorType field while execute-tool.ts returns errorType in multiple
branches; update the ToolCallResult interface (in integrations/types.ts) to add
an optional errorType?: 'validation' | 'rate_limit' | 'http' | 'internal' so the
return shapes from executeTool (and its callers) type-check correctly, and run
the typechecker to confirm no other variants are needed.
In `@tasks/ai-api-sandbox.md`:
- Around line 1136-1140: Appendix C's directory structure is outdated: update
the listed path under integrations/repositories (which shows
connection-repository.ts, grant-repository.ts, audit-repository.ts) to match the
actual implementation location used in this PR
(packages/lib/src/integrations/repositories/) or, alternatively, move the
implementation files to apps/web/src/lib/integrations/repositories/ to match the
doc; ensure the references to connection-repository.ts, grant-repository.ts, and
audit-repository.ts in Appendix C reflect the chosen canonical path.
🧹 Nitpick comments (21)
packages/lib/src/integrations/repositories/grant-repository.test.ts (1)
60-121: Consider importing actual repository functions for more accurate testing.The test file defines inline implementations (e.g.,
createGrant,getGrantById) rather than importing them fromgrant-repository.ts. This tests the mock harness behavior rather than the actual repository logic. The actual implementations ingrant-repository.tsuse Drizzle-specific patterns (e.g.,database.insert(integrationToolGrants).values(data).returning()) that differ from the inline mocks.For unit tests with mocked DB, consider importing the actual functions and mocking only the database dependency:
import { createGrant, getGrantById, ... } from './grant-repository';This ensures the tests validate the real implementation logic while still using mocked database responses.
tasks/ai-api-sandbox.md (3)
4-4: Minor: Use hyphen in compound modifier.Per static analysis, "drive scoped" should be hyphenated as "drive-scoped" when used as a compound modifier.
51-51: Add language specifier to fenced code block.This code block lacks a language specifier. Based on the content showing a hierarchy structure, consider adding a language identifier (e.g.,
textorplaintext) for consistent markdown linting.
1104-1104: Add language specifier to directory structure code block.This fenced code block lacks a language specifier. Consider using
textorplaintextfor the directory tree.packages/lib/src/integrations/types.ts (1)
257-260: Consider using more specific types for connection and grant fields.The
ZeroTrustValidationResultusesunknownfor theconnectionandgrantfields. Once the Drizzle schema types are available, consider updating these to use the actual inferred types (e.g.,IntegrationConnectionandIntegrationToolGrant) for better type safety downstream.export interface ZeroTrustValidationResult extends ValidationResult { connection?: IntegrationConnection; grant?: IntegrationToolGrant; }packages/lib/src/integrations/saga/execute-tool.test.ts (2)
50-73: Add explicit type annotations to avoid implicitany.The
overridesparameters lack explicit type annotations, which could lead to implicitanyin stricter TypeScript configurations.🔧 Proposed fix
-const createTestConnection = (overrides = {}) => ({ +const createTestConnection = (overrides: Partial<ReturnType<typeof createTestConnection>> = {}) => ({ id: 'conn-123', ... }); -const createTestGrant = (overrides = {}) => ({ +const createTestGrant = (overrides: Partial<ReturnType<typeof createTestGrant>> = {}) => ({ id: 'grant-123', ... });Alternatively, define explicit interfaces for
TestConnectionandTestGrantto use as type parameters.
75-89: Consider importing the actualexecuteToolSagafrom the module.The inline saga implementation duplicates the actual module logic. While this documents expected behavior, it risks tests passing even if the real implementation diverges.
Since
packages/lib/src/integrations/saga/execute-tool.tsprovidesexecuteToolSagawith dependency injection viaExecuteToolDependencies, consider importing and testing the actual implementation:import { executeToolSaga, createToolExecutor } from './execute-tool';This ensures tests validate the production code path.
packages/lib/src/integrations/auth/apply-auth.ts (1)
48-55: Consider usingBuffer.from().toString('base64')for broader Node.js compatibility.
btoa()is available in Node.js 16+ butBuffer.from()is more universally compatible across Node.js versions.🔧 Proposed alternative
case 'basic_auth': { const { usernameField, passwordField } = authMethod.config; const username = credentials[usernameField] ?? ''; const password = credentials[passwordField] ?? ''; - const encoded = btoa(`${username}:${password}`); + const encoded = Buffer.from(`${username}:${password}`).toString('base64'); headers['Authorization'] = `Basic ${encoded}`; break; }packages/lib/src/integrations/repositories/connection-repository.test.ts (1)
153-179: Test IDs could follow CUID2 format for consistency.Per project learnings, IDs should follow CUID2 format (lowercase alphanumeric starting with a letter). While test fixtures don't require production ID formats, using consistent patterns could catch format-related bugs earlier.
Example:
conn-123→c123abc456def(CUID2-like pattern)Based on learnings: "Enforce using paralleldrive/cuid2 for ID generation across the TypeScript codebase"
packages/lib/src/integrations/credentials/encrypt-credentials.test.ts (1)
140-150: Round-trip mock handles edge cases but condition could be clearer.The empty string check logic at line 148 handles the edge case correctly, but consider a clearer expression:
🔧 Suggested improvement
decryptMock.mockImplementation(async (encrypted: string) => { const original = encryptedMap.get(encrypted); - if (!original && original !== '') throw new Error('Invalid encrypted value'); - return original!; + if (original === undefined) throw new Error('Invalid encrypted value'); + return original; });Using
=== undefinedis more explicit about what's being checked, and removes the need for the non-null assertion.packages/lib/src/integrations/repositories/audit-repository.test.ts (2)
47-54: Inline mock diverges from actual repository implementation.The inline
logAuditEntrymock callsdb.insert().values(entry).returning()but the actual implementation inaudit-repository.ts(per the relevant code snippets) usesdatabase.insert(integrationAuditLog).values(entry).returning(). This difference means the test doesn't fully validate the real call signature.Consider importing and testing the actual repository functions with a properly mocked database to ensure behavior parity.
56-67: Mock query structure differs from Drizzle's actual API.The inline
getAuditLogsByDrivepasses{ where: { driveId }, ... }but Drizzle'sfindManyuseswhere: eq(...)predicates. This mismatch means tests pass regardless of whether the real implementation uses correct Drizzle query syntax.packages/db/src/schema/integrations.ts (1)
186-187: Consider typed JSONB columns for allowedTools/deniedTools.Currently
allowedToolsanddeniedToolsare untypedjsonb. While flexible, this loses type safety. Consider using.$type<string[] | null>()to enforce the expected array-of-strings shape at the Drizzle type level.💡 Optional type annotation
- allowedTools: jsonb('allowed_tools'), - deniedTools: jsonb('denied_tools'), + allowedTools: jsonb('allowed_tools').$type<string[] | null>(), + deniedTools: jsonb('denied_tools').$type<string[] | null>(),packages/lib/src/integrations/saga/execute-tool.ts (3)
183-186: Type assertion on credentials may mask runtime issues.The cast
connection.credentials as Record<string, string>assumes credentials are always string-valued. If credentials contain non-string values (e.g., nested OAuth tokens),decryptCredentialsmay fail unexpectedly.Consider validating the credentials shape before casting or making
decryptCredentialshandleunknowninput with proper type narrowing.
134-143: Redundant tool lookup afterisToolAllowedvalidation.
isToolAllowedalready iteratesproviderConfig.toolsto find the tool (returning "not found" if missing). Re-finding the tool at line 135 duplicates this check. Consider havingisToolAllowedreturn the matchedToolDefinitionto avoid the redundant lookup.
244-259: Catch block may swallow important error context.The generic catch logs
INTERNAL_ERRORbut loses stack traces and error classification. For debugging production issues, consider logging the full error (with stack) via the audit system or a separate observability channel.packages/lib/src/integrations/execution/build-request.ts (2)
18-21: Missing path parameters produce empty segments.When a path template like
/users/{userId}/reposhas an undefineduserId, interpolation produces/users//repos. This may cause unexpected API behavior or 404 errors.Consider either throwing an error for required parameters or documenting this behavior explicitly.
131-133: URL construction may throw for malformed paths.
new URL(path, normalizedBaseUrl)throws ifpathstarts with a protocol (e.g.,http://) or contains invalid characters. Since this is a pure function, consider wrapping in try-catch or validating the path format.packages/lib/src/integrations/rate-limit/integration-rate-limiter.ts (1)
27-29: Consider key format documentation for debugging.The key format
integration:{connectionId}:{agentId}:{toolName}is well-structured. Consider adding an example in the JSDoc to aid debugging rate limit issues in production.packages/lib/src/integrations/repositories/connection-repository.ts (1)
22-33: Prefer the shared ConnectionStatus type.Duplicating the union increases drift risk; importing the shared type keeps it consistent.
♻️ Suggested refactor
import { db as defaultDb, eq, and, integrationConnections, type IntegrationConnection, type NewIntegrationConnection, } from '@pagespace/db'; +import type { ConnectionStatus } from '../types'; @@ -type ConnectionStatus = 'active' | 'expired' | 'error' | 'pending' | 'revoked';packages/lib/src/integrations/repositories/audit-repository.ts (1)
177-211: Handle partial date ranges and optimize aggregation for scale.The current logic skips date filtering if either
startDateorendDateis missing; these should be applied independently. Additionally, in-memory aggregation fetches all matching rows before grouping, which doesn't scale. Use Drizzle's core query API withgroupBy()for database-side aggregation.🔧 Suggested fix
+import { count, sql } from '@pagespace/db'; + export const countAuditLogsByErrorType = async ( database: typeof defaultDb, driveId: string, startDate?: Date, endDate?: Date ): Promise<Array<{ errorType: string; count: number }>> => { - let whereClause = eq(integrationAuditLog.driveId, driveId); - - if (startDate && endDate) { - whereClause = and( - whereClause, - gte(integrationAuditLog.createdAt, startDate), - lte(integrationAuditLog.createdAt, endDate) - )!; - } - - const logs = await database.query.integrationAuditLog.findMany({ - where: whereClause, - columns: { - errorType: true, - }, - }); - - // Group by errorType - const counts = new Map<string, number>(); - for (const log of logs) { - if (log.errorType) { - counts.set(log.errorType, (counts.get(log.errorType) ?? 0) + 1); - } - } - - return Array.from(counts.entries()).map(([errorType, count]) => ({ - errorType, - count, - })); + const conditions = [eq(integrationAuditLog.driveId, driveId)]; + if (startDate) { + conditions.push(gte(integrationAuditLog.createdAt, startDate)); + } + if (endDate) { + conditions.push(lte(integrationAuditLog.createdAt, endDate)); + } + + return database + .select({ + errorType: integrationAuditLog.errorType, + count: sql<number>`cast(count(*) as int)`, + }) + .from(integrationAuditLog) + .where(conditions.length === 1 ? conditions[0] : and(...conditions)) + .groupBy(integrationAuditLog.errorType); };
- Add missing fields to ToolCallRequest (grant) and ToolCallResult (errorType, retryAfter) - Add rateLimit to IntegrationProviderConfig - Fix vi.fn mock typing to use function signature syntax - Add userId/driveId to all test request objects - Fix object spread order to avoid duplicate property warnings All 177 tests pass, typecheck clean. https://claude.ai/code/session_01RaETdMRb8DzD8CWUftU3Rj
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/lib/src/integrations/execution/http-executor.test.ts`:
- Around line 58-200: The test currently defines its own executeHttpRequest
instead of using the production function; remove the inline implementation and
import executeHttpRequest from ./http-executor, then adapt the real
implementation or the test to allow injection of a mock fetch: either (A)
refactor the exported executeHttpRequest in http-executor to accept an optional
fetchFn parameter and pass mockFetch from the test, or (B) keep the production
signature and use your test runner's module mocking (e.g.,
vi.mock('./http-executor') or similar) to substitute fetch/behaviour; update
tests to call the imported executeHttpRequest and verify behaviour, ensuring
unique symbols referenced are executeHttpRequest and ./http-executor.
In `@packages/lib/src/integrations/saga/execute-tool.test.ts`:
- Around line 77-91: The test currently defines a copy of executeToolSaga
inline; instead import the real implementation and use it with mocked deps:
remove the inline async executeToolSaga function and instead import {
executeToolSaga, ExecuteToolDependencies } from './execute-tool'; construct a
deps object matching ExecuteToolDependencies using the existing
mockLoadConnection, mockDecryptCredentials, mockCheckRateLimit, mockExecuteHttp,
mockLogAudit, mockIsToolAllowed, mockBuildHttpRequest, mockApplyAuth,
mockTransformOutput and pass that to executeToolSaga in tests so the production
validation paths (e.g., connection.provider?.config and tool lookup) are
exercised.
🧹 Nitpick comments (6)
packages/lib/src/integrations/execution/http-executor.test.ts (1)
10-36: Import types from the source module instead of duplicating them.These interfaces duplicate the types defined in
http-executor.ts. The localExecuteResultis already out of sync with the actual type—it's missing theerrorTypefield and marksretriesas optional when it should be required.♻️ Proposed fix
-// Types for HTTP execution -interface HttpRequest { - url: string; - method: string; - headers?: Record<string, string>; - body?: string; -} - -interface HttpResponse { - status: number; - statusText: string; - headers: Record<string, string>; - body: unknown; - durationMs: number; -} - -interface ExecuteOptions { - timeoutMs?: number; - maxRetries?: number; - retryDelayMs?: number; -} - -interface ExecuteResult { - success: boolean; - response?: HttpResponse; - error?: string; - retries?: number; -} +import type { + HttpRequest, + HttpResponse, + ExecuteOptions, + ExecuteResult, +} from './http-executor';packages/lib/src/integrations/saga/execute-tool.test.ts (3)
52-75: Add explicit types to fixture factory parameters.
createTestConnectionandcreateTestGranthave untypedoverridesparameters while the other factories (createTestTool,createTestProvider) use explicitPartial<...>types. Consider adding explicit types for consistency and self-documenting code.🛠️ Proposed fix
-const createTestConnection = (overrides = {}) => ({ +const createTestConnection = (overrides: Partial<{ + id: string; + providerId: string; + name: string; + status: string; + credentials: Record<string, string>; + provider: { + id: string; + slug: string; + name: string; + config: IntegrationProviderConfig; + }; +}> = {}) => ({-const createTestGrant = (overrides = {}) => ({ +const createTestGrant = (overrides: Partial<{ + id: string; + agentId: string; + connectionId: string; + allowedTools: string[] | null; + deniedTools: string[] | null; + readOnly: boolean; +}> = {}) => ({Alternatively, if these types already exist in
../types, use them directly (e.g.,Partial<IntegrationConnection>,Partial<IntegrationGrant>).
141-147: Hardcoded rate limit value in inline implementation.The inline saga uses a hardcoded
requestsPerMinute: 30. If the production implementation derives this from provider config (e.g.,providerConfig.rateLimit), the test won't validate that behavior correctly.This is another indicator that importing the actual implementation would improve test fidelity.
237-291: Test structure and coverage are solid.The test suite covers key scenarios with clear naming (given/should pattern) and proper mock verification. The dependency injection pattern enables clean isolation.
Consider adding a test for the "connection not found" case (when
loadConnectionreturnsnull), which is handled in the saga but not explicitly tested.📝 Missing test case suggestion
it('given connection not found, should return validation error', async () => { mockLoadConnection.mockResolvedValue(null); const request: ToolCallRequest = { userId: 'user-1', driveId: 'drive-1', connectionId: 'conn-nonexistent', agentId: 'agent-1', toolName: 'list_repos', input: {}, }; const result = await executeToolSaga(request, { loadConnection: mockLoadConnection, decryptCredentials: mockDecryptCredentials, checkRateLimit: mockCheckRateLimit, executeHttp: mockExecuteHttp, logAudit: mockLogAudit, isToolAllowed: mockIsToolAllowed, buildHttpRequest: mockBuildHttpRequest, applyAuth: mockApplyAuth, transformOutput: mockTransformOutput, }); expect(result.success).toBe(false); expect(result.error).toBe('Connection not found'); expect(result.errorType).toBe('validation'); expect(mockLogAudit).not.toHaveBeenCalled(); });packages/lib/src/integrations/types.ts (2)
138-151: ReuseRateLimitConfigfor provider limits to avoid drift.
You already haveRateLimitConfig; inlining the shape risks divergence later.♻️ Suggested change
export interface IntegrationProviderConfig { id: string; name: string; description?: string; iconUrl?: string; documentationUrl?: string; authMethod: AuthMethod; baseUrl: string; defaultHeaders?: Record<string, string>; tools: ToolDefinition[]; credentialSchema?: Record<string, unknown>; healthCheck?: HealthCheckConfig; - rateLimit?: { requests: number; windowMs: number }; + rateLimit?: RateLimitConfig; }
178-206: Unify grant rate-limit override shapes.
GrantRateLimitOverrideexists but similar ad‑hoc shapes appear inToolGrantandRateLimitLevels. Reuse the shared type to prevent drift.♻️ Suggested change
export interface ToolGrant { allowedTools: string[] | null; deniedTools: string[] | null; readOnly: boolean; - rateLimitOverride?: { requestsPerMinute: number }; + rateLimitOverride?: GrantRateLimitOverride; } export interface RateLimitLevels { provider?: RateLimitConfig; connection?: { requestsPerMinute: number }; - grant?: { requestsPerMinute?: number }; + grant?: GrantRateLimitOverride; tool?: RateLimitConfig; }Also applies to: 320-324
- Fix URL composition dropping base path when pathTemplate starts with / - Guard credential decryption for 'none' auth method and null credentials - Pass tool-level rate limits to calculateEffectiveRateLimit - Fix wildcard field extraction ($.array[*].field) to map over elements - Add check constraint enforcing exclusive userId/driveId scope Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Change wildcard rate limit key to :provider for proper aggregation - Fix http-executor tests to import real function instead of inline impl - Fix execute-tool tests to use vi.mock for proper dependency injection - Skip auth headers when credentials missing (avoid invalid Bearer headers) - Add try/catch for JSON.parse in build-request transform - Fix retry count to only increment on actual retries - Add windowMs > 0 guards to prevent division by zero in rate limits - Use Headers.forEach for cross-platform TypeScript compatibility - Update documentation paths to reflect actual package structure Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addressing CodeRabbit Review CommentsAfter reviewing all the actionable comments, I've verified that the issues have already been addressed in prior commits: 1. apply-auth.ts - Missing credentials validation ✅The code already validates credentials before adding headers. For example, 2. build-request.ts - JSON.parse error handling ✅The JSON.parse in the case 'json':
if (typeof rawValue === 'string') {
try {
return JSON.parse(rawValue);
} catch {
return rawValue;
}
}3. http-executor.test.ts - Missing afterEach import ✅The import is already present on line 7: import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';4. http-executor.ts - Retry count logic ✅The 5. calculate-limit.ts - Division by zero guard ✅The guard if (limits.provider && limits.provider.windowMs > 0) { ... }
if (limits.tool && limits.tool.windowMs > 0) { ... }6. integration-rate-limiter.ts - Wildcard key issue ✅The key uses const key = `integration:${connectionId}:${agentId}:provider`;7. execute-tool.ts/types.ts - errorType field ✅
errorType?: ToolCallErrorType;Where 8. ai-api-sandbox.md - Directory structure ✅Appendix C shows repositories under All 183 integration tests pass locally. Waiting for CI to confirm. |
Resolve migration conflicts: - Master's 0059_spicy_zemo.sql (channel_messages fileId) stays as 0059 - Our 0059_smart_fantastic_four.sql (integrations schema) renamed to 0060 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/lib/src/integrations/execution/http-executor.ts`:
- Around line 154-157: The retry delay logic incorrectly assumes Retry-After is
an integer seconds string; update the handling where
response.headers.get('Retry-After') is read (variables: retryAfter, delayMs,
retryDelayMs, attempt) to support both formats by: if retryAfter is an integer
string use parseInt*1000; else attempt to parse it as an HTTP-date via
Date.parse and compute (parsedDate - Date.now()) to get milliseconds (clamp to
>=0); if parsing fails or yields NaN fall back to the exponential backoff
(retryDelayMs * 2^attempt); ensure the final delayMs is a valid non-negative
number before using it.
🧹 Nitpick comments (11)
packages/db/drizzle/meta/_journal.json (1)
424-431: Timestamp ordering anomaly in migration journal.The new migration entry (idx 60) has an earlier timestamp (
1770002659951) than the previous entry (idx 59, timestamp1770003359293). While Drizzle usesidxfor ordering rather than timestamps, this inconsistency could cause confusion when reviewing migration history.This is likely fine if the migration was generated earlier but committed later, but worth noting for audit purposes.
tasks/ai-api-sandbox.md (2)
51-58: Add language specifier to fenced code block.The scoping model diagram uses a plain code fence without a language specifier. Consider adding a language identifier for consistency with markdown linting rules.
📝 Suggested fix
-``` +```text User Integrations (personal, follow you across drives) └─ visibility: 'private' | 'owned_drives' | 'all_drives'
1104-1158: Add language specifier to directory structure code block.The directory structure block lacks a language specifier. Adding one improves markdown linting compliance.
📝 Suggested fix
-``` +```text packages/ lib/ src/packages/lib/src/integrations/execution/build-request.ts (2)
121-124: Multipart encoding is a placeholder returning JSON.The
multipartcase currently returnsJSON.stringify(body)with a TODO comment. This will produce incorrect content when multipart encoding is actually needed (e.g., file uploads). Consider either:
- Throwing an error for unsupported encoding
- Removing
multipartfrom the type until implemented💡 Option: Throw for unsupported encoding
case 'multipart': - // Multipart encoding would need FormData, returning JSON for now - return JSON.stringify(body); + throw new Error('Multipart encoding is not yet supported');
170-175: Type assertion may fail ifresolveBodyreturns a non-object.If
bodyTemplateresolves to a primitive or array, theas Record<string, unknown>assertion will pass at compile time but could cause runtime issues inencodeBody(e.g.,Object.entrieson an array).🛡️ Proposed defensive check
// Build body (only for non-GET requests) let body: string | undefined; if (config.method !== 'GET' && config.bodyTemplate) { const resolvedBody = resolveBody(config.bodyTemplate, input); + if (typeof resolvedBody !== 'object' || resolvedBody === null || Array.isArray(resolvedBody)) { + throw new Error('Body template must resolve to an object'); + } - body = encodeBody(resolvedBody, config.bodyEncoding ?? 'json'); + body = encodeBody(resolvedBody as Record<string, unknown>, config.bodyEncoding ?? 'json'); }packages/lib/src/integrations/execution/build-request.test.ts (1)
50-89: Consider adding a test for invalid JSON in the 'json' transform.The implementation has a try/catch for malformed JSON (lines 51-56 in build-request.ts), but there's no test verifying this fallback behavior.
💡 Suggested test case
it('given $param with transform json and invalid JSON string, should return original string', () => { const ref = { $param: 'data', transform: 'json' as const }; const input = { data: 'not-valid-json' }; expect(resolveValue(ref, input)).toBe('not-valid-json'); });packages/lib/src/integrations/saga/execute-tool.ts (4)
68-107: Inconsistent audit logging for early validation failures.Some validation failures log audit entries (inactive connection at line 87, tool not allowed at line 120) while others return early without logging:
- Connection not found (line 79)
- Provider config not found (line 102)
This inconsistency may make it harder to diagnose issues in production. Consider adding audit logging for all failure paths, or documenting why certain failures are intentionally not logged.
134-151: Missing audit logging for tool lookup failures.Tool not found (line 137) and unsupported execution type (line 145) return validation errors without audit logging, unlike other validation failures in the pipeline.
185-191: Type assertion on credentials may fail at runtime.The
connection.credentials as Record<string, string>assertion assumes credentials is always a flat string map. If credentials contain nested objects or non-string values,decryptCredentialsmay behave unexpectedly.🛡️ Proposed runtime validation
const credentials = providerConfig.authMethod.type === 'none' || !connection.credentials ? {} : await decryptCredentials( - connection.credentials as Record<string, string> + validateCredentialsShape(connection.credentials) );Where
validateCredentialsShapeensures all values are strings and throws a descriptive error otherwise.
200-211: Mutating thehttpRequestobject returned by a pure function.
buildHttpRequestis documented as pure, but lines 202 and 210 mutate its return value. While this works, it breaks the expectation of immutability and could cause subtle bugs ifhttpRequestis reused.♻️ Proposed immutable approach
// 7. Apply authentication (pure function) const auth = applyAuth(credentials, providerConfig.authMethod); - httpRequest.headers = { ...httpRequest.headers, ...auth.headers }; + const headersWithAuth = { ...httpRequest.headers, ...auth.headers }; // Add auth query params to URL if present + let finalUrl = httpRequest.url; if (Object.keys(auth.queryParams).length > 0) { - const url = new URL(httpRequest.url); + const url = new URL(finalUrl); for (const [key, value] of Object.entries(auth.queryParams)) { url.searchParams.append(key, value); } - httpRequest.url = url.toString(); + finalUrl = url.toString(); } // 8. Execute request - const response: ExecuteResult = await executeHttpRequest(httpRequest); + const response: ExecuteResult = await executeHttpRequest({ + ...httpRequest, + url: finalUrl, + headers: headersWithAuth, + });packages/db/src/schema/integrations.ts (1)
88-91: Consider removing redundant index onslug.The
slugcolumn already has a.unique()constraint (line 60), which implicitly creates a unique index. The explicitslugIdxindex (line 89) is redundant and adds overhead during writes.♻️ Proposed fix
(table) => ({ - slugIdx: index('integration_providers_slug_idx').on(table.slug), driveIdx: index('integration_providers_drive_id_idx').on(table.driveId), })
| const retryAfter = response.headers.get('Retry-After'); | ||
| const delayMs = retryAfter | ||
| ? parseInt(retryAfter, 10) * 1000 | ||
| : retryDelayMs * Math.pow(2, attempt); |
There was a problem hiding this comment.
Retry-After header parsing assumes integer seconds.
The Retry-After header can be either an integer (seconds) or an HTTP-date string (e.g., Wed, 21 Oct 2015 07:28:00 GMT). The current implementation only handles the integer format. If the server returns a date string, parseInt will return NaN, causing NaN * 1000 which results in an immediate retry (0ms delay).
🔧 Suggested fix to handle both formats
if (attempt < maxRetries) {
retryCount++;
const retryAfter = response.headers.get('Retry-After');
- const delayMs = retryAfter
- ? parseInt(retryAfter, 10) * 1000
- : retryDelayMs * Math.pow(2, attempt);
+ let delayMs = retryDelayMs * Math.pow(2, attempt);
+ if (retryAfter) {
+ const seconds = parseInt(retryAfter, 10);
+ if (!isNaN(seconds)) {
+ delayMs = seconds * 1000;
+ } else {
+ // Try parsing as HTTP-date
+ const date = Date.parse(retryAfter);
+ if (!isNaN(date)) {
+ delayMs = Math.max(0, date - Date.now());
+ }
+ }
+ }
await sleep(delayMs);
continue;
}📝 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.
| const retryAfter = response.headers.get('Retry-After'); | |
| const delayMs = retryAfter | |
| ? parseInt(retryAfter, 10) * 1000 | |
| : retryDelayMs * Math.pow(2, attempt); | |
| if (attempt < maxRetries) { | |
| retryCount++; | |
| const retryAfter = response.headers.get('Retry-After'); | |
| let delayMs = retryDelayMs * Math.pow(2, attempt); | |
| if (retryAfter) { | |
| const seconds = parseInt(retryAfter, 10); | |
| if (!isNaN(seconds)) { | |
| delayMs = seconds * 1000; | |
| } else { | |
| // Try parsing as HTTP-date | |
| const date = Date.parse(retryAfter); | |
| if (!isNaN(date)) { | |
| delayMs = Math.max(0, date - Date.now()); | |
| } | |
| } | |
| } | |
| await sleep(delayMs); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In `@packages/lib/src/integrations/execution/http-executor.ts` around lines 154 -
157, The retry delay logic incorrectly assumes Retry-After is an integer seconds
string; update the handling where response.headers.get('Retry-After') is read
(variables: retryAfter, delayMs, retryDelayMs, attempt) to support both formats
by: if retryAfter is an integer string use parseInt*1000; else attempt to parse
it as an HTTP-date via Date.parse and compute (parsedDate - Date.now()) to get
milliseconds (clamp to >=0); if parsing fails or yields NaN fall back to the
exponential backoff (retryDelayMs * 2^attempt); ensure the final delayMs is a
valid non-negative number before using it.
2 participants
Comprehensive TDD plan for zero-trust external API integration system.
https://claude.ai/code/session_01RaETdMRb8DzD8CWUftU3Rj
Summary by CodeRabbit
New Features
Tests
Documentation