Onboarding : take multiple credentials from endpoint#453
Conversation
WalkthroughOnboarding now accepts an optional list of provider-specific credentials instead of a single OpenAI API key. Models, validators, CRUD, route response metadata, docs, and tests were updated to validate, encrypt, persist multiple credentials, and to return metadata when credentials are saved. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Route as OnboardingRoute
participant Validator as PydanticValidator
participant CRUD as OnboardingCRUD
participant DB as Database
Client->>Route: POST /onboard (payload with credentials list)
Route->>Validator: Validate request (credentials)
Validator-->>Route: Validated OnboardingRequest
Route->>CRUD: onboard(request)
CRUD->>DB: create org/project/user/api_key (deferred, no commit)
alt credentials provided
CRUD->>CRUD: iterate credential items
CRUD->>CRUD: extract provider & values
CRUD->>CRUD: encrypt credential values
CRUD->>DB: create Credential rows (collect ids)
end
CRUD->>DB: commit (all-or-nothing)
CRUD-->>Route: created resources + credential ids
Route->>Route: build metadata if credentials saved
Route-->>Client: APIResponse.success_response(data, metadata?)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
backend/app/tests/api/routes/test_onboarding.py (1)
30-39: Consider adding assertions to verify saved credentials.The test correctly uses the new
credentiallist structure with both OpenAI and Langfuse credentials. However, unliketest_onboard_project_new_organization_project_userintest_onboarding.py(CRUD tests) which verifies the credential record in the database (lines 82-90), this API route test doesn't verify that both credentials were actually persisted.Consider adding assertions to verify:
- Both credential records exist in the database
- The response metadata contains the expected note about saved credentials
backend/app/crud/onboarding.py (1)
47-49: Update docstring to reflect multi-credential support.The docstring still refers to "OpenAI API Key" but the implementation now supports multiple credential types.
- - OpenAI API Key (optional): - - If provided, encrypted and stored as project credentials. - - If omitted, project is created without OpenAI credentials. + - Credentials (optional): + - If provided, each credential is encrypted and stored as project credentials. + - If omitted, project is created without credentials.backend/app/models/onboarding.py (2)
22-22: Update docstring to reflect the newcredentialfield.The docstring still references
openai_api_keywhich has been replaced by thecredentialfield.- - **openai_api_key**: Optional. If provided, it will be encrypted and stored with the project. + - **credential**: Optional. A list of provider credentials (e.g., OpenAI, Langfuse). If provided, they will be encrypted and stored with the project.
85-94: Type hint inconsistency and redundant check.
The type hint on line 87 (
list[dict[str, dict[str, str]]]) is more specific than the field type on line 55 (list[dict[str, Any]]). Consider aligning them for consistency.The
isinstance(v, list)check on lines 91-94 is defensive but redundant since Pydantic performs type coercion before the field validator runs. If a non-list is passed, Pydantic would either coerce it or raise a validation error before reaching this validator.@field_validator("credential") @classmethod - def _validate_credential_list(cls, v: list[dict[str, dict[str, str]]] | None): + def _validate_credential_list(cls, v: list[dict[str, Any]] | None): if v is None: return v - if not isinstance(v, list): - raise TypeError( - "credential must be a list of single-key dicts (e.g., {'openai': {...}})." - ) - errors: list[str] = []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/api/docs/onboarding/onboarding.md(1 hunks)backend/app/api/routes/onboarding.py(1 hunks)backend/app/crud/onboarding.py(2 hunks)backend/app/models/onboarding.py(3 hunks)backend/app/tests/api/routes/test_onboarding.py(1 hunks)backend/app/tests/crud/test_onboarding.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/tests/crud/test_onboarding.pybackend/app/api/routes/onboarding.pybackend/app/models/onboarding.pybackend/app/tests/api/routes/test_onboarding.pybackend/app/crud/onboarding.py
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Files:
backend/app/api/routes/onboarding.py
backend/app/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define SQLModel entities (database tables and domain objects) in backend/app/models/
Files:
backend/app/models/onboarding.py
backend/app/crud/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database access operations in backend/app/crud/
Files:
backend/app/crud/onboarding.py
🧬 Code graph analysis (4)
backend/app/api/routes/onboarding.py (1)
backend/app/utils.py (2)
APIResponse(30-54)success_response(37-40)
backend/app/models/onboarding.py (1)
backend/app/core/providers.py (2)
validate_provider(36-57)validate_provider_credentials(60-81)
backend/app/tests/api/routes/test_onboarding.py (1)
backend/app/tests/utils/utils.py (1)
random_lower_string(20-21)
backend/app/crud/onboarding.py (2)
backend/app/core/providers.py (2)
validate_provider(36-57)validate_provider_credentials(60-81)backend/app/models/credentials.py (1)
Credential(48-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (5)
backend/app/api/routes/onboarding.py (1)
27-29: LGTM with minor style note.The conditional metadata logic is correct. The parentheses around the string on line 29 are unnecessary but harmless:
metadata = {"note": ("Given credential(s) have been saved for this project.")}Could be simplified to:
metadata = {"note": "Given credential(s) have been saved for this project."}backend/app/api/docs/onboarding/onboarding.md (1)
21-24: LGTM!The documentation accurately reflects the new multi-credential structure and clearly explains the behavior when credentials are provided or omitted.
backend/app/tests/crud/test_onboarding.py (2)
41-41: LGTM!The test correctly uses the new credential list structure and verifies that the OpenAI credential record is created in the database with the expected provider and active status.
249-249: LGTM!The test payload correctly updated to use the new credential list structure.
backend/app/models/onboarding.py (1)
98-127: LGTM!The validation logic is well-structured:
- Validates each entry is a single-key dict
- Validates provider against supported providers
- Validates nested credentials have required fields
- Aggregates all errors for comprehensive feedback
This provides clear error messages for malformed credential input.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/app/crud/onboarding.py (1)
12-12: Remove unused imports.
validate_providerandvalidate_provider_credentialsare imported but never used in this file. Validation is performed in the Pydantic model (OnboardingRequest._validate_credential_list).-from app.core.providers import validate_provider, validate_provider_credentials
🧹 Nitpick comments (2)
backend/app/crud/onboarding.py (1)
47-49: Update docstring to reflect multiple credentials support.The docstring still refers to "OpenAI API Key (optional)" but the implementation now supports multiple provider credentials. Update for accuracy.
- - OpenAI API Key (optional): - - If provided, encrypted and stored as project credentials. - - If omitted, project is created without OpenAI credentials. + - Credentials (optional): + - If provided, encrypted and stored as project credentials (supports multiple providers). + - If omitted, project is created without credentials.backend/app/tests/api/routes/test_onboarding.py (1)
1-7: Remove unused imports.
pytest,ValidationError, andOnboardingRequestare imported but not used in this file.-import pytest from fastapi.testclient import TestClient from sqlmodel import Session -from pydantic import ValidationError from app.core.config import settings -from app.models import OnboardingRequest from app.tests.utils.utils import random_email, random_lower_string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/crud/onboarding.py(2 hunks)backend/app/tests/api/routes/test_onboarding.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/crud/onboarding.pybackend/app/tests/api/routes/test_onboarding.py
backend/app/crud/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database access operations in backend/app/crud/
Files:
backend/app/crud/onboarding.py
🧬 Code graph analysis (2)
backend/app/crud/onboarding.py (3)
backend/app/core/security.py (1)
encrypt_credentials(151-168)backend/app/core/providers.py (2)
validate_provider(36-57)validate_provider_credentials(60-81)backend/app/models/credentials.py (1)
Credential(48-97)
backend/app/tests/api/routes/test_onboarding.py (1)
backend/app/models/onboarding.py (1)
OnboardingRequest(11-127)
🪛 Ruff (0.14.5)
backend/app/tests/api/routes/test_onboarding.py
177-177: Redefinition of unused test_onboard_project_duplicate_project_in_organization from line 105
(F811)
178-178: Unused function argument: db
(ARG001)
218-218: Unused function argument: db
(ARG001)
248-248: Unused function argument: db
(ARG001)
279-279: Unused function argument: db
(ARG001)
310-310: Unused function argument: db
(ARG001)
343-343: Unused function argument: db
(ARG001)
🔇 Additional comments (3)
backend/app/crud/onboarding.py (1)
106-131: LGTM!The credential processing loop correctly iterates over the credential list, encrypts each provider's credentials, and creates corresponding database records. The deferred commit ensures atomicity, and the
cred_idsextraction for logging is appropriately placed after commit when IDs are populated.backend/app/tests/api/routes/test_onboarding.py (2)
12-67: LGTM!The test comprehensively validates the multi-credential onboarding flow with both OpenAI and Langfuse providers. Response structure assertions are thorough.
247-374: LGTM!The validation tests comprehensively cover edge cases: non-dict values, missing required fields for specific providers, and aggregated error messages with index markers. The test structure is consistent and assertions are appropriate.
Note: The
dbfixture parameter flagged by static analysis is a common pytest pattern where the fixture handles session setup/teardown even when not directly referenced in the test body.
|
Are we allowing multiple keys of a single provider? If yes, then may be the input schema could improved upon i.e Also, nested taking nested lists as an input can be a bit tricky to handle as stringify/parsing JSON array sometimes changes the data type (int to str, weird escape characters "\n") |
|
|
||
| if not isinstance(v, list): | ||
| raise TypeError( | ||
| "credential must be a list of single-key dicts (e.g., {'openai': {...}})." |
There was a problem hiding this comment.
probably we should make sure the user can only put a few pre-defined keys as enums
There was a problem hiding this comment.
we are already validating whatever we are receiving using the functions here
| - If omitted, the project will be created **without OpenAI credentials**. | ||
| ## 🔑 Credentials (Optional) | ||
| - If provided, the given credentials will be **encrypted** and stored as project credentials. | ||
| - The `credential` parameter accepts a list of one or more credentials (e.g., an OpenAI key, Langfuse credentials, etc.). |
There was a problem hiding this comment.
would be good to also show a sample json for the credentials key (should be plural);
we can put this in the swagger schema for credentials model also if not here
There was a problem hiding this comment.
also, we should have a list of providers that we currenlty support for eg. openai and langfuse; these should also be validated against (not sure if teh current code does this)
"credentials": [
{
"openai": {
"api_key": "sk-proj-..."
}
},
{
"langfuse": {
"public_key": "pk-lf-...",
"secret_key": "sk-lf-...",
"host": "https://cloud.langfuse.com"
}
}
]
do cross-check if this is the way we want the credentials to look like
There was a problem hiding this comment.
yes that’s the intended structure for the credentials. The current code already validates both the provider and the required fields for that provider, once we receive the request body. you’re right that adding the list of supported providers and an example in the endpoint documentation would make it clearer.
There was a problem hiding this comment.
i have added them to the documentation
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/crud/onboarding.py (1)
32-49: Update onboarding docstring to reflect multi-providercredentialsfieldThe docstring still describes a single
openai_api_keyand OpenAI-only behavior, but the implementation now supports a genericcredentialslist for multiple providers. Please update this section so it matches the current request model and behavior (multi-provider credentials, optional list).backend/app/models/onboarding.py (1)
15-27: RefreshOnboardingRequestdocstring to describecredentialsinstead ofopenai_api_keyThe behavior section still documents an
openai_api_keyfield, which no longer exists. Please update this narrative to explain thecredentialslist (structure, examples, and that multiple providers can be passed) so it matches the actual model.
🧹 Nitpick comments (4)
backend/app/models/onboarding.py (2)
55-58: Consider expandingcredentialsfield description with expected shapeThe
credentialsfield works as[{ "provider": { ...creds... } }, ...]but the description is very high level. Adding a brief example or referencing the docs example here would make the schema clearer to API consumers.
85-127: Validator logic is solid; align type hints and optionally guard duplicate providersThe
_validate_credential_listimplementation does a good job enforcing per-item structure, provider support, and required fields, and the indexed error aggregation is very helpful.Two small follow-ups:
- Align type hints with the field: change the parameter/return type to
list[dict[str, Any]] | None(and annotate the return type) to avoid confusion with the slightly narrowerdict[str, dict[str, str]].- (Optional, but nice) Detect duplicate providers within the list and fail validation early instead of relying on the DB unique constraint to catch it later.
For example:
- @field_validator("credentials") - @classmethod - def _validate_credential_list(cls, v: list[dict[str, dict[str, str]]] | None): + @field_validator("credentials") + @classmethod + def _validate_credential_list( + cls, v: list[dict[str, Any]] | None + ) -> list[dict[str, Any]] | None: @@ - errors: list[str] = [] + errors: list[str] = [] + seen_providers: set[str] = set() @@ - (provider_key,) = item.keys() + (provider_key,) = item.keys() values = item[provider_key] @@ - validate_provider(provider_key) + validate_provider(provider_key) + + if provider_key in seen_providers: + raise ValueError( + f"duplicate provider '{provider_key}' is not allowed in credentials list." + ) + seen_providers.add(provider_key)This keeps error reporting at the validation layer and avoids a less clear integrity error at commit time.
backend/app/tests/api/routes/test_onboarding.py (1)
19-39: Happy-path multi-credential onboarding test looks goodThe extended test covering both OpenAI and Langfuse credentials exercises the new list-based payload well and keeps the core response assertions intact. Consider also asserting on any
metadatafield the route now returns when credentials are supplied, if you want explicit coverage of that behavior.backend/app/api/docs/onboarding/onboarding.md (1)
21-45: Docs accurately describe behavior; align naming and fix markdownlint issuesThe new credentials section matches the implemented payload and supported providers, but there are a couple of small cleanups:
- Refer to the
credentialsfield (plural) instead of acredentialparameter for consistency with the model.- Fix heading indentation and add a language to the fenced code block to satisfy markdownlint.
Example diff:
-## 🔑 Credentials (Optional) -- If provided, the given credentials will be **encrypted** and stored as project credentials. -- The `credential` parameter accepts a list of one or more credentials (e.g., an OpenAI key, Langfuse credentials, etc.). -- If omitted, the project will be created **without credentials**. - ### Example: For sending multiple credentials - - ``` - "credentials": [ - ... - ] - ``` - ### Supported Providers - - openai - - langfuse +## 🔑 Credentials (Optional) +- If provided, the given credentials will be **encrypted** and stored as project credentials. +- The `credentials` field accepts a list of one or more credentials (e.g., an OpenAI key, Langfuse credentials, etc.). +- If omitted, the project will be created **without credentials**. + +### Example: For sending multiple credentials +```json +"credentials": [ + { + "openai": { + "api_key": "sk-proj-..." + } + }, + { + "langfuse": { + "public_key": "pk-lf-...", + "secret_key": "sk-lf-...", + "host": "https://cloud.langfuse.com" + } + } +] +``` + +### Supported Providers +- openai +- langfuseThis keeps the docs in sync with the API and resolves the markdownlint warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/api/docs/onboarding/onboarding.md(1 hunks)backend/app/api/routes/onboarding.py(1 hunks)backend/app/crud/onboarding.py(2 hunks)backend/app/models/onboarding.py(3 hunks)backend/app/tests/api/routes/test_onboarding.py(2 hunks)backend/app/tests/crud/test_onboarding.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/tests/crud/test_onboarding.py
- backend/app/api/routes/onboarding.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/crud/onboarding.pybackend/app/models/onboarding.pybackend/app/tests/api/routes/test_onboarding.py
backend/app/crud/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database access operations in backend/app/crud/
Files:
backend/app/crud/onboarding.py
backend/app/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define SQLModel entities (database tables and domain objects) in backend/app/models/
Files:
backend/app/models/onboarding.py
🧬 Code graph analysis (3)
backend/app/crud/onboarding.py (2)
backend/app/core/security.py (2)
encrypt_credentials(151-168)get_password_hash(100-110)backend/app/models/credentials.py (1)
Credential(48-97)
backend/app/models/onboarding.py (1)
backend/app/core/providers.py (2)
validate_provider(36-57)validate_provider_credentials(60-81)
backend/app/tests/api/routes/test_onboarding.py (2)
backend/app/tests/utils/utils.py (2)
random_lower_string(20-21)random_email(28-29)backend/app/tests/conftest.py (2)
superuser_token_headers(72-73)db(28-45)
🪛 markdownlint-cli2 (0.18.1)
backend/app/api/docs/onboarding/onboarding.md
25-25: Headings must start at the beginning of the line
(MD023, heading-start-left)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
42-42: Headings must start at the beginning of the line
(MD023, heading-start-left)
43-43: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Ruff (0.14.6)
backend/app/tests/api/routes/test_onboarding.py
175-175: Unused function argument: db
(ARG001)
205-205: Unused function argument: db
(ARG001)
236-236: Unused function argument: db
(ARG001)
267-267: Unused function argument: db
(ARG001)
300-300: Unused function argument: db
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (2)
backend/app/crud/onboarding.py (1)
93-132: Credential creation loop and logging look correctThe flow to encrypt each provider-specific credential, persist
Credentialrows, commit once, and then logcred_idsis sound and consistent with the model’s unique-per-provider constraint. No functional issues here.backend/app/tests/api/routes/test_onboarding.py (1)
174-331: Let me examine the Ruff configuration and check ifdbis actually used in these functions:
<function_calls>
Check Ruff configuration for ARG001
Check Ruff configuration files
echo "=== ruff.toml ==="
cat ruff.tomlecho -e "\n=== backend/pyproject.toml (tool.ruff section) ==="
grep -A 50 "[tool.ruff]" backend/pyproject.toml | head -60
</function_calls># Extract and examine each of the 5 test functions completely to verify if db is used python3 << 'EOF' import re with open('backend/app/tests/api/routes/test_onboarding.py', 'r') as f: content = f.read() # Find the 5 test functions test_names = [ "test_onboard_project_invalid_provider", "test_onboard_project_non_dict_values_in_credential", "test_onboard_project_missing_required_fields_for_openai", "test_onboard_project_missing_required_fields_for_langfuse", "test_onboard_project_aggregates_multiple_credential_errors", ] for test_name in test_names: # Find the function definition pattern = rf"def {test_name}\(.*?\n(?:.*?\n)*?(?=def |\Z)" match = re.search(pattern, content, re.DOTALL) if match: func_text = match.group(0) # Check if 'db' is used anywhere in the function body (not just in signature) lines = func_text.split('\n') # Skip the signature line (first line) body = '\n'.join(lines[1:]) uses_db = 'db' in body and not body.count('db: Session') print(f"\n{'='*60}") print(f"Function: {test_name}") print(f"Uses 'db' in body: {uses_db}") print(f"First 30 lines:") print('\n'.join(lines[:30])) EOF </parameter> </invoke> </function_calls> ```shell # Check for other test patterns using _db in the codebase rg "_db\s*:" backend/app/tests/ --type py | head -10# Check if there are any # noqa: ARG001 patterns in the test files rg "noqa.*ARG001" backend/app/tests/ --type py
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/docs/onboarding/onboarding.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
backend/app/api/docs/onboarding/onboarding.md
26-26: Headings must start at the beginning of the line
(MD023, heading-start-left)
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (1)
backend/app/api/docs/onboarding/onboarding.md (1)
21-42: Excellent! Documentation now comprehensively addresses prior feedback.The credentials section is now well-documented with:
- Clear explanation of optional credentials, encryption, and no-credentials fallback behavior
- Multi-credential example showing OpenAI and Langfuse structures
- Explicit list of supported providers
This directly addresses kartpop's earlier requests for example JSON and provider documentation. Content looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/api/docs/onboarding/onboarding.md (1)
26-27: Resolve unresolved markdown linting violations.These formatting issues were previously flagged and remain unfixed:
- Line 26: Heading indentation — "### Example: ..." must start at column 0 (MD023)
- Line 27: Code block language — add
jsonidentifier to the fenced code block (MD040)- Lines 44–45: List indentation — bullets should use 2-space indentation, not 4-space (MD007)
Apply this diff to fix all violations:
- ### Example: For sending multiple credentials - +### Example: For sending multiple credentials - ``` +```json "credentials": [ { "openai": { "api_key": "sk-proj-..." } }, { "langfuse": { "public_key": "pk-lf-...", "secret_key": "sk-lf-...", "host": "https://cloud.langfuse.com" } } ] - ``` +``` ### Supported Providers - - openai - - langfuse + - openai + - langfuseAlso applies to: 44-45
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/docs/onboarding/onboarding.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
backend/app/api/docs/onboarding/onboarding.md
26-26: Headings must start at the beginning of the line
(MD023, heading-start-left)
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/app/api/docs/config/get.md (1)
1-5: Consider adding standard API documentation elements.The documentation clearly describes what the endpoint returns, but it lacks some standard API documentation details that would help developers integrate this endpoint. Depending on your documentation pattern and whether fuller details are available in OpenAPI/Swagger specs or elsewhere, consider adding:
- HTTP method and route pattern (e.g.,
GET /api/config/{id})- Path/query parameters (if any)
- Response status codes (200, 404, 401, etc.)
- Response body schema or structure
- Authentication requirements (if any)
- Example request/response
If these details are defined in an OpenAPI spec or centralized schema, this brief overview is appropriate. Otherwise, even a minimal addition of the HTTP method and response format would be helpful.
backend/app/api/docs/config/create.md (1)
26-26: Optional: Consider using a heading instead of bold text.Line 26 uses bold text for what appears to be a section heading. Converting to a markdown heading (e.g.,
### Example for the config blob: OpenAI Responses API with File Search) would improve document structure and navigation.Apply this diff:
-**Example for the config blob: OpenAI Responses API with File Search** +### Example for the config blob: OpenAI Responses API with File Search
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/app/api/docs/config/create.md(1 hunks)backend/app/api/docs/config/create_version.md(1 hunks)backend/app/api/docs/config/delete.md(1 hunks)backend/app/api/docs/config/delete_version.md(1 hunks)backend/app/api/docs/config/get.md(1 hunks)backend/app/api/docs/config/get_version.md(1 hunks)backend/app/api/docs/config/list.md(1 hunks)backend/app/api/docs/config/list_versions.md(1 hunks)backend/app/api/docs/config/update.md(1 hunks)backend/app/api/docs/llm/llm_call.md(1 hunks)backend/app/api/routes/config/config.py(5 hunks)backend/app/api/routes/config/version.py(4 hunks)backend/app/api/routes/llm.py(2 hunks)
✅ Files skipped from review due to trivial changes (7)
- backend/app/api/docs/config/get_version.md
- backend/app/api/docs/config/delete_version.md
- backend/app/api/docs/config/list_versions.md
- backend/app/api/docs/config/update.md
- backend/app/api/docs/config/list.md
- backend/app/api/docs/config/create_version.md
- backend/app/api/docs/config/delete.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/api/routes/config/version.pybackend/app/api/routes/llm.pybackend/app/api/routes/config/config.py
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Files:
backend/app/api/routes/config/version.pybackend/app/api/routes/llm.pybackend/app/api/routes/config/config.py
🧬 Code graph analysis (3)
backend/app/api/routes/config/version.py (2)
backend/app/utils.py (2)
APIResponse(30-54)load_description(288-293)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/api/routes/llm.py (1)
backend/app/utils.py (1)
load_description(288-293)
backend/app/api/routes/config/config.py (2)
backend/app/utils.py (2)
APIResponse(30-54)load_description(288-293)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
🪛 markdownlint-cli2 (0.18.1)
backend/app/api/docs/config/create.md
26-26: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (3)
backend/app/api/routes/config/version.py (1)
12-12: LGTM! Enhanced API documentation for version endpoints.The addition of endpoint-specific descriptions improves the API documentation. The same type compatibility verification requested for
load_descriptioninbackend/app/api/routes/llm.pyapplies here.Also applies to: 20-20, 47-47, 77-77, 104-104
backend/app/api/routes/config/config.py (1)
15-15: LGTM! Enhanced API documentation for config CRUD endpoints.The addition of endpoint-specific descriptions for all five config operations (create, list, get, update, delete) improves the API documentation. The type compatibility verification requested for
load_descriptioninbackend/app/api/routes/llm.pyapplies here as well.Also applies to: 23-23, 48-48, 72-72, 94-94, 120-120
backend/app/api/docs/llm/llm_call.md (1)
1-42: LGTM! Comprehensive endpoint documentation.The documentation clearly explains the LLM call endpoint's asynchronous behavior, configuration modes, and request fields. The structure is well-organized with clear distinctions between stored and ad-hoc configuration modes.
| * Supports optional commit messages for tracking changes | ||
| * Provider-agnostic storage - params are passed through to the provider as-is | ||
|
|
||
| **Request Field Details:** |
There was a problem hiding this comment.
not sure if we should give a detailed description of the fields like the following; the above 'Key Features' looks good and maybe is enough, otherwise we'll need to update the below list every time a small change is made
There was a problem hiding this comment.
Removed request field details because key feature does summarize it well, however with the intention of being hyper specific i had aded it before, but now as i realize that config managment will go through constant changes , it is indeed better to remove this thorough of request field definations
backend/app/api/docs/llm/llm_call.md
Outdated
| - `auto_create` (optional, boolean, default false): Create new conversation if no ID provided | ||
| - **Note**: Cannot specify both `id` and `auto_create=true` | ||
|
|
||
| **`config`** (required) - Configuration for the LLM call (choose one mode): |
There was a problem hiding this comment.
for this, we can redirect the user to refer the config endpoint documentation, instead of repeating
There was a problem hiding this comment.
since you can either put config id and version id, or you can put config blob, i agree that for the config blob part we can refer them to config endpoint docs which i am doing for the example, but for config id and version part, we would have to keep it as is
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/api/docs/config/create.md(1 hunks)backend/app/api/docs/llm/llm_call.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/app/api/docs/llm/llm_call.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
backend/app/api/docs/config/create.md
14-14: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
3 participants
Summary
Target issue is #422
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
app/models/onboarding: Added validation for the credential field to make sure it’s a proper list of single-key provider objects (like {"openai": {"api_key": "..."}}).The validator now checks that each provider is supported, the value is a dictionary, and all required fields are present.
If there are multiple issues, it collects and returns all of them together with clear, indexed error messages.
Also added tests to cover valid cases and all major error scenarios.
app/routes/onboarding: If the credential parameter has valid values and they have been saved to the credential table, you get an additional metadata in your response body which is -Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.