Uh oh!
There was an error while loading. Please reload this page.
ref(core): Set system message as separate attribute - #18978
Conversation
…s-separate-attribute
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
nicohrubec
left a comment
There was a problem hiding this comment.
two comments/questions but nothing major so lgtm
| [GEN_AI_REQUEST_MODEL_ATTRIBUTE]: 'gpt-3.5-turbo', | ||
| [GEN_AI_REQUEST_TEMPERATURE_ATTRIBUTE]: 0.7, | ||
| [GEN_AI_INPUT_MESSAGES_ORIGINAL_LENGTH_ATTRIBUTE]: 2, | ||
| [GEN_AI_INPUT_MESSAGES_ORIGINAL_LENGTH_ATTRIBUTE]: 1, |
There was a problem hiding this comment.
l: are we sure we want to exclude the system message from this count? I would probably expect it to be included given that it is sent by the user in the messages array
There was a problem hiding this comment.
I thought about this too. I chose to exclude it because the messages attribute now reflects messages without the system message. If the original message count is set to 2 but we display the system message in a separate attribute, that feels inconsistent. Also the frontend team won’t be able to tell whether the message was truncated or system attribute was just emitted.
| data: expect.objectContaining({ | ||
| [GEN_AI_SYSTEM_INSTRUCTIONS_ATTRIBUTE]: JSON.stringify([ | ||
| { type: 'text', content: 'You are a helpful assistant' }, | ||
| ]), |
There was a problem hiding this comment.
l: I think we should check for these new tests the content of the input messages attribute as well to verify that the system message is no longer there
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| .completed(); | ||
| }); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Tests don't verify system message removed from input
Low Severity · Bugbot Rules
Flagged per review rules: The new 'extracts system instructions from messages' tests across all integrations (OpenAI, Anthropic, Google GenAI, LangChain, LangGraph, Vercel AI) use expect.objectContaining to verify GEN_AI_SYSTEM_INSTRUCTIONS_ATTRIBUTE is set, but don't verify that the system message is removed from GEN_AI_INPUT_MESSAGES_ATTRIBUTE or that GEN_AI_INPUT_MESSAGES_ORIGINAL_LENGTH_ATTRIBUTE reflects the filtered count. This was explicitly requested by reviewer @nicohrubec in the PR discussion.
Additional Locations (2)
Uh oh!
There was an error while loading. Please reload this page.
So far we put all the messages including the system prompt on gen_ai.request.messages. Instead, the system prompt should be pulled out and set as a separate attribute called gen_ai.system_instructions. For now we will search for the first system message and use that.
Closes#18917