fix picoschema modifier descriptions

Author: groksrc

src/basic_memory/schema/parser.py

@@ -8,7 +8,9 @@
   field: type, description          # required field
   field?: type, description         # optional field
   field(array): type                # array of values
+  field(array, description): type   # array with description
   field?(enum): [val1, val2]        # enumeration
+  field?(enum, description): [val1, val2]  # enum with description
   field?(object):                   # nested object
     sub_field: type
   EntityName as type (capitalized)  # entity reference
@@ -59,45 +61,58 @@ class SchemaDefinition:
 
 SCALAR_TYPES = frozenset({"string", "integer", "number", "boolean", "any"})
 
+MODIFIER_RE = re.compile(r"\(\s*(array|enum|object)\s*(?:,\s*([^)]*?))?\s*\)\s*$")
+
 
 # --- Field Name Parsing ---
 
 
-def _parse_field_key(key: str) -> tuple[str, bool, bool, bool, bool]:
+def _parse_field_key_parts(key: str) -> tuple[str, bool, bool, bool, bool, str | None]:
     """Parse a Picoschema field key into its components.
 
-    Returns (name, required, is_array, is_enum, is_object).
-    The key format is: name[?][(array|enum|object)]
+    Returns (name, required, is_array, is_enum, is_object, description).
+    The key format is: name[?][(array|enum|object[, description])]
 
     Examples:
         "name"           -> ("name", True, False, False)
         "role?"          -> ("role", False, False, False)
         "tags?(array)"   -> ("tags", False, True, False)
+        "tags?(array, labels)" -> ("tags", False, True, False) + "labels"
         "status?(enum)"  -> ("status", False, False, True)
         "metadata?(object)" -> ("metadata", False, False, False) + children
     """
     required = True
     is_array = False
     is_enum = False
     is_object = False
-
-    # Check for modifier suffix: (array), (enum), (object)
-    if key.endswith("(array)"):
-        is_array = True
-        key = key[: -len("(array)")]
-    elif key.endswith("(enum)"):
-        is_enum = True
-        key = key[: -len("(enum)")]
-    elif key.endswith("(object)"):
-        is_object = True
-        key = key[: -len("(object)")]
+    description = None
+
+    modifier_match = MODIFIER_RE.search(key)
+    if modifier_match:
+        modifier = modifier_match.group(1)
+        description_match = modifier_match.group(2)
+        description = description_match.strip() if description_match else None
+        key = key[: modifier_match.start()].rstrip()
+
+        if modifier == "array":
+            is_array = True
+        elif modifier == "enum":
+            is_enum = True
+        elif modifier == "object":
+            is_object = True
 
     # Check for optional marker
     if key.endswith("?"):
         required = False
         key = key[:-1]
 
-    return key, required, is_array, is_enum, is_object
+    return key.strip(), required, is_array, is_enum, is_object, description
+
+
+def _parse_field_key(key: str) -> tuple[str, bool, bool, bool, bool]:
+    """Parse a Picoschema field key, discarding any modifier description."""
+    name, required, is_array, is_enum, is_object, _description = _parse_field_key_parts(key)
+    return name, required, is_array, is_enum, is_object
 
 
 def _parse_type_and_description(value: str) -> tuple[str, str | None]:
@@ -170,7 +185,9 @@ def parse_picoschema(yaml_dict: dict) -> list[SchemaField]:
     fields: list[SchemaField] = []
 
     for key, value in yaml_dict.items():
-        name, required, is_array, is_enum, is_object = _parse_field_key(key)
+        name, required, is_array, is_enum, is_object, key_description = _parse_field_key_parts(
+            key
+        )
 
         # --- Enum fields ---
         # Trigger: value is a list or a string containing bracketed enum values
@@ -179,11 +196,12 @@ def parse_picoschema(yaml_dict: dict) -> list[SchemaField]:
         #   in YAML to avoid parse errors)
         # Outcome: SchemaField with is_enum=True and enum_values populated
         if is_enum:
-            description = None
+            description = key_description
             if isinstance(value, list):
                 enum_values = [str(v) for v in value]
             else:
-                enum_values, description = _parse_enum_string(str(value))
+                enum_values, value_description = _parse_enum_string(str(value))
+                description = description or value_description
             fields.append(
                 SchemaField(
                     name=name,
@@ -207,13 +225,15 @@ def parse_picoschema(yaml_dict: dict) -> list[SchemaField]:
                     name=name,
                     type="object",
                     required=required,
+                    description=key_description,
                     children=children,
                 )
             )
             continue
 
         # --- Scalar and entity ref fields ---
-        type_str, description = _parse_type_and_description(str(value))
+        type_str, value_description = _parse_type_and_description(str(value))
+        description = key_description or value_description
         is_entity_ref = _is_entity_ref_type(type_str)
 
         fields.append(

tests/mcp/test_tool_schema.py

@@ -116,6 +116,72 @@ async def test_schema_validate_json_output(app, test_project, sync_service):
     assert result["results"][0]["passed"] is True
 
 
+@pytest.mark.asyncio
+async def test_schema_validate_picoschema_modifier_descriptions(
+    app, test_project, sync_service
+):
+    """Modifier descriptions should not become literal field names."""
+    project_path = Path(test_project.path)
+
+    _write_schema_file(
+        project_path,
+        "schemas/PicoTest.md",
+        """\
+---
+title: PicoTest
+type: schema
+entity: pico_test
+schema:
+  name: string
+  status(enum, current state): [active, inactive]
+  tags(array, list of tags): string
+settings:
+  validation: warn
+---
+
+# PicoTest
+""",
+    )
+    _write_schema_file(
+        project_path,
+        "pico/PicoTest1.md",
+        """\
+---
+title: PicoTest1
+type: pico_test
+permalink: pico/pico-test-1
+---
+
+# PicoTest1
+
+## Observations
+- [name] PicoTest1
+- [status] active
+- [tags] foo
+- [tags] bar
+""",
+    )
+
+    await sync_service.sync(project_path)
+
+    result = await schema_validate(
+        note_type="pico_test",
+        project=test_project.name,
+        output_format="json",
+    )
+
+    note_result = result["results"][0]
+    field_statuses = {fr["field_name"]: fr["status"] for fr in note_result["field_results"]}
+    assert result["valid_count"] == 1
+    assert note_result["warnings"] == []
+    assert note_result["unmatched_observations"] == {}
+    assert field_statuses == {
+        "name": "present",
+        "status": "present",
+        "tags": "present",
+    }
+
+
 @pytest.mark.asyncio
 async def test_schema_validate_by_identifier(app, test_project, sync_service):
     """Validate a specific note by identifier."""

tests/schema/test_parser.py

@@ -43,18 +43,52 @@ def test_optional_array(self):
         assert required is False
         assert is_array is True
 
+    def test_array_with_description_in_modifier(self):
+        name, required, is_array, is_enum, is_object = _parse_field_key(
+            "tags(array, list of tags)"
+        )
+        assert name == "tags"
+        assert required is True
+        assert is_array is True
+        assert is_enum is False
+        assert is_object is False
+
+    def test_optional_array_with_description_in_modifier(self):
+        name, required, is_array, is_enum, is_object = _parse_field_key(
+            "tags?(array, list of tags)"
+        )
+        assert name == "tags"
+        assert required is False
+        assert is_array is True
+
     def test_enum(self):
         name, required, is_array, is_enum, is_object = _parse_field_key("status?(enum)")
         assert name == "status"
         assert required is False
         assert is_enum is True
 
+    def test_enum_with_description_in_modifier(self):
+        name, required, is_array, is_enum, is_object = _parse_field_key(
+            "status(enum, current state)"
+        )
+        assert name == "status"
+        assert required is True
+        assert is_enum is True
+
     def test_object(self):
         name, required, is_array, is_enum, is_object = _parse_field_key("metadata?(object)")
         assert name == "metadata"
         assert required is False
         assert is_object is True
 
+    def test_object_with_description_in_modifier(self):
+        name, required, is_array, is_enum, is_object = _parse_field_key(
+            "metadata?(object, nested metadata)"
+        )
+        assert name == "metadata"
+        assert required is False
+        assert is_object is True
+
     def test_required_enum(self):
         name, required, is_array, is_enum, is_object = _parse_field_key("status(enum)")
         assert name == "status"
@@ -152,6 +186,13 @@ def test_array_field(self):
         assert fields[0].is_array is True
         assert fields[0].required is False
 
+    def test_array_field_with_description_in_modifier(self):
+        fields = parse_picoschema({"tags(array, list of tags)": "string"})
+        assert fields[0].name == "tags"
+        assert fields[0].type == "string"
+        assert fields[0].is_array is True
+        assert fields[0].description == "list of tags"
+
     def test_entity_ref_field(self):
         fields = parse_picoschema({"works_at?": "Organization, employer"})
         assert fields[0].name == "works_at"
@@ -165,6 +206,15 @@ def test_enum_field_with_list(self):
         assert fields[0].is_enum is True
         assert fields[0].enum_values == ["active", "inactive"]
 
+    def test_enum_field_with_description_in_modifier(self):
+        fields = parse_picoschema({"status(enum, current state)": ["active", "inactive"]})
+        assert fields[0].name == "status"
+        assert fields[0].type == "enum"
+        assert fields[0].required is True
+        assert fields[0].is_enum is True
+        assert fields[0].enum_values == ["active", "inactive"]
+        assert fields[0].description == "current state"
+
     def test_enum_field_with_string(self):
         fields = parse_picoschema({"status?(enum)": "active"})
         assert fields[0].is_enum is True
@@ -204,6 +254,20 @@ def test_object_field(self):
         assert fields[0].children[0].name == "street"
         assert fields[0].children[1].name == "city"
 
+    def test_object_field_with_description_in_modifier(self):
+        fields = parse_picoschema(
+            {
+                "address?(object, mailing address)": {
+                    "street": "string",
+                }
+            }
+        )
+        assert fields[0].name == "address"
+        assert fields[0].type == "object"
+        assert fields[0].required is False
+        assert fields[0].description == "mailing address"
+        assert fields[0].children[0].name == "street"
+
     def test_dict_value_treated_as_object(self):
         """A dict value without explicit (object) is still treated as an object."""
         fields = parse_picoschema(
@@ -315,6 +379,31 @@ def test_settings_frontmatter_parsed_into_frontmatter_fields(self):
         assert status_field.is_enum is True
         assert status_field.enum_values == ["draft", "published"]
 
+    def test_settings_frontmatter_parses_modifier_descriptions(self):
+        frontmatter = {
+            "entity": "Person",
+            "schema": {"name": "string"},
+            "settings": {
+                "validation": "warn",
+                "frontmatter": {
+                    "tags?(array, note tags)": "string",
+                    "status?(enum, publication state)": ["draft", "published"],
+                },
+            },
+        }
+        result = parse_schema_note(frontmatter)
+
+        tags_field = next(f for f in result.frontmatter_fields if f.name == "tags")
+        assert tags_field.required is False
+        assert tags_field.is_array is True
+        assert tags_field.description == "note tags"
+
+        status_field = next(f for f in result.frontmatter_fields if f.name == "status")
+        assert status_field.required is False
+        assert status_field.is_enum is True
+        assert status_field.enum_values == ["draft", "published"]
+        assert status_field.description == "publication state"
+
     def test_no_settings_frontmatter_defaults_to_empty(self):
         frontmatter = {
             "entity": "Person",