Code review fixes/pr 132#134
Merged
pontemonti merged 6 commits intousers/johanb/RTP_AgentFrameworkfrom Jan 25, 2026
Merged
Conversation
…-010) Rename methods to follow Python conventions and codebase patterns: - send_chat_history_messages_async -> send_chat_history_messages - send_chat_history_async -> send_chat_history_from_store Rename test file accordingly: - test_send_chat_history_async.py -> test_send_chat_history.py Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…009, CRM-013) - CRM-003: Add defensive handling for role value access using hasattr check - CRM-009: Convert debug and warning logging to lazy format (% style) - CRM-013: Simplify redundant empty content check (remove `not content or`) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…06, 011, 012) - CRM-001: Add test for ChatMessageStore exception propagation - CRM-004: Add test for whitespace-only content filtering - CRM-005: Add test for None role handling - CRM-006: Add test for all messages filtered out scenario - CRM-011: Add test for default ToolOptions creation - CRM-012: Make UUID assertion more robust using uuid.UUID() Also adds test for defensive role handling (string role without .value) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The core send_chat_history method now returns OperationResult.success() for empty lists instead of raising ValueError. This is consistent with the extension behavior and makes the API more forgiving. Updated test to verify new behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update design.md to include: - send_chat_history_messages and send_chat_history_from_store methods - Parameter tables for both methods - Integration flow diagram showing message conversion - Message filtering behavior documentation - Example usage code Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Document that _async suffix should NOT be used on async methods in this SDK since we only provide async versions. This prevents future naming inconsistencies. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
pontemonti
added a commit
that referenced
this pull request
Jan 26, 2026
* fix(agentframework): address code review comments CRM-003 and CRM-004
- CRM-003: Add turn_context validation in send_chat_history_async method
to ensure fail-fast behavior and consistency with docstring
- CRM-004: Document empty message filtering behavior in _convert_chat_messages_to_history
docstring and elevate log level from DEBUG to WARNING when messages are skipped
Note: CRM-001 and CRM-002 (copyright header format) are not applicable because
the existing format ("Microsoft. All rights reserved.") is required by the
pyproject.toml linter configuration (notice-rgx), while CLAUDE.md specifies a
different format. The linter-enforced format takes precedence to ensure CI passes.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* fix: update copyright notices to reflect Microsoft Corporation and include license information
* fix(mcp_tool_registration_service): add warning for messages with missing role during conversion
* Code review fixes/pr 132 (#134)
* refactor(agentframework): remove _async suffix from method names (CRM-010)
Rename methods to follow Python conventions and codebase patterns:
- send_chat_history_messages_async -> send_chat_history_messages
- send_chat_history_async -> send_chat_history_from_store
Rename test file accordingly:
- test_send_chat_history_async.py -> test_send_chat_history.py
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* fix(agentframework): improve role handling and logging (CRM-003, CRM-009, CRM-013)
- CRM-003: Add defensive handling for role value access using hasattr check
- CRM-009: Convert debug and warning logging to lazy format (% style)
- CRM-013: Simplify redundant empty content check (remove `not content or`)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* test(agentframework): add missing test coverage (CRM-001, 004, 005, 006, 011, 012)
- CRM-001: Add test for ChatMessageStore exception propagation
- CRM-004: Add test for whitespace-only content filtering
- CRM-005: Add test for None role handling
- CRM-006: Add test for all messages filtered out scenario
- CRM-011: Add test for default ToolOptions creation
- CRM-012: Make UUID assertion more robust using uuid.UUID()
Also adds test for defensive role handling (string role without .value)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* fix(tooling): return success for empty chat history list (CRM-008)
The core send_chat_history method now returns OperationResult.success()
for empty lists instead of raising ValueError. This is consistent with
the extension behavior and makes the API more forgiving.
Updated test to verify new behavior.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* docs(agentframework): add Chat History API documentation (CRM-002)
Update design.md to include:
- send_chat_history_messages and send_chat_history_from_store methods
- Parameter tables for both methods
- Integration flow diagram showing message conversion
- Message filtering behavior documentation
- Example usage code
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* docs: add async method naming convention to CLAUDE.md (CRM-010)
Document that _async suffix should NOT be used on async methods in this
SDK since we only provide async versions. This prevents future naming
inconsistencies.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
---------
Co-authored-by: Johan Broberg <johanb@microsoft.com>
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
---------
Co-authored-by: Johan Broberg <johanb@microsoft.com>
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.