fix(core): parse picoschema modifier descriptions

[groksrc]

Summary

Closes #794.

Implements parsing for picoschema modifier descriptions in schema field keys, so valid syntax like status(enum, current state): [active, inactive] and tags(array, list of tags): string no longer treats the entire key as the field name.

Root Cause

_parse_field_key only recognized exact suffixes like (enum), (array), and (object). When the modifier included a picoschema description, such as (enum, current state), the parser left the suffix in the field name. Validation then looked for observations like [status(enum, current state)] instead of [status].

Changes

• Adds linear modifier parsing that supports (array|enum|object, description) suffixes without using a regex on user-controlled schema keys.
• Preserves descriptions from modifier keys while keeping the existing value-side description behavior.
• Adds parser tests for array, enum, object, and frontmatter modifier descriptions.
• Adds direct _parse_field_key_parts tests for the description-return contract, including descriptions containing parentheses.
• Adds an MCP schema_validate regression test matching the issue shape.
• Pins unrelated MCP search unit tests that assert text search behavior to search_type="text", so Windows unit CI does not depend on loading the semantic embedding model for pagination/filter assertions.

Validation

• Reproduced the bug before the fix on fresh main: status(enum, current state) and tags(array, list of tags) were literal field names and real status/tags observations were unmatched.
• Confirmed the same repro passes after the fix with no warnings and no unmatched observations.
• just typecheck -> passed.
• BASIC_MEMORY_ENV=test uv run pytest -p pytest_mock -v --no-cov tests/mcp/test_tool_search.py -> 48 passed.
• uv run pytest tests/schema tests/mcp/test_tool_schema.py -> 148 passed.
• uv run ruff check tests/mcp/test_tool_search.py tests/mcp/test_tool_schema.py src/basic_memory/schema/parser.py tests/schema/test_parser.py -> passed.

[github-actions]

Claude finished @groksrc's task in 1m 18s —— View job

---

PR Review: Fix picoschema modifier descriptions

Tasks:

• Read context and understand the PR
• Read changed files (parser.py, test files)
• Review against team checklist
• Post final review

---

Summary

This is a well-targeted bug fix. The root cause was correctly identified: _parse_field_key used exact string suffix matching (key.endswith("(array)")) which broke when a description was included in the modifier, e.g. (array, list of tags). The fix replaces brittle suffix matching with a regex (MODIFIER_RE) that handles both forms.

---

Code Quality Checklist

Item	Status
Follows coding conventions (line length, formatting, async, Pydantic)	✅
Python 3.12+ type annotations	✅
No commented-out code	✅
Unit tests for new logic	✅
MCP integration test (test_schema_validate_picoschema_modifier_descriptions)	✅
Edge cases covered (optional/required, all three modifier types)	✅
Documentation (module docstring updated)	✅
No hardcoded secrets or credentials	✅
No security issues	✅
Local-first / knowledge graph consistency preserved	✅
No emoji/unicode in logs	✅
File path comparisons (not applicable here)	N/A

---

Observations

The core change is correct and clean. The regex:

MODIFIER_RE = re.compile(r"\(\s*(array|enum|object)\s*(?:,\s*([^)]*?))?\s*\)\s*$")

correctly captures the modifier type (group 1) and optional description (group 2). The [^)]*? lazy match safely handles any description text without risking runaway backtracking.

Description precedence is consistent. For all three field types (enum, object, scalar), the key-modifier description takes priority over the value-side description. This is a sane policy and applied consistently:

• Enum: description = key_description or value_description
• Scalar: description = key_description or value_description
• Object: uses key_description directly (no value-side description for objects)

Backward-compat wrapper is a good pattern. Keeping _parse_field_key as a thin wrapper around _parse_field_key_parts means existing tests using the 5-tuple form continue to work without changes, while internal code gets the richer 6-tuple. Good.

---

Minor Issues

1. Docstring examples in _parse_field_key_parts show 4-element tuples, not 6

In parser.py:78–83, the examples show:

"name"           -> ("name", True, False, False)
"role?"          -> ("role", False, False, False)
"tags?(array)"   -> ("tags", False, True, False)

But the function returns (name, required, is_array, is_enum, is_object, description) — 6 elements. The examples should be updated to reflect the actual return shape. This won't cause bugs, but it's misleading to maintainers.

Fix this →

2. New _parse_field_key_parts is not imported or tested directly

test_parser.py imports _parse_field_key but not _parse_field_key_parts. All new test cases go through the _parse_field_key wrapper, which discards the description. This means the description return value from _parse_field_key_parts is exercised via parse_picoschema tests but not via direct unit tests of the function. Not a coverage gap per se — the TestParsePicoschema tests verify descriptions end up on SchemaField — but a direct test like assert _parse_field_key_parts("tags(array, list of tags)") == ("tags", True, True, False, False, "list of tags") would make the contract explicit. This is a very minor point; the integration coverage is sufficient.

---

Result

This PR is ready to merge. The fix is correct, the tests are thorough, and the code is clean. The minor docstring inconsistency is the only thing worth noting, and it can be fixed in this PR or as a follow-up.

[github-actions]

Claude finished @groksrc's task in 2m 15s —— View job

---

PR Review: fix(core): parse picoschema modifier descriptions

Tasks:

• Read context and understand the PR
• Read changed files (parser.py, test_parser.py, test_tool_schema.py)
• Review against team checklist
• Address CodeQL security concern (polynomial regex)
• Post final review

---

Summary

Correct, well-scoped bug fix. The root cause was properly identified and the replacement implementation is clean. The previous review's minor docstring issue (4-element tuple examples) has already been fixed in the current code — _parse_field_key_parts now shows the correct 6-element tuples.

---

Code Quality Checklist

Item	Status	Notes
Coding conventions (CLAUDE.md)	✅	Literate style, section headers, decision comments
Python 3.12+ type annotations	✅	Full annotations on all new functions
SQLAlchemy 2.0 / FastAPI / Typer	✅ N/A	Parser is pure Python, no DB/web layer touched
100-character line length	✅
No commented-out code	✅
Unit tests for new functions	✅	TestParseFieldKeyParts, new TestParseFieldKey cases
Integration tests	✅	test_schema_validate_picoschema_modifier_descriptions
Edge cases covered	✅	array/enum/object × required/optional × with/without description
100% test coverage maintained	✅	No pragma: no cover added; all new paths exercised
Docstrings updated	✅	Module docstring and _parse_field_key_parts examples correct
No hardcoded secrets	✅
Security: polynomial regex (CodeQL)	✅ Resolved	See below
Local-first / knowledge graph consistency	✅	Parser-only change, no DB or file handling affected
No emoji/unicode in output	✅
Windows-compatible file paths	✅ N/A	No path comparisons introduced

---

CodeQL Security Concern — Resolved

The CodeQL alert references MODIFIER_RE, which appeared in an earlier version of this PR. The current implementation has replaced that regex entirely with _split_modifier_suffix, which uses rstrip() → rfind("(") → partition(",") — all linear string operations with no backtracking risk. The concern is resolved.

The one pre-existing regex in _parse_enum_string (r"\[([^\]]+)\](?:\s*,\s*(.+))?") was not introduced by this PR and uses bounded character classes; it's linear and not subject to the same concern.

---

Observations

_split_modifier_suffix is correct for all realistic inputs. Traced through the key cases:

• "tags(array, list of tags)" → key="tags", modifier="array", description="list of tags" ✓
• "status?(enum, current state)" → key="status?", modifier="enum", description="current state" ✓
• "name" (no modifier) → returns original, None, None ✓
• "tags?(array)" → separator is "" (falsy) → description is None ✓

One minor robustness note: The function uses rfind("(") (last occurrence) rather than find("(") (first occurrence). For the unlikely case where a description contains a literal (, e.g., "field(array, items (plural))", rfind would find the inner ( and fail to parse the modifier at all, returning the full key unchanged. Using find would instead correctly extract "field" as the name with "items (plural" as the description (imperfect but closer). This edge case is extremely unlikely in real picoschema usage, and neither approach handles nested parens perfectly — not a blocker.

[Fix this (optional) →](https://claude.ai/code?q=In%20src%2Fbasic_memory%2Fschema%2Fparser.py%20on%20branch%20fix%2Fschema-picoschema-modifier-descriptions%2C%20in%20_split_modifier_suffix%2C%20change%20%60stripped_key.rfind(%22(%22)%60%20to%20%60stripped_key.find(%22(%22)%60%20so%20that%20descriptions%20containing%20nested%20parentheses%20are%20handled%20more%20gracefully.%20The%20modifier%20parenthesis%20is%20always%20the%20first%20one%2C%20not%20the%20last.&repo=basicmachines-co%2Fbasic-memory)

Description precedence is consistent and intentional. Key-modifier description takes priority over value-side description for all field types. The description = key_description or value_description pattern is applied uniformly for enum and scalar fields; object fields use key_description directly (no value-side description for objects). This is correct.

_parse_field_key as a thin wrapper is the right pattern. Existing tests that unpack 5-tuples from _parse_field_key continue to work without modification.

---

Result

Ready to merge. The fix is correct, the CodeQL concern from the earlier PR version is resolved by the non-regex implementation, tests are thorough, and the code follows project conventions. The rfind vs find note above is optional polish, not a correctness issue for any real picoschema input.

[github-actions]

Claude finished @groksrc's task in 1m 55s —— View job

---

PR Review: fix(core): parse picoschema modifier descriptions

Tasks:

• Read context and understand the PR
• Read changed files (parser.py, test_parser.py, test_tool_schema.py)
• Review against team checklist
• Post final review

---

Summary

Targeted, correct bug fix. The root cause was clear: _parse_field_key used exact suffix matching (key.endswith("(array)")) which broke when a description was embedded in the modifier, e.g. (array, list of tags). The replacement — _split_modifier_suffix with linear string operations — is more robust, avoids regex backtracking concerns entirely, and is well-tested.

---

Code Quality Checklist

Item	Status	Notes
Coding conventions (CLAUDE.md)	✅	Literate style with section headers and decision-point comments
Python 3.12+ type annotations	✅	Full annotations on all new/changed functions
SQLAlchemy 2.0 / FastAPI / Typer	✅ N/A	Pure parser change, no DB or web layer touched
100-character line length	✅
No commented-out code	✅
Unit tests for new functions	✅	TestParseFieldKeyParts added; TestParseFieldKey extended with modifier-description cases
Integration tests	✅	test_schema_validate_picoschema_modifier_descriptions covers the exact failure shape from the issue
Edge cases covered	✅	array/enum/object × required/optional × with/without description; nested parens in description
100% test coverage maintained	✅	All new branches exercised; no pragma: no cover added
Docstrings updated	✅	Module docstring and _parse_field_key_parts examples show correct 6-element tuples
No hardcoded secrets	✅
Security: polynomial regex (CodeQL)	✅ N/A	The current implementation has no regex at all — see note below
Local-first / knowledge graph consistency	✅	Parser-only change, no DB or file handling affected
No emoji/unicode in output	✅
Windows-compatible file paths	✅ N/A	No path comparisons introduced
CLAUDE.md updated if conventions change	✅ N/A	No convention changes

---

Key Design Decisions Verified

_split_modifier_suffix uses find("(") (first occurrence) — correct.

A previous review draft raised a concern about rfind vs find, but the current code correctly uses find. Tracing through the nested-paren test case "tags(array, labels (freeform))":

• find("(") → index 4 (the modifier's opening paren, which is first)
• stripped_key[5:-1] → "array, labels (freeform)" (inner content, preserving the nested paren)
• partition(",") → ("array", ",", " labels (freeform)")
• description.strip() → "labels (freeform)" ✓

The test at test_parser.py:131 (test_description_can_contain_parentheses) validates this path explicitly.

No regex = no CodeQL concern. The CodeQL alert referenced MODIFIER_RE from an earlier version of this PR. The final implementation has replaced it entirely with rstrip() → find("(") → partition(",") — all O(n) string operations. The only remaining regex in the file is _parse_enum_string's r"\[([^\]]+)\](?:\s*,\s*(.+))?", which was pre-existing and uses bounded character classes (linear).

Description precedence is consistent. Key-modifier description takes priority over value-side description for all field types:

• Enum: description = key_description or value_description (parser.py:219)
• Scalar: description = key_description or value_description (parser.py:251)
• Object: uses key_description directly (parser.py:243) — no value-side description for objects

---

Result

Ready to merge. The fix is correct, the tests are thorough, all checklist items pass. No issues found.

[github-actions]

Claude finished @groksrc's task in 1m 0s —— View job

---

PR Review: fix(core): parse picoschema modifier descriptions

Tasks:

• Read context and understand the PR
• Read changed files (parser.py, test_parser.py, test_tool_schema.py)
• Review against team checklist
• Post final review

---

Summary

Correct, well-scoped bug fix. The root cause (_parse_field_key using exact suffix matching that broke when descriptions were embedded in modifiers) is resolved with a clean linear-scan replacement. All prior review concerns have been addressed in the current code.

---

Code Quality Checklist

Item	Status	Notes
Coding conventions (CLAUDE.md)	✅	Literate style, section headers, decision-point comments throughout
Python 3.12+ type annotations	✅	Full annotations on all new/changed functions
SQLAlchemy 2.0 / FastAPI / Typer	✅ N/A	Pure parser change — no DB or web layer touched
100-character line length	✅
No commented-out code	✅
Unit tests for new functions	✅	TestParseFieldKeyParts added; TestParseFieldKey extended; TestParsePicoschema covers all three modifier types with descriptions
Integration tests	✅	test_schema_validate_picoschema_modifier_descriptions covers exact failure shape from #794
Edge cases covered	✅	array/enum/object × required/optional × with/without description; nested parens in description; frontmatter modifier descriptions
100% test coverage maintained	✅	All new branches exercised; no pragma: no cover added
Docstrings updated	✅	Module docstring and _parse_field_key_parts examples show correct 6-element tuples
No hardcoded secrets	✅
Security: polynomial regex (CodeQL)	✅ Resolved	No regex for modifier parsing — see note below
Local-first / knowledge graph consistency	✅	Parser-only change; no DB or file handling affected
No emoji/unicode in output	✅
Windows-compatible file paths	✅ N/A	No path comparisons introduced

---

Key Design Decisions Verified

_split_modifier_suffix uses find("(") (first occurrence) — correct.

Traced through the edge case "tags(array, labels (freeform))":

• find("(") → index 4 (the modifier's opening paren, which is first)
• inner content: "array, labels (freeform)"
• partition(",") → ("array", ",", " labels (freeform)")
• description.strip() → "labels (freeform)" ✓

This is validated by test_description_can_contain_parentheses at test_parser.py:131. A previous review draft raised the rfind vs find concern — the current code already uses find and is correct.

Empty description handled cleanly. parsed_description or None at parser.py:130 collapses "" to None, so "field(array, )" doesn't produce a spurious empty-string description.

CodeQL alert is resolved. The alert referenced MODIFIER_RE from an earlier version of this PR. The final implementation replaces it entirely with rstrip() → find("(") → partition(",") — all O(n) string operations, no backtracking risk. The only remaining regex (_parse_enum_string's r"\[([^\]]+)\](?:\s*,\s*(.+))?") was pre-existing, uses bounded character classes, and is linear.

Description precedence is consistent across all field types:

• Enum (parser.py:219): description = key_description or value_description
• Scalar (parser.py:251): description = key_description or value_description
• Object (parser.py:243): uses key_description directly — correct, no value-side description for objects

Backward-compat wrapper is sound. _parse_field_key as a thin 5-tuple wrapper means all existing callers continue to work unchanged. parse_picoschema itself was updated to use _parse_field_key_parts directly to access key_description.

---

Result

Ready to merge. The fix is correct, the tests are thorough, all checklist items pass, and the CodeQL concern from the earlier version is fully resolved. No issues found.

[github-actions]

Claude finished @groksrc's task in 1m 32s —— View job

---

PR Review: fix(core): parse picoschema modifier descriptions

Tasks:

• Read context and understand the PR
• Read changed files (parser.py, test_parser.py, test_tool_schema.py, test_tool_search.py)
• Review against team checklist
• Post final review

---

Summary

Correct, well-scoped bug fix. _parse_field_key previously used exact string suffix matching (key.endswith("(array)")) which broke when a description was embedded in the modifier — e.g. (array, list of tags). The replacement _split_modifier_suffix uses linear string operations (rstrip() → find("(") → partition(",")) with no backtracking risk.

---

Code Quality Checklist

Item	Status	Notes
Coding conventions (CLAUDE.md)	✅	Literate style, section headers, decision-point comments
Python 3.12+ type annotations	✅	Full annotations on all new/changed functions
SQLAlchemy 2.0 / FastAPI / Typer	✅ N/A	Pure parser change — no DB or web layer
100-character line length	✅
No commented-out code	✅
Unit tests for new functions	✅	TestParseFieldKeyParts added; TestParseFieldKey extended with modifier-description cases
Integration tests	✅	test_schema_validate_picoschema_modifier_descriptions covers exact failure shape from #794
Edge cases covered	✅	array/enum/object × required/optional × with/without description; nested parens in description
100% test coverage maintained	✅	All new branches exercised; no pragma: no cover added
Docstrings updated	✅	Module docstring and _parse_field_key_parts examples show correct 6-element tuples
No hardcoded secrets	✅
Security: polynomial regex (CodeQL alert)	✅ Resolved	No regex for modifier parsing — see note below
Local-first / knowledge graph consistency	✅	Parser-only change; no DB or file handling affected
No emoji/unicode in output	✅
Windows-compatible file paths	✅ N/A	No path comparisons introduced

---

Key Design Decisions Verified

_split_modifier_suffix correctness. Traced through the edge cases:

• "tags(array, list of tags)" → key="tags", modifier="array", desc="list of tags" ✓
• "status?(enum, current state)" → key="status?", modifier="enum", desc="current state" ✓
• "name" (no modifier) → returns original, None, None ✓
• "tags?(array)" → separator is "" (falsy) → parsed_description or None → None ✓
• "tags(array, labels (freeform))" → find("(") = 4 (first paren) → "array, labels (freeform)" → partition(",") → ("labels (freeform)") ✓ — validated by test_description_can_contain_parentheses at test_parser.py:131

parsed_description or None at parser.py:130 cleanly collapses an empty description "field(array, )" to None rather than producing a spurious empty string. Good defensive choice.

Non-modifier parens handled correctly. If a key has a paren suffix but the inner token isn't in MODIFIER_TYPES, the function returns the original key unchanged (parser.py:125-126). This prevents false-positive modifier detection on unusual key names.

CodeQL alert is resolved. The alert referenced MODIFIER_RE from an earlier PR version. The final implementation has no modifier regex — only _parse_enum_string's pre-existing r"\[([^\]]+)\](?:\s*,\s*(.+))?" which uses bounded character classes and is linear.

Description precedence is consistent across all field types:

• Enum (parser.py:214, 219): description = key_description, then description = description or value_description
• Scalar (parser.py:251): description = key_description or value_description
• Object (parser.py:243): uses key_description directly — correct, objects have no value-side description

_parse_field_key as thin wrapper means all existing callers continue to work without modification. parse_picoschema was updated to call _parse_field_key_parts directly to gain access to key_description.

---

Note on test_tool_search.py Changes

The test_tool_search.py file has +21/-4 changes that appear unrelated to the picoschema fix (they cover search behavior). These seem incidental. No issues with the added tests — they follow the established mock pattern used throughout the file.

---

Result

Ready to merge. The fix is correct, the CodeQL concern from the earlier PR version is fully resolved by the non-regex implementation, tests are thorough across all three modifier types, and the code follows project conventions. No issues found.

[chatgpt-codex-connector]

💡 Codex Review

basic-memory/src/basic_memory/schema/parser.py

Line 118 in 2de95d1

open_paren_index = stripped_key.find("(")

Parse the trailing modifier instead of the first parenthesis

When a schema field name itself contains parentheses and also uses a modifier, such as risk(score)(array), this now fails to recognize the trailing modifier because find("(") captures from the first parenthesis through the final ), producing score)(array as the modifier text. The previous suffix-based parser handled this shape as field risk(score) with is_array=True; after this change it becomes a literal field named risk(score)(array), so matching observations like [risk(score)] are reported as unmatched and the array rule is lost. Using the opening parenthesis for the trailing modifier avoids regressing these field names.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude finished @phernandez's task in 2m 50s —— View job

---

PR Review: fix(core): parse picoschema modifier descriptions

Tasks:

• Read context and understand the PR
• Read changed files (parser.py, test_parser.py, test_tool_schema.py, test_tool_search.py)
• Assess the Codex find("(") concern — addressed in commit 2350845
• Review against team checklist
• Post final review

---

Summary

Two-commit fix: the first parses modifier descriptions in picoschema field keys; the second (added in response to the Codex review) correctly handles field names that themselves contain parentheses, like risk(score)(array). Both commits together produce a robust, well-tested parser.

---

Code Quality Checklist

Item	Status	Notes
Coding conventions (CLAUDE.md)	✅	Literate style; decision-point comments on the depth-scan loop
Python 3.12+ type annotations	✅	Full annotations on all new functions
SQLAlchemy 2.0 / FastAPI / Typer	✅ N/A	Pure parser change — no DB or web layer
100-character line length	✅
No commented-out code	✅
Unit tests for new functions	✅	TestParseFieldKeyParts added with 6 direct tests; TestParseFieldKey extended
Integration tests	✅	test_schema_validate_picoschema_modifier_descriptions covers exact failure shape from #794
Edge cases covered	✅	array/enum/object × required/optional × with/without description; nested parens in description; field name containing parens; both together
100% test coverage maintained	✅	All branches exercised; no pragma: no cover added
Docstrings updated	✅	Module docstring and _parse_field_key_parts examples show correct 6-element tuples
No hardcoded secrets	✅
Security: polynomial regex (CodeQL)	✅ Resolved	No regex for modifier parsing at all — see note below
Local-first / knowledge graph consistency	✅	Parser-only change; no DB or file handling affected
No emoji/unicode in output	✅
Windows-compatible file paths	✅ N/A	No path comparisons introduced
CLAUDE.md updated if conventions change	✅ N/A	No convention changes

---

Key Design Decisions Verified

Codex concern resolved by commit 2350845

The Codex review identified that find("(") would break for field names like risk(score)(array) — find would grab the first ( at index 4, capturing score)(array as the modifier text instead of just array.

The second commit replaces find("(") with a right-to-left depth-scanning loop:

depth = 0
for index in range(len(stripped_key) - 1, -1, -1):
    char = stripped_key[index]
    if char == ")":
        depth += 1
    elif char == "(":
        depth -= 1
        if depth == 0:
            open_paren_index = index
            break

This finds the outermost ( that pairs with the final ), handling all three edge cases:

Input	open_paren_index	Result
risk(score)(array)	11 ((array)	name=risk(score), modifier=array ✓
tags(array, labels (freeform))	4	name=tags, desc=labels (freeform) ✓
risk(score)?(array, score buckets)	12	name=risk(score), required=False, desc=score buckets ✓

All three cases are now explicitly tested in TestParseFieldKeyParts (lines 139–157) and TestParsePicoschema (line 257–262).

Implementation is purely O(n) — CodeQL concern closed

The only regex remaining is _parse_enum_string's pre-existing r"\[([^\]]+)\](?:\s*,\s*(.+))?" which uses bounded character classes and is linear. The modifier path has no regex at all.

Description precedence is consistent

• Enum (parser.py:231): description = description or value_description (key-modifier wins)
• Scalar (parser.py:263): description = key_description or value_description (key-modifier wins)
• Object (parser.py:255): uses key_description directly — correct, no value-side description for objects

test_tool_search.py changes (incidental, correct)

Four existing tests pinned to search_type="text" to avoid loading the semantic embedding model on Windows CI during pagination and filter assertions. Appropriate defensive fix; the tests still cover their intended behavior.

---

Result

Ready to merge. The fix is correct and complete. The Codex concern about parenthesized field names is resolved by the depth-scanning approach in commit 2350845. Tests are thorough, cover all modifier types, required/optional, descriptions, nested parens, and field names with parens. No issues found.