feat: bring the Ask AI assistant to the community edition - #42065
Merged
Conversation
Ask AI (AI-assisted code editing in the JS/query editors, plus an admin page to configure the provider) shipped as an EE-only feature in appsmith-ee#8845, while CE received only inert stubs in #41692. Nothing about the feature is actually enterprise-specific: enablement is ordinary organization configuration set by an instance admin, and the EE client selectors carry no license or entitlement check. This ports it to CE. The implementation is taken from appsmith-ee/release as-is rather than from the original CE branch (feat/enable-ai, closed as #41590), because EE has since refactored it: AI settings moved from flat OrganizationConfiguration fields into a nested AIAssistantConfig document, provider dispatch was extracted, and datasource-schema enrichment moved from the client to AiDatasourceSchemaSerializerCE on the server. Porting EE's current version keeps CE and EE byte-identical on the ce-package files so the community sync stays a no-op. Server, all in ce packages behind the existing controller -> ce service -> ce_compatible -> ee override layering: - AIConfigControllerCE / AIConfigController for the /ai-config endpoints (test-connection, fetch-models, test-api-key), each gated on MANAGE_ORGANIZATION - AIConfigServiceCE(Impl), AIAssistantServiceCE(Impl), AIReferenceServiceCE(Impl) and their ee override points - AIAssistantConfig on OrganizationConfiguration, Migration075, and the AIProvider/DTO types - POST /users/ai-assistant/request on UserControllerCE - ai-references/*.md prompt reference resources Client: the implementation moves from src/ee into src/ce, since a CE-owned feature belongs there and the EE files had no EE-only imports. The existing src/ee shims from #41692 are left untouched and now re-export real code instead of stubs; this commit adds no src/ee files at all, because the CE pre-push architecture guard forbids it. The modules that therefore have no ee shim to import through (aiAssistantReducer, AIAssistantSagas, GPT/shared, the admin AI config) are imported from ce/ directly, each with a documented eslint-disable for no-restricted-imports — the same accommodation used by the native Custom Widget copilot in #42063. The AI reducer and saga are registered in ce/reducers and ce/sagas rather than their ee counterparts, and the admin AI settings page is registered for superusers. Adds react-markdown and remark-gfm, used by the assistant's response renderer. Verified: yarn tsc --noEmit introduces no new errors (the 7 reported in packages/ast and packages/design-system are present on an unmodified release checkout); prettier clean; eslint 0 errors (44 pre-existing-style react-perf warnings carried over from EE); jest src/ce/utils/aiSchemaSerializer.test.ts 20/20; mvn spotless:check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBx1z6GCvod1ENrBMVMcj7
Contributor
|
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:
WalkthroughThe change adds the Ask AI assistant across the client and server. It adds provider configuration, encrypted credentials, AI requests, Redux state, editor and global panels, administrator settings, schema serialization, reference content, migrations, rate limiting, and analytics. ChangesAsk AI assistant
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
The server did not compile. AIConfigServiceCEImpl references AnalyticsEvents.ASK_AI_ORG_CONFIG_UPDATED and ASK_AI_ORG_TEST_RUN, which exist in the enterprise copy of the enum but had never been added to CE's, so the port failed with "cannot find symbol" and a cascading Mono<Object> to Mono<Void> inference error in the same method. Both constants are added with the same names and event strings the enterprise repo uses, so the files stay aligned for the community sync. Also ports AIConfigServiceCEImplTest, which the enterprise repo carries and the first pass missed. It covers the config-save analytics payload, the API-key and local-LLM test-run events, and the skip path when analytics is inactive — exactly the code the missing constants sat in. Verified: mvn -pl appsmith-server -am compile clean; AIConfigServiceCEImplTest 7/7 and AiDatasourceSchemaSerializerCETest 5/5 green; spotless clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBx1z6GCvod1ENrBMVMcj7
… Ask AI Four defects found by review of the ported code. All four are inherited byte-identical from the enterprise repo and are therefore live there too; because EE's AIConfigServiceImpl delegates every method to the CE class and AIAssistantServiceImpl overrides nothing, fixing them here carries them to EE through the community sync rather than needing a parallel EE change. SSRF, two call sites. callLocalLLMAPI built a raw WebClient, bypassing WebClientUtils entirely, and substituted a hand-rolled check that tested only isLinkLocalAddress on the first resolved address — missing loopback, and racy besides, since the address it checked was not the one the client went on to resolve and connect to. callAzureOpenAIAPI looked protected but was not: it chained .clientConnector(...) after WebClientUtils.builder(), which replaces the connector that carries the DNS-aware resolver, leaving only the literal host check. Both now build through WebClientUtils.builder(httpClient). The 16 MB buffer the local path set by hand is already WebClientUtils' default, so that override is gone. Note the behaviour change this implies: an admin-supplied local LLM URL pointing at loopback is now refused, where before it was reachable. In the single-container CE deployment 127.0.0.1 is Mongo, Redis and RTS rather than the operator's Ollama, which is the reason to refuse it; a local model on another host or container remains reachable by hostname or private IP. This also makes the runtime path agree with /ai-config/test-connection, which already went through WebClientUtils — previously "Test connection" could fail against a URL the assistant would happily call. Authorization. getAIConfig was the only one of the five service methods without a MANAGE_ORGANIZATION check, so any authenticated user — including a viewer — could read localLlmUrl, azureOpenaiEndpoint, the deployment name and model identifiers, and the editor fetches this on every session. It now returns the full configuration to organization managers and, to everyone else, only what the client actually consumes: enablement, provider, and credential-presence booleans. hasLocalLlmUrl is added for that purpose so the URL itself no longer has to be sent. Credential storage. The @Encrypted annotations on AIAssistantConfig never applied: the encryption traversal only descends into AppsmithDomain types and both OrganizationConfigurationCE and AIAssistantConfig are plain Serializable, and separately the write is a sparse updateById rather than an entity save, so the lifecycle listener that performs encryption never fires. Keys were landing in Mongo, and in the Redis organization cache, in cleartext. Rather than change encryption traversal for the whole organization document and the write path for all organization configuration, AIConfigSecretsCE encrypts and decrypts at the few points these secrets are written and read. Values that do not decrypt are treated as legacy cleartext and passed through, so an instance keeps working between the upgrade and Migration076, which encrypts existing values in place and is idempotent for the same reason. Also fixes the admin key field that made the corruption possible: a stored key was loaded into the input as the literal string "••••••••" and the save guard was a comparison against that mask, so typing a new key without first clearing the field persisted "••••••••sk-..." behind a success toast. The input now holds only a newly typed key and "a key is stored" is tracked separately. Verified: mvn -pl appsmith-server -am compile clean; AIConfigServiceCEImplTest 7/7 and AiDatasourceSchemaSerializerCETest 5/5 green; tsc introduces no new errors; eslint 0 errors; prettier and spotless clean. Not covered by a test: AIConfigSecretsCE round-trip and the Migration076 idempotency check both go through EncryptionHelper, whose static initialiser requires APPSMITH_ENCRYPTION_PASSWORD and APPSMITH_ENCRYPTION_SALT. Those are set for the integration-test and Docker CI jobs but not for server-unit-tests, so a unit test there would fail on class initialisation. This belongs in the integration suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBx1z6GCvod1ENrBMVMcj7
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
![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: 20
🧹 Nitpick comments (12)
app/client/src/ce/components/editorComponents/GPT/index.tsx (1)
76-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
editoroptional, and avoid theReact.ReactElementcast.Line 84 guards with
editor &&, butTAIWrapperProps.editoris declared as requiredCodeMirror.Editor. The guard tells the reader thateditorcan be absent at runtime. Declare it optional so TypeScript agrees with the guard.Line 77 casts
childrentoReact.ReactElement.childrenisReact.ReactNode, so the cast is unsound forundefined, a string, or an array.🔧 Proposed changes
- editor: CodeMirror.Editor; + editor?: CodeMirror.Editor;if (!enableAIAssistance) { - return children as React.ReactElement; + return <>{children}</>; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/client/src/ce/components/editorComponents/GPT/index.tsx` around lines 76 - 92, Update TAIWrapperProps so editor is optional, matching the editor && guard before rendering AISidePanel. Replace the early-return React.ReactElement cast in the wrapper component with a type-safe return that preserves the original children without asserting an element type.app/client/src/ce/components/editorComponents/GPT/AISidePanel.tsx (2)
249-275: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRemove the 100 ms
setTimeoutinhandleQuickAction.The dispatch does not depend on the
setPromptstate update. It readsactionPrompt,editor,currentValue, andmodedirectly. The timer only delays the request and it is never cleared, so it can fire after unmount.Dispatch synchronously.
♻️ Proposed change
const handleQuickAction = useCallback( (actionPrompt: string) => { setPrompt(actionPrompt); - - setTimeout(() => { - if (!editor) return; - - const cursorPosition = editor.getCursor(); - const context = getAIContext({ - cursorPosition, - editor, - }); - - dispatch( - fetchAIResponse({ - prompt: actionPrompt, - context: { - ...context, - currentValue, - mode, - }, - }), - ); - }, 100); + + if (!editor) return; + + const cursorPosition = editor.getCursor(); + const context = getAIContext({ cursorPosition, editor }); + + dispatch( + fetchAIResponse({ + prompt: actionPrompt, + context: { ...context, currentValue, mode }, + }), + ); }, [editor, mode, currentValue, dispatch], );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/client/src/ce/components/editorComponents/GPT/AISidePanel.tsx` around lines 249 - 275, Update handleQuickAction to remove the 100 ms setTimeout and execute the editor/context lookup and fetchAIResponse dispatch synchronously after setPrompt. Preserve the existing editor guard and dependency list, using actionPrompt, editor, currentValue, and mode directly.
165-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo smaller points on state and props.
Line 165: the effect clears the conversation on every mount.
AIWindowmountsAISidePanelwhenever AI assistance is enabled, not only when the panel opens. Any remount of the editor wrapper therefore discards the conversation. The reducer already resets messages inOPEN_AI_PANEL_WITH_CONTEXT, so the reset now lives in two places.Line 177:
contextInfomemoiseseditor.getCursor()with dependencies[editor, mode]. The displayed line number does not update when the cursor moves.Line 287: the component returns
nullwhen closed, so theisOpenprop onPanelContainerand itsdisplay: nonerule never apply.Also applies to: 287-291
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/client/src/ce/components/editorComponents/GPT/AISidePanel.tsx` around lines 165 - 191, Remove the reset effect around clearAIResponse and setPrompt, relying on OPEN_AI_PANEL_WITH_CONTEXT for conversation resets instead of clearing on AISidePanel mounts or dependency changes. Update contextInfo so the cursor position and displayed line number react to editor cursor movement, rather than depending only on editor and mode. Keep AISidePanel mounted when closed and let PanelContainer use isOpen and its display behavior instead of returning null before rendering it.app/client/src/ce/api/OrganizationApi.ts (1)
134-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the seven positional parameters of
testApiKeywith a single options object.All parameters after
providerare optional strings. A caller can silently swapapiVersionandbaseUrl. The caller inapp/client/src/pages/AdminSettings/AI/index.tsx(Lines 778-794) already passes six nested ternaries in order.♻️ Proposed signature
- static async testApiKey( - provider: string, - apiKey?: string, - endpoint?: string, - deploymentName?: string, - apiVersion?: string, - baseUrl?: string, - model?: string, - ): Promise<AxiosPromise<ApiResponse<Record<string, unknown>>>> { - return Api.post(`${OrganizationApi.tenantsUrl}/ai-config/test-api-key`, { - provider, - apiKey, - endpoint, - deploymentName, - apiVersion, - baseUrl, - model, - }); - } + static async testApiKey(request: { + provider: string; + apiKey?: string; + endpoint?: string; + deploymentName?: string; + apiVersion?: string; + baseUrl?: string; + model?: string; + }): Promise<AxiosPromise<ApiResponse<Record<string, unknown>>>> { + return Api.post( + `${OrganizationApi.tenantsUrl}/ai-config/test-api-key`, + request, + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/client/src/ce/api/OrganizationApi.ts` around lines 134 - 152, Update OrganizationApi.testApiKey to accept provider plus a single options object containing the optional apiKey, endpoint, deploymentName, apiVersion, baseUrl, and model fields. Preserve the existing request payload mapping, and update the caller in the AI admin settings flow to pass these values by property name instead of positional arguments.app/client/src/ce/components/editorComponents/GPT/trigger.tsx (1)
7-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe two mode predicates diverge.
isAISupportedModeacceptsmode === "graphql"only.getAIContextalso acceptsmode?.includes("graphql"). A CodeMirror mode name such as"text/x-graphql"therefore gets a context window but no entry point, becauseisAIEnabledreturns false for it.mode === "sql"is also already covered bymode?.includes("sql").Extract one classification helper and call it from both places.
♻️ Proposed helper
+type AIModeKind = "javascript" | "query" | null; + +export function getAIModeKind(mode?: string): AIModeKind { + if (mode === "javascript") return "javascript"; + + if ( + mode?.includes("sql") || + mode?.includes("graphql") || + mode?.includes("json") + ) { + return "query"; + } + + return null; +}Also applies to: 61-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/client/src/ce/components/editorComponents/GPT/trigger.tsx` around lines 7 - 14, Unify mode classification by extracting a shared helper for supported AI modes, then use it in both isAISupportedMode and getAIContext/isAIEnabled. Ensure the helper recognizes GraphQL modes containing “graphql” as well as SQL and JSON variants, while preserving JavaScript support, so context generation and entry-point availability use identical predicates.app/client/src/pages/AdminSettings/AI/index.tsx (1)
517-598: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe mount effect writes state without an unmount guard.
fetchAIConfigruns an async request and then calls many setters. If the administrator navigates away before the response arrives, React logs an update-on-unmounted-component warning. Add anignoreflag or anAbortController.Line 570: the preset branch reads as inverted. When
matchingPresetis found, the code sets the preset tonull; when it is not found, it sets"custom". The behaviour is correct, but a short comment would help the next reader.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/client/src/pages/AdminSettings/AI/index.tsx` around lines 517 - 598, Add an unmount guard to the fetchAIConfigOnMount effect so fetchAIConfig cannot update state after navigation; check it before all setters, including the loading-state update in finally, or abort the request. Preserve existing success and error behavior while preventing post-unmount updates. Add a brief comment around the CONTEXT_PRESETS matchingPreset branch explaining that null represents a recognized preset and custom is used only when no preset matches.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration076EncryptAIAssistantApiKeys.java (1)
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a single field mapping instead of a list plus a string switch.
KEY_FIELDSandreadKeyhold the same field names in two places. If a provider is added toKEY_FIELDSonly,readKeyreturnsnulland the migration skips that credential without any signal. A single map of field name to getter removes that risk.♻️ Proposed refactor
- private static final List<String> KEY_FIELDS = - List.of("claudeApiKey", "openaiApiKey", "copilotApiKey", "azureOpenaiApiKey"); + private static final Map<String, Function<AIAssistantConfig, String>> KEY_FIELDS = Map.of( + "claudeApiKey", AIAssistantConfig::getClaudeApiKey, + "openaiApiKey", AIAssistantConfig::getOpenaiApiKey, + "copilotApiKey", AIAssistantConfig::getCopilotApiKey, + "azureOpenaiApiKey", AIAssistantConfig::getAzureOpenaiApiKey);Then iterate the entries and drop
readKey:- for (String field : KEY_FIELDS) { - String stored = readKey(aiConfig, field); + for (Map.Entry<String, Function<AIAssistantConfig, String>> entry : KEY_FIELDS.entrySet()) { + String field = entry.getKey(); + String stored = entry.getValue().apply(aiConfig);Also applies to: 93-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration076EncryptAIAssistantApiKeys.java` around lines 38 - 39, Replace the duplicated KEY_FIELDS list and readKey switch with one mapping from each credential field name to its corresponding getter. Update the migration loop to iterate mapping entries and read values directly, then remove readKey while preserving the existing encryption behavior.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.java (3)
77-84: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
getOrBuildWebClientnever caches.The name implies a lookup, but the method builds a new
WebClientand a newHttpClientfor every request that uses a non-default base URL. Lines 519-520 (local LLM) and Lines 697-698 (Azure OpenAI) build one per request unconditionally. Provider configuration changes rarely. Cache the clients in a small bounded map keyed by base URL, or rename the method tobuildWebClientso the cost is explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.java` around lines 77 - 84, Update getOrBuildWebClient and its callers for non-default base URLs so WebClient and HttpClient instances are reused instead of rebuilt on every request; either add a small bounded cache keyed by baseUrl while preserving cachedClient for default URLs, or rename the method to buildWebClient to accurately reflect uncached behavior.
402-414: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe OpenAI and Azure paths skip prompt validation.
callClaudeAPI(Lines 323-329) andcallLocalLLMAPI(Lines 464-470) reject an empty prompt and a prompt above 150000 characters.callOpenAIAPIandcallAzureOpenAIAPI(Lines 667-691) apply neither check. An empty prompt therefore produces a billed provider call, and an oversized prompt is only rejected after upload. Extract the two checks into one helper and call it from all four provider methods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.java` around lines 402 - 414, Extract the empty-prompt and 150000-character limit checks from callClaudeAPI and callLocalLLMAPI into a shared validation helper, then invoke it at the start of callOpenAIAPI and callAzureOpenAIAPI as well as the existing Claude and local LLM paths. Preserve the current rejection behavior and ensure validation occurs before any provider request or upload.
342-360: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSend the Claude system prompt through the
systemfield in both branches.On the first turn the system prompt is concatenated into the user message; on later turns it moves to the top-level
systemfield. The model receives different prompt structures for the same configuration. The Messages API acceptssystemunconditionally, so set it always and keep the user content clean. The!messages.isEmpty()clause on Line 358 is also always true, because Line 349 already appended a message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.java` around lines 342 - 360, Update the message construction in the Claude request flow to keep user content limited to userPrompt and always place systemPrompt in requestBody’s system field. Remove the first-turn concatenation and the redundant messages.isEmpty() check around the system assignment, while preserving conversationHistory handling as applicable.app/server/appsmith-server/src/main/java/com/appsmith/server/domains/AIAssistantConfig.java (1)
93-113: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe merge cannot clear a field.
ObjectUtils.defaultIfNullkeeps the existing value when the source field is null. An administrator therefore cannot remove a staleazureOpenaiEndpoint,azureOpenaiDeploymentName,localLlmUrl, or custom base URL once it is saved. If clearing must be supported, the update path needs an explicit tombstone (for example, treat empty string as "clear") or a full-replace semantic.Confirm the admin UI never needs to unset these fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/domains/AIAssistantConfig.java` around lines 93 - 113, The copyNonSensitiveValues method preserves existing values when source fields are null, preventing administrators from clearing previously saved configuration. Confirm the admin update contract and UI do not require unsetting these fields; if clearing is supported, replace the null-preserving merge with an explicit clear/tombstone or full-replace behavior for fields such as azureOpenaiEndpoint, azureOpenaiDeploymentName, localLlmUrl, and custom base URLs.app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/AiDatasourceSchemaSerializerCETest.java (1)
52-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a tier-3 (hard-truncation) test to match TS coverage.
The TypeScript suite tests the hard-truncation branch with a very small budget. This Java suite stops at tier 2 (
serializeLegacy_prioritizesTablesFromQueryOnly). Add a test with a budget smaller thantruncationNotice.length()to verifyserializeWithPrioritytruncates safely and does not throw when the budget is too small to fit the header.✅ Proposed test addition
`@Test` void serializeLegacy_truncatesWhenBudgetTooSmall() { DatasourceStructure structure = smallSchema(); String out = AiDatasourceSchemaSerializerCE.serializeLegacy(structure, "SELECT * FROM users", 30); assertThat(out.length()).isLessThanOrEqualTo(30); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/AiDatasourceSchemaSerializerCETest.java` around lines 52 - 60, Add a test alongside serializeLegacy_prioritizesTablesFromQueryOnly that calls AiDatasourceSchemaSerializerCE.serializeLegacy with smallSchema, the users query, and a budget below truncationNotice.length() (such as 30), then assert the result length is at most the requested budget to cover safe tier-3 truncation without throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/client/src/ce/components/editorComponents/GlobalAISidePanel/index.tsx`:
- Around line 245-267: Prevent overlapping AI requests by updating
handleQuickAction to return immediately when isLoading is true, and include
isLoading in its dependency array. Pass the loading state to QuickActionChip and
disable the chip while isLoading, preserving the existing dispatch behavior when
no request is active.
In `@app/client/src/ce/components/editorComponents/GPT/AISidePanel.tsx`:
- Around line 195-218: Update handleResizeMouseDown to store the active
onMouseMove and onMouseUp handlers in a ref, and add an unmount cleanup effect
that removes both document listeners and resets the drag state. Preserve the
existing mouseup cleanup while ensuring listeners cannot call setPanelWidth
after the component unmounts.
- Around line 310-318: In the AISidePanel component, add a document-level
keydown listener while the panel is open so pressing Escape invokes the existing
onClose callback. Reuse the component’s lifecycle cleanup pattern to remove the
listener when the panel closes or unmounts, while preserving the existing
handleKeyDown behavior for PromptInput.
In
`@app/client/src/ce/components/editorComponents/GPT/shared/AIMarkdownRenderer.tsx`:
- Around line 248-260: Update the code renderer’s isInline condition in
AIMarkdownRenderer to require both no language class and no newline in children,
so language-less fenced blocks render through CodeBlockWithCopy while truly
inline code remains inline.
In `@app/client/src/ce/sagas/AIAssistantSagas.ts`:
- Around line 203-257: The catch path in loadAISettingsSaga must not mark
configuration as successfully loaded after a transient fetch failure. Dispatch a
distinct failure action handled by the reducer without setting isConfigLoaded,
or otherwise preserve the unloaded state so implicit loads can retry; keep the
existing disabled-state reset while ensuring fetchAIResponseSaga does not
permanently report the assistant as disabled after a temporary error.
- Around line 130-182: Update the retry logic in the AI response saga around
UserApi.requestAIResponse to retry only transient failures: network errors,
timeouts, HTTP 429 responses, and 5xx responses. Classify both thrown errors and
unsuccessful response bodies before delaying; dispatch the existing error and
show the toast immediately for invalid credentials, bad requests, quota or other
non-transient failures. Replace the fixed retry delay with increasing backoff
between eligible attempts while preserving the existing success and final-error
handling.
In `@app/client/src/ce/utils/aiSchemaSerializer.ts`:
- Around line 83-94: Update the table serialization around the columns mapper in
the schema serializer so missing col.type values produce an empty type string,
matching AiDatasourceSchemaSerializerCE.serializeTable, instead of interpolating
the literal “undefined”. Preserve the existing PK and FK annotations and output
formatting for columns with defined types.
In `@app/client/src/pages/AdminSettings/AI/index.tsx`:
- Around line 956-964: Update the disabled-state logic for the Claude, OpenAI,
and Azure “Test Key” buttons to enable testing when either a newly entered API
key or the corresponding stored key exists. Preserve the existing loading-state
behavior and ensure the checks use each provider’s stored-key and input-key
symbols.
In `@app/client/src/sagas/ActionSagas.ts`:
- Around line 1199-1202: Update the slash-command flow in the relevant saga to
dispatch ReduxActionTypes.OPEN_AI_PANEL_WITH_CONTEXT instead of OPEN_AI_PANEL,
including the context payload expected by aiAssistantReducer. Preserve the
existing behavior of opening the AI panel while ensuring the reducer updates
context and clears stale conversation state when the entity or mode changes.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/constants/AIConstants.java`:
- Line 10: Update the DEFAULT_OPENAI_MODEL constant in AIConstants to a
currently supported OpenAI model instead of gpt-4, ensuring the default remains
valid for CE users who do not provide an override before the announced shutdown
date.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/AIConfigControllerCE.java`:
- Around line 37-48: Update the onErrorResume error handling in
AIConfigControllerCE so AppsmithException cases with ACL_NO_RESOURCE_FOUND
return HttpStatus.FORBIDDEN, while preserving HttpStatus.BAD_REQUEST for all
other errors.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.java`:
- Around line 215-231: Update requestAIResponse’s onErrorResume so AI failures
produce an actual non-2xx HTTP response rather than a normally completed
ResponseDTO with a 400 metadata field, preferably by propagating
AppsmithException to the global handler. In AIAssistantServiceCEImpl, ensure
getAIErrorMessage maps provider failures to fixed sanitized user-facing messages
and never returns exception messages containing upstream response bodies.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.java`:
- Around line 405-414: Require non-blank role and content on AIMessageDTO using
Jakarta validation, then reject or filter invalid conversation-history entries
before constructing provider payloads. Apply the guard consistently in the
message-building flows of AIAssistantServiceCEImpl, including the existing
OpenAI, local LLM, and Azure paths, while preserving valid message handling.
- Around line 370-383: The four AI provider error handlers in
AIAssistantServiceCEImpl must emit the existing mapped error even when the
response body is empty. Add an empty-body fallback such as defaultIfEmpty("")
before each bodyToMono(String.class).flatMap chain at the handlers around the
visible status-processing blocks, preserving the current status-code-specific
exceptions.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCEImpl.java`:
- Around line 261-268: Update the errorSummary handling in
testLlmConnectionInternal to store a fixed classified error code rather than
String.valueOf(error), preserving the existing truncation only if still
applicable. Ensure values derived from connection or unexpected exception
messages cannot reach analytics, while retaining distinct classifications for
the supported error cases.
- Around line 910-911: Replace manual provider JSON string construction in both
payload-building branches with an objectMapper-built JSON tree or equivalent
serialization so model is escaped correctly. In the success branches, parse each
provider response with objectMapper.readTree(responseBody) and retrieve the
response field through JSON navigation rather than indexOf/substring scanning.
Apply this consistently to the request and response handling locations
identified in the comment, preserving the existing success and error behavior.
- Line 908: Update the diagnostic steps in testOpenAIKey so the “API Key Format”
entry reflects an actual validation performed by the method; remove or replace
the hard-coded “Key starts with 'sk-'” success message unless the key prefix is
explicitly checked, and ensure custom baseUrl configurations are not incorrectly
constrained by this diagnostic.
- Around line 377-379: Update the URL validation in AIConfigServiceCEImpl’s
parse block to reject null schemes and any scheme other than http or https
before calculating the default port. Ensure the validation produces the method’s
existing structured failure response, and only perform the HTTPS comparison
after the scheme has been validated.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIReferenceServiceCEImpl.java`:
- Around line 44-61: Update getReferenceContent to accept only modes present in
the existing SUPPORTED_MODES collection, returning an empty result for
unsupported client-supplied values before cache lookup or loadReference;
normalize with Locale.ROOT. Update warmCache to iterate SUPPORTED_MODES rather
than maintaining a separate literal mode list, ensuring only supported modes are
cached.
In
`@app/server/appsmith-server/src/main/resources/ai-references/sql-reference.md`:
- Around line 16-23: Replace the concatenated Input1.text WHERE example in the
Conditional Binding section with a parameterized boolean-short-circuit pattern,
and explicitly state not to concatenate raw input into SQL. Update the Dynamic
table name binding comment to require validating the selected identifier against
a known allow-list because identifiers cannot be parameterized.
---
Nitpick comments:
In `@app/client/src/ce/api/OrganizationApi.ts`:
- Around line 134-152: Update OrganizationApi.testApiKey to accept provider plus
a single options object containing the optional apiKey, endpoint,
deploymentName, apiVersion, baseUrl, and model fields. Preserve the existing
request payload mapping, and update the caller in the AI admin settings flow to
pass these values by property name instead of positional arguments.
In `@app/client/src/ce/components/editorComponents/GPT/AISidePanel.tsx`:
- Around line 249-275: Update handleQuickAction to remove the 100 ms setTimeout
and execute the editor/context lookup and fetchAIResponse dispatch synchronously
after setPrompt. Preserve the existing editor guard and dependency list, using
actionPrompt, editor, currentValue, and mode directly.
- Around line 165-191: Remove the reset effect around clearAIResponse and
setPrompt, relying on OPEN_AI_PANEL_WITH_CONTEXT for conversation resets instead
of clearing on AISidePanel mounts or dependency changes. Update contextInfo so
the cursor position and displayed line number react to editor cursor movement,
rather than depending only on editor and mode. Keep AISidePanel mounted when
closed and let PanelContainer use isOpen and its display behavior instead of
returning null before rendering it.
In `@app/client/src/ce/components/editorComponents/GPT/index.tsx`:
- Around line 76-92: Update TAIWrapperProps so editor is optional, matching the
editor && guard before rendering AISidePanel. Replace the early-return
React.ReactElement cast in the wrapper component with a type-safe return that
preserves the original children without asserting an element type.
In `@app/client/src/ce/components/editorComponents/GPT/trigger.tsx`:
- Around line 7-14: Unify mode classification by extracting a shared helper for
supported AI modes, then use it in both isAISupportedMode and
getAIContext/isAIEnabled. Ensure the helper recognizes GraphQL modes containing
“graphql” as well as SQL and JSON variants, while preserving JavaScript support,
so context generation and entry-point availability use identical predicates.
In `@app/client/src/pages/AdminSettings/AI/index.tsx`:
- Around line 517-598: Add an unmount guard to the fetchAIConfigOnMount effect
so fetchAIConfig cannot update state after navigation; check it before all
setters, including the loading-state update in finally, or abort the request.
Preserve existing success and error behavior while preventing post-unmount
updates. Add a brief comment around the CONTEXT_PRESETS matchingPreset branch
explaining that null represents a recognized preset and custom is used only when
no preset matches.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/domains/AIAssistantConfig.java`:
- Around line 93-113: The copyNonSensitiveValues method preserves existing
values when source fields are null, preventing administrators from clearing
previously saved configuration. Confirm the admin update contract and UI do not
require unsetting these fields; if clearing is supported, replace the
null-preserving merge with an explicit clear/tombstone or full-replace behavior
for fields such as azureOpenaiEndpoint, azureOpenaiDeploymentName, localLlmUrl,
and custom base URLs.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration076EncryptAIAssistantApiKeys.java`:
- Around line 38-39: Replace the duplicated KEY_FIELDS list and readKey switch
with one mapping from each credential field name to its corresponding getter.
Update the migration loop to iterate mapping entries and read values directly,
then remove readKey while preserving the existing encryption behavior.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.java`:
- Around line 77-84: Update getOrBuildWebClient and its callers for non-default
base URLs so WebClient and HttpClient instances are reused instead of rebuilt on
every request; either add a small bounded cache keyed by baseUrl while
preserving cachedClient for default URLs, or rename the method to buildWebClient
to accurately reflect uncached behavior.
- Around line 402-414: Extract the empty-prompt and 150000-character limit
checks from callClaudeAPI and callLocalLLMAPI into a shared validation helper,
then invoke it at the start of callOpenAIAPI and callAzureOpenAIAPI as well as
the existing Claude and local LLM paths. Preserve the current rejection behavior
and ensure validation occurs before any provider request or upload.
- Around line 342-360: Update the message construction in the Claude request
flow to keep user content limited to userPrompt and always place systemPrompt in
requestBody’s system field. Remove the first-turn concatenation and the
redundant messages.isEmpty() check around the system assignment, while
preserving conversationHistory handling as applicable.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/AiDatasourceSchemaSerializerCETest.java`:
- Around line 52-60: Add a test alongside
serializeLegacy_prioritizesTablesFromQueryOnly that calls
AiDatasourceSchemaSerializerCE.serializeLegacy with smallSchema, the users
query, and a budget below truncationNotice.length() (such as 30), then assert
the result length is at most the requested budget to cover safe tier-3
truncation without throwing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3db90930-c4c3-4d5f-924f-6f39404e6a5b
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (65)
app/client/package.jsonapp/client/src/ce/api/OrganizationApi.tsapp/client/src/ce/api/UserApi.tsxapp/client/src/ce/components/editorComponents/GPT/AISidePanel.tsxapp/client/src/ce/components/editorComponents/GPT/AskAIButton.tsxapp/client/src/ce/components/editorComponents/GPT/index.tsxapp/client/src/ce/components/editorComponents/GPT/shared/AIMarkdownRenderer.tsxapp/client/src/ce/components/editorComponents/GPT/shared/constants.tsapp/client/src/ce/components/editorComponents/GPT/shared/helpers.tsapp/client/src/ce/components/editorComponents/GPT/shared/index.tsapp/client/src/ce/components/editorComponents/GPT/shared/styledComponents.tsapp/client/src/ce/components/editorComponents/GPT/shared/types.tsapp/client/src/ce/components/editorComponents/GPT/trigger.tsxapp/client/src/ce/components/editorComponents/GlobalAISidePanel/index.tsxapp/client/src/ce/pages/AdminSettings/config/ai.tsxapp/client/src/ce/pages/AdminSettings/config/index.tsapp/client/src/ce/pages/AdminSettings/config/types.tsapp/client/src/ce/reducers/aiAssistantReducer.tsapp/client/src/ce/reducers/index.tsxapp/client/src/ce/sagas/AIAssistantSagas.tsapp/client/src/ce/sagas/index.tsxapp/client/src/ce/selectors/aiAssistantSelectors.tsapp/client/src/ce/utils/aiSchemaSerializer.test.tsapp/client/src/ce/utils/aiSchemaSerializer.tsapp/client/src/pages/AdminSettings/AI/index.tsxapp/client/src/sagas/ActionSagas.tsapp/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/AnalyticsEvents.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/constants/AIConstants.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/AIConfigController.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/UserController.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/AIConfigControllerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/OrganizationControllerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/domains/AIAssistantConfig.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/domains/AIProvider.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AIConfigDTO.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AIEditorContextDTO.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AIMessageDTO.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/dtos/AIRequestDTO.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/AIConfigSecretsCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/AiDatasourceSchemaSerializerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration075AddIsAIAssistantEnabledToOrganizationConfiguration.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration076EncryptAIAssistantApiKeys.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/AIAssistantService.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/AIAssistantServiceImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/AIConfigService.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/AIConfigServiceImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/AIReferenceService.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/AIReferenceServiceImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIReferenceServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIReferenceServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/AIConfigServiceCECompatible.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/AIConfigServiceCECompatibleImpl.javaapp/server/appsmith-server/src/main/resources/ai-references/README.mdapp/server/appsmith-server/src/main/resources/ai-references/common-issues.mdapp/server/appsmith-server/src/main/resources/ai-references/graphql-reference.mdapp/server/appsmith-server/src/main/resources/ai-references/javascript-reference.mdapp/server/appsmith-server/src/main/resources/ai-references/sql-reference.mdapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/AiDatasourceSchemaSerializerCETest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/AIConfigServiceCEImplTest.java
…AI request cost A nine-seat review blocked this branch on two security claims the code did not actually deliver, plus a crash path and an unbounded cost path. Each is fixed at the cause and covered by a test that fails on the unpatched code. getAIConfig's non-manager branch was unreachable. OrganizationServiceCEImpl.findById ends in switchIfEmpty(Mono.error(NO_RESOURCE_FOUND)), so it SIGNALS rather than completing empty, and a switchIfEmpty fallback beneath it could never run. The disclosure was closed, but every non-manager received an error instead of the enablement flags the editor reads on session start, and buildAIConfigStatusResponse was dead code. Note this was dead in BOTH editions — EE's copy of OrganizationServiceCEImpl.findById is identical, so there is no CE/EE behavioural divergence to reconcile, contrary to the initial diagnosis. Resolved with a narrowed onErrorResume on the not-found/denied codes so a genuine failure still surfaces. Provider credentials could still be written in cleartext. PUT /organizations binds the whole OrganizationConfiguration and @JSONVIEW does not filter a request body unless a view is active, so the Views.Internal key fields deserialize there and the sparse updateById persisted them raw, bypassing the encryption /ai-config applies. AIConfigSecretsCE.decrypt's legacy-cleartext fallback meant nothing ever looked broken. Encryption is now normalised on the way into storage rather than being a property of one code path. A scheme-relative URL crashed the connection test outside its error handling. URI.create("//host/x") yields a host but a null scheme, clearing the host guard and then throwing on getScheme().equals(), before any Mono exists — so it escaped the structured error response entirely. The scheme is now validated inside the parse block, matching the runtime path which already did this correctly. The port derivation was also made case-insensitive so HTTPS:// no longer resolves to 80. The AI endpoint had no cost control. Any authenticated organization member, including a view-only user, could loop it and spend the organization's third-party LLM budget at whatever rate the client could issue requests. A per-user bucket (20/min) is now spent before any provider work. It is setter-injected deliberately: EE's AIAssistantServiceImpl calls super(...) with six arguments, and adding a seventh would break EE's compile the moment this file syncs; EE instantiates the subclass as a Spring bean, so setter injection resolves in both editions. Not addressed here: an authorization gate on the AI endpoint. Choosing the wrong permission would lock out legitimate developers, and which role may use AI is a product decision. The rate limit addresses the abuse vector, and the entity-scoped path already enforces execute permission. The PR description now carries the required CE→EE sync resolution, because two of the sync's failure modes arrive through clean merges that no conflict marker will surface: EE would register the AI saga and admin category twice (duplicate provider requests, duplicate sidebar entry), and Migration076 would encrypt EE's keys while EE's readers still expect cleartext. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NTAwi7o4KMo8cNRm8hUsL
Follows the blocker commit with the non-blocking findings from the council and CodeRabbit. The reference-mode lookup was an unbounded, client-keyed cache. `mode` arrives straight from the client and was both the cache key and part of a classpath resource path, in a ConcurrentHashMap with no eviction — an authenticated loop over random modes grew the heap without bound. An allowlist of the three modes that actually have a bundled reference closes that and the unvalidated path construction together, and warmCache now shares the same source of truth. Locale.ROOT so a Turkish-locale server does not fold "I" and miss the list. The local-LLM copy pushed operators toward disabling SSRF protection instance-wide. Both the admin placeholder and the server's suggestion list told them to use http://localhost:11434 — an address the filter now refuses, whose only apparent remedy is APPSMITH_DISABLE_SSRF_FILTER=true, which turns the filter off for every datasource on the instance. They now suggest host.docker.internal and explain that inside the container loopback is Appsmith's own Mongo, Redis and RTS. The connection test was an internal network scanner with a read oracle. matchesBlockedAddressClass deliberately permits RFC1918, and the response carried resolvedIp plus a 500-char responsePreview, so a manager could map the private network and read back what answered. Those fields are gone, and the DNS step now reports THAT the host resolved rather than what it resolved to — the step message was leaking the same address the dropped field did. sql-reference.md is loaded into the assistant's system prompt, so its examples are training data. It taught three injectable patterns, not the one reported: string-concatenating user input into a WHERE clause, wrapping a binding in quotes (which defeats parameterization, since only standalone bindings are sent as parameters), and interpolating a widget value into a table position. Replaced with correct/incorrect pairs, a safe optional-filter idiom, and an explicit warning that identifiers cannot be parameterized. Provider error bodies no longer reach API responses. "Local LLM request failed: " + errorBody put unbounded upstream text — attacker-influencable via a self-hosted "local LLM" — into the message that getAIErrorMessage returns to the client. Logged server-side, with a stable message returned. Both new controllers completed normally with a 400 inside the body, making the wire status 200 for every failure: clients branching on HTTP status saw success and error metrics stayed clean. Neither is house convention — they are the only two such sites in the controllers, both added by this PR — so they now signal the error. The AI-config path also distinguishes an authorization refusal from bad input rather than collapsing both to 400. Migration076 matched every organization, because Migration075 gives them all an aiAssistantConfig, and loaded full documents into memory. Narrowed to organizations that actually hold a credential. Client: retries are classified, so an invalid key, a quota rejection or a bad request no longer costs a second completion call; quick-action chips are disabled and guarded while a request is in flight; a fenced code block with no language renders as a block again rather than inline; img is overridden with the same scheme allowlist as anchors so model output cannot make the viewer's browser fetch from a server the model chose; and a column with no type no longer serializes the literal "undefined" into the schema sent to the model. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NTAwi7o4KMo8cNRm8hUsL
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/client/src/ce/components/editorComponents/GPT/shared/AIMarkdownRenderer.tsx`:
- Around line 248-258: Update the img renderer override in AIMarkdownRenderer so
model-generated remote image URLs are not requested: permit only same-origin
assets or approved controlled-proxy URLs, while rejecting external http/https
sources. Replace the current SAFE_URL_PATTERN-only validation for safeSrc and
preserve the existing null return for disallowed images and referrer policy for
allowed ones.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fd1d92f-28c3-4933-bc41-d899d33968d9
📒 Files selected for processing (12)
app/client/src/ce/components/editorComponents/GPT/shared/AIMarkdownRenderer.tsxapp/client/src/ce/components/editorComponents/GlobalAISidePanel/index.tsxapp/client/src/ce/sagas/AIAssistantSagas.tsapp/client/src/ce/utils/aiSchemaSerializer.tsapp/client/src/pages/AdminSettings/AI/index.tsxapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/AIConfigControllerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration076EncryptAIAssistantApiKeys.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIReferenceServiceCEImpl.javaapp/server/appsmith-server/src/main/resources/ai-references/sql-reference.md
🚧 Files skipped from review as they are similar to previous changes (7)
- app/client/src/ce/utils/aiSchemaSerializer.ts
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration076EncryptAIAssistantApiKeys.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/UserControllerCE.java
- app/client/src/pages/AdminSettings/AI/index.tsx
- app/client/src/ce/components/editorComponents/GlobalAISidePanel/index.tsx
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIConfigServiceCEImpl.java
… to work on Hacktron flagged the AI request endpoint as reachable by any authenticated user, including a read-only WORKSPACE_VIEWER, which matches what the security review raised independently. Two sources on the same gap, so this tightens it rather than leaving it to the rate limit alone. The entity-scoped path resolved context.entityId with getExecutePermission(). Execute is a viewer-level permission, so a read-only member could point the assistant at an entity they cannot modify and have it rewrite the query — spending the organization's provider credits to do it. The assistant is an authoring tool, so the bar is the permission for changing the entity, not for running it: now getEditPermission(). This does not close the context-free case, where no entity is supplied and there is nothing to scope a permission check against. That path stays bounded by the per-user rate limit added earlier. Closing it properly means deciding whether the assistant should be developer-only instance-wide, which is a product call rather than something to infer here — picking a permission wrong locks out the developers the feature exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NTAwi7o4KMo8cNRm8hUsL
Contributor
Author
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/30831224825. |
The img override added in the previous commit only validated the URL scheme, which is not enough. An attacker-controlled https:// tracking pixel passes a scheme allowlist and still causes the viewer's browser to fetch it, handing over IP address, user agent and request timing — referrerPolicy="no-referrer" suppresses the Referer header, not the request itself. Images are now rendered only from the same origin, so nothing the model writes can cause an outbound fetch. A remote source degrades to its alt text rather than disappearing, so the user can still see that something was referenced. Caught by CodeRabbit on the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NTAwi7o4KMo8cNRm8hUsL
|
Deploy-Preview-URL: https://ce-42065.dp.appsmith.com |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.java`:
- Around line 287-292: Update the permission validation flow around
findActionDTObyIdAndViewMode and getAIResponse so an empty EDIT-permission
lookup terminates with an authorization error rather than falling back via
switchIfEmpty(Mono.just(context)). Ensure callers without EDIT permission cannot
dispatch to the provider or receive an AI response, while preserving the
existing authorized path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b3c9e09-c260-4cb4-bcd5-8315b11b08b3
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AIAssistantServiceCEImpl.java
…equest The previous commit tightened the entity lookup in enrichContextWithDatasourceSchema from EXECUTE to EDIT, but that method is built to degrade gracefully: it ends in switchIfEmpty(Mono.just(context)) and onErrorResume, so a missing datasource costs the prompt its schema rather than failing the request. That fallback swallows an EMPTY result identically — and a permission-filtered lookup returns empty precisely when the caller lacks the permission. So the tightening gated only whether schema was attached, never whether the request ran. A caller without edit rights still received an answer and still spent the organization's provider credits. That is the same shape as the getAIConfig defect fixed earlier on this branch: a check positioned where it cannot refuse anything. Worth naming, because it is clearly easy to write twice. The authorization decision now lives in its own gate ahead of dispatch, where an empty lookup is an error rather than a fallback. Enrichment keeps its graceful degradation, which is correct for the schema it is actually responsible for. Unchanged and still deliberate: a request carrying no entity id has nothing to scope a check against and remains bounded by the per-user rate limit. Caught by CodeRabbit on the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NTAwi7o4KMo8cNRm8hUsL
…uthentication Ask AI only exists in the editor — there is no surface for it in a deployed application — so any caller who cannot edit the thing they are asking about has no legitimate route to this endpoint. Until now the endpoint accepted any authenticated organization member and merely rate-limited them. The gate added previously only covered requests carrying an entityId. That left a real developer flow unauthorized: the panel opens from a widget property binding too, where entityInformation supplies no entityId, so those requests fell through with nothing checked. The context also carried no application scope, so the server had nothing else to authorize against. AIEditorContextDTO now carries applicationId, and authorization resolves in that order: edit rights on the entity when there is one, otherwise edit rights on the application whose editor the request came from. A request that identifies neither is refused rather than allowed, because an unscoped request cannot be shown to come from a developer and this endpoint spends the organization's provider credits. The client sends applicationId from the existing getCurrentApplicationId selector. This closes the gap left open in the previous two commits and flagged on the Hacktron and CodeRabbit threads, now that the product question behind it is settled: the feature is developer-only by construction, so gating it cannot lock out a legitimate user. Note for the CE→EE sync: AIEditorContextDTO was byte-identical with EE and now differs, so it joins the set of files the sync-resolution section of the PR description covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NTAwi7o4KMo8cNRm8hUsL
A CE-owned feature registers its admin category in ce/pages/AdminSettings/config, and an edition that also registers the same category in its own ee/ index calls ConfigFactory.register twice for one category. Those two files merge cleanly during a CE→EE sync, so nothing surfaces the duplicate until someone opens admin settings and sees the entry listed twice. register() was a raw push into three collections, and all three accumulated: categories (the visible duplicate in the sidebar), settings (a duplicated row), and savableCategories (a duplicated save target). Only settingsMap was safe, because it is keyed rather than appended. Guarding at the single entry point rather than inside each collection keeps the three consistent — a category is either fully registered or not at all — and makes a duplicate registration harmless instead of something every caller has to remember to avoid. All 13 existing callers go through register(); nothing calls the two sub-methods directly. This removes one of the three steps the CE→EE sync section of this PR describes: the duplicate Ask AI admin category now collapses on its own, with no EE-side change. The duplicate saga registration in the same section still needs one, because EE appends its own entry after spreading CE's list and CE cannot reach into that array. Tests cover all three collections and fail without the guard, plus a control asserting genuinely distinct categories still register. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NTAwi7o4KMo8cNRm8hUsL
Contributor
Author
|
@subrata71 Yes — all of them are now addressed in this PR.
|
subrata71
approved these changes
Aug 4, 2026
subrata71
left a comment
subrata71
left a comment
Collaborator
There was a problem hiding this comment.
The changes look good. Hope it won't cause any regressions once it gets synced to EE post successful conflict resolution. Godspeed!
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
TL;DR — Ask AI (AI-assisted code editing in the JS/query editors, plus an admin page to configure the AI provider) currently exists only in the enterprise edition. Nothing about it is actually enterprise-specific, so this brings it to CE. The code is ported from
appsmith-ee/releaseunchanged where it already lives incepackages, and moved fromsrc/eeintosrc/cewhere it does not.Background
Ask AI was originally built for CE on
feat/enable-ai(PR #41590). That PR was handed over for review in March, went quiet, and was closed by the stale bot on 2026-04-03 without ever being merged. The feature instead shipped in the enterprise repo asappsmith-ee#8845, and CE received only inert stubs via #41692 —ce/selectors/aiAssistantSelectors.tsand friends returningnull/false/[].That split was not driven by any technical requirement:
ee/selectors/aiAssistantSelectors.tscontains no license or entitlement check — it readsstate.aiAssistantand nothing else.AIAssistantConfig.isAIAssistantEnabled), set by an instance admin.appsmith-ee#9119.The enterprise-only placement was a product decision, and this PR reverses it.
Why port from EE rather than revive the original branch
feat/enable-aiis four months stale and EE has since reworked the implementation:feat/enable-aiappsmith-ee/releaseOrganizationConfigurationAIAssistantConfigdocumentifchaindispatchToProviderAiDatasourceSchemaSerializerCE/ai-configendpointsOrganizationControllerCEAIConfigControllerCE+AIConfigServiceCEAIReferenceServiceCEImplReviving the branch would land a divergent second implementation and guarantee a conflict with the community sync. Porting EE's current version starts CE and EE byte-identical on the
ce-package files. Note the security commit and the review fixes on top of it deliberately move CE ahead of EE, so the sync is no longer a no-op — see CE→EE sync: required resolution below for the exact steps.Architecture
Server — follows the existing
controller → ce service → ce_compatible → ee overridelayering, all incepackages:AIConfigControllerCE/AIConfigControllerfor/ai-config(test-connection,fetch-models,test-api-key), each gated onMANAGE_ORGANIZATIONAIConfigServiceCE(Impl),AIAssistantServiceCE(Impl),AIReferenceServiceCE(Impl)with theireeoverride points andAIConfigServiceCECompatible(Impl)AIAssistantConfigonOrganizationConfiguration,Migration075, and theAIProvider/ DTO typesPOST /users/ai-assistant/requestonUserControllerCEai-references/*.mdprompt-reference resourcesClient — the implementation moves from
src/eeintosrc/ce, which is where a CE-owned feature belongs. This is safe because the EE UI files import nothing EE-only; every import is a package, a shared path, or anee/alias that resolves through CE's shims. The existingsrc/eeshims from #41692 are untouched and now re-export real code instead of stubs, and the exported symbol surface of every relocated file is unchanged. The AI reducer and saga are registered ince/reducersandce/sagasrather than theireecounterparts, and the admin AI settings page is registered for superusers.This PR adds no
src/eefiles, because the CE pre-push architecture guard rejects them. Four CE-owned modules therefore have noeeshim to import through —aiAssistantReducer,AIAssistantSagas,GPT/shared, and the admin AI config — so they are imported fromce/directly, each with a documentedeslint-disableforno-restricted-imports. That is the same accommodation the native Custom Widget copilot uses in #42063. If EE would rather route these throughee/shims, those shims belong in a companion EE PR.Adds
react-markdownandremark-gfm, used by the assistant's response renderer.Impact on existing instances
Inert by default.
AIAssistantConfigis absent until an admin configures a provider,isAIAssistantEnableddefaults to false, andMigration075only adds the field. With nothing configured, the Ask AI affordances stay hidden exactly as they do today — no feature flag is involved, matching how EE ships it since #9119.Security fixes included
A nine-reviewer council on the ported code surfaced four defects. All four are inherited byte-identical from EE and are therefore live in EE production today; because EE's
AIConfigServiceImpldelegates every method to the CE class andAIAssistantServiceImploverrides nothing, fixing them here carries them into EE through the sync rather than needing a parallel EE change.callLocalLLMAPIbuilt a rawWebClient, bypassingWebClientUtilsand substituting a check that tested onlyisLinkLocalAddresson the first resolved address — missing loopback, and racy.callAzureOpenAIAPIchained.clientConnector(...)afterWebClientUtils.builder(), which replaces the connector carrying the DNS-aware resolver; it read as protected and was not. Both now build throughWebClientUtils.builder(httpClient).getAIConfigauthorization. It was the only one of five service methods withoutMANAGE_ORGANIZATION, disclosinglocalLlmUrl,azureOpenaiEndpointand the deployment name to any authenticated user, on every session. Managers still get the full configuration; everyone else gets enablement, provider, and credential-presence booleans — exactly what the client consumes.@Encryptedannotations never applied — the traversal only descends intoAppsmithDomaintypes, and the write is a sparseupdateByIdso the encrypting lifecycle listener never fires.AIConfigSecretsCEnow encrypts and decrypts at the few write/read points, andMigration076encrypts existing values in place, idempotently.••••••••with a save guard comparing against that mask, so typing without clearing persisted••••••••sk-…behind a success toast.Behaviour change worth calling out: a local-LLM URL pointing at loopback is now refused. In the single-container CE deployment
127.0.0.1is Mongo, Redis and RTS rather than the operator's Ollama — which is the reason to refuse it. A local model on another host or container stays reachable by hostname or private IP. This also makes the runtime path agree with/ai-config/test-connection, which already went throughWebClientUtils.Follow-ups (tracked, not addressed here)
/users/ai-assistant/requesthas no rate limit or per-user quota; on an open-signup CE instance any account can drain the admin's provider billing.max_tokens./users/ai-assistant/requestmaps every failure to 400, including upstream timeouts, which hurts monitoring.AiDatasourceSchemaSerializerCE.extractReferencedTableNamesat the DTO's own size ceilings.AIWindowand the in-editorAISidePanelhave no importer, andce/utils/aiSchemaSerializer.tshas no consumer but its own test.AIConfigSecretsCEandMigration076both route throughEncryptionHelper, whose static initialiser needsAPPSMITH_ENCRYPTION_PASSWORD/SALT. Those are set for the integration-test and Docker CI jobs but not forserver-unit-tests, so this coverage belongs in the integration suite.A follow-up in
appsmith-eeshould reduce EE'ssrc/eeAsk AI files to re-export shims and drop itsee/reducers+ee/sagasregistration, so EE consumes this CE implementation instead of shadowing it.https://linear.app/appsmith/issue/APP-15737
Supersedes the original, stale-closed CE attempt in #41590.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/30836147916
Commit: deab697
Cypress dashboard.
Tags:
@tag.AllSpec:
Mon, 03 Aug 2026 18:47:45 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Ask AI becoming available in the community edition is a user-facing change worth announcing.
CE→EE sync: required resolution
This section is load-bearing. The sync of this PR is not a no-op, and two of its failure modes arrive through clean merges — no conflict marker will surface them. Whoever runs the sync should follow this, and it is the condition the architecture review set for unblocking.
Of the changed files that also exist in EE, 18 differ from EE's copy. Most are the deliberate hardening in the security commit, which moves CE ahead of EE on purpose. Five are add/add conflicts (
ce/sagas/AIAssistantSagas.ts,ce/pages/AdminSettings/config/ai.tsx,pages/AdminSettings/AI/index.tsx,AIAssistantServiceCEImpl.java,AIConfigServiceCEImpl.java).1. Remove EE's now-duplicate saga registration
CE now registers the AI saga itself, and that CE file merges cleanly into EE — so EE ends up registering it twice:
app/client/src/ee/sagas/index.tsxaiAssistantSagasimport (fromee/sagas/AIAssistantSagas, a shim that re-exportsce/sagas/AIAssistantSagas) and its entry in the saga array — CE's registration now covers EELeft as-is, EE runs the same watcher generator twice, so every Ask AI action fires duplicate requests to the provider — double latency and double spend.
This one genuinely needs an EE change: EE builds
sagasArras[...CE_Sagas, …, aiAssistantSagas], appending its own entry after spreading CE's list, so CE cannot deduplicate it from its side.The duplicate admin category no longer needs an EE change.
ConfigFactory.registeris CE-owned and was a raw push into three collections (categories,settings,savableCategories— onlysettingsMapwas keyed and therefore safe). It is now idempotent, so a category registered from bothce/andee/collapses to one entry on its own. EE'sConfigFactory.register(AIConfig)can stay exactly as it is.2. Resolve the five add/add conflicts toward CE, wholesale
Migration076EncryptAIAssistantApiKeysmerges cleanly and will encrypt EE's stored keys on first boot. EE's currentAIAssistantServiceCEImpl/AIConfigServiceCEImplread those keys withoutAIConfigSecretsCE.decrypt. If either file is resolved toward EE's copy, EE sends ciphertext as itsAuthorizationheader and Ask AI breaks in EE.pages/AdminSettings/AI/index.tsxmust also move together withAIConfigServiceCEImpl— the client'shasStoredXmodel depends on the server'shas*response shape and the manager-only full config.3. Collapse the duplicated analytics enum
AnalyticsEvents.java: CE addsASK_AI_ORG_CONFIG_UPDATED/ASK_AI_ORG_TEST_RUNat the enum tail (lines 106/109); EE already has both at lines 140/143. Naive resolution produces duplicate enum constants and fails to compile. Keep one pair.Correction to an earlier claim in this description
An earlier revision said porting from EE keeps the two editions "byte-identical on the
ce-package files, so the sync stays a no-op". That was true of the initial port and is no longer true: the security commit, and the review fixes on top of it, deliberately move CE ahead. The byte-identity argument still holds for the ~40 untouched ported files and for the reason to port rather than revivefeat/enable-ai, but the sync itself needs the three steps above.Summary by CodeRabbit