Fix conversations before they hit the LLM

[DOsinga]

try to get a conversations into shape before we send them to the LLM

[alexhancock]

Is this added in this method because it's the last method we call of the four from fix_conversation? Wondering if we could simply run this logic as part of that method instead

[DOsinga]

the thinking is that if you have merged messages with the same role and then you've stripped any assistant messages from beginning and end, that you now have a conversation that starts with a user message and ends with one, unless we have no messages left. would you like me to call this method differently?

[alexhancock]

I was just thinking in terms of https://en.wikipedia.org/wiki/Principle_of_least_astonishment fix_lead_trail would not also do this. I was thinking instead we'd do it as a separate method similar to the others

let (messages, empty_removed) = Self::remove_empty_messages(messages);
let (messages, tool_calling_fixed) = Self::fix_tool_calling(messages);
let (messages, messages_merged) = Self::merge_consecutive_messages(messages);
let (messages, lead_trail_fixed) = Self::fix_lead_trail(messages);
let (messages, populated_if_empty) = Self::populate_if_empty(messages);

Where populate_if_empty does just this part

if messages.is_empty() {
  issues.push("Added placeholder user message to empty conversation".to_string());
  messages.push(Message::user().with_text(PLACEHOLDER_USER_MESSAGE));
}

But it's not a huge difference! Just posting this to explain my thought

I think the branch has a ton of value as-is and should merge unless you prefer to also do this suggestion

[alexhancock]

I was just thinking in terms of https://en.wikipedia.org/wiki/Principle_of_least_astonishment fix_lead_trail would not also do this. I was thinking instead we'd do it as a separate method similar to the others

let (messages, empty_removed) = Self::remove_empty_messages(messages);
let (messages, tool_calling_fixed) = Self::fix_tool_calling(messages);
let (messages, messages_merged) = Self::merge_consecutive_messages(messages);
let (messages, lead_trail_fixed) = Self::fix_lead_trail(messages);
let (messages, populated_if_empty) = Self::populate_if_empty(messages);

Where populate_if_empty does just this part

if messages.is_empty() {
  issues.push("Added placeholder user message to empty conversation".to_string());
  messages.push(Message::user().with_text(PLACEHOLDER_USER_MESSAGE));
}

But it's not a huge difference! Just posting this to explain my thought

I think the branch has a ton of value as-is and should merge unless you prefer to also do this suggestion

[DOsinga]

yeah I like it. will make it so