Repository navigation
Conversation
Signed-off-by: RainieLLM <rainie97cs@gmail.com>
RainieLLM
requested review from
bbrowning,
franciscojavierarceo,
haoshan98,
jiahuei,
leseb,
maralbahari,
noobHappylife,
qandrew and
tjtanaa
as code owners
September 28, 2026 00:23
maralbahari
requested changes
Sep 29, 2026
maralbahari
left a comment
maralbahari
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the fix. I have have found couple regression in conversation API in this PR.
| let rows = sqlx::query_as::<_, ConversationSnapshotRow>( | ||
| "SELECT conversations.latest_response_id, conversations.revision, \ | ||
| items.id AS item_id, items.reference_id AS item_reference_id, \ | ||
| items.id AS item_id, items.reference_id AS item_reference_id, items.public_id AS item_public_id, \ |
Collaborator
There was a problem hiding this comment.
This changes response items to use a fresh database row ID, but create_in_tx_with_next_conversation_seq still resolves existing items through source.id = incoming.id. It therefore no longer finds these items by their public ID.
I reproduced two regressions on a fresh SQLite database:
- Adding a stored response item to a conversation with replacement text stores that replacement instead of preserving the original content.
- Adding that ID twice in one request fails with
UNIQUE constraint failed: items.id.
Both cases pass on the parent revision. Could we update conversation item resolution to handle public_id and add coverage for this Responses-to-Conversations flow?
Contributor
|
i think the mechanism here still looks right on current
@RainieLLM if u want i can rebase this, apply the resolution fix and add the two regression tests if that helps. |
Collaborator
|
@RainieLLM thanks for the PR closing this in favor of #455 |
maralbahari
added a commit
that referenced
this pull request
Oct 20, 2026
## summary fixes #385. stored response history used each item's public id as the `items.id` primary key. a client that sent a previous response's `output` back as `input` with `store: true` (the default) hit `UNIQUE constraint failed: items.id` and a 500, even though the request is valid: Persistence(Storage(Database(Database(SqliteError { code: 1555, message: "UNIQUE constraint failed: items.id" })))) `item::create_in_tx` now gives each response-history occurrence a fresh `item_…` row id and keeps the public id in a new nullable `items.public_id` column. `Item::public_id()` still returns the public id, so clients see stable ids, and repeated occurrences keep every position. conversation inserts keep their membership checks, sequence allocation, and reference resolution in the same function. conversation inserts, cursors, retrieval and detach resolve an item by public identity (`COALESCE(public_id, reference_id, id)`). the lookup is indexed on that expression with the tenant, because a plain `public_id` index is not usable by the resolved predicate. the join resolves one source row per incoming item first (a `source_rows` CTE): several rows can share a public identity, and joining on the identity alone fans out into duplicate row ids, which then fails on `items.id`. this supersedes #388, which used the same mechanism but no longer compiles on main and left the conversation resolution untouched. thanks to @RainieLLM for the original approach. ## test plan - `resubmitted_item_history_test` replays the recorded two-turn `OpenAI` responses (`text_only/responses/`, streaming and non-streaming). it asserts the upstream request the second turn sends, then the rehydrated order, roles, content, public ids, and distinct row ids. the resubmission case is constructed from the same recording and keeps every submitted occurrence. - `stateful_conversation_integration` asserts the same for the recorded two-turn conversation path (`text_only/conversation/`, streaming and non-streaming): rehydrated history, public ids, row keys, and the upstream history. - `conversation_storage_regressions` covers repeated occurrences of one public id, cross-tenant non-reuse, and batch rollback. - `conversations_api_cassette_test` covers the conversation path, including `edge_replay::gateway_matches_all_openai_edge_case_status_codes_and_bodies`, which posts one stored public id twice and caught the fan-out. - reverting the resolution predicate reproduces `UNIQUE constraint failed: items.id`, and removing the single-source reduction reproduces `UNIQUE constraint failed: items.conversation_id, items.seq`. - index check on sqlite with 100k existing items in one tenant: inserting 100 items takes 1.0 ms with the expression index and 640 ms without it. the plan changes from `SEARCH source USING INDEX idx_items_tenant_id` to `SEARCH source USING INDEX idx_items_public_identity (tenant_id=? AND <expr>=?)`. - also ran `cargo fmt -- --check`, `cargo clippy -p agentic-server-core --all-targets -- -D warnings`, `cargo test --workspace`, and the production file-size check. ## notes - the migration is `0007_response_item_public_ids.sql`. open #350 also proposes a `0007`; whichever merges second renumbers to `0008`. - consolidating the insert into `item.rs` crosses the 500-line production cap, so `.rust-file-sizes.json` records a reviewed exception (510) with the reason. - existing rows keep the public id as their row id and a null `public_id`; `Item::public_id()` falls back to `reference_id` and then `id`, so no backfill is needed. - postgres was not exercised locally (no server available). the expression index uses standard `CREATE INDEX` syntax, and the ci `postgres` job applies the migration and runs the storage tests. --------- Signed-off-by: Abhinandan <abhinandanverma551@gmail.com> Signed-off-by: maral <maralbahari.98@gmail.com> Co-authored-by: maral <maralbahari.98@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A request with
store: truecan fail with500when a client sends a previous response's output back as input. The insert reuses public item IDs as database primary keys, so IDs already stored cause a conflict.This change stores each response history occurrence under a fresh row ID and keeps the public ID in a separate nullable column. Response history records the row IDs in input order, so repeated public IDs keep their content and position. Existing rows still use their current IDs and references.
Fixes #385.
Test Plan
cargo test: 1,782 tests passed.cargo clippy --all-targets -- -D warnings: passed.pre-commit run --all-files: passed.The new tests cover resubmitted output through
ExecuteRequestwith existing recorded responses, repeated IDs, ordered history, continuation, transaction rollback, and an ID already owned by another tenant. The two SQLite regressions fail with a primary-key conflict on the old insert path.