diff --git a/packages/client/client_tests/test_integration_complete.py b/packages/client/client_tests/test_integration_complete.py index fc753ca22..fd8c803a0 100644 --- a/packages/client/client_tests/test_integration_complete.py +++ b/packages/client/client_tests/test_integration_complete.py @@ -1218,7 +1218,7 @@ def test_create_semantic_set_type(self, memory, unique_test_ids): set_type_id = memory.create_semantic_set_type( metadata_tags=["user_id", "session_id"], is_org_level=False, - name="User Sessions", + name="user_sessions", description="Set type for user sessions", ) @@ -1240,7 +1240,7 @@ def test_list_semantic_set_types(self, memory, unique_test_ids): # Create a set type first memory.create_semantic_set_type( metadata_tags=["list_test_tag"], - name="List Test Set Type", + name="list_test_set_type", ) # List set types @@ -1255,7 +1255,7 @@ def test_delete_semantic_set_type(self, memory, unique_test_ids): # Create a set type to delete set_type_id = memory.create_semantic_set_type( metadata_tags=["delete_test_tag"], - name="Delete Test Set Type", + name="delete_test_set_type", ) # Delete it @@ -1342,7 +1342,7 @@ def test_semantic_set_type_lifecycle(self, memory, unique_test_ids): # Step 1: Create set type set_type_id = memory.create_semantic_set_type( metadata_tags=["lifecycle_tag"], - name="Lifecycle Test", + name="lifecycle_test", description="Testing full lifecycle", ) assert set_type_id is not None @@ -1417,7 +1417,7 @@ def test_semantic_category_template_lifecycle(self, memory, unique_test_ids): # Step 1: Create a set type for templates set_type_id = memory.create_semantic_set_type( metadata_tags=["template_test_tag"], - name="Template Test Type", + name="template_test_type", description="Testing category templates", ) assert set_type_id is not None @@ -1481,7 +1481,7 @@ def test_semantic_category_disable(self, memory, unique_test_ids): # Step 1: Create a set type with a category template set_type_id = memory.create_semantic_set_type( metadata_tags=["disable_test_tag"], - name="Disable Test Type", + name="disable_test_type", ) assert set_type_id is not None diff --git a/packages/client/client_tests/test_memory.py b/packages/client/client_tests/test_memory.py index bebf27beb..667206eb7 100644 --- a/packages/client/client_tests/test_memory.py +++ b/packages/client/client_tests/test_memory.py @@ -1497,7 +1497,7 @@ def test_create_semantic_set_type_success(self, mock_client): result = memory.create_semantic_set_type( metadata_tags=["user_id", "session_id"], is_org_level=False, - name="User Sessions", + name="user_sessions", description="Set type for user sessions", ) @@ -1511,7 +1511,7 @@ def test_create_semantic_set_type_success(self, mock_client): assert json_data["project_id"] == "test_project" assert json_data["metadata_tags"] == ["user_id", "session_id"] assert json_data["is_org_level"] is False - assert json_data["name"] == "User Sessions" + assert json_data["name"] == "user_sessions" assert json_data["description"] == "Set type for user sessions" def test_create_semantic_set_type_minimal(self, mock_client): diff --git a/packages/common/src/memmachine_common/api/spec.py b/packages/common/src/memmachine_common/api/spec.py index 7d9f6c100..c3675f57f 100644 --- a/packages/common/src/memmachine_common/api/spec.py +++ b/packages/common/src/memmachine_common/api/spec.py @@ -190,10 +190,47 @@ def _validate_int_compatible(v: str) -> str: return v +# Stricter rule for user-supplied field/property names (issue #1383), +# distinct from `_is_valid_name`/`SafeId` which validates *identifiers* +# (org_id, project_id, UIDs) and stays permissive for backward compat. +MAX_FIELD_NAME_LENGTH = 32 +RESERVED_FIELD_NAME_PREFIX = "_" +_FIELD_NAME_REGEX = regex.compile(r"^[a-z][a-z0-9_]*$") + + +def _is_valid_field_name(v: str) -> str: + """Validate a field/property name against the rule from issue #1383. + + - 1-32 characters + - starts with a lowercase letter, then ``[a-z0-9_]`` + - leading underscore reserved for system use + + Three checks instead of one combined regex so the error message names + the specific rule that was violated. + """ + if not v or len(v) > MAX_FIELD_NAME_LENGTH: + raise InvalidNameError( + f"Field/property name must be 1-{MAX_FIELD_NAME_LENGTH} chars, " + f"got {len(v)}: '{v}'", + ) + if v.startswith(RESERVED_FIELD_NAME_PREFIX): + raise InvalidNameError( + "Field/property names beginning with '_' are reserved for system " + f"use: '{v}'", + ) + if not _FIELD_NAME_REGEX.fullmatch(v): + raise InvalidNameError( + "Field/property name must start with a-z and contain only " + f"[a-z0-9_]: '{v}'", + ) + return v + + IntCompatibleId = Annotated[str, AfterValidator(_validate_int_compatible), Field(...)] SafeId = Annotated[str, AfterValidator(_is_valid_name), Field(...)] SafeIdWithDefault = Annotated[SafeId, Field(default=DEFAULT_ORG_AND_PROJECT_ID)] +SafeFieldName = Annotated[str, AfterValidator(_is_valid_field_name), Field(...)] class _WithOrgAndProj(BaseModel): @@ -473,7 +510,7 @@ class MemoryMessage(BaseModel): default="", description=SpecDoc.MEMORY_ROLE, ) - metadata: dict[str, str] = Field( + metadata: dict[SafeFieldName, str] = Field( default_factory=dict, description=SpecDoc.MEMORY_METADATA, ) @@ -582,7 +619,7 @@ class SearchMemoriesSpec(_WithOrgAndProj): ), ] set_metadata: Annotated[ - dict[str, JsonValue] | None, + dict[SafeFieldName, JsonValue] | None, Field( default=None, description=SpecDoc.SET_METADATA, @@ -672,7 +709,7 @@ class ListMemoriesSpec(_WithOrgAndProj): ), ] set_metadata: Annotated[ - dict[str, JsonValue] | None, + dict[SafeFieldName, JsonValue] | None, Field( default=None, description=SpecDoc.SET_METADATA, @@ -773,6 +810,10 @@ class AddFeatureSpec(_WithOrgAndProj): description=SpecDoc.FEATURE_SET_ID, ), ] + # Lookup field referencing an existing category by name (see + # ``SemanticMemory.add_new_feature``, which raises ``CategoryNotFoundError`` + # for unknown categories). Kept on permissive ``str`` so features can + # still be added to legacy categories whose names predate the #1383 rule. category_name: Annotated[ str, Field( @@ -781,14 +822,14 @@ class AddFeatureSpec(_WithOrgAndProj): ), ] tag: Annotated[ - str, + SafeFieldName, Field( ..., description=SpecDoc.FEATURE_TAG, ), ] feature: Annotated[ - str, + SafeFieldName, Field( ..., description=SpecDoc.FEATURE_NAME, @@ -802,7 +843,7 @@ class AddFeatureSpec(_WithOrgAndProj): ), ] feature_metadata: Annotated[ - dict[str, JsonValue] | None, + dict[SafeFieldName, JsonValue] | None, Field( default=None, description=SpecDoc.FEATURE_METADATA, @@ -858,6 +899,9 @@ class UpdateFeatureSpec(_WithOrgAndProj): description=SpecDoc.FEATURE_ID, ), ] + # Lookup of an existing category by name (see ``SemanticMemory.add_new_feature`` + # at ``semantic_memory.py:299``). Permissive so legacy categories stay + # addressable, matching ``AddFeatureSpec.category_name``. category_name: Annotated[ str | None, Field( @@ -866,14 +910,14 @@ class UpdateFeatureSpec(_WithOrgAndProj): ), ] tag: Annotated[ - str | None, + SafeFieldName | None, Field( default=None, description=SpecDoc.FEATURE_TAG, ), ] feature: Annotated[ - str | None, + SafeFieldName | None, Field( default=None, description=SpecDoc.FEATURE_NAME, @@ -887,7 +931,7 @@ class UpdateFeatureSpec(_WithOrgAndProj): ), ] metadata: Annotated[ - dict[str, str] | None, + dict[SafeFieldName, str] | None, Field( default=None, description=SpecDoc.FEATURE_METADATA, @@ -1055,14 +1099,14 @@ class CreateSemanticSetTypeSpec(_WithOrgAndProj): ), ] metadata_tags: Annotated[ - list[str], + list[SafeFieldName], Field( ..., description=SpecDoc.SET_TYPE_METADATA_TAGS, ), ] name: Annotated[ - str | None, + SafeFieldName | None, Field( default=None, description=SpecDoc.SET_TYPE_NAME, @@ -1168,14 +1212,14 @@ class GetSemanticSetIdSpec(_WithOrgAndProj): ), ] metadata_tags: Annotated[ - list[str], + list[SafeFieldName], Field( ..., description=SpecDoc.SET_TYPE_METADATA_TAGS, ), ] set_metadata: Annotated[ - dict[str, JsonValue] | None, + dict[SafeFieldName, JsonValue] | None, Field( default=None, description=SpecDoc.SET_METADATA, @@ -1199,7 +1243,7 @@ class ListSemanticSetIdsSpec(_WithOrgAndProj): """Specification model for listing semantic set IDs.""" set_metadata: Annotated[ - dict[str, JsonValue] | None, + dict[SafeFieldName, JsonValue] | None, Field( default=None, description=SpecDoc.SET_METADATA, @@ -1344,7 +1388,7 @@ class AddSemanticCategorySpec(_WithOrgAndProj): ), ] category_name: Annotated[ - str, + SafeFieldName, Field( ..., description=SpecDoc.CATEGORY_NAME, @@ -1389,7 +1433,7 @@ class AddSemanticCategoryTemplateSpec(_WithOrgAndProj): ), ] category_name: Annotated[ - str, + SafeFieldName, Field( ..., description=SpecDoc.CATEGORY_NAME, @@ -1485,6 +1529,9 @@ class DisableSemanticCategorySpec(_WithOrgAndProj): description=SpecDoc.SEMANTIC_SET_ID, ), ] + # Lookup field referencing an existing category by name; kept on the + # permissive `str` type so legacy categories remain disable-able even + # if their names predate the #1383 rule. category_name: Annotated[ str, Field( @@ -1544,7 +1591,7 @@ class AddSemanticTagSpec(_WithOrgAndProj): ), ] tag_name: Annotated[ - str, + SafeFieldName, Field( ..., description=SpecDoc.TAG_NAME, diff --git a/packages/server/server_tests/memmachine_server/server/api_v2/test_router.py b/packages/server/server_tests/memmachine_server/server/api_v2/test_router.py index 4633f57d6..6db0eb71f 100644 --- a/packages/server/server_tests/memmachine_server/server/api_v2/test_router.py +++ b/packages/server/server_tests/memmachine_server/server/api_v2/test_router.py @@ -985,12 +985,14 @@ def test_update_feature_error(client, mock_memmachine): def test_create_semantic_set_type(client, mock_memmachine): + # `name` and `metadata_tags` items must conform to the #1383 rule + # (lowercase alphanumeric/underscore, ≤32 chars, no leading "_"). payload = { "org_id": "test_org", "project_id": "test_proj", "metadata_tags": ["user_id", "session_id"], "is_org_level": False, - "name": "User Sessions", + "name": "user_sessions", "description": "Set type for user sessions", } @@ -1005,7 +1007,7 @@ def test_create_semantic_set_type(client, mock_memmachine): assert call_args["session_data"].session_key == "test_org/test_proj" assert call_args["metadata_tags"] == ["user_id", "session_id"] assert call_args["is_org_level"] is False - assert call_args["name"] == "User Sessions" + assert call_args["name"] == "user_sessions" assert call_args["description"] == "Set type for user sessions" diff --git a/packages/server/server_tests/memmachine_server/server/api_v2/test_spec.py b/packages/server/server_tests/memmachine_server/server/api_v2/test_spec.py index 64a8cffd0..1dc0a1006 100644 --- a/packages/server/server_tests/memmachine_server/server/api_v2/test_spec.py +++ b/packages/server/server_tests/memmachine_server/server/api_v2/test_spec.py @@ -10,13 +10,17 @@ from memmachine_common.api.doc import SpecDoc from memmachine_common.api.spec import ( DEFAULT_ORG_AND_PROJECT_ID, + MAX_FIELD_NAME_LENGTH, + AddFeatureSpec, AddMemoriesResponse, AddMemoriesSpec, AddMemoryResult, CreateProjectSpec, + CreateSemanticSetTypeSpec, DeleteEpisodicMemorySpec, DeleteProjectSpec, DeleteSemanticMemorySpec, + DisableSemanticCategorySpec, InvalidNameError, ListMemoriesSpec, MemoryMessage, @@ -24,6 +28,8 @@ ProjectResponse, SearchMemoriesSpec, SearchResult, + UpdateFeatureSpec, + _is_valid_field_name, _is_valid_name, ) from pydantic import ValidationError @@ -438,3 +444,105 @@ def test_timestamp_invalid_type(): MemoryMessage.model_validate( {"content": "hello", "timestamp": {"bad": "value"}} ) + + +# --- Field/property name validation (#1383) ---------------------------------- + + +@pytest.mark.parametrize( + "value", + ["a", "foo", "foo_bar", "a1", "user_id", "a" * MAX_FIELD_NAME_LENGTH], +) +def test_validate_field_name_valid(value): + assert _is_valid_field_name(value) == value + + +@pytest.mark.parametrize( + ("value", "expected_phrase"), + [ + ("", "1-32 chars"), + ("a" * (MAX_FIELD_NAME_LENGTH + 1), "1-32 chars"), + ("_internal", "reserved for system"), + ("0", "start with a-z and contain only"), # digit-leading + ("Foo", "start with a-z and contain only"), # uppercase + ("foo-bar", "start with a-z and contain only"), # hyphen + ("foo bar", "start with a-z and contain only"), # space + ], +) +def test_validate_field_name_invalid(value, expected_phrase): + with pytest.raises(InvalidNameError) as exc_info: + _is_valid_field_name(value) + assert expected_phrase in str(exc_info.value) + + +def test_dict_key_wiring_via_memory_message_metadata(): + """One sample of `dict[SafeFieldName, V]` enforcement; same wiring is + reused on `*.set_metadata`, `feature_metadata`, `UpdateFeatureSpec.metadata`.""" + with pytest.raises(ValidationError): + MemoryMessage.model_validate({"content": "hi", "metadata": {"Bad-Key": "x"}}) + msg = MemoryMessage.model_validate( + {"content": "hi", "metadata": {"user_id": "alice"}} + ) + assert msg.metadata == {"user_id": "alice"} + + +def test_scalar_wiring_via_add_feature_spec(): + """One sample of scalar `SafeFieldName` enforcement; same wiring is + reused on `tag`, `feature`, `name`, `category_name` (create paths), + `tag_name`.""" + with pytest.raises(ValidationError): + AddFeatureSpec.model_validate( + { + "org_id": "org", + "project_id": "proj", + "set_id": "set:legacy", + "category_name": "prefs", + "tag": "Bad-Tag", + "feature": "favorite_food", + "value": "pizza", + } + ) + + +def test_list_item_wiring_via_metadata_tags(): + """One sample of `list[SafeFieldName]` enforcement.""" + with pytest.raises(ValidationError): + CreateSemanticSetTypeSpec.model_validate( + { + "org_id": "org", + "project_id": "proj", + "metadata_tags": ["user_id", "Session Id"], + } + ) + + +def test_lookup_carveout_stays_permissive(): + """Lookup-by-name fields (`AddFeatureSpec.category_name`, + `UpdateFeatureSpec.category_name`, `DisableSemanticCategorySpec.category_name`) + remain permissive so legacy categories can still be addressed — + `SemanticMemory.add_new_feature` raises `CategoryNotFoundError` for + unknowns, so the API can't reject names it doesn't own.""" + spec = DisableSemanticCategorySpec.model_validate( + { + "org_id": "org", + "project_id": "proj", + "set_id": "set:legacy", + "category_name": "Old-Cat", + } + ) + assert spec.category_name == "Old-Cat" + + +def test_update_feature_spec_partial_update_skips_unset_fields(): + """None defaults skip validation, so PATCH-style updates that omit + name fields keep working.""" + spec = UpdateFeatureSpec.model_validate( + { + "org_id": "org", + "project_id": "proj", + "feature_id": "feat-uuid", + "value": "new_value", + } + ) + assert spec.value == "new_value" + assert spec.tag is None