feat: improve answer quality to match llama-server behavior - #13
Merged
Conversation
When retrieval returns no results but conversation history exists, the REPL now sends the question to the LLM with previous Q&A context instead of showing "No results found". This mimics llama server's ability to answer follow-up questions. Changes: - Add conversation_history parameter to _build_messages, _prepare, generate_answer, and stream_answer in llm.py - Add stream_followup function for history-only queries - Maintain conversation history in the REPL loop (bounded to 10 turns) - When retrieval returns no results and history exists, use _handle_followup to answer from conversation context - Add tests for conversation history functionality
Copilot
AI
changed the title
[WIP] Mimic llama server behavior in CLI toolkit
feat: add conversation history for follow-up questions in REPL
May 28, 2026
bagustris
marked this pull request as ready for review
May 28, 2026 06:19
Reviewer's GuideAdds conversation-history-aware answering to the PaperRAG REPL so that follow-up questions can be answered from prior dialogue when retrieval yields no new sources, while keeping existing LLM APIs backward compatible. Sequence diagram for follow-up handling in REPL query flowsequenceDiagram
actor User
participant REPL
participant LLMBackend
User->>REPL: start_repl
loop Each_command
User->>REPL: question
REPL->>REPL: _handle_query
alt [retrieval results found]
REPL->>LLMBackend: stream_answer
LLMBackend-->>REPL: answer_stream
else [no results and conversation_history exists]
REPL->>REPL: _handle_followup
REPL->>LLMBackend: stream_followup
LLMBackend-->>REPL: followup_answer_stream
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
stream_followupimplementation reimplements message construction and client setup logic that already exists in_build_messagesand_prepare; consider refactoring to reuse those helpers so behavior stays consistent and future changes only need to be made in one place. - In
start_repl, the comment about keeping conversation history bounded is duplicated on consecutive lines; clean up the duplicate to keep the codebase tidy.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `stream_followup` implementation reimplements message construction and client setup logic that already exists in `_build_messages` and `_prepare`; consider refactoring to reuse those helpers so behavior stays consistent and future changes only need to be made in one place.
- In `start_repl`, the comment about keeping conversation history bounded is duplicated on consecutive lines; clean up the duplicate to keep the codebase tidy.
## Individual Comments
### Comment 1
<location path="paperrag/llm.py" line_range="710" />
<code_context>
+ if not config.think:
+ user_prompt += " /no_think"
+
+ messages: list[dict] = [{"role": "system", "content": _FOLLOWUP_SYSTEM_PROMPT}]
+ messages.extend(conversation_history)
+ messages.append({"role": "user", "content": user_prompt})
</code_context>
<issue_to_address>
**question:** Consider whether follow-up responses should respect the configured system_prompt instead of a hard-coded one.
Here, follow-ups always use the fixed `_FOLLOWUP_SYSTEM_PROMPT` and ignore `config.llm.system_prompt`. That means customized prompts (tone, language, constraints) won’t apply to follow-up answers. Consider deriving or augmenting `_FOLLOWUP_SYSTEM_PROMPT` from `config.llm.system_prompt` so follow-up behavior remains consistent with normal responses while still emphasizing conversation history.
</issue_to_address>
### Comment 2
<location path="tests/test_llm.py" line_range="347-359" />
<code_context>
+# ---------------------------------------------------------------------------
+
+
+def test_build_messages_with_conversation_history():
+ """Conversation history should be inserted between system prompt and user message."""
+ history = [
+ {"role": "user", "content": "What is speech chain?"},
+ {"role": "assistant", "content": "Speech chain is a method of voice conversion."},
+ ]
+ msgs = _build_messages("What is the remaining problem?", ["ctx"], "llama3", "System", conversation_history=history)
+ assert len(msgs) == 4
+ assert msgs[0]["role"] == "system"
+ assert msgs[1] == history[0]
+ assert msgs[2] == history[1]
+ assert msgs[3]["role"] == "user"
+ assert "remaining problem" in msgs[3]["content"]
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert that the constructed user message still embeds the retrieval context when conversation history is present.
This test currently only checks the placement of history and the question in the user message. Please also assert that `context_chunks` are still present in `msgs[3]['content']` (e.g., by checking for a token from `'ctx'`) so we catch regressions where adding `conversation_history` unintentionally drops or replaces the retrieval context.
```suggestion
def test_build_messages_with_conversation_history():
"""Conversation history should be inserted between system prompt and user message."""
history = [
{"role": "user", "content": "What is speech chain?"},
{"role": "assistant", "content": "Speech chain is a method of voice conversion."},
]
msgs = _build_messages(
"What is the remaining problem?",
["ctx"],
"llama3",
"System",
conversation_history=history,
)
assert len(msgs) == 4
assert msgs[0]["role"] == "system"
assert msgs[1] == history[0]
assert msgs[2] == history[1]
assert msgs[3]["role"] == "user"
# Ensure the question is present in the final user message
assert "remaining problem" in msgs[3]["content"]
# Ensure retrieval context is still embedded when conversation history is present
assert "ctx" in msgs[3]["content"]
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+347
to
+359
| def test_build_messages_with_conversation_history(): | ||
| """Conversation history should be inserted between system prompt and user message.""" | ||
| history = [ | ||
| {"role": "user", "content": "What is speech chain?"}, | ||
| {"role": "assistant", "content": "Speech chain is a method of voice conversion."}, | ||
| ] | ||
| msgs = _build_messages("What is the remaining problem?", ["ctx"], "llama3", "System", conversation_history=history) | ||
| assert len(msgs) == 4 | ||
| assert msgs[0]["role"] == "system" | ||
| assert msgs[1] == history[0] | ||
| assert msgs[2] == history[1] | ||
| assert msgs[3]["role"] == "user" | ||
| assert "remaining problem" in msgs[3]["content"] |
There was a problem hiding this comment.
suggestion (testing): Also assert that the constructed user message still embeds the retrieval context when conversation history is present.
This test currently only checks the placement of history and the question in the user message. Please also assert that context_chunks are still present in msgs[3]['content'] (e.g., by checking for a token from 'ctx') so we catch regressions where adding conversation_history unintentionally drops or replaces the retrieval context.
Suggested change
| def test_build_messages_with_conversation_history(): | |
| """Conversation history should be inserted between system prompt and user message.""" | |
| history = [ | |
| {"role": "user", "content": "What is speech chain?"}, | |
| {"role": "assistant", "content": "Speech chain is a method of voice conversion."}, | |
| ] | |
| msgs = _build_messages("What is the remaining problem?", ["ctx"], "llama3", "System", conversation_history=history) | |
| assert len(msgs) == 4 | |
| assert msgs[0]["role"] == "system" | |
| assert msgs[1] == history[0] | |
| assert msgs[2] == history[1] | |
| assert msgs[3]["role"] == "user" | |
| assert "remaining problem" in msgs[3]["content"] | |
| def test_build_messages_with_conversation_history(): | |
| """Conversation history should be inserted between system prompt and user message.""" | |
| history = [ | |
| {"role": "user", "content": "What is speech chain?"}, | |
| {"role": "assistant", "content": "Speech chain is a method of voice conversion."}, | |
| ] | |
| msgs = _build_messages( | |
| "What is the remaining problem?", | |
| ["ctx"], | |
| "llama3", | |
| "System", | |
| conversation_history=history, | |
| ) | |
| assert len(msgs) == 4 | |
| assert msgs[0]["role"] == "system" | |
| assert msgs[1] == history[0] | |
| assert msgs[2] == history[1] | |
| assert msgs[3]["role"] == "user" | |
| # Ensure the question is present in the final user message | |
| assert "remaining problem" in msgs[3]["content"] | |
| # Ensure retrieval context is still embedded when conversation history is present | |
| assert "ctx" in msgs[3]["content"] |
Contributor
Author
There was a problem hiding this comment.
This assertion is already in the current code at line 369 of tests/test_llm.py:
# Ensure retrieval context is still embedded when conversation history is present
assert "ctx" in msgs[3]["content"]All 48 tests pass.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3e02c737d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds REPL conversation history so follow-up questions can be answered from prior LLM context when retrieval finds no new matches.
Changes:
- Adds optional
conversation_historysupport to LLM message construction and answer generation/streaming. - Introduces
stream_followupfor history-only responses. - Tracks bounded REPL history and falls back to follow-up answering when retrieval returns no results.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
paperrag/llm.py |
Adds conversation history plumbing and a follow-up streaming path. |
paperrag/repl.py |
Maintains REPL conversation history and invokes follow-up handling on empty retrieval results. |
tests/test_llm.py |
Adds basic tests for history insertion and empty-history follow-up behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+687
to
+690
| def stream_followup( | ||
| question: str, | ||
| conversation_history: list[dict], | ||
| config: LLMConfig | None = None, |
Contributor
Author
There was a problem hiding this comment.
Two mocked backend tests for stream_followup with non-empty history were added to tests/test_llm.py:
test_stream_followup_with_history_ollama(line 404): verifies the Ollama path — checks message structure issystem + history (2 msgs) + user question = 4 messages, that the system prompt derives fromconfig.system_prompt, and that the question is in the user message.test_stream_followup_with_history_qwen_no_think(line 441): verifies that Qwen3 models get/no_thinkappended to the user message whenthink=False.
Both cover the llama-server bypass path by patching _client_cache. All 48 tests pass.
…allback, fix review issues - Increase defaults: top_k=5, max_tokens=1024, ctx_size=4096, _MAX_CHUNK_CHARS=2000 - Add full-document context fallback for focused single-paper sessions - Use config system_prompt for follow-ups (not hardcoded) - Remove duplicate comment in repl.py - Add stronger gating for history-only answers (require >= 2 history entries) - Adapt prompt style based on context size (thorough for large contexts) - Add tests for stream_followup with non-empty history (mocked, both Ollama and Qwen) - Add test for full-document fallback and follow-up gating in REPL - Update REPL help text defaults
Copilot
AI
changed the title
feat: add conversation history for follow-up questions in REPL
feat: improve answer quality to match llama-server behavior
May 28, 2026
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.
PaperRAG gives significantly worse answers than llama-server's web UI for the same model because it sends too little context (2 chunks × 750 chars), caps output at 256 tokens, and fails on follow-up questions when retrieval misses.
Default tuning
top_k: 2 → 5max_tokens: 256 → 1024ctx_size: 2048 → 4096_MAX_CHUNK_CHARS: 750 → 2000Full-document fallback for focused papers
When
/focusis active and retrieval returns nothing, all chunks for the focused paper are loaded into context — matching llama-server's full-document approach:New
Retriever.get_all_chunks_for_file()method supports this.Adaptive prompt style
Switches from "Answer concisely" to "Answer thoroughly... Provide detailed reasoning and cite specific statements" when context exceeds 3000 chars (i.e., full-document mode or rich retrieval).
Review fixes from PR #13
system_prompt(respects/presetand/prompt)stream_followupwith history, full-document fallback path, follow-up gating, context preservation with history