feat: wire AI sandbox integrations end-to-end#737
Open
2witstudios wants to merge 6 commits intomasterfrom
Open
feat: wire AI sandbox integrations end-to-end#7372witstudios wants to merge 6 commits intomasterfrom
2witstudios wants to merge 6 commits intomasterfrom
Conversation
Add OAUTH_STATE_SECRET, INTEGRATION_GITHUB_CLIENT_ID, and INTEGRATION_GITHUB_CLIENT_SECRET documentation to both root and apps/web .env.example files so developers know what to configure for the AI sandbox integration OAuth flows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add seedBuiltinProviders() to provider-repository that idempotently inserts any missing builtin providers into the database. Wire it into GET /api/integrations/providers as lazy init — when zero providers exist, builtins are seeded automatically. This removes the manual admin install step that blocked end-to-end flow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Confirms resolvePageAgentIntegrationTools and resolveGlobalAssistantIntegrationTools correctly wire resolution, conversion, and execution dependencies — the critical path for integration tools appearing in chat. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds OAUTH_STATE_SECRET environment variable configuration, integrates GitHub OAuth App credentials, implements lazy seeding of builtin integration providers in the API route, exports the seeding function, and adds comprehensive test coverage for integration tool resolution logic. Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIRoute as API Route<br/>(providers)
participant ProviderRepo as Provider<br/>Repository
participant Database
Client->>APIRoute: GET /api/integrations/providers
APIRoute->>ProviderRepo: listEnabledProviders()
ProviderRepo->>Database: Query enabled providers
Database-->>ProviderRepo: Empty or partial list
ProviderRepo-->>APIRoute: providers = []
alt Providers Empty && Builtins Available
APIRoute->>ProviderRepo: seedBuiltinProviders(builtins)
ProviderRepo->>Database: Filter existing providers
ProviderRepo->>Database: Insert missing builtins
Database-->>ProviderRepo: Seeded providers
ProviderRepo-->>APIRoute: Success + seeded count
APIRoute->>ProviderRepo: listEnabledProviders()
ProviderRepo->>Database: Query updated list
Database-->>ProviderRepo: Full provider list
ProviderRepo-->>APIRoute: Complete providers
else Seeding Fails
APIRoute->>APIRoute: Log warning, continue
end
APIRoute-->>Client: Return providers (stripped)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
The logger's warn() method expects LogInput (an object), not Error. Convert seedError to a message string in an object to fix the CI build type error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/lib/src/integrations/repositories/provider-repository.ts (2)
156-173: Consider wrapping sequential inserts in a transaction.The loop inserts providers sequentially without a transaction. If an insert fails mid-loop, some providers will be seeded while others won't, leaving the database in a partially seeded state. A transaction would ensure all-or-nothing semantics.
♻️ Suggested transaction wrapper
+ return database.transaction(async (tx) => { const seeded: IntegrationProvider[] = []; for (const builtin of toSeed) { - const [provider] = await database + const [provider] = await tx .insert(integrationProviders) .values({ slug: builtin.id, name: builtin.name, description: builtin.description ?? null, iconUrl: builtin.iconUrl ?? null, documentationUrl: builtin.documentationUrl ?? null, providerType: 'builtin', config: builtin as unknown as Record<string, unknown>, isSystem: true, enabled: true, }) .returning(); seeded.push(provider); } - return seeded; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/integrations/repositories/provider-repository.ts` around lines 156 - 173, The seeding loop currently inserts each provider sequentially (using database.insert(integrationProviders).values(...).returning()) and pushes results into the seeded array, which can leave a partial state if one insert fails; wrap the entire loop in a single database transaction so all inserts are committed together or rolled back on error. Locate the block that iterates over toSeed and performs database.insert(...) against integrationProviders, start a transaction via your database client (e.g., database.transaction or equivalent), perform all inserts inside that transactional callback, and on success return/assign the seeded results; ensure any thrown error causes the transaction to rollback and that you propagate or log the error accordingly.
167-167: Type cast loses type safety.The cast
builtin as unknown as Record<string, unknown>bypasses TypeScript's type checking. SinceIntegrationProviderConfigshould already be compatible with the JSON column type, consider using a more specific cast or ensuring the schema types align properly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/integrations/repositories/provider-repository.ts` at line 167, The current cast "builtin as unknown as Record<string, unknown>" on the config field loses type safety; instead ensure the value assigned to config is typed to the expected schema type (e.g., IntegrationProviderConfig) or narrow it with a specific cast/transform rather than double-casting to unknown. Update the assignment in provider-repository.ts so that the "builtin" value is either declared/validated as IntegrationProviderConfig (or the precise JSON column type used by your ORM) before assignment, or explicitly convert it to a safe Record<string, unknown> via a typed mapping/serialization step; reference the "config" property assignment and the "builtin" symbol when making the change.apps/web/src/lib/ai/core/__tests__/integration-tool-resolver.test.ts (1)
109-125: Consider adding test coverage forresolveGlobalAssistantIntegrationToolswith active grants.The test suite for
resolvePageAgentIntegrationToolsincludes both "no grants" and "active grants" scenarios, butresolveGlobalAssistantIntegrationToolsonly tests the "no grants" case. For symmetry and completeness, consider adding a test that verifies behavior when grants are present.💡 Example test case to add
it('given active grants, should convert to AI SDK tools', async () => { const mockGrants = [{ id: 'grant-1', agentId: null, connectionId: 'conn-1', allowedTools: null, deniedTools: null, readOnly: false, rateLimitOverride: null, connection: { id: 'conn-1', name: 'GitHub', status: 'active', providerId: 'prov-1', provider: { id: 'prov-1', slug: 'github', name: 'GitHub', config: { id: 'github', name: 'GitHub', tools: [], baseUrl: 'https://api.github.com', authMethod: { type: 'oauth2', config: {} } }, }, }, }]; mockResolveGlobalIntegrations.mockResolvedValue(mockGrants); const mockExecutor = vi.fn(); mockCreateExecutor.mockReturnValue(mockExecutor); mockConvert.mockReturnValue({ 'int__github__conn1234__list_repos': { description: '[GitHub] List repos', parameters: {} as never, execute: vi.fn(), }, }); const result = await resolveGlobalAssistantIntegrationTools({ userId: 'user-1', driveId: 'drive-1', userDriveRole: 'owner', }); expect(Object.keys(result)).toHaveLength(1); expect(mockConvert).toHaveBeenCalledWith( mockGrants, { userId: 'user-1', agentId: null, driveId: 'drive-1' }, mockExecutor ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/ai/core/__tests__/integration-tool-resolver.test.ts` around lines 109 - 125, Add a positive test for resolveGlobalAssistantIntegrationTools that mirrors the existing active-grants test for resolvePageAgentIntegrationTools: mock mockResolveGlobalIntegrations to return a sample grant array, mock mockCreateExecutor to return a mock executor, and mockConvert to return a mapped tool object; then call resolveGlobalAssistantIntegrationTools with a non-null driveId and userDriveRole and assert that the returned tool keys length and that mockConvert was invoked with (mockGrants, { userId: 'user-1', agentId: null, driveId: 'drive-1' }, mockExecutor) to verify conversion is performed.apps/web/src/app/api/integrations/providers/route.ts (1)
31-48: Consider race condition during concurrent first-access requests.If multiple requests hit this endpoint simultaneously when no providers exist, they could all pass the
providers.length === 0check before any completes seeding. WhileseedBuiltinProvidersis idempotent (skips existing slugs), this could result in:
- Multiple concurrent insert attempts for the same providers
- Duplicate log entries for "Auto-seeded builtin integration providers"
This is likely low-risk given the idempotent design, but worth noting for high-traffic scenarios.
One potential mitigation is to use a database-level constraint (unique on slug) and handle the constraint violation gracefully, or implement an application-level mutex/lock for the seeding operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/integrations/providers/route.ts` around lines 31 - 48, Concurrent requests can race through the providers.length === 0 branch and cause duplicate insert attempts and duplicate "Auto-seeded" logs; fix by making seeding resilient: ensure a DB-unique constraint on provider.slug and update seedBuiltinProviders to catch unique-constraint/duplicate-key errors and treat them as non-fatal (ignore duplicate errors), then have the caller (the block using listEnabledProviders and seedBuiltinProviders) re-query providers and only log when the final re-query returns newly inserted slugs (i.e., compute actualInserted = seeded.filter(s => !awaitExistingSlugs.includes(s.slug)) or simply rely on the post-seed listEnabledProviders() result to decide whether to log), so duplicate insert attempts are safely ignored and the "Auto-seeded builtin integration providers" log only occurs for real inserts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lib/src/integrations/repositories/provider-repository.ts`:
- Around line 147-152: The code currently only queries enabled providers via
database.query.integrationProviders.findMany({ where:
eq(integrationProviders.enabled, true) }) so disabled-but-existing slugs are
treated as missing; update the query to fetch all existing providers (or at
minimum select all slugs) from integrationProviders, build installedSlugs from
that full result, and then compute toSeed = builtins.filter(b =>
!installedSlugs.has(b.id)) to avoid inserting duplicate slugs for previously
disabled providers.
---
Nitpick comments:
In `@apps/web/src/app/api/integrations/providers/route.ts`:
- Around line 31-48: Concurrent requests can race through the providers.length
=== 0 branch and cause duplicate insert attempts and duplicate "Auto-seeded"
logs; fix by making seeding resilient: ensure a DB-unique constraint on
provider.slug and update seedBuiltinProviders to catch
unique-constraint/duplicate-key errors and treat them as non-fatal (ignore
duplicate errors), then have the caller (the block using listEnabledProviders
and seedBuiltinProviders) re-query providers and only log when the final
re-query returns newly inserted slugs (i.e., compute actualInserted =
seeded.filter(s => !awaitExistingSlugs.includes(s.slug)) or simply rely on the
post-seed listEnabledProviders() result to decide whether to log), so duplicate
insert attempts are safely ignored and the "Auto-seeded builtin integration
providers" log only occurs for real inserts.
In `@apps/web/src/lib/ai/core/__tests__/integration-tool-resolver.test.ts`:
- Around line 109-125: Add a positive test for
resolveGlobalAssistantIntegrationTools that mirrors the existing active-grants
test for resolvePageAgentIntegrationTools: mock mockResolveGlobalIntegrations to
return a sample grant array, mock mockCreateExecutor to return a mock executor,
and mockConvert to return a mapped tool object; then call
resolveGlobalAssistantIntegrationTools with a non-null driveId and userDriveRole
and assert that the returned tool keys length and that mockConvert was invoked
with (mockGrants, { userId: 'user-1', agentId: null, driveId: 'drive-1' },
mockExecutor) to verify conversion is performed.
In `@packages/lib/src/integrations/repositories/provider-repository.ts`:
- Around line 156-173: The seeding loop currently inserts each provider
sequentially (using
database.insert(integrationProviders).values(...).returning()) and pushes
results into the seeded array, which can leave a partial state if one insert
fails; wrap the entire loop in a single database transaction so all inserts are
committed together or rolled back on error. Locate the block that iterates over
toSeed and performs database.insert(...) against integrationProviders, start a
transaction via your database client (e.g., database.transaction or equivalent),
perform all inserts inside that transactional callback, and on success
return/assign the seeded results; ensure any thrown error causes the transaction
to rollback and that you propagate or log the error accordingly.
- Line 167: The current cast "builtin as unknown as Record<string, unknown>" on
the config field loses type safety; instead ensure the value assigned to config
is typed to the expected schema type (e.g., IntegrationProviderConfig) or narrow
it with a specific cast/transform rather than double-casting to unknown. Update
the assignment in provider-repository.ts so that the "builtin" value is either
declared/validated as IntegrationProviderConfig (or the precise JSON column type
used by your ORM) before assignment, or explicitly convert it to a safe
Record<string, unknown> via a typed mapping/serialization step; reference the
"config" property assignment and the "builtin" symbol when making the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.env.exampleapps/web/.env.exampleapps/web/src/app/api/integrations/providers/route.tsapps/web/src/lib/ai/core/__tests__/integration-tool-resolver.test.tspackages/lib/src/integrations/index.tspackages/lib/src/integrations/repositories/provider-repository.test.tspackages/lib/src/integrations/repositories/provider-repository.ts
The GrantWithConnectionAndProvider type has deeply nested IntegrationProviderConfig that's impractical to fully satisfy in test mocks. Use 'as unknown as' assertion for the mock data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…isions Previously queried only enabled providers, which could miss disabled ones and attempt to re-seed their slugs causing unique constraint violations. Now fetches all provider slugs regardless of enabled status. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
1 participant
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
OAUTH_STATE_SECRET,INTEGRATION_GITHUB_CLIENT_ID, andINTEGRATION_GITHUB_CLIENT_SECRETto.env.examplefiles so developers know what to configureseedBuiltinProviders()with lazy init inGET /api/integrations/providers— builtin providers auto-seed on first access, removing the admin-only install steproute.ts:535-552(already implemented — added tests)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Chores