-
Notifications
You must be signed in to change notification settings - Fork 30
Bug fixes, updating supported model versions, and summary views #103
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?
Changes from 1 commit
3412954
6b98c25
3b250f1
ec0edc1
598ffd9
65e779b
758277b
c2fa117
df5229a
275c81c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |||||||||||||||||||||||||||||||||||
| from fastapi import APIRouter, Depends, Header, HTTPException, Query | ||||||||||||||||||||||||||||||||||||
| from mcp.server.fastmcp.prompts import base | ||||||||||||||||||||||||||||||||||||
| from mcp.types import TextContent | ||||||||||||||||||||||||||||||||||||
| from ulid import ULID | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| from agent_memory_server import long_term_memory, working_memory | ||||||||||||||||||||||||||||||||||||
| from agent_memory_server.auth import UserInfo, get_current_user | ||||||||||||||||||||||||||||||||||||
|
|
@@ -16,6 +17,7 @@ | |||||||||||||||||||||||||||||||||||
| from agent_memory_server.models import ( | ||||||||||||||||||||||||||||||||||||
| AckResponse, | ||||||||||||||||||||||||||||||||||||
| CreateMemoryRecordRequest, | ||||||||||||||||||||||||||||||||||||
| CreateSummaryViewRequest, | ||||||||||||||||||||||||||||||||||||
| EditMemoryRecordRequest, | ||||||||||||||||||||||||||||||||||||
| GetSessionsQuery, | ||||||||||||||||||||||||||||||||||||
| MemoryMessage, | ||||||||||||||||||||||||||||||||||||
|
|
@@ -24,14 +26,30 @@ | |||||||||||||||||||||||||||||||||||
| MemoryRecord, | ||||||||||||||||||||||||||||||||||||
| MemoryRecordResultsResponse, | ||||||||||||||||||||||||||||||||||||
| ModelNameLiteral, | ||||||||||||||||||||||||||||||||||||
| RunSummaryViewPartitionRequest, | ||||||||||||||||||||||||||||||||||||
| RunSummaryViewRequest, | ||||||||||||||||||||||||||||||||||||
| SearchRequest, | ||||||||||||||||||||||||||||||||||||
| SessionListResponse, | ||||||||||||||||||||||||||||||||||||
| SummaryView, | ||||||||||||||||||||||||||||||||||||
| SummaryViewPartitionResult, | ||||||||||||||||||||||||||||||||||||
| SystemMessage, | ||||||||||||||||||||||||||||||||||||
| Task, | ||||||||||||||||||||||||||||||||||||
| TaskStatusEnum, | ||||||||||||||||||||||||||||||||||||
| TaskTypeEnum, | ||||||||||||||||||||||||||||||||||||
| UpdateWorkingMemory, | ||||||||||||||||||||||||||||||||||||
| WorkingMemory, | ||||||||||||||||||||||||||||||||||||
| WorkingMemoryResponse, | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| from agent_memory_server.summarization import _incremental_summary | ||||||||||||||||||||||||||||||||||||
| from agent_memory_server.summary_views import ( | ||||||||||||||||||||||||||||||||||||
| get_summary_view as get_summary_view_config, | ||||||||||||||||||||||||||||||||||||
| list_partition_results, | ||||||||||||||||||||||||||||||||||||
| list_summary_views, | ||||||||||||||||||||||||||||||||||||
| save_partition_result, | ||||||||||||||||||||||||||||||||||||
| save_summary_view, | ||||||||||||||||||||||||||||||||||||
| summarize_partition_for_view, | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| from agent_memory_server.tasks import create_task, get_task | ||||||||||||||||||||||||||||||||||||
| from agent_memory_server.utils.redis import get_redis_conn | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
@@ -1030,3 +1048,232 @@ async def memory_prompt( | |||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return MemoryPromptResponse(messages=_messages) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| def _validate_summary_view_keys(payload: CreateSummaryViewRequest) -> None: | ||||||||||||||||||||||||||||||||||||
| """Validate group_by and filter keys for a SummaryView. | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| For v1 we explicitly restrict these keys to a small, known set so we can | ||||||||||||||||||||||||||||||||||||
| implement execution safely. We also currently only support long-term | ||||||||||||||||||||||||||||||||||||
| memory as the source for SummaryViews. | ||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if payload.source != "long_term": | ||||||||||||||||||||||||||||||||||||
| raise HTTPException( | ||||||||||||||||||||||||||||||||||||
| status_code=400, | ||||||||||||||||||||||||||||||||||||
| detail=( | ||||||||||||||||||||||||||||||||||||
| "SummaryView.source must be 'long_term' for now; " | ||||||||||||||||||||||||||||||||||||
| "'working_memory' is not yet supported." | ||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| allowed_group_by = {"user_id", "namespace", "session_id", "memory_type"} | ||||||||||||||||||||||||||||||||||||
| allowed_filters = { | ||||||||||||||||||||||||||||||||||||
| "user_id", | ||||||||||||||||||||||||||||||||||||
| "namespace", | ||||||||||||||||||||||||||||||||||||
| "session_id", | ||||||||||||||||||||||||||||||||||||
| "memory_type", | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| invalid_group = [k for k in payload.group_by if k not in allowed_group_by] | ||||||||||||||||||||||||||||||||||||
| if invalid_group: | ||||||||||||||||||||||||||||||||||||
| raise HTTPException( | ||||||||||||||||||||||||||||||||||||
| status_code=400, | ||||||||||||||||||||||||||||||||||||
| detail=("Unsupported group_by fields: " + ", ".join(sorted(invalid_group))), | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1122
to
+1127
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| invalid_filters = [k for k in payload.filters if k not in allowed_filters] | ||||||||||||||||||||||||||||||||||||
| if invalid_filters: | ||||||||||||||||||||||||||||||||||||
| raise HTTPException( | ||||||||||||||||||||||||||||||||||||
| status_code=400, | ||||||||||||||||||||||||||||||||||||
| detail=("Unsupported filter fields: " + ", ".join(sorted(invalid_filters))), | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1135
to
+1136
|
||||||||||||||||||||||||||||||||||||
| # Disallow configurations where the same field is used in both filters and group_by. | |
| conflicting_keys = sorted(set(payload.group_by) & set(payload.filters.keys())) | |
| if conflicting_keys: | |
| raise HTTPException( | |
| status_code=400, | |
| detail=( | |
| "SummaryView.group_by and SummaryView.filters may not use the same " | |
| "fields: " + ", ".join(conflicting_keys) | |
| ), | |
| ) |
Copilot
AI
Dec 17, 2025
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.
The delete endpoint doesn't remove partition summaries, which could lead to orphaned data accumulation. The docstring acknowledges this ("Stored partition summaries are left as-is for now"), but this creates a maintenance burden and potential confusion for users who expect deletion to be complete. Consider implementing cleanup of partition summaries, or at minimum, documenting this limitation in the API response or endpoint description.
| return AckResponse(status="ok") | |
| return AckResponse( | |
| status=( | |
| "ok (SummaryView deleted; stored partition summaries were not removed)" | |
| ) | |
| ) |
Copilot
AI
Dec 17, 2025
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.
The list_summary_view_partitions endpoint allows filtering by any combination of user_id, namespace, session_id, and memory_type, but doesn't validate that these fields are actually in the view's group_by definition. This could lead to confusing empty results when users filter by a field that isn't part of the grouping. Consider validating that filter parameters correspond to fields in view.group_by, or document this behavior clearly.
| # Validate that all requested filter fields are part of the view's group_by definition. | |
| # This avoids confusing empty results when filtering on a field that is not grouped. | |
| if group_filter: | |
| allowed_group_fields = list(getattr(view, "group_by", []) or []) | |
| invalid_fields = [key for key in group_filter.keys() if key not in allowed_group_fields] | |
| if invalid_fields: | |
| raise HTTPException( | |
| status_code=400, | |
| detail=( | |
| "Invalid partition filter field(s): " | |
| f"{', '.join(invalid_fields)}. " | |
| "Filters must correspond to fields in the SummaryView's group_by: " | |
| f"{', '.join(allowed_group_fields) if allowed_group_fields else '(none)'}." | |
| ), | |
| ) |
Copilot
AI
Dec 17, 2025
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.
The expression "group_filter or None" at line 1278 is redundant. If group_filter is an empty dict, it's truthy, so this expression will always pass group_filter (never None). If the intent is to pass None when the dict is empty, use "group_filter if group_filter else None" or just pass group_filter directly since the function handles empty dicts correctly.
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.
The error message format is inconsistent with other validation error messages in the function. The other validation errors use parentheses and "Unsupported" prefix (e.g., "Unsupported group_by fields:"), while this one uses "must be" phrasing. For consistency, consider using a similar format like "SummaryView source must be 'long_term'; 'working_memory' is not yet supported" or reformatting all messages to use a consistent style.