-
Notifications
You must be signed in to change notification settings - Fork 18
Add configurable state reference to ChatMessageTrigger #3531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add configurable state reference to ChatMessageTrigger #3531
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
bc2b18d to
b31409a
Compare
tests/workflows/chat_message_trigger_custom_state/workflows/custom_state_workflow.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01967b6621
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e7d3610de
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
- Add chat_history_key field to ChatMessageTrigger.Config with default 'chat_history' - Update __on_workflow_initiated__ and __on_workflow_fulfilled__ to use configurable key - Update TypeScript types and codegen to support chatHistoryKey in exec_config - Update serialization to include chat_history_key when it differs from default - Add tests for custom chat_history_key functionality Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
- Change chat_history_key: str to state: StateValueReference[List[ChatMessage]] - Update serialization to use state_variable_id format - Update codegen to generate state = State.something syntax - Update tests and snapshots for new API Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
… attribute Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
…licate type Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
3bf419e to
47b38e2
Compare
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
| # AND the error should be a TriggerValidationError with the expected message | ||
| error = state_errors[0] | ||
| assert isinstance(error, TriggerValidationError) | ||
| assert "Chat Trigger state must be specified" in str(error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test when state is not specified
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a219b8dff6
ℹ️ 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".
| if state is None: | ||
| self.display_context.add_validation_error( | ||
| TriggerValidationError( | ||
| message="Chat Trigger state must be specified.", | ||
| trigger_class_name=trigger_class.__name__, | ||
| ) | ||
| ) | ||
|
|
||
| if output is None or state is None: | ||
| return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preserve default chat_history when Config.state is unset
This serializer now treats Config.state as mandatory and returns None for the entire exec_config when it’s missing. That’s inconsistent with the runtime behavior in ChatMessageTrigger, which still falls back to state.chat_history when Config.state is None. As a result, workflows that rely on the default chat history (no explicit state set, but chat_history present on the workflow state) will suddenly serialize with missing exec_config/state and be flagged as invalid, even though they still execute. Consider defaulting to the workflow’s chat_history state variable during serialization (or at least emitting the default exec_config) so serialization matches runtime behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI can we default to chat_history
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Updated the serialization to default to "chat_history" when Config.state is not specified. The serializer now looks for a state value with name "chat_history" in the workflow's state class and uses that if found. Updated the test to verify this default behavior works correctly.
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
| break | ||
|
|
||
| if state is None: | ||
| return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI should we still raise error here if its none?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, added a validation error when state is still None after trying to default to "chat_history". The error message is: "Chat Trigger state must be specified or state class must have 'chat_history' attribute."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI make the error message only Chat Trigger state must be specified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Simplified the error message to just "Chat Trigger state must be specified."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI can we just allow state to be none for backward compatibility? or will that break anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can allow state to be None for backward compatibility. It won't break anything - the serialization will just return None for exec_config, and the runtime already handles this gracefully by returning early without appending to chat history.
Should I remove the validation error so that state can be None without raising an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Removed the validation error so state can be None for backward compatibility. When state is None (and no "chat_history" exists), the exec_config will still include the output but won't include the state field.
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
| break | ||
|
|
||
| if state is None: | ||
| return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI make the error message only Chat Trigger state must be specified
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4583bb7bc
ℹ️ 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".
| # If state is not specified, default to "chat_history" state value | ||
| if state is None: | ||
| for state_value in self.display_context.global_state_value_displays: | ||
| if state_value.name == "chat_history": | ||
| state = state_value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default to local chat_history state before global
The defaulting logic iterates global_state_value_displays and picks the first state named chat_history. In nested workflows, that dict is built by copying the parent’s state values and then adding the child’s, so a parent chat_history will be chosen first. This means a ChatMessageTrigger in a nested workflow that omits Config.state will serialize its exec_config to the parent state_variable_id even though runtime defaults to the nested workflow’s state, causing a mismatch in serialized workflow displays and state tracking. This only occurs when both parent and child define chat_history and Config.state is omitted, but in that case it should prefer the local state_value_displays or explicitly prioritize the current workflow’s state class.
Useful? React with 👍 / 👎.
Co-Authored-By: vincent@vellum.ai <0426vincent@gmail.com>
Adds support for configurable chat history state in
ChatMessageTriggerusing a descriptor-based reference pattern. Users must now specify bothoutputandstatein the Config class. Thestateattribute specifies which state attribute to use for chat history viastate = State.messagessyntax.Updates since last revision:
List[ArrayChatMessageContentItem]format for message parametercontentwithArrayChatMessageContentinstead oftext