Open
Conversation
…versations - Restructure get_messages() for group chat: other participants' responses now appear as USER turns with [name]: attribution instead of prefixing ASSISTANT content. This prevents models from self-prefixing their own responses or generating multiple personas in one block. - Add _current_group_participant_id PrivateAttr to Conversation so get_messages() knows whose POV to build history from. - Fix empty 'User:' lines appearing between debate rounds: is_group_followup now also skips user request display when request content is empty. - Fix duplicate user question: only position 0 in round 0 receives the original prompt; all others get an empty sentinel. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Open
Contributor
WalkthroughThis pull request implements perspective-based message history assembly for group conversations. It adds tracking of the current group participant, sets profile name mappings for attribution, gates initial request content exclusively to the first participant, and reworks how messages are presented so each participant sees their own responses as assistant turns and others' responses as user turns with proper attribution. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable sequence diagrams in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@basilisk/provider_engine/base_engine.py`:
- Around line 155-178: When rebuilding group history in the block-handling
branch (the code using current_pid, block.group_id, pid_str, name_map and
creating Message with MessageRoleEnum.USER), also replay the original request
for non-own participants so later participants receive the opener turn;
specifically, when handling "another participant's response" append the
block.request as a user turn before or together with the attributed response
using prepare_message_request, and change the guard so it includes requests that
have attachments even if request.content == "" (e.g., check block.request exists
and either content or attachments are present). Apply the same fix to the
analogous code path around the second region referenced (the block range also at
~188-191) so both group-history reconstruction sites include attachment-only or
empty-content opener requests.
In `@basilisk/views/history_msg_text_ctrl.py`:
- Around line 234-239: The condition computing is_group_followup should guard
new_block.group_position against None before comparing to 0; update the
expression that references new_block.group_position in the history message
controller (the new_block variable in basilisk.views.history_msg_text_ctrl) to
first check new_block.group_position is not None (e.g., use
"new_block.group_position is not None and new_block.group_position > 0") so that
when new_block.group_id is present but group_position is None you won't get a
TypeError; keep the existing checks for new_block.group_id and not
new_block.request.content as-is.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa7927eb-3e71-475a-b33b-56631606e413
📒 Files selected for processing (6)
basilisk/conversation/conversation_model.pybasilisk/presenters/group_conversation_presenter.pybasilisk/provider_engine/base_engine.pybasilisk/views/history_msg_text_ctrl.pytests/presenters/test_group_conversation_presenter.pytests/test_group_message_block.py
Comment on lines
+155
to
+178
| if current_pid is not None and block.group_id is not None: | ||
| # Group block: rebuild history from this participant's POV. | ||
| pid_str = str(block.profile_id) if block.profile_id else None | ||
| if pid_str == current_pid: | ||
| # Own previous response: include request (if non-empty) | ||
| # then own response as ASSISTANT. | ||
| if block.request.content: | ||
| messages.append( | ||
| self.prepare_message_request(block.request) | ||
| ) | ||
| messages.append( | ||
| self.prepare_message_response(block.response) | ||
| ) | ||
|
|
||
| response = Message( | ||
| role=MessageRoleEnum.ASSISTANT, | ||
| content=f"[{profile_name}]: {response.content}", | ||
| else: | ||
| # Another participant's response: frame as USER turn so | ||
| # the model doesn't imitate the attribution format. | ||
| other_name = ( | ||
| name_map.get(pid_str, "Other") if pid_str else "Other" | ||
| ) | ||
| attributed = Message( | ||
| role=MessageRoleEnum.USER, | ||
| content=f"[{other_name}]: {block.response.content}", | ||
| ) | ||
| messages.append(self.prepare_message_response(response)) | ||
| messages.append(self.prepare_message_request(new_block.request)) | ||
| messages.append(self.prepare_message_request(attributed)) |
Contributor
There was a problem hiding this comment.
Replay the opener request when rebuilding group history.
Once participant 2+ is sent request_content="", this method becomes the
only place they can still receive the original user turn. Right now non-own
group blocks contribute only block.response, and the if ...content
guards also drop attachment-only requests. In normal mode that leaves later
participants without the user's prompt/files.
🛠️ Proposed fix
+ def has_user_input(message: Message) -> bool:
+ return bool(message.content or message.attachments)
+
for i, block in enumerate(conversation.messages):
if stop_block_index is not None and i >= stop_block_index:
break
if not block.response:
continue
if current_pid is not None and block.group_id is not None:
# Group block: rebuild history from this participant's POV.
pid_str = str(block.profile_id) if block.profile_id else None
if pid_str == current_pid:
# Own previous response: include request (if non-empty)
# then own response as ASSISTANT.
- if block.request.content:
+ if has_user_input(block.request):
messages.append(
self.prepare_message_request(block.request)
)
messages.append(
self.prepare_message_response(block.response)
)
else:
+ if has_user_input(block.request):
+ messages.append(
+ self.prepare_message_request(block.request)
+ )
# Another participant's response: frame as USER turn so
# the model doesn't imitate the attribution format.
other_name = (
name_map.get(pid_str, "Other") if pid_str else "Other"
)
attributed = Message(
role=MessageRoleEnum.USER,
content=f"[{other_name}]: {block.response.content}",
)
messages.append(self.prepare_message_request(attributed))
# Append the new block's request only when it carries content.
# For group follow-ups the last USER turn is already an attribution.
- if new_block.request.content:
+ if has_user_input(new_block.request):
messages.append(self.prepare_message_request(new_block.request))Also applies to: 188-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@basilisk/provider_engine/base_engine.py` around lines 155 - 178, When
rebuilding group history in the block-handling branch (the code using
current_pid, block.group_id, pid_str, name_map and creating Message with
MessageRoleEnum.USER), also replay the original request for non-own participants
so later participants receive the opener turn; specifically, when handling
"another participant's response" append the block.request as a user turn before
or together with the attributed response using prepare_message_request, and
change the guard so it includes requests that have attachments even if
request.content == "" (e.g., check block.request exists and either content or
attachments are present). Apply the same fix to the analogous code path around
the second region referenced (the block range also at ~188-191) so both
group-history reconstruction sites include attachment-only or empty-content
opener requests.
Comment on lines
+234
to
239
| # Skip user request for group follow-up blocks: | ||
| # - any block with group_position > 0 (not the chain opener), or | ||
| # - position-0 blocks in subsequent rounds (empty sentinel request). | ||
| is_group_followup = new_block.group_id is not None and ( | ||
| new_block.group_position > 0 or not new_block.request.content | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
group_position = None
try:
group_position > 0
except Exception as exc:
print(type(exc).__name__, str(exc))
PYRepository: SigmaNight/basiliskLLM
Length of output: 135
🏁 Script executed:
# Check the MessageBlock.group_position type definition
grep -n "group_position" basilisk/conversation/conversation_model.py | head -20Repository: SigmaNight/basiliskLLM
Length of output: 119
🏁 Script executed:
# Read the exact code at lines 234-239 in history_msg_text_ctrl.py
sed -n '234,239p' basilisk/views/history_msg_text_ctrl.pyRepository: SigmaNight/basiliskLLM
Length of output: 381
🏁 Script executed:
# Get broader context around lines 234-239 to understand the flow
sed -n '230,245p' basilisk/views/history_msg_text_ctrl.pyRepository: SigmaNight/basiliskLLM
Length of output: 778
Guard group_position against None before comparison.
Line 238 performs new_block.group_position > 0 without checking if group_position is None. Since MessageBlock.group_position is typed as int | None, this comparison raises TypeError when group_id is present but group_position is None. The first check (new_block.group_id is not None) does not guarantee group_position is also non-None.
Proposed fix
- is_group_followup = new_block.group_id is not None and (
- new_block.group_position > 0 or not new_block.request.content
- )
+ group_position = new_block.group_position
+ is_group_followup = new_block.group_id is not None and (
+ (
+ group_position is not None
+ and group_position > 0
+ )
+ or not new_block.request.content
+ )📝 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.
Suggested change
| # Skip user request for group follow-up blocks: | |
| # - any block with group_position > 0 (not the chain opener), or | |
| # - position-0 blocks in subsequent rounds (empty sentinel request). | |
| is_group_followup = new_block.group_id is not None and ( | |
| new_block.group_position > 0 or not new_block.request.content | |
| ) | |
| # Skip user request for group follow-up blocks: | |
| # - any block with group_position > 0 (not the chain opener), or | |
| # - position-0 blocks in subsequent rounds (empty sentinel request). | |
| group_position = new_block.group_position | |
| is_group_followup = new_block.group_id is not None and ( | |
| ( | |
| group_position is not None | |
| and group_position > 0 | |
| ) | |
| or not new_block.request.content | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@basilisk/views/history_msg_text_ctrl.py` around lines 234 - 239, The
condition computing is_group_followup should guard new_block.group_position
against None before comparing to 0; update the expression that references
new_block.group_position in the history message controller (the new_block
variable in basilisk.views.history_msg_text_ctrl) to first check
new_block.group_position is not None (e.g., use "new_block.group_position is not
None and new_block.group_position > 0") so that when new_block.group_id is
present but group_position is None you won't get a TypeError; keep the existing
checks for new_block.group_id and not new_block.request.content as-is.
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
1 participant
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 by CodeRabbit