chore(event_mapping) handle new ide context format #8796
Conversation
![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: a35500cf5c
ℹ️ 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".
| if content.is_empty() { | ||
| None | ||
| } else { | ||
| Some(UserMessageItem::new(&content)) | ||
| } |
There was a problem hiding this comment.
Returning None when content is empty drops any user message that only contains OutputText items. Prior to this change those messages still produced a UserMessageItem, which truncate_before_nth_user_message and collect_user_messages use to count user turns. If older rollout histories or alternate sources encode user text as OutputText (as seen in existing tests and other utilities that treat OutputText as valid text), those turns will now be skipped and forking/compaction can cut at the wrong point or omit user inputs. Consider keeping a user turn when the message has text in OutputText, or otherwise explicitly handling this legacy format.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@aibrahim-oai I believe that dropping OutputText items is expected here, but double-checking this change. We would not expect OutputText to ever appear in a UserMessage, right? conversation_manager.rs appeared to do this, I updated those tests in 8950a17.
There was a problem hiding this comment.
supposedly yeah. This abstraction isn't great tbf
dylan-hurd-oai
force-pushed
the
dh--ide-context-tags
branch
from
8950a17 to
c25b975
Compare
| if is_session_prefix(text) || is_user_shell_command_text(text) { | ||
| return None; | ||
| } | ||
| // user_ide_context is bundled with the user's prompt in its own content item. |
There was a problem hiding this comment.
the logic of this file is very brittle and having another behavior is a bit worrying. Why don't attach it to a new message and use the is_session_prefix and is_user_shell_command_text logic?
There was a problem hiding this comment.
I think we should migrate to a serialized field that denotes this, rather than relying on the message content itself - planning to follow up on that soon.
dylan-hurd-oai
force-pushed
the
dh--ide-context-tags
branch
from
06a37a2 to
bdaf072
Compare
Summary
We're migrating the VSCE IDE context to more clearly separate this content in a way that more closely aligns with environment_context and skills. We want to preserve existing split behavior for old threads, at least for now. We might migrate old conversations in the future, but new threads from the VSCE will soon start appearing with a <user_ide_context> tag:
Testing