Skip to content

Add ChatRole enum for message history role validation#518

Open
nabilasiregar wants to merge 5 commits intoredis:mainfrom
nabilasiregar:feat/484-Add_ChatRole_enum
Open

Add ChatRole enum for message history role validation#518
nabilasiregar wants to merge 5 commits intoredis:mainfrom
nabilasiregar:feat/484-Add_ChatRole_enum

Conversation

@nabilasiregar
Copy link
Contributor

@nabilasiregar nabilasiregar commented Feb 27, 2026

Changes for #484

  • Add ChatRole(str, Enum) to schema.py as the single source of truth
    for valid roles
  • Update ChatMessage.role to use ChatRole with a field_validator
    that coerces valid strings automatically
  • Align _validate_roles in base_history.py to derive valid roles from
    ChatRole instead of hardcoded values
  • Export ChatRole from the message_history package __init__.py
  • Add unit tests for valid string coercion, direct enum input, deprecated role warning message, invalid role rejection, and string serialization via to_dict()

Note

Medium Risk
Changes the public ChatMessage.role type/validation and role filtering behavior, which may affect downstream callers that assume plain strings (especially the legacy llm role), though deprecated values are still accepted with warnings.

Overview
Adds a ChatRole enum (user/assistant/system/tool) as the single source of truth for message roles, and updates ChatMessage.role to coerce valid role strings into the enum while emitting DeprecationWarnings for legacy/unknown string roles.

Aligns BaseMessageHistory._validate_roles to derive allowed roles from ChatRole (warning-but-allow for llm), exports ChatRole from message_history.__init__, and expands unit tests to cover coercion, deprecation warnings, invalid role rejection, and to_dict() role serialization.

Written by Cursor Bugbot for commit 94211c8. This will update automatically on new commits. Configure here.

@nabilasiregar
Copy link
Contributor Author

Current implementation will break existing string-based behavior (e.g. llm), will fix this asap, don't review just yet

@tylerhutcherson
Copy link
Collaborator

Current implementation will break existing string-based behavior (e.g. llm), will fix this asap, don't review just yet

All good - we can hold

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

DeprecationWarning,
stacklevel=2,
)
return v
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validator accepts any string as deprecated instead of rejecting invalid

High Severity

The coerce_role validator treats every non-ChatRole string as "deprecated," emitting a DeprecationWarning and accepting it. Truly invalid values like "potato" or "" are never rejected with a ValidationError. This contradicts the PR's stated goal and is inconsistent with _validate_roles in base_history.py, which correctly maintains a separate deprecated_roles set and raises ValueError for unknown strings. The validator needs to distinguish genuinely deprecated roles (e.g. "llm") from invalid ones.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_validate_roles in base_history.py previously enforced valid_roles = {"system", "user", "llm", "tool"} for filtering (get_recent(role=...)), and "llm" was the only role removed from that set.
But ChatMessage.role in schema.py was previously role: str with no validation (any string was accepted at message creation). To stay non-breaking, coerce_role warns on all deprecated strings rather than raising. Raising ValueError on strings like "admin" or "bot" would break existing users who stored messages with custom roles, which the old schema never prevented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants