ref(core): Set system message as separate attribute#18978
ref(core): Set system message as separate attribute#18978RulaKhaled merged 5 commits intodevelopfrom
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.
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