[Project] AI Standardization - #239
Conversation
… and update UI for testing functionality
…king and visualization of AI requests
…age handling in AI requests
…d model actions, enhancing UI clarity
…role, and study options, including UI components for selection and expiry date management
…components for improved modularity and maintainability
…data binding for user, role, and study selections
…service for detailed model insights
…nership validation with new utility methods
…nhance model validation logic
… functionality for AI model updates
…ons and improve model instance tracking
… for a credential and integrate with AIService and chat components
…error handling in model testing
…nd enhance model normalization logic
…l retrieval logic
…unexpected keyword arguments
…lution logic in chat and runtime services
…andling with streamlined functions in chat services
…label logic for improved clarity
…Completion and streamline request handling
…g messages for clarity
…amline model handling for improved clarity
…et default to null for improved flexibility
… related components for improved model loading functionality
… improved soft-delete handling and transaction management
…ing on unavailable hooks
…er resolution of unavailable hooks and improve documentation clarity
| AiCredential.init({ | ||
| userId: DataTypes.INTEGER, | ||
| name: DataTypes.STRING, | ||
| apiKey: DataTypes.TEXT, |
There was a problem hiding this comment.
apiKey is stored in plaintext, can't we do pgcrypto? or is it supposed to be that way?
There was a problem hiding this comment.
Karim is working on the encryption so i can adapt it after he is done
| return this.$store.getters["table/configuration/getAll"] || []; | ||
| }, | ||
| // First validation configuration (type 1) — defines what files a submission must contain. | ||
| // TODO: support dynamicty here |
There was a problem hiding this comment.
is there a created issue for this TODO?
…ng logic to streamline server functionality
| && Number.isFinite(costLimitValue) | ||
| && costLimitValue >= 0; | ||
| // Standard appDataUpdate chain: save the model, then update or create the ai_budget row. | ||
| if (wantsCap) { |
There was a problem hiding this comment.
(Similar problem as the ones in AIHookStepperModal.vue and AIModelShareStepper.vue) If an existing model has a cost cap and the user clears the "Cost limit" field in the frontend, the existing ai_budget row is left active because deleted is never set to true in the backend.
Could we detect that case and soft-delete the existing ai_budget row (deleted: true) so clearing the field in the frontend actually removes the cap in the backend? An else if (existing && !hasCostLimit) branch could handle this where const existing = this.findExistingCapRow(savedModelId); could be defined above if(wantsCap)
|
|
||
| // Save / update the hook-level cost limit via appDataUpdate. | ||
| const costLimitValue = Number(this.hookForm.costLimit); | ||
| if (Number.isFinite(costLimitValue) && costLimitValue > 0) { |
There was a problem hiding this comment.
(Similar problem as the ones in AIModel.vue and AIModelShareStepper.vue) If an existing hook has a cost cap and the user clears the "Cost limit" field in the frontend, this condition becomes false and the ai_budget update is skipped, leaving the existing row active with deleted: false and the backend is not updated.
Could we detect when an existing cap has been cleared and soft-delete the corresponding ai_budget row (deleted: true) so the backend no longer enforces the old cap?
| this.shareForm = { | ||
| mode: config.mode, | ||
| expiryDate: config.expiryDate ? this.toDateInputString(config.expiryDate) : "", | ||
| costLimit: null, |
There was a problem hiding this comment.
When editing an existing share, costLimit is initialized to null instead of being populated from the existing ai_budget cap. This can make the frontend show no limit even though an active cap still exists in the backend.
Could we pre-fill costLimit using findExistingShareCap() when opening an existing share?
| // batch, via the standard appDataUpdate path. | ||
| const costLimitValue = Number(this.shareForm.costLimit); | ||
| const wantsCap = Number.isFinite(costLimitValue) && costLimitValue > 0; | ||
| if (wantsCap) { |
There was a problem hiding this comment.
(Similar problem as the ones in AIModel.vue and AIHookStepperModal.vue) If an existing share has a cost cap and the user clears the "Cost limit" field in the frontend, wantsCap becomes false and this block is skipped, leaving the existing ai_budget row active with deleted: false. This case is not handled here.
Could we detect when an existing cap has been cleared and soft-delete the corresponding ai_budget row (deleted: true) so the backend no longer enforces the old cap?
|
|
||
| ----- | ||
|
|
||
| **Extended form properties** |
There was a problem hiding this comment.
This PR also adds wrapperClass (e.g. in FormFields.vue) and labelButton (e.g. in Element.vue) as reusable field options, but they don't seem to be documented here yet.
Could we add them to the extended form properties so developers using BasicForm know these options are available?
|
|
||
| .. _Types: | ||
|
|
||
| **Specific form properties** |
There was a problem hiding this comment.
This PR adds type: "button" (e.g. in FormFields.vue) as a new BasicForm field type and forwards its clicks through the new button-click event (e.g. Form.vue), but I don't see this documented in form.rst.
Could we add a Button subsection describing its options and the corresponding button-click event?
| await this.models["user_setting"].set(key, value, data.userId, { bypassSystemSettingCheck: true }); | ||
| } else { | ||
| // Default: set for current user and refresh their settings | ||
| console.log(`Setting ${key} for user ${this.userId} to ${value}`); |
There was a problem hiding this comment.
This looks like a leftover debug console.log in the backend. Could we remove it or use CARE's logger (e.g. this.logger.info(...)) instead to keep backend logging consistent?
| if (!service) return []; | ||
|
|
||
| if (service.hookId) { | ||
| const keys = [service.name, service.type].filter(Boolean); |
There was a problem hiding this comment.
For hooks this returns the service name alone, but a trigger saves the result as nlpRequest_<hookName> (buildStudyHookKey, aiPreprocessing.js:135), so ~nlpAssessmentSuggestion~ never finds it. The step config already stores service.hookName and nothing reads it yet. Please build this key with buildStudyHookKey(service.name, service.hookName) and change the frontend twins (buildHookResultKey and NlpRequest.saveHookResult) to match, so all four sites use the same shape.
| } | ||
| }, | ||
| }, | ||
| isAdmin: async () => true, |
There was a problem hiding this comment.
This stub always returns true, so the permission check in backgroundTask.js:104 (noPreprocessSubmissionPermission) passes on every trigger run, whatever rights the trigger owner currently has. Please check the real rights of trigger.userId here instead of hardcoding true.
| server.db.models, | ||
| options | ||
| ); | ||
| await server.sendMail(recipient.email, template.name, body, { isHtml: true }); |
There was a problem hiding this comment.
server.sendMail hands the message to nodemailer with a callback and returns right away (Server.js:225), so a send error only reaches the logger and this await always resolves. The address is still pushed to sent and the queue item finishes as COMPLETED, so Trigger Logs reports a delivery that actually failed. Please make sendMail reject when the transport reports an error (drop the callback and await nodemailer's promise) and fail the run on that, while a disabled mail service keeps returning early without failing.
| attributes: ["id", "userId", "provider", "apiKey", "apiBaseUrl", "apiVersion", "enabled"], | ||
| }); | ||
| if (!credential) { | ||
| throw new Error("Credential not found"); |
There was a problem hiding this comment.
These messages are plain English, but backend/webserver/services/ uses TranslatableError everywhere else (9 uses in dev, none here). createSocket only localizes a message that matches an i18n key, so these reach the client as raw English. Please throw TranslatableError with keys added to utils/modules/i18n/en/errors.json, here and for the other new throws in services/ai/.
| * `configuration` carrying the action's collected data. | ||
| */ | ||
| class Trigger extends MetaModel { | ||
| static autoTable = true; |
There was a problem hiding this comment.
Security issue / Blocking: Since trigger is writable through the generic appDataUpdate path, the isAdmin() checks in TriggerSocket can be bypassed. appDataUpdate has no equivalent admin check, and this model has no beforeCreate/beforeUpdate authorization hook, so a non-admin can create or modify trigger records directly.
Could we enforce the admin restriction at the model/data layer (and likewise for the related trigger tables, e.g. trigger_queue, trigger_action, trigger_event) so all write paths are protected?
| @@ -85,7 +85,14 @@ | |||
|
|
|||
| // check or set user information | |||
| if ("userId" in data.data && !await this.checkUserAccess(data.data.userId)) { | |||
There was a problem hiding this comment.
Security issue / Blocking: validateForeignUserId() is only reached when checkUserAccess(userId) fails. If the caller shares another user's model to themselves (or omits userId and it later defaults to themselves) checkUserAccess() passes and the referenced model ownership check is skipped entirely. This allows a user to forge an ai_model_share for themselves, which _findActiveShare() later accepts as valid access.
Could we make the parent-ownership validation unavoidable for share writes, including update paths, ideally at the model layer, and apply the same fix to ai_hook_share?
| throw new Error("Selected AI credential does not exist"); | ||
| } | ||
|
|
||
| if (credential.userId !== aiModel.userId) { |
There was a problem hiding this comment.
Security issue / Blocking: This validates that the credential and model belong to the same user, but it does not verify that the authenticated requester (options.context.currentUserId) owns this ai_model. As a result, another authenticated user can update/disable/delete someone else's model as long as the existing credential-to-model ownership relationship is valid.
Could we also enforce aiModel.userId === options.context.currentUserId on updates?
| allowNull: false, | ||
| defaultValue: Sequelize.fn('NOW'), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
ai_log is queried frequently by combinations of userId, studySessionId, status, aiModelId, and later aiHookId, but this migration doesn't add indexes for those hot lookup paths. Since this table will grow with every AI request, these lookups and budget aggregations could become increasingly expensive.
Could we add indexes based on the actual query patterns here, similar to the indexes added for trigger / trigger_queue, and also review the share/budget tables for the same issue?
| studyId, studySessionId, studyStepId, documentId, | ||
| } = request || {}; | ||
|
|
||
| if (await _hasInflight(service, userId, studySessionId)) { |
There was a problem hiding this comment.
_hasInflight() and the subsequent ai_log.add() are separate DB operations, so two near-simultaneous requests can both observe no active row and then both create an in_progress log.
Could we make this single-flight check atomic (e.g. via a DB constraint/locking or another atomic claim mechanism) so duplicate concurrent requests cannot slip through?
| priority > 1 && | ||
| primaryRow && | ||
| Number(primaryRow.id) !== currentId && | ||
| Number(primaryRow.aiModelId) === aiModelId |
There was a problem hiding this comment.
Blocking: This validation also runs when an ai_hook_models row is being soft-deleted. For example, with primary A / fallback B, changing the primary to B commits that update first, then the subsequent { deleted: true } for the old fallback B is rejected here because it still appears as an active fallback matching the new primary during beforeUpdate. This leaves the model configuration partially updated.
Could we skip the priority/conflict validation for rows being deleted (after still verifying ownership), so cleanup deletes cannot be blocked by the state they're removing?
| this.isLoadingProviders = true; | ||
| this.providerLookupError = ""; | ||
| try { | ||
| const result = await this.emitAiServiceCommand("getProviders"); |
There was a problem hiding this comment.
(Same problem as in AIModel.vue) This local socket wrapper has no client-side timeout. If the client disconnects after the request is sent but before the ack arrives, Socket.IO can abandon the callback, leaving the returned Promise pending and isLoadingProviders stuck until the modal is reset.
Could we route this through the existing $ai helper, or add equivalent client-side timeout handling here?
| this.isLoadingModels = true; | ||
| this.modelLookupError = ""; | ||
| try { | ||
| const result = await this.emitAiServiceCommand("getValidModels", { |
There was a problem hiding this comment.
(Same problem as in AICredential.vue) This local socket wrapper has no client-side timeout. If the client disconnects after the request is sent but before the ack arrives, Socket.IO can abandon the callback, leaving the returned Promise pending and isLoadingModels stuck until the modal is reset.
Could we route this through the existing $ai helper, or add equivalent client-side timeout handling here?
| if (!this.$refs.form.validate()) return; | ||
|
|
||
| this.isTestingModel = true; | ||
| this.$socket.emit("serviceCommand", { |
There was a problem hiding this comment.
This call has no client-side timeout. If the socket disconnects after the request is sent but before the ack arrives, the callback may never run, so isTestingModel stays true and the test button remains stuck until the modal is reset.
Could we route this through the existing $ai helper or add equivalent timeout handling here?
| const capData = existing | ||
| ? { id: existing.id, costLimit: costLimitValue } | ||
| : { aiModelId: Number(savedModelId), limitType: 0, costLimit: costLimitValue }; | ||
| this.$socket.emit("appDataUpdate", { table: "ai_budget", data: capData }, (capResult) => { |
There was a problem hiding this comment.
The modal is closed and a success toast is shown immediately after sending the budget update, before capResult is known. If the budget write fails, the model has already been saved while the configured cost cap may not have been applied.
Could we wait for the budget acknowledgement before closing/showing success, and include client-side timeout handling so a disconnect cannot leave that wait pending indefinitely?
a60497c to
8078a7b
Compare
Summary
Introduces CARE’s AI stack (LiteLLM RPC, credentials/models/hooks/budgets/logs, admin dashboards) and an event-driven trigger system, plus prompt-template placeholders for AI hooks and study NLP workflows.
New User Features
ai_features.rst,triggers.rst)New Dev Features
LiteLLMRPCservice +utils/rpcs/litellmPython bridge;docker-compose.yml/docker-dev.ymlwiringAIService(chat,hook,request,runtime,helpers) exposed viaserviceCommandai_*andtrigger_*tables; nav/right migrations for new dashboard pagesTriggerSocket+triggerHandlers.jsfor trigger CRUD and queue managementMetaModel.foreignOwner/validateForeignUserIdfor AppSocket foreign-userId guardtemplateResolverandplaceholderTokensextended for prompt-template resolutionsubscribeTabledashboards and$aifrontend plugin for AI service callstriggerQueueStatus.js; budget cap tests inbackend/tests/budget.caps.test.jsImprovements
Selectgains searchable mode;Radiouses stable option-value keysInputMaptable-source rules aligned for Skills vs Hooksfs.promisesusage innlp.jsBug Fixes
Known Limitations
apiKeyis not yet excluded frombroadcastTransactionChanges(only fromsendTable) — credential saves may leak keys to the client store until fixed.envforRPC_LITELLM_*; vars should live in docs/.env.exampleonlyabortChatCompletiondoes not yet verify the caller owns theai_logrequeststudyNlpDocumentData.findAssessmentNlpServiceOR predicate does not matchAssessment.vueAND checkFuture Steps
make docand full CI on the branchdevbefore merge