Fix: correctly order messages when getting the system prompt one - #633
Conversation
…rove the query by applying a role filter Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
📝 WalkthroughWalkthroughThe message mapper now accepts an optional role filter for Merge Risk: 🟡 Moderate · up to This change can return messages in the wrong order and may select the wrong system message when timestamps are shared or caller-provided. Merge should wait until ordering uses the persisted message ID. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The pull request explicitly selects the system-role message and prevents an empty prompt when other messages are returned first. However, the summary does not show that getFirstNMessages now orders by ID as required by issue
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08792957-f3c5-4d67-960d-d4416f09ef23
📒 Files selected for processing (2)
lib/Db/ChattyLLM/MessageMapper.phplib/Service/ChatService.php
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| if ($role !== null) { | ||
| $qb->andWhere($qb->expr()->eq('role', $qb->createPositionalParameter($role, IQueryBuilder::PARAM_STR))); | ||
| } | ||
| $qb->orderBy('timestamp', 'ASC') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Order the filtered messages by ID.
getFirstNMessages() must return the first persisted message deterministically. timestamp can be caller-provided or shared by multiple messages, so ORDER BY timestamp ASC can select the wrong system message or produce database-dependent results. Use ORDER BY id ASC, consistent with the other message-history queries.
Proposed fix
- $qb->orderBy('timestamp', 'ASC')
+ $qb->orderBy('id', 'ASC')📝 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.
| if ($role !== null) { | |
| $qb->andWhere($qb->expr()->eq('role', $qb->createPositionalParameter($role, IQueryBuilder::PARAM_STR))); | |
| } | |
| $qb->orderBy('timestamp', 'ASC') | |
| if ($role !== null) { | |
| $qb->andWhere($qb->expr()->eq('role', $qb->createPositionalParameter($role, IQueryBuilder::PARAM_STR))); | |
| } | |
| $qb->orderBy('id', 'ASC') |
There was a problem hiding this comment.
mmh, Id is not guaratneed to be sequential
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Candidate schema and migration files =='
fd -t f . | rg -i '(^|/)(migration|schema|message).*\.(php|xml|json|sql)$' | head -n 200
printf '%s\n' '== assistant_chat_msgs declarations and inserts =='
rg -n -C 4 --glob '*.php' --glob '*.xml' --glob '*.sql' \
'(assistant_chat_msgs|createTable\(.assistant_chat_msgs.|INSERT\s+INTO\s+.*assistant_chat_msgs|insert\(.assistant_chat_msgs.)' .
printf '%s\n' '== Message entity structure and persistence call sites =='
fd -t f 'Message.php' . -x sh -c 'echo "== $1 =="; ast-grep outline "$1" --items all --view expanded; sed -n "1,260p" "$1"' sh {} \
| head -n 500
rg -n -C 4 --glob '*.php' '(insert\(\$message\)|insert\(\$[A-Za-z_]+Message\)|new Message\()' libLength of output: 14146
@marcelklehr, correct. The IDs do not need to be sequential.
ORDER BY id ASC selects the lowest persisted auto-increment ID. Gaps from deleted rows or rolled-back inserts do not change that order. The migration defines assistant_chat_msgs.id as an auto-increment column, and the insert path does not set the ID.
The issue with timestamp remains. Callers can provide the same timestamp for multiple messages, or a timestamp that does not match insertion order. I recommend ORDER BY id ASC for a deterministic persisted-message order.
You are interacting with an AI system.
There was a problem hiding this comment.
We can add the id to the orderBy but I don't think we should remve the timestamp. We might soon move away from autoincrement IDs
There was a problem hiding this comment.
@marcelklehr, agreed. timestamp should remain the primary ordering field.
Add id only as a deterministic tie-breaker:
$qb->orderBy('timestamp', 'ASC')
->addOrderBy('id', 'ASC')This does not require sequential IDs. It only requires that id has a stable order when two messages have the same timestamp. The primary behavior remains compatible with a future non-auto-increment ID scheme.
✏️ Learnings added
Learnt from: marcelklehr
Repo: nextcloud/assistant PR: 633
File: lib/Db/ChattyLLM/MessageMapper.php:42-45
Timestamp: 2026-08-26T12:00:59.687Z
Learning: In `lib/Db/ChattyLLM/MessageMapper.php`, `MessageMapper::getFirstNMessages()` must preserve `timestamp ASC` as the primary message order. Add `id ASC` only as a deterministic tie-breaker for equal timestamps. The application may replace auto-increment message IDs in the future.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
2 participants
closes #625
And improve the related query by applying a role filter.
🤖 AI (if applicable)