-
Notifications
You must be signed in to change notification settings - Fork 64
LCORE-1032: REST API doc update #934
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughReplaces the single PlantUML REST API diagram file to reorganize and expand the public API surface: adds styling, groups endpoints into Root/Metrics/Status/Info/Query/Conversations/Feedback/Providers+Models+Tools+RAGs, and introduces new endpoint paths and request/response types. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this 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
🧹 Nitpick comments (2)
docs/rest_api.uml (2)
147-150: Inconsistent path parameter notation.Use
{RAG_ID}placeholder syntax to match the convention used elsewhere (e.g.,{conversation_id}on lines 91, 96, 101).-Client->>API_server: GET /v1/rags/RAG_ID +Client->>API_server: GET /v1/rags/{RAG_ID}
50-53: Consider moving authorized endpoint to its own section.The
/v1/authorizedendpoint is grouped under "Readiness and liveness endpoints" but is semantically different—it's an authorization check rather than a health probe. Consider creating a separate section header (e.g.,== Authorization endpoint ==) for clarity, or adding a brief comment explaining why it's grouped with health endpoints.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/rest_api.pngis excluded by!**/*.pngdocs/rest_api.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
docs/rest_api.uml(1 hunks)
⏰ 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). (8)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: library mode / vertexai
- GitHub Check: E2E: server mode / vertexai
22d820b to
5efd11e
Compare
There was a problem hiding this 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
🧹 Nitpick comments (1)
docs/rest_api.uml (1)
39-54: Reorganize section to separate authorization from health checks.The
/v1/authorizedendpoint (lines 50-53) is grouped under "Readiness and liveness endpoints" but serves a different purpose—it checks authorization status, not service health. Consider either:
- Renaming the section to "Health and authorization endpoints"
- Creating a separate section for the authorization endpoint
This improves documentation clarity by grouping endpoints by their functional purpose.
Apply this diff to split into separate sections:
== Readiness and liveness endpoints == Client->>API_server: GET /readiness API_server->> health_py: Request health_py-->>API_server: ReadinessResponse API_server-->>Client: ReadinessResponse Client->>API_server: GET /liveness API_server->> health_py: Request health_py-->>API_server: LivenessResponse API_server-->>Client: LivenessResponse + +== Authorization endpoint == Client->>API_server: GET /v1/authorized API_server->> authorized_py: Request authorized_py-->>API_server: AuthorizedResponse API_server-->>Client: AuthorizedResponse
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/rest_api.pngis excluded by!**/*.pngdocs/rest_api.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
docs/rest_api.uml(1 hunks)
⏰ 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). (8)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: library mode / vertexai
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: server mode / vertexai
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (3)
docs/rest_api.uml (3)
68-82: Query endpoints section looks good.The query endpoints are properly structured with consistent request/response types. The previously flagged typo ("QueryRequst") and response type inconsistency for the
/inferendpoint have been corrected.
126-156: Providers, models, tools, and RAGs section is well-structured.The endpoint sequences in this section are properly organized with consistent patterns. The only issue is the path parameter notation on line 147, which has been flagged separately.
114-123: Verify that status endpoints should route to feedback module.Lines 115 and 120 show status-related endpoints (
/v1/status) being routed tofeedback_py. This couples status management with feedback functionality, which may affect modularity and separation of concerns. Please confirm this architectural decision is intentional.If these endpoints are implemented in separate modules, update the diagram accordingly. For example:
#!/bin/bash # Description: Check which module actually handles the /v1/status endpoints # Search for the status endpoint definitions rg -n "GET.*['\"]\/v1\/status['\"]|PUT.*['\"]\/v1\/status['\"]" --type py -C3
| == Feedback endpoint == | ||
| Client->>API_server: POST /v1/feedback (FeedbackRequest) | ||
| API_server->> feedback_py: FeedbackRequest | ||
| feedback_py-->>API_server: FeedbackResponse | ||
| API_server-->>Client: FeedbackResponse | ||
|
|
||
| Client->>API_server: GET /v1/status | ||
| API_server->> feedback_py: Request | ||
| feedback_py-->>API_server: StatusResponse | ||
| API_server-->>Client: StatusResponse | ||
|
|
||
| Client->>API_server: PUT /v1/status | ||
| API_server->> feedback_py: Request | ||
| feedback_py-->>API_server: FeedbackStatusUpdateResponse | ||
| API_server-->>Client: FeedbackStatusUpdateResponse | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Section title doesn't match content.
The section is titled "Feedback endpoint" (singular) but contains three different endpoints: /v1/feedback, /v1/status (GET), and /v1/status (PUT). Update the title to reflect the actual content.
Apply this diff:
-== Feedback endpoint ==
+== Feedback and Status endpoints ==📝 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.
| == Feedback endpoint == | |
| Client->>API_server: POST /v1/feedback (FeedbackRequest) | |
| API_server->> feedback_py: FeedbackRequest | |
| feedback_py-->>API_server: FeedbackResponse | |
| API_server-->>Client: FeedbackResponse | |
| Client->>API_server: GET /v1/status | |
| API_server->> feedback_py: Request | |
| feedback_py-->>API_server: StatusResponse | |
| API_server-->>Client: StatusResponse | |
| Client->>API_server: PUT /v1/status | |
| API_server->> feedback_py: Request | |
| feedback_py-->>API_server: FeedbackStatusUpdateResponse | |
| API_server-->>Client: FeedbackStatusUpdateResponse | |
| == Feedback and Status endpoints == | |
| Client->>API_server: POST /v1/feedback (FeedbackRequest) | |
| API_server->> feedback_py: FeedbackRequest | |
| feedback_py-->>API_server: FeedbackResponse | |
| API_server-->>Client: FeedbackResponse | |
| Client->>API_server: GET /v1/status | |
| API_server->> feedback_py: Request | |
| feedback_py-->>API_server: StatusResponse | |
| API_server-->>Client: StatusResponse | |
| Client->>API_server: PUT /v1/status | |
| API_server->> feedback_py: Request | |
| feedback_py-->>API_server: FeedbackStatusUpdateResponse | |
| API_server-->>Client: FeedbackStatusUpdateResponse | |
🤖 Prompt for AI Agents
In docs/rest_api.uml around lines 108 to 123, the section title "Feedback
endpoint" is inaccurate because the block documents three endpoints
(/v1/feedback, GET /v1/status, PUT /v1/status); update the section title to a
plural or broader heading such as "Feedback endpoints" or "Feedback and Status
endpoints" so it correctly reflects the multiple routes, leaving the sequence
diagram body unchanged.
| Client->>API_server: GET /v1/rags/RAG_ID | ||
| API_server->> rags_py: Request | ||
| rags_py-->>API_server: RAGInfoResponse | ||
| API_server-->>Client: RAGInfoResponse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent path parameter notation.
Line 147 uses RAG_ID as a literal path component, which is inconsistent with other path parameters in the diagram (e.g., lines 91, 96, 101 use {conversation_id} with braces). Use braces to indicate it's a variable path parameter.
Apply this diff:
-Client->>API_server: GET /v1/rags/RAG_ID
+Client->>API_server: GET /v1/rags/{rag_id}📝 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.
| Client->>API_server: GET /v1/rags/RAG_ID | |
| API_server->> rags_py: Request | |
| rags_py-->>API_server: RAGInfoResponse | |
| API_server-->>Client: RAGInfoResponse | |
| Client->>API_server: GET /v1/rags/{rag_id} | |
| API_server->> rags_py: Request | |
| rags_py-->>API_server: RAGInfoResponse | |
| API_server-->>Client: RAGInfoResponse |
🤖 Prompt for AI Agents
In docs/rest_api.uml around lines 147 to 150, the path parameter is written as a
literal RAG_ID which is inconsistent with other endpoints; change the diagram to
use brace notation for variable path parameters (e.g., replace RAG_ID with
{rag_id}) so the GET line and any related arrows reflect the parameter in braces
to match the rest of the file.
Description
LCORE-1032: REST API doc update
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.