Skip to content

fix: Avoid insert empty api key when creating openrag assistant 🐛#1674

Merged
lethemanh merged 1 commit intomasterfrom
adjust-creating-assistant-function
Mar 13, 2026
Merged

fix: Avoid insert empty api key when creating openrag assistant 🐛#1674
lethemanh merged 1 commit intomasterfrom
adjust-creating-assistant-function

Conversation

@lethemanh
Copy link
Copy Markdown
Contributor

@lethemanh lethemanh commented Mar 2, 2026

Related to:

linagora/cozy-libs#2938

Summary by CodeRabbit

  • Enhancements

    • Introduced a formal Assistant data type for create/edit operations; providerId is now included.
    • icon and apiKey are optional; improved model and base URL handling and provider metadata preservation.
  • Bug Fixes

    • Validates required credentials for certain models and removes created account on failures to avoid orphans.
    • More accurate account/provider updates to prevent stale or incorrect data.
  • Documentation

    • API docs updated to describe the Assistant interface and revised parameter types.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 2, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Replaces inline assistantData shapes with a new exported Assistant type (adds required providerId, makes icon and apiKey optional). Introduces ASSISTANT_DOCTYPE, ACCOUNT_DOCTYPE, and OPENRAG model constant. createAssistant validates apiKey for non‑OPENRAG models, creates account documents with data/auth fields, embeds providerId in provider metadata, and cleans up created account on error. editAssistant merges and updates provider and account fields (login/model, baseUrl, apiKey) and removes baseUrl/password for OPENRAG. JSDoc and docs updated; JS function names unchanged, TypeScript signatures now use Assistant.

Possibly related PRs

Suggested reviewers

  • paultranvan
  • zatteo
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: preventing empty API key insertion for OpenRAG assistants, which aligns with the validation logic added in createAssistant.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch adjust-creating-assistant-function

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/cozy-client/src/models/assistant.js`:
- Around line 23-25: The current check uses !assistantData.apiKey which accepts
whitespace-only strings; normalize assistantData.apiKey (e.g., trim it) before
validating and before assigning so whitespace-only keys are rejected for
non-'openrag' models — update the validation that currently does if
(!assistantData.apiKey && assistantData.model !== 'openrag') to use a trimmed
value (reference assistantData.apiKey and assistantData.model) and ensure the
assignment later (the code around Line 37 that assigns the API key) stores the
normalized (trimmed) key instead of the raw input.
- Around line 23-25: Update the JSDoc for the assistantData parameter in this
module to mark assistantData.apiKey as optional/conditional: state that apiKey
is required for all models except when assistantData.model === 'openrag' (in
which case apiKey may be omitted). Edit the JSDoc block that documents the
assistantData shape (referencing assistantData, assistantData.apiKey and
assistantData.model) to explicitly describe the model-specific rule and then
re-generate the docs so the docs/api/* MD files reflect the change.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 89ecca5 and 43fd538.

📒 Files selected for processing (2)
  • docs/api/cozy-client/modules/models.assistant.md
  • packages/cozy-client/src/models/assistant.js

Comment on lines +23 to +25
if (!assistantData.apiKey && assistantData.model !== 'openrag') {
throw new Error('API key is empty')
}
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Reject whitespace-only API keys for non-openrag models.

!assistantData.apiKey at Line 23 allows values like ' ', which still end up as invalid credentials. Normalize before validating and before assigning at Line 37.

Proposed fix
-    if (!assistantData.apiKey && assistantData.model !== 'openrag') {
+    const apiKey = assistantData.apiKey?.trim()
+    if (!apiKey && assistantData.model !== 'openrag') {
       throw new Error('API key is empty')
     }
@@
-    if (assistantData.apiKey) {
-      account.auth.password = assistantData.apiKey
+    if (apiKey) {
+      account.auth.password = apiKey
     }

Also applies to: 36-38

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cozy-client/src/models/assistant.js` around lines 23 - 25, The
current check uses !assistantData.apiKey which accepts whitespace-only strings;
normalize assistantData.apiKey (e.g., trim it) before validating and before
assigning so whitespace-only keys are rejected for non-'openrag' models — update
the validation that currently does if (!assistantData.apiKey &&
assistantData.model !== 'openrag') to use a trimmed value (reference
assistantData.apiKey and assistantData.model) and ensure the assignment later
(the code around Line 37 that assigns the API key) stores the normalized
(trimmed) key instead of the raw input.

⚠️ Potential issue | 🟡 Minor

Update the function contract to reflect conditional apiKey requirement.

Behavior now allows missing apiKey for openrag, but current public docs still describe assistantData.apiKey as required. Please update the source JSDoc in this module to mark it conditional/optional and describe the model-specific rule.

Based on learnings: Documentation in docs/api//.md files is auto-generated from source code JSDoc comments. Do not edit these Markdown files directly; instead update the relevant JSDoc in the source files and re-generate the docs so the Markdown reflects the changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cozy-client/src/models/assistant.js` around lines 23 - 25, Update
the JSDoc for the assistantData parameter in this module to mark
assistantData.apiKey as optional/conditional: state that apiKey is required for
all models except when assistantData.model === 'openrag' (in which case apiKey
may be omitted). Edit the JSDoc block that documents the assistantData shape
(referencing assistantData, assistantData.apiKey and assistantData.model) to
explicitly describe the model-specific rule and then re-generate the docs so the
docs/api/* MD files reflect the change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreeing with the documentation comment. But no need to worry if the user enters ' ' for me.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zatteo, thank you for the feedback! I understand and will respect your decision on the whitespace validation—I won't flag that in future reviews.

Looking forward to seeing the JSDoc documentation updated to reflect that apiKey is optional for the openrag model. 👍


✏️ Learnings added
Learnt from: zatteo
Repo: linagora/cozy-client PR: 1674
File: packages/cozy-client/src/models/assistant.js:23-25
Timestamp: 2026-03-02T07:46:53.007Z
Learning: In the cozy-client repository, specifically in `packages/cozy-client/src/models/assistant.js`, whitespace-only API keys do not need validation or trimming. The project accepts API keys as-is without normalizing whitespace.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@lethemanh lethemanh force-pushed the adjust-creating-assistant-function branch from 43fd538 to eb9eef1 Compare March 6, 2026 06:33
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/cozy-client/src/models/assistant.js (1)

152-167: ⚠️ Potential issue | 🟠 Major

Clear inherited provider fields when switching an existing assistant to openrag.

account starts from ...provider, so the openrag branch only skips overwriting data and password; it does not remove the old values. Editing a previously credential-based assistant to openrag will resave the old baseUrl, and may keep the old API key on the account as well.

💡 Suggested fix
     const account = {
       ...provider,
       auth: {
         ...(provider.auth || {}),
         login: assistantData.model
       }
     }
     if (assistantData.model !== 'openrag') {
       account.data = {
         ...(provider.data || {}),
         baseUrl: assistantData.baseUrl
       }
-    }
-    if (assistantData.apiKey) {
-      account.auth.password = assistantData.apiKey
+      if (assistantData.apiKey) {
+        account.auth.password = assistantData.apiKey
+      }
+    } else {
+      delete account.data
+      delete account.auth.password
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cozy-client/src/models/assistant.js` around lines 152 - 167, The
current construction of account by spreading ...provider preserves old provider
fields when switching to 'openrag'; ensure that when assistantData.model ===
'openrag' you explicitly clear credential-specific fields: remove or reset
account.data (so old baseUrl isn't retained) and remove account.auth.password
(so old API keys aren't kept). Concretely, after creating account from provider,
add a branch that if assistantData.model === 'openrag' then set account.data =
{} (or delete account.data) and delete account.auth.password; otherwise keep the
existing logic that sets account.data.baseUrl and account.auth.password from
assistantData.apiKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@packages/cozy-client/src/models/assistant.js`:
- Around line 152-167: The current construction of account by spreading
...provider preserves old provider fields when switching to 'openrag'; ensure
that when assistantData.model === 'openrag' you explicitly clear
credential-specific fields: remove or reset account.data (so old baseUrl isn't
retained) and remove account.auth.password (so old API keys aren't kept).
Concretely, after creating account from provider, add a branch that if
assistantData.model === 'openrag' then set account.data = {} (or delete
account.data) and delete account.auth.password; otherwise keep the existing
logic that sets account.data.baseUrl and account.auth.password from
assistantData.apiKey.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a42c0590-c55c-4c11-b5b8-98ae3141a470

📥 Commits

Reviewing files that changed from the base of the PR and between 43fd538 and eb9eef1.

📒 Files selected for processing (2)
  • docs/api/cozy-client/modules/models.assistant.md
  • packages/cozy-client/src/models/assistant.js

_id: createdAccountId
},
cozyMetadata: {
providerId: assistantData.providerId
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remind me what is providerId here? the name of the provider like openrag ?

Copy link
Copy Markdown
Contributor Author

@lethemanh lethemanh Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relationships: {
provider: {
...existedAssistantData.relationships.provider,
cozyMetadata: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark about metadata vs cozyMetadata

Copy link
Copy Markdown
Member

@rezk2ll rezk2ll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createAssistant correctly rejects a missing API key for non-openrag models. editAssistant does not; it silently accepts a missing key and saves the account without a password.

If the intent is that edits without an apiKey keep the existing one, that's fine, but it should be explicit (e.g. a comment or a guard that only sets password when a new key is provided and the model requires one).

Right now it's ambiguous whether this is intentional or an oversight.

isCustomModel: assistantData.isCustomModel,
relationships: {
provider: {
...existedAssistantData.relationships.provider,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a user switches an existing assistant from a credentialed model to openrag, the old baseUrl and auth.password survive on the account because nothing clears them. The if (model !== 'openrag') guard only prevents overwriting, it doesn't remove.

After the account construction block (~line 158), an else branch should explicitly delete account.data and account.auth.password to avoid leaking old credentials into the openrag account.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -14,22 +14,31 @@ import logger from '../logger'
* @param {string} assistantData.model - Model identifier
* @param {string} assistantData.baseUrl - Provider's base URL
* @param {string} assistantData.apiKey - API key for authentication
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the apiKey is not required here i guess

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

export const createAssistant = async (client, assistantData) => {
let createdAccountId = null
try {
if (!assistantData.apiKey && assistantData.model !== 'openrag') {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openrad appears 4 times across the two functions i would move it to named constant

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@lethemanh lethemanh force-pushed the adjust-creating-assistant-function branch from eb9eef1 to 7dc1e0f Compare March 9, 2026 04:16
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
packages/cozy-client/src/models/assistant.js (1)

76-78: Use the ACCOUNT_DOCTYPE constant in cleanup.

The cleanup block uses a hardcoded 'io.cozy.accounts' string while the rest of the function uses the ACCOUNT_DOCTYPE constant. This inconsistency could lead to subtle bugs if the doctype ever changes.

Proposed fix
       try {
         await client.stackClient
-          .collection('io.cozy.accounts')
+          .collection(ACCOUNT_DOCTYPE)
           .destroy({ _id: createdAccountId })
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cozy-client/src/models/assistant.js` around lines 76 - 78, Replace
the hardcoded doctype string in the cleanup call with the ACCOUNT_DOCTYPE
constant: locate the call to
client.stackClient.collection('io.cozy.accounts').destroy({ _id:
createdAccountId }) and change it to use the ACCOUNT_DOCTYPE symbol so it
matches the rest of the function and avoids divergence if the doctype changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/cozy-client/src/models/assistant.js`:
- Around line 185-195: The metadata on the provider relationship is being
replaced instead of merged; update the object construction that builds
relationships.provider.data so it preserves
existedAssistantData.relationships.provider.data.metadata and only
adds/overwrites providerId from assistantData (i.e., merge existing metadata
with the new providerId) when creating the assistant object (look for
assistantData, existedAssistantData and relationships.provider.data in this
file).

In `@packages/cozy-client/types/models/assistant.d.ts`:
- Around line 18-20: The type declaration for the editAssistant payload
currently requires apiKey but the implementation/JSDoc marks it optional; update
the assistant model type so the apiKey property is optional (make apiKey?:
string) in the editAssistant-related interface/type (the declaration that
contains baseUrl, providerId and apiKey) to match the JS JSDoc and runtime
behavior.
- Around line 8-9: The TypeScript type for the assistant model marks apiKey as
required but JSDoc in assistant.js indicates apiKey is optional; update the
declaration so apiKey is optional (make the property optional on the exported
assistant model/interface/type that currently lists apiKey and providerId) to
match the JSDoc and allow omitting apiKey for openrag models; leave providerId
as required. Ensure the change targets the symbol that currently declares
"apiKey: string; providerId: string;" so consumers compile correctly.

---

Nitpick comments:
In `@packages/cozy-client/src/models/assistant.js`:
- Around line 76-78: Replace the hardcoded doctype string in the cleanup call
with the ACCOUNT_DOCTYPE constant: locate the call to
client.stackClient.collection('io.cozy.accounts').destroy({ _id:
createdAccountId }) and change it to use the ACCOUNT_DOCTYPE symbol so it
matches the rest of the function and avoids divergence if the doctype changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 86f665a7-8b6a-447b-9e55-5ab8df3341bd

📥 Commits

Reviewing files that changed from the base of the PR and between eb9eef1 and 7dc1e0f.

📒 Files selected for processing (3)
  • docs/api/cozy-client/modules/models.assistant.md
  • packages/cozy-client/src/models/assistant.js
  • packages/cozy-client/types/models/assistant.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/api/cozy-client/modules/models.assistant.md

Comment thread packages/cozy-client/src/models/assistant.js Outdated
Comment thread packages/cozy-client/types/models/assistant.d.ts Outdated
Comment thread packages/cozy-client/types/models/assistant.d.ts Outdated
@lethemanh lethemanh force-pushed the adjust-creating-assistant-function branch from 7dc1e0f to 24b5f2c Compare March 9, 2026 04:31
@lethemanh lethemanh force-pushed the adjust-creating-assistant-function branch from 24b5f2c to 7d31bec Compare March 9, 2026 09:03
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/cozy-client/src/models/assistant.js (1)

80-82: Use the ACCOUNT_DOCTYPE constant for consistency.

The cleanup logic hardcodes 'io.cozy.accounts' while the rest of the file uses the ACCOUNT_DOCTYPE constant defined at line 6.

Proposed fix
       try {
         await client.stackClient
-          .collection('io.cozy.accounts')
+          .collection(ACCOUNT_DOCTYPE)
           .destroy({ _id: createdAccountId })
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cozy-client/src/models/assistant.js` around lines 80 - 82, Replace
the hardcoded doctype string in the cleanup call with the existing constant: use
ACCOUNT_DOCTYPE instead of 'io.cozy.accounts' where the code calls
client.stackClient.collection(...).destroy({ _id: createdAccountId }); update
the collection invocation so it references ACCOUNT_DOCTYPE to keep consistency
with other usages in this module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/cozy-client/src/models/assistant.js`:
- Line 129: Update the JSDoc comment for the parameter in
packages/cozy-client/src/models/assistant.js by correcting the typo in the
`@param` description: change "editted" to "edited" for the {Assistant}
assistantData parameter so generated API docs use the correct spelling.

---

Nitpick comments:
In `@packages/cozy-client/src/models/assistant.js`:
- Around line 80-82: Replace the hardcoded doctype string in the cleanup call
with the existing constant: use ACCOUNT_DOCTYPE instead of 'io.cozy.accounts'
where the code calls client.stackClient.collection(...).destroy({ _id:
createdAccountId }); update the collection invocation so it references
ACCOUNT_DOCTYPE to keep consistency with other usages in this module.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b80cefde-f800-40e5-8e2c-06c3a9d96018

📥 Commits

Reviewing files that changed from the base of the PR and between 7dc1e0f and 7d31bec.

📒 Files selected for processing (4)
  • docs/api/cozy-client/interfaces/models.assistant.Assistant.md
  • docs/api/cozy-client/modules/models.assistant.md
  • packages/cozy-client/src/models/assistant.js
  • packages/cozy-client/types/models/assistant.d.ts

* @param {string} assistantData.model - Model identifier
* @param {string} assistantData.baseUrl - Provider's base URL
* @param {string} [assistantData.apiKey] - API key for authentication
* @param {Assistant} assistantData - Data for the editted assistant
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix typo: "editted" → "edited".

This typo will propagate to the auto-generated API documentation.

Proposed fix
- * `@param` {Assistant} assistantData - Data for the editted assistant
+ * `@param` {Assistant} assistantData - Data for the edited assistant
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* @param {Assistant} assistantData - Data for the editted assistant
* `@param` {Assistant} assistantData - Data for the edited assistant
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cozy-client/src/models/assistant.js` at line 129, Update the JSDoc
comment for the parameter in packages/cozy-client/src/models/assistant.js by
correcting the typo in the `@param` description: change "editted" to "edited" for
the {Assistant} assistantData parameter so generated API docs use the correct
spelling.

@lethemanh lethemanh force-pushed the adjust-creating-assistant-function branch from 7d31bec to 4286722 Compare March 9, 2026 09:32
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
packages/cozy-client/src/models/assistant.js (2)

9-19: Consider making baseUrl optional for consistency with openrag model.

The typedef marks baseUrl as required, but for openrag models, baseUrl is not used (lines 42-46 skip setting it, and lines 168-169 delete it). This could confuse consumers who must provide a baseUrl even when it's ignored.

Proposed fix
  * `@property` {string} model - Model identifier
- * `@property` {string} baseUrl - Provider's base URL
+ * `@property` {string} [baseUrl] - Provider's base URL (not used for openrag model)
  * `@property` {string} [apiKey] - API key for authentication
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cozy-client/src/models/assistant.js` around lines 9 - 19, The
Assistant typedef incorrectly marks baseUrl as required; make baseUrl optional
in the Assistant typedef so it matches openrag handling (update the JSDoc
property to "[baseUrl] - Provider's base URL"). Also ensure code paths that set
or delete baseUrl (references around the openrag handling logic and anywhere
Assistant objects are constructed/validated) accept its absence without forcing
a value—adjust constructors/validators that assume assistant.baseUrl exists
(e.g., places where baseUrl is added or deleted) to treat it as optional.

168-171: Potential issue with delete and optional chaining.

Using delete account.data?.baseUrl may not behave as expected. If account.data is undefined, the expression evaluates to undefined rather than performing a deletion. Since account.data is explicitly set at line 158-161, this should work here, but the pattern is misleading and can fail silently in edge cases.

Safer approach
     if (assistantData.model === OPENRAG_MODEL) {
-      delete account.data?.baseUrl
-      delete account.auth?.password
+      if (account.data) {
+        delete account.data.baseUrl
+      }
+      if (account.auth) {
+        delete account.auth.password
+      }
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cozy-client/src/models/assistant.js` around lines 168 - 171, The
current use of optional chaining with delete (in the block checking
assistantData.model === OPENRAG_MODEL) is misleading and can be a no-op if
account.data or account.auth are undefined; instead explicitly guard existence
before deleting: check that account.data exists then delete
account.data.baseUrl, and similarly check account.auth exists before deleting
account.auth.password, so replace the optional-chained deletes with conditional
checks around account.data and account.auth in the assistantData.model ===
OPENRAG_MODEL branch to ensure the properties are actually removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/api/cozy-client/interfaces/models.assistant.Assistant.md`:
- Around line 33-41: The generated docs mark the assistant properties "icon" and
"apiKey" as required even though the source in
packages/cozy-client/src/models/assistant.js declares them optional ([icon],
[apiKey]); fix this by ensuring the doc generator recognizes optional props:
either add an explicit `@optional/`@optional true JSDoc tag to the icon and apiKey
declarations in models/assistant.js or update the docgen template/parser to
treat bracketed property names (e.g. [icon], [apiKey]) as optional and render
"Optional" in the generated docs for those fields.
- Around line 9-17: The generated docs mark apiKey as required but the source
JSDoc defines it as optional; fix by ensuring the JSDoc optional syntax is
emitted/handled: update the JSDoc for the Assistant model in
packages/cozy-client/src/models/assistant.js to use the explicit optional form
(e.g., `@property` {string} [apiKey]) if it isn’t already, then adjust the docs
generation config/template that produces
docs/api/cozy-client/interfaces/models.assistant.Assistant.md so optional
properties are rendered (showing brackets or “optional”); locate the Assistant
model and the apiKey property in models/assistant.js and update either the JSDoc
or the doc generator mapping/template to output the optional marker for apiKey
and re-run the doc generation.
- Line 17: Update the source repo path used by the docs generator and any JSDoc
source links so generated files under docs/api/cozy-client/ point to
linagora/cozy-client instead of github.com/cozy/cozy-client: adjust the docs
generator configuration (where repository or baseUrl is defined) and/or update
JSDoc `@see/`@link tags in files under packages/cozy-client/src/ (for example
packages/cozy-client/src/models/assistant.js) to use the linagora path; also fix
hardcoded links in manual docs (edit docs/architecture.md,
docs/relationships.md, docs/hooks.md, docs/mobile-guide.md) to replace
github.com/cozy/cozy-client with github.com/linagora/cozy-client so all
references are consistent.

---

Nitpick comments:
In `@packages/cozy-client/src/models/assistant.js`:
- Around line 9-19: The Assistant typedef incorrectly marks baseUrl as required;
make baseUrl optional in the Assistant typedef so it matches openrag handling
(update the JSDoc property to "[baseUrl] - Provider's base URL"). Also ensure
code paths that set or delete baseUrl (references around the openrag handling
logic and anywhere Assistant objects are constructed/validated) accept its
absence without forcing a value—adjust constructors/validators that assume
assistant.baseUrl exists (e.g., places where baseUrl is added or deleted) to
treat it as optional.
- Around line 168-171: The current use of optional chaining with delete (in the
block checking assistantData.model === OPENRAG_MODEL) is misleading and can be a
no-op if account.data or account.auth are undefined; instead explicitly guard
existence before deleting: check that account.data exists then delete
account.data.baseUrl, and similarly check account.auth exists before deleting
account.auth.password, so replace the optional-chained deletes with conditional
checks around account.data and account.auth in the assistantData.model ===
OPENRAG_MODEL branch to ensure the properties are actually removed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8b5216b1-eedd-4e7d-8e85-cc676a265b0a

📥 Commits

Reviewing files that changed from the base of the PR and between 7d31bec and 4286722.

📒 Files selected for processing (4)
  • docs/api/cozy-client/interfaces/models.assistant.Assistant.md
  • docs/api/cozy-client/modules/models.assistant.md
  • packages/cozy-client/src/models/assistant.js
  • packages/cozy-client/types/models/assistant.d.ts

Comment thread docs/api/cozy-client/interfaces/models.assistant.Assistant.md Outdated
Comment thread docs/api/cozy-client/interfaces/models.assistant.Assistant.md Outdated
Comment thread docs/api/cozy-client/interfaces/models.assistant.Assistant.md Outdated
@lethemanh lethemanh force-pushed the adjust-creating-assistant-function branch 2 times, most recently from b03bbcb to 175d2a7 Compare March 9, 2026 10:00
login: assistantData.model
}
}
if (assistantData.model !== OPENRAG_MODEL) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like for apiKey, I don' think we should rely on the OPENRAG_MODEL magic const, but simply check whether or not the baseUrl is provided. So it's assistant's job to provide the baseUrl or not, depending on if we use openrag or not

data: {
login: assistantData.model
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add the providerId ? Like this:
account.account_type = providerId
So we can identify which account it is

See https://github.com/cozy/cozy-doctypes/blob/master/docs/io.cozy.accounts.md
(the "konnector" concept does not apply here)

export const createAssistant = async (client, assistantData) => {
let createdAccountId = null
try {
if (!assistantData.apiKey && assistantData.model !== OPENRAG_MODEL) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not bother about OPENRAG_MODEL here, it's the client responsibility to provide the correct data and know the logic behind OPENRAG_MODEL (no apiKey, no baseUrl required)

@lethemanh lethemanh force-pushed the adjust-creating-assistant-function branch from 175d2a7 to 5833cdf Compare March 10, 2026 04:42
@lethemanh lethemanh requested a review from paultranvan March 10, 2026 07:53
prompt: assistantData.prompt,
icon: assistantData.icon || null,
isCustomModel: assistantData.isCustomModel
isCustomModel: assistantData.isCustomModel,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one is not required anymore, right? It is determined by the value of providerId

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@lethemanh lethemanh force-pushed the adjust-creating-assistant-function branch from 5833cdf to 17450ed Compare March 12, 2026 06:52
@lethemanh lethemanh merged commit 8f0199a into master Mar 13, 2026
4 checks passed
@lethemanh lethemanh deleted the adjust-creating-assistant-function branch March 13, 2026 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants