feat(conversations): complete OpenAI-compatible lifecycle - #576
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds OpenAI-compatible conversation-item CRUD and pagination APIs, changes metadata updates to merge keys, strengthens storage concurrency and size enforcement, preserves unknown JSON fields and numeric values, and adds unit, end-to-end, and PostgreSQL/MongoDB integration coverage. ChangesConversation feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ConversationAPI
participant ConversationStore
participant ResponsesAPI
Client->>ConversationAPI: Create, list, retrieve, or delete conversation items
ConversationAPI->>ConversationStore: Normalize and persist item mutations
ConversationStore-->>ConversationAPI: Return updated conversation or error
Client->>ResponsesAPI: Submit a response turn
ResponsesAPI->>ConversationStore: Persist completed output items
ConversationStore-->>ResponsesAPI: Return persisted item IDs
ResponsesAPI-->>Client: Return response with committed IDs
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
20b4dfd to
161d147
Compare
![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
🤖 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 `@internal/conversationstore/store_postgresql.go`:
- Around line 170-198: Update DeleteItem so the matching element lookup and
JSONB array deletion occur in the same correlated UPDATE against c.items,
avoiding a stale positional index from a separate target CTE. Preserve
expiration and first-match semantics, and add a concurrency test that deletes
two different item IDs from the same conversation and verifies neither wrong
element is removed.
In `@internal/server/conversation_item_projection.go`:
- Around line 45-51: Clamp the pagination limit in the conversation item
projection flow to the documented maximum of 100 after applying the existing
default for non-positive values. Update the limit handling near remaining and
hasMore so oversized caller values cannot request more than 100 items.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e16e8ac0-c79f-415b-8de0-109a8de79db6
📒 Files selected for processing (35)
cmd/gomodel/docs/docs.godocs/advanced/api-endpoints.mdxdocs/advanced/conversations-api.mdxdocs/openapi.jsoninternal/conversationstore/store.gointernal/conversationstore/store_memory.gointernal/conversationstore/store_memory_test.gointernal/conversationstore/store_mongodb.gointernal/conversationstore/store_postgresql.gointernal/conversationstore/store_sqlite.gointernal/conversationstore/store_sqlite_test.gointernal/core/conversations.gointernal/core/conversations_test.gointernal/core/endpoints_test.gointernal/core/responses.gointernal/core/responses_json.gointernal/core/responses_json_test.gointernal/server/conversation_handlers.gointernal/server/conversation_handlers_test.gointernal/server/conversation_item_normalization.gointernal/server/conversation_item_projection.gointernal/server/conversation_responses.gointernal/server/conversation_responses_test.gointernal/server/http.gointernal/server/native_conversation_items_service.gointernal/server/native_conversation_service.gointernal/server/native_response_service.gointernal/server/raw_json_object.gointernal/server/response_input_items.gointernal/server/response_input_items_test.gointernal/server/translated_inference_service.gotests/contract/testdata/golden/xai/responses.golden.jsontests/e2e/conversations_test.gotests/integration/conversations_test.gotests/integration/setup_test.go
Confidence Score: 4/5Streaming completion can report success without persisting the turn, so this path should be fixed before merging.
internal/server/conversation_responses.go and internal/conversationstore/store_mongodb.go Sequence DiagramsequenceDiagram
participant Client
participant Gateway
participant Provider
participant Store as Conversation Store
Client->>Gateway: Responses request with conversation ID
Gateway->>Store: Load stored items
Store-->>Gateway: Conversation history
Gateway->>Provider: Request with replayed history
Provider-->>Gateway: Response or SSE stream
Gateway->>Store: Append input and output items
alt Append succeeds
Store-->>Gateway: Persisted exchange
Gateway-->>Client: Response with stable item IDs
else Non-streaming append fails
Gateway-->>Client: 500 persistence error
else Streaming append fails after completion
Gateway->>Gateway: Log failure
Note over Client,Store: Client saw success but history remains stale
end
Reviews (1): Last reviewed commit: "feat(conversations): complete OpenAI-com..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/server/conversation_item_normalization.go`:
- Around line 80-89: Update the function-call arguments and function-call-output
output validation to use rawJSONValuePresent rather than only checking map-key
existence. Ensure explicit JSON null is rejected as missing before unmarshalling
or normalization, while preserving the existing invalid-request errors and
parameter context in the function-call normalization logic.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6bb1a142-378a-4d00-b52e-9edf2be6ef65
📒 Files selected for processing (35)
cmd/gomodel/docs/docs.godocs/advanced/api-endpoints.mdxdocs/advanced/conversations-api.mdxdocs/openapi.jsoninternal/conversationstore/store.gointernal/conversationstore/store_memory.gointernal/conversationstore/store_memory_test.gointernal/conversationstore/store_mongodb.gointernal/conversationstore/store_postgresql.gointernal/conversationstore/store_sqlite.gointernal/conversationstore/store_sqlite_test.gointernal/core/conversations.gointernal/core/conversations_test.gointernal/core/endpoints_test.gointernal/core/responses.gointernal/core/responses_json.gointernal/core/responses_json_test.gointernal/server/conversation_handlers.gointernal/server/conversation_handlers_test.gointernal/server/conversation_item_normalization.gointernal/server/conversation_item_projection.gointernal/server/conversation_responses.gointernal/server/conversation_responses_test.gointernal/server/http.gointernal/server/native_conversation_items_service.gointernal/server/native_conversation_service.gointernal/server/native_response_service.gointernal/server/raw_json_object.gointernal/server/response_input_items.gointernal/server/response_input_items_test.gointernal/server/translated_inference_service.gotests/contract/testdata/golden/xai/responses.golden.jsontests/e2e/conversations_test.gotests/integration/conversations_test.gotests/integration/setup_test.go
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Review disposition for 42c7ef9:\n\n- Fixed all five substantive correctness/resilience findings: PostgreSQL concurrent deletion, helper-level pagination bounds, explicit-null function fields, streamed completion persistence ordering, and MongoDB CAS contention.\n- Added focused unit tests plus a concurrent-delete integration regression; both PostgreSQL and MongoDB scenarios passed 10 repeated runs.\n- Classified CodeQL alerts 54–59 as false positives. All query keys/operators are compile-time BSON structure and request-derived data is driver-encoded only as string/scalar-array values. The CodeQL gate is now green.\n- Intentionally did not bulk-add docstrings to unexported helpers for CodeRabbit’s generic 80% heuristic; that is not a repository gate and would add low-signal comments contrary to the project’s KISS guidance.\n- The Checkov ModuleNotFoundError is in CodeRabbit’s tool environment, not this repository.\n\nAll current CI checks pass. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/translated_inference_service.go (1)
315-323: 🩺 Stability & Availability | 🔵 TrivialConsider client-retry/duplicate-provider-call exposure when append fails.
Once
appendResponsefails here, the provider call has already succeeded, but the client receives a 500. A client retrying on that 500 would re-invoke the (possibly costly) provider call again, since there's no idempotency-key safeguard in this path. This is a reasonable tradeoff for the persistence-correctness fix, but worth tracking as an operational risk if provider costs/side-effects are significant.🤖 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 `@internal/server/translated_inference_service.go` around lines 315 - 323, Track the post-provider persistence failure in the translated inference flow around conversationTurnFromContext and turn.appendResponse as an operational risk: a 500 after provider success can cause client retries to invoke the provider again. Add or update the appropriate follow-up tracking/documentation for an idempotency-key safeguard, without changing the existing persistence error behavior.
🤖 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 `@internal/server/conversation_responses.go`:
- Around line 216-274: Update conversationPersistingStream.Read and the
surrounding persistingStream flow so the complete terminal
response.completed/response.done SSE event is buffered and parsed before any of
its bytes are returned to the caller. If conversationStreamObserver.OnJSONEvent
records an append or marshal failure, suppress the entire terminal event rather
than only subsequent reads; preserve normal streaming for non-terminal events
and successful persistence.
---
Outside diff comments:
In `@internal/server/translated_inference_service.go`:
- Around line 315-323: Track the post-provider persistence failure in the
translated inference flow around conversationTurnFromContext and
turn.appendResponse as an operational risk: a 500 after provider success can
cause client retries to invoke the provider again. Add or update the appropriate
follow-up tracking/documentation for an idempotency-key safeguard, without
changing the existing persistence error behavior.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 30203721-0d91-4022-8fff-a48caa2a6541
📒 Files selected for processing (11)
internal/conversationstore/store_mongodb.gointernal/conversationstore/store_mongodb_test.gointernal/conversationstore/store_postgresql.gointernal/server/conversation_handlers_test.gointernal/server/conversation_item_normalization.gointernal/server/conversation_item_projection.gointernal/server/conversation_responses.gointernal/server/conversation_responses_test.gointernal/server/native_response_service.gointernal/server/translated_inference_service.gotests/integration/conversations_test.go
|
Follow-up review disposition (1227876):
Validation: race tests, lint, |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/advanced/conversations-api.mdx`:
- Around line 87-93: Update the conversation persistence guidance in the
`/v1/responses` documentation to state that history inspection is not reliable
protection against duplicate retries because the successful provider call may
not have been persisted. Advise clients to avoid automatic retries or use an
independent durable idempotency mechanism, while preserving the existing
behavior description for streaming and non-streaming requests.
In `@internal/server/conversation_persisting_stream.go`:
- Around line 135-160: The conversationSSEPayload and OnJSONEvent flow must
preserve response.output JSON bytes without numeric round-tripping through
map[string]any. Decode output elements as []json.RawMessage, pass those raw
fragments unchanged to appendExchange, and add a fragmented SSE regression test
covering a large integer and unknown fields.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f428da8e-c2a3-4c8d-b14e-dfa6a0007ced
📒 Files selected for processing (4)
docs/advanced/conversations-api.mdxinternal/server/conversation_persisting_stream.gointernal/server/conversation_persisting_stream_test.gointernal/server/conversation_responses.go
💤 Files with no reviewable changes (1)
- internal/server/conversation_responses.go
Summary
QA findings fixed
Validation
Notes
The current persistent representation remains a conversation snapshot with an item array. A normalized conversation_items table/collection would be a separate scaling-oriented follow-up rather than part of this compatibility change.
Summary by CodeRabbit
includeprojections for returned fields.