fix(moim): avoid injecting context into active tool-call loops

[rabi]

Summary

MOIM insertion now only happens before the last assistant message that does not contain a tool request; if no such anchor exists, MOIM is skipped for that turn. This preserves tool-call sequencing and prevents MOIM from being inserted into in-flight tool loops that breaks for gemini models.

Type of Change

• [] Feature
• Bug fix
• Refactor / Code quality
• Performance improvement
• Documentation
• Tests
• Security fix
• Build / Release
• Other (specify below)

AI Assistance

• This PR was created or reviewed with AI assistance

Testing

Tested locally with the reproducer in #6293

Related Issues

Closes: #6293

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c127156305

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[tlongwell-block]

MOIM is already skipped if it introduces an error.

Please fix fix_conversation to catch an error if there is one.

Also, MOIM doesn't break tool call pairs. That should be checked for in fix_conversation anyway.

[DOsinga]

hmm, I think we should make sure that fix_conversation can fix any obvious errors, but MOIM should not rely on that - ideally fix_conversation sits there as an in case of emergency (I have had some ideas to turn it off in dev so we see the errors). if MOIM breaks gemini and then we rely on fix_conversation, we're effectively doing the same thing as this PR.

[tlongwell-block]

hmm, I think we should make sure that fix_conversation can fix any obvious errors, but MOIM should not rely on that - ideally fix_conversation sits there as an in case of emergency (I have had some ideas to turn it off in dev so we see the errors). if MOIM breaks gemini and then we rely on fix_conversation, we're effectively doing the same thing as this PR.

100%. MOIM reports when errors occurs and we should fix them at the source. But afaik fix_conversation is not reporting errors here, though it apparently should be!

[rabi]

Thanks @DOsinga @tlongwell-block for having a look! Here is what I've noticed..

MOIM injection between tool pairs passes through fix_conversation silently and it reaches gemin provider where it responds with an empty response. The reason is probably because merge_consecutive_messages uses effective_role, which treats user(tool_response) as "tool" and user(text) as "user". When MOIM (user text) is injected between a user(tool_response) and an assistant(tool_call), the merge step sees different effective roles and neither merges nor flags them. The rest of the pipeline also doesn't detect consecutive Role::User messages.

I also tried changing merge_consecutive_messages to use message.role instead of effective_role, which does merge the messages, but the resulting Content block mixes functionResponse and text parts, and Gemini echoes the MOIM text verbatim in its response.

[owtaylor]

Also, MOIM doesn't break tool call pairs. That should be checked for in fix_conversation anyway.

To be clear, the problem here is not breaking up a tool call pair, but rather breaking up the "current turn" in Google's terminology - (https://ai.google.dev/gemini-api/docs/thought-signatures) - the series of assistant messages and tool responses leading up to the final answer. Maybe there's some variation that would make Gemini OK with that, but it's not clear what that is or how we would find it. Given current information it should be avoided.

With:

<user message>
<tool call>
<tool response>
.... this goes on for thousands of tokens, 10s of minutes....
<tool call>
<tool response>

The only safe place to inject the MOIM for Gemini seems to be right at the beginning. But that might not be effective at the goals of MOIM (See: #5027 (comment).) Does this need to be some sort of provider flag?

[jamadeo]

So, what is our conclusion here? @tlongwell-block @DOsinga I think either we decide the gemini-compatible ordering is sufficient for all uses of MOIM, or we have to make it's behavior provider-specific.

[rabi]

I think either we decide the gemini-compatible ordering is sufficient for all uses of MOIM, or we have to make it's behavior provider-specific.

I agree, let me know and I'll update the PR, if required.

[michaelneale]

hey might close this, @rabi if it is still needed, please open again and lets get it to top of queue

[owtaylor]

Nothing was fixed here, doesn't make sense to close it. The question in #7758 (comment) still stands.