Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion basilisk/conversation/conversation_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,12 @@ class Conversation(BaseModel):
version: int = Field(default=BSKC_VERSION, ge=0, le=BSKC_VERSION)
group_participants: list[GroupParticipant] = Field(default_factory=list)
# Private: populated by GroupConversationPresenter before each chain step.
# Maps str(profile_id) -> profile name for display in history.
# Maps str(profile_id) -> profile name for attribution in group context.
# Excluded from BSKC serialization automatically by PrivateAttr.
_profile_name_map: dict[str, str] = PrivateAttr(default_factory=dict)
# Private: set to str(profile_id) of the participant being called next so
# get_messages() can restructure the history from that participant's POV.
_current_group_participant_id: str | None = PrivateAttr(default=None)

@model_validator(mode="before")
@classmethod
Expand Down
18 changes: 12 additions & 6 deletions basilisk/presenters/group_conversation_presenter.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,14 @@ def _submit_next_participant(self):
participant = self.participants[self._current_participant_index]
position = self._current_participant_index

# Populate the profile name map so base_engine prefixes responses.
# Tell get_messages() which participant is being called so it can
# rebuild history from that participant's point of view.
self.conversation._profile_name_map = {
str(p.profile_id): p.name for p in self.participants
}
self.conversation._current_group_participant_id = str(
participant.profile_id
)

# Resolve account and engine for this participant
account = self._resolve_account(participant)
Expand All @@ -178,14 +182,15 @@ def _submit_next_participant(self):
if participant.system_prompt:
system_message = SystemMessage(content=participant.system_prompt)

# For debate rounds > 0 use an empty sentinel request so the model
# responds only based on conversation history.
if self._current_round > 0:
request_content = ""
else:
# Only the first participant of the first round gets the original
# prompt. All others receive an empty sentinel so the model continues
# from the existing conversation history without a duplicate question.
if self._current_round == 0 and position == 0:
request_content = (
self._pending_request.content if self._pending_request else ""
)
else:
request_content = ""
request_attachments = (
self._pending_request.attachments
if (
Expand Down Expand Up @@ -336,6 +341,7 @@ def _on_completion_error(self, error_message: str):
def _on_chain_complete(self):
"""Called when the full chain has completed or been stopped."""
self.conversation._profile_name_map = {}
self.conversation._current_group_participant_id = None
self.view.set_active_participant(None)
self.view.on_chain_complete()

Expand Down
79 changes: 52 additions & 27 deletions basilisk/provider_engine/base_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,55 +115,80 @@ def get_messages(
) -> list[Message]:
"""Prepares message history for API requests.

For group chat blocks (``group_position > 0``) the user request is
**not** re-emitted because the same prompt was already sent by the
block at position 0. When ``_profile_name_map`` is set on the
conversation, assistant responses are prefixed with the participant
name so each model knows who said what.
For group chat conversations, the history is restructured from each
participant's point of view:
- The current participant's own previous responses appear as ASSISTANT
turns (natural continuation).
- Other participants' responses appear as USER turns, attributed with
``[name]: content`` so the model sees them as external input rather
than its own prior output. This prevents models from imitating the
``[name]:`` prefix format in their own responses.

Args:
new_block: Current message block being processed.
conversation: Full conversation history.
system_message: Optional system-level instruction message.
stop_block_index: Optional index to stop processing messages at. If None, all messages are processed.
stop_block_index: Stop processing history at this index (exclusive).

Returns:
List of prepared messages in provider-specific format.
"""
from basilisk.conversation.conversation_model import (
Message,
MessageRoleEnum,
)

messages = []
if system_message:
messages.append(self.prepare_message_request(system_message))
name_map: dict[str, str] = getattr(
conversation, "_profile_name_map", {}
)
current_pid: str | None = getattr(
conversation, "_current_group_participant_id", None
)
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
# For group blocks after the first participant, skip re-emitting
# the user request (position 0 already emitted it).
skip_user_request = (
block.group_position is not None and block.group_position > 0
)
if not skip_user_request:
messages.append(self.prepare_message_request(block.request))
# Optionally prefix the response with the participant name.
response = block.response
if name_map and block.profile_id is not None:
profile_name = name_map.get(str(block.profile_id))
if profile_name:
from basilisk.conversation.conversation_model import (
Message,
MessageRoleEnum,
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))
Comment on lines +155 to +178
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

else:
# Standard (non-group) block.
skip_user = (
block.group_position is not None
and block.group_position > 0
)
if not skip_user:
messages.append(self.prepare_message_request(block.request))
messages.append(self.prepare_message_response(block.response))
# 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:
messages.append(self.prepare_message_request(new_block.request))
return messages

@abstractmethod
Expand Down
8 changes: 5 additions & 3 deletions basilisk/views/history_msg_text_ctrl.py
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,11 @@ def display_new_block(
"""
absolute_length = self.GetLastPosition()
new_block_ref = weakref.ref(new_block)
is_group_followup = (
new_block.group_position is not None
and new_block.group_position > 0
# 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
)
Comment on lines +234 to 239
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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))
PY

Repository: 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 -20

Repository: 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.py

Repository: 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.py

Repository: 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.

if not self.IsEmpty():
absolute_length = self.append_suffix(new_block_ref, absolute_length)
Expand Down
26 changes: 26 additions & 0 deletions tests/presenters/test_group_conversation_presenter.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,32 @@ def test_debate_progresses_through_rounds(self, mocker):
# Chain done
assert not presenter._is_running_chain

def test_second_participant_in_round_0_uses_empty_request(self, mocker):
"""Position > 0 in round 0 must use empty content (no duplicate question)."""
presenter = _make_presenter()

mock_account = MagicMock()
mock_account.provider = MagicMock()
mock_account.provider.id = "openai"
mock_account.provider.engine_cls = MagicMock(return_value=MagicMock())

mocker.patch.object(
presenter, "_resolve_account", return_value=mock_account
)
mock_start = mocker.patch.object(
presenter.completion_handler, "start_completion"
)

presenter.on_submit()

# Complete round 0, participant 0
presenter._on_completion_end(True)

# Round 0, participant 1 — request must be empty, not the original prompt
assert mock_start.call_count == 2
call_kwargs = mock_start.call_args.kwargs
assert call_kwargs["new_block"].request.content == ""

def test_debate_uses_empty_sentinel_in_round_1(self, mocker):
"""Subsequent debate rounds should use an empty request content."""
presenter = _make_presenter()
Expand Down
12 changes: 12 additions & 0 deletions tests/test_group_message_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,18 @@ def test_profile_name_map_not_in_serialization(self):
data = json.loads(conv.model_dump_json())
assert "_profile_name_map" not in data

def test_current_group_participant_id_defaults_none(self):
"""_current_group_participant_id defaults to None (private attr)."""
conv = Conversation()
assert conv._current_group_participant_id is None

def test_current_group_participant_id_not_in_serialization(self):
"""_current_group_participant_id is excluded from JSON serialization."""
conv = Conversation()
conv._current_group_participant_id = "some-uuid"
data = json.loads(conv.model_dump_json())
assert "_current_group_participant_id" not in data

def test_group_participants_excluded_from_bskc_when_empty(self):
"""group_participants absent from JSON payload when empty is fine."""
conv = Conversation()
Expand Down
Loading