Refactor: completion -> completions#14584
Conversation
📝 WalkthroughWalkthroughThis PR standardizes API endpoint naming by renaming completion endpoints from singular to plural paths ( ChangesChat & Search Completion Endpoint Standardization
Dataset API Test Helpers
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
dosubot
Bot
added
the
size:M
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/references/http_api_reference.md (1)
4483-4606:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAgent completions section still mixes old route semantics with new route format.
Line 4483 still presents
POST /api/v1/agents/{agent_id}/completions, while Line 4496 (and examples) usePOST /api/v1/agents/chat/completions.
Also, Line 4603 documentsagent_idas a Path parameter, but this endpoint now carriesagent_idin the request body (Line 4507).
This is a contract mismatch and will mislead SDK/client implementers.Suggested doc fix
-**POST** `/api/v1/agents/{agent_id}/completions` +**POST** `/api/v1/agents/chat/completions` @@ -##### Request parameters - -- `agent_id`: (*Path parameter*), `string` - The ID of the associated agent. -- `"question"`: (*Body Parameter*), `string`, *Required* - The question to start an AI-powered conversation. +##### Request parameters + +- `"agent_id"`: (*Body parameter*), `string`, *Required* + The ID of the associated agent. +- `"query"`: (*Body parameter*), `string`, *Required* + The question to start an AI-powered conversation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/references/http_api_reference.md` around lines 4483 - 4606, The docs mix two endpoint semantics: remove the deprecated path-style route POST /api/v1/agents/{agent_id}/completions and consistently document the new POST /api/v1/agents/chat/completions route used in the examples; update all examples and the "Request" section to show agent_id as a JSON body field (not a path parameter), change the "Request parameters" entry that currently lists `agent_id` as a Path parameter to a Body parameter, and ensure the documented body fields (e.g., "agent_id", "query"/"question", "stream", etc.) and SSE stream descriptions reference POST /api/v1/agents/chat/completions exclusively so SDKs/clients are not misled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/apps/restful_apis/agent_api.py`:
- Around line 848-849: Remove the deprecated singular route decorator
"@manager.route(\"/agents/chat/completion\", methods=[\"POST\"])" from the
handler so only "/agents/chat/completions" is registered as the primary
endpoint; ensure the handler (the function decorated with manager.route for
completions) remains unchanged. Add a separate backward-compatibility wrapper
route elsewhere that registers "/agents/chat/completion" and calls the same
handler but logs/emits a deprecation warning (use the existing logging facility)
before forwarding the request. Make sure to reference the same handler function
when forwarding to avoid duplicating logic and keep only the plural route as
first-class.
In `@api/apps/restful_apis/search_api.py`:
- Around line 176-177: Remove the deprecated singular route by deleting the
`@manager.route`("/searches/<search_id>/completion", methods=["POST"]) decorator
in search_api.py so only the plural
`@manager.route`("/searches/<search_id>/completions", methods=["POST"]) remains;
if a compatibility shim is required, implement it only inside
api/apps/backward_compat.py (not in this module) and ensure that shim logs an
explicit deprecation warning when invoked.
---
Outside diff comments:
In `@docs/references/http_api_reference.md`:
- Around line 4483-4606: The docs mix two endpoint semantics: remove the
deprecated path-style route POST /api/v1/agents/{agent_id}/completions and
consistently document the new POST /api/v1/agents/chat/completions route used in
the examples; update all examples and the "Request" section to show agent_id as
a JSON body field (not a path parameter), change the "Request parameters" entry
that currently lists `agent_id` as a Path parameter to a Body parameter, and
ensure the documented body fields (e.g., "agent_id", "query"/"question",
"stream", etc.) and SSE stream descriptions reference POST
/api/v1/agents/chat/completions exclusively so SDKs/clients are not misled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 835df4b6-7410-497e-a387-bb3b5e13e2d4
📒 Files selected for processing (10)
admin/client/ragflow_client.pyapi/apps/backward_compat.pyapi/apps/restful_apis/agent_api.pyapi/apps/restful_apis/search_api.pyapi/apps/services/canvas_replica_service.pydocs/references/http_api_reference.mdsdk/python/ragflow_sdk/modules/session.pytest/testcases/test_http_api/common.pytest/testcases/test_sdk_api/test_session_management/test_create_session_with_chat_assistant.pyweb/src/utils/api.ts
| @manager.route("/agents/chat/completion", methods=["POST"]) # noqa: F821 | ||
| @manager.route("/agents/chat/completions", methods=["POST"]) # noqa: F821 |
There was a problem hiding this comment.
Deprecated singular agent route is still first-class in the main API.
Line 848 keeps /agents/chat/completion active without deprecation handling. That conflicts with the objective to keep only /completions. Recommend removing the singular decorator from this handler and serving old-path compatibility only via backward-compat routes with warnings.
Suggested diff
-@manager.route("/agents/chat/completion", methods=["POST"]) # noqa: F821
`@manager.route`("/agents/chat/completions", methods=["POST"]) # noqa: F821
`@login_required`
`@add_tenant_id_to_kwargs`
async def agent_chat_completion(tenant_id, agent_id=None):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/apps/restful_apis/agent_api.py` around lines 848 - 849, Remove the
deprecated singular route decorator "@manager.route(\"/agents/chat/completion\",
methods=[\"POST\"])" from the handler so only "/agents/chat/completions" is
registered as the primary endpoint; ensure the handler (the function decorated
with manager.route for completions) remains unchanged. Add a separate
backward-compatibility wrapper route elsewhere that registers
"/agents/chat/completion" and calls the same handler but logs/emits a
deprecation warning (use the existing logging facility) before forwarding the
request. Make sure to reference the same handler function when forwarding to
avoid duplicating logic and keep only the plural route as first-class.
| @manager.route("/searches/<search_id>/completion", methods=["POST"]) # noqa: F821 | ||
| @manager.route("/searches/<search_id>/completions", methods=["POST"]) # noqa: F821 |
There was a problem hiding this comment.
Primary API still exposes deprecated singular /completion path.
Line 176 keeps the old route active in the main API module, which contradicts the stated migration goal (“keep only /completions and deprecate /completion”). Please remove the singular route here and keep any compatibility shim only in api/apps/backward_compat.py with explicit warning logs.
Suggested diff
-@manager.route("/searches/<search_id>/completion", methods=["POST"]) # noqa: F821
`@manager.route`("/searches/<search_id>/completions", methods=["POST"]) # noqa: F821
`@login_required`
`@validate_request`("question")
async def completion(search_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.
| @manager.route("/searches/<search_id>/completion", methods=["POST"]) # noqa: F821 | |
| @manager.route("/searches/<search_id>/completions", methods=["POST"]) # noqa: F821 | |
| `@manager.route`("/searches/<search_id>/completions", methods=["POST"]) # noqa: F821 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/apps/restful_apis/search_api.py` around lines 176 - 177, Remove the
deprecated singular route by deleting the
`@manager.route`("/searches/<search_id>/completion", methods=["POST"]) decorator
in search_api.py so only the plural
`@manager.route`("/searches/<search_id>/completions", methods=["POST"]) remains;
if a compatibility shim is required, implement it only inside
api/apps/backward_compat.py (not in this module) and ensure that shim logs an
explicit deprecation warning when invoked.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14584 +/- ##
=======================================
Coverage 94.16% 94.16%
=======================================
Files 10 10
Lines 703 703
Branches 112 112
=======================================
Hits 662 662
Misses 25 25
Partials 16 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
LGTM |
2 participants
What problem does this PR solve?
Keep only /completions, deprecated /completion
Type of change