fix: Quote string values in YAML frontmatter to handle special characters

[phernandez]

Summary

Fixes #408 - YAML frontmatter parsing now handles titles with colons and other special characters.

Changes

• Modified dump_frontmatter() in file_utils.py to use a custom YAML dumper
• Custom QuotedDumper class ensures all string values are double-quoted
• Added 6 comprehensive test cases for special characters in titles
• Tests include round-trip verification for YAML parsing

Solution

Implemented Option 4 from the issue (always quote all string values) as the safest approach. This:

• Prevents YAML parsing errors when strings contain colons
• Handles all YAML special characters safely
• Maintains Obsidian compatibility
• Is backward compatible with existing notes

Testing

All new test cases verify:

• Titles with colons are properly quoted
• YAML can be parsed back without errors
• Original values are preserved after round-trip
• Multiple special characters are handled correctly

---

Generated with Claude Code

[github-actions]

Claude finished @phernandez's task —— View job

---

Code Review Complete ✓

I've completed a comprehensive review of PR #418 against Basic Memory's team checklist. Here are my findings:

---

Review Summary

This PR successfully fixes issue #408 by implementing Option 4 (always quote all string values). The implementation is solid, well-tested, and follows Basic Memory's conventions. I recommend approval with one minor documentation suggestion.

---

Detailed Checklist Results

✅ Code Quality & Standards

• PASS - Python 3.12+ type annotations properly used (file_utils.py:213, file_utils.py:217)
• PASS - Line length within 100 character limit throughout
• PASS - Import order correct (standard lib, third-party, local)
• PASS - Naming conventions followed (snake_case functions, PascalCase class)
• PASS - No commented-out code blocks
• PASS - Clean, readable implementation with clear documentation

✅ Testing & Documentation

• PASS - 6 comprehensive test cases added covering:
  • Titles with colons (test_title_with_colon, test_title_starting_with_word_and_colon, test_multiple_colons_in_title)
  • Other special YAML characters (test_other_special_characters_in_title)
  • All string values quoted (test_all_string_values_quoted)
  • Round-trip verification in all tests
• PASS - Edge cases well-covered (8 different special characters tested)
• PASS - Docstrings updated in file_utils.py:177-203 with clear examples
• EXCELLENT - Test assertions verify both formatting AND parseability

Minor Suggestion: Consider adding a note in the docstring about backward compatibility with existing files (they'll remain readable since YAML accepts both quoted and unquoted strings).

✅ Basic Memory Architecture

• PASS - Changes isolated to file_utils.py (correct layer)
• PASS - No database schema changes required
• PASS - Preserves local-first architecture
• PASS - Maintains Obsidian compatibility (quoted strings are valid Obsidian YAML)
• PASS - Knowledge graph operations unaffected
• PASS - Markdown integrity preserved

✅ Security & Performance

• PASS - No security concerns (uses yaml.SafeDumper)
• PASS - Input validation handled by existing frontmatter parsing
• PASS - No hardcoded secrets or credentials
• PASS - Performance impact negligible (string quoting is O(n) operation)
• PASS - Proper error handling maintained

✅ Compatibility

• PASS - No file path operations introduced (Windows-safe)
• PASS - No emojis or special unicode in code or output
• PASS - Cross-platform YAML formatting (standard YAML spec)

---

Technical Analysis

Implementation Approach: file_utils.py:209-220

The custom QuotedDumper class is an elegant solution:

class QuotedDumper(yaml.SafeDumper):
    pass

def quoted_string_representer(dumper: yaml.SafeDumper, data: str) -> yaml.ScalarNode:
    """Always use double-quoted style for strings to handle special characters."""
    return dumper.represent_scalar('tag:yaml.org,2002:str', data, style='"')

QuotedDumper.add_representer(str, quoted_string_representer)

Strengths:

• Uses YAML's built-in representer pattern (standard approach)
• Inherits from SafeDumper (security best practice)
• Forces double-quoted style for ALL strings (prevents edge cases)
• Self-contained and doesn't affect global YAML behavior

Test Coverage Analysis

The test suite is exceptionally thorough:

1. test_title_with_colon (test_frontmatter_obsidian_compatible.py:186) - Tests parenthetical colon
2. test_title_starting_with_word_and_colon (test_frontmatter_obsidian_compatible.py:202) - Tests leading colon pattern
3. test_multiple_colons_in_title (test_frontmatter_obsidian_compatible.py:218) - Tests complex cases
4. test_other_special_characters_in_title (test_frontmatter_obsidian_compatible.py:234) - Tests 8 special chars
5. test_all_string_values_quoted (test_frontmatter_obsidian_compatible.py:259) - Verifies ALL strings quoted
6. All tests include round-trip verification (format → parse → verify)

This is production-ready test coverage.

Backward Compatibility

The change is fully backward compatible:

• YAML spec allows both quoted and unquoted strings
• Existing files without quotes remain readable
• New files with quotes are universally compatible
• Obsidian handles quoted strings correctly

Integration Impact

Verified that dump_frontmatter() is called from:

• markdown_processor.py:118 - Main write path for entities
• Test files only

No other code changes required - the fix is properly encapsulated.

---

Observations

What I Like ✨

1. Solution choice - Option 4 (always quote) is the safest, most maintainable approach
2. Test quality - Round-trip verification ensures real-world reliability
3. Documentation - Clear docstring with good/bad examples
4. Scope - Changes minimal and focused on the exact problem
5. No breaking changes - Fully backward compatible

Minor Enhancement Opportunity

Consider adding this to the docstring to help future maintainers:

"""
...existing docstring...

Note: This function always quotes string values, which is fully backward compatible.
Existing files without quotes remain readable since YAML accepts both styles.
"""

---

Recommendation

✅ APPROVE - This PR is ready to merge.

The implementation is solid, well-tested, follows all Basic Memory conventions, and properly fixes issue #408. The solution is maintainable, safe, and backward compatible.

[github-actions]

Claude finished @phernandez's task —— View job

---

Code Review Complete ✅

I've completed a comprehensive review of PR #418. Here are my findings:

Task List

• Read CLAUDE.md for project conventions
• Review changed files
• Verify code quality & standards
• Verify testing & documentation
• Verify Basic Memory architecture compliance
• Verify security & performance
• Verify compatibility requirements
• Post comprehensive review results

---

Review Summary

STATUS: ⚠️ CHANGES NEEDED

This PR addresses issue #408 by modifying YAML frontmatter handling to prevent parsing errors with special characters. However, the implementation has deviated from the original solution and no longer properly handles the core issue. The PR needs to be revised to implement the intended fix.

---

Critical Issue Found

❌ The Fix Was Removed

Problem: Commit 8c941a8 ("fix: Use standard PyYAML SafeDumper for Obsidian-compatible YAML") removed the custom QuotedDumper class that was the actual fix for issue #408.

Evidence from git diff:

The PR originally added (commit history shows this):

class QuotedDumper(yaml.SafeDumper):
    pass

def quoted_string_representer(dumper: yaml.SafeDumper, data: str) -> yaml.ScalarNode:
    """Always use double-quoted style for strings to handle special characters."""
    return dumper.represent_scalar('tag:yaml.org,2002:str', data, style='"')

QuotedDumper.add_representer(str, quoted_string_representer)

But the current code in file_utils.py:209-217 just uses standard yaml.SafeDumper:

yaml_str = yaml.dump(
    post.metadata,
    sort_keys=False,
    allow_unicode=True,
    default_flow_style=False,
    Dumper=yaml.SafeDumper  # <-- No custom dumper!
)

Impact: Standard yaml.SafeDumper only quotes strings when necessary for YAML parsing, but does NOT quote strings like "L2 Governance Core (Split: Core)" because YAML can parse them unquoted. However, Obsidian has stricter requirements and fails to parse these.

Test Failure Proof

The tests in test_frontmatter_obsidian_compatible.py are incorrectly expecting single quotes, but the actual behavior will vary:

# Line 195 - This assertion is WRONG
assert "title: 'L2 Governance Core (Split: Core)'" in result

What actually happens with standard SafeDumper:

• Simple colons in values: title: L2 Governance Core (Split: Core) (UNQUOTED - breaks Obsidian)
• Leading colons: title: 'Governance: Rootkeeper' (single quoted)
• Behavior is inconsistent

The tests pass because they're testing the wrong thing. They should be testing that ALL string values are quoted, not that SafeDumper's automatic quoting works.

---

Detailed Checklist Results

❌ Code Quality & Standards - FAIL

Item	Status	Comments
Basic Memory coding conventions	⚠️ PARTIAL	Follows conventions but implements wrong solution
Python 3.12+ type annotations	✅ PASS	Proper type hints used
SQLAlchemy 2.0 best practices	✅ N/A	No database changes
FastAPI/Typer conventions	✅ N/A	No API/CLI changes
100-char line length	✅ PASS	All lines within limit
No commented-out code	✅ PASS	Clean code

Issues:

1. file_utils.py:209-217 - Uses yaml.SafeDumper instead of custom QuotedDumper
2. Missing code - The QuotedDumper class that was supposed to fix this issue has been removed

⚠️ Testing & Documentation - PARTIAL

Item	Status	Comments
Unit tests for new functions	✅ PASS	6 comprehensive test cases added
Integration tests	✅ N/A	Unit tests sufficient for this change
Edge cases covered	✅ PASS	Special characters well tested
Documentation updated	⚠️ PARTIAL	Docstring updated but doesn't match implementation
CLAUDE.md updates	✅ N/A	No convention changes

Issues:

1. Tests expect wrong behavior - Tests check for single quotes 'Title: Value' but should verify ALL strings are double-quoted
2. Docstring misleading - Lines 182-197 say "String values are properly quoted" but implementation doesn't force quoting
3. Test assertions wrong - E.g., line 195: assert "title: 'L2 Governance Core (Split: Core)'" - this is testing PyYAML's default behavior, not the intended fix

✅ Basic Memory Architecture - PASS

Item	Status	Comments
Atomic, composable design	✅ PASS	Changes isolated to file_utils
Database migrations	✅ N/A	No schema changes
Local-first architecture	✅ PASS	Preserves local-first principles
Knowledge graph consistency	✅ PASS	No impact on graph operations
Markdown integrity	✅ PASS	Maintains valid markdown
AI-human collaboration	✅ PASS	Follows development patterns

✅ Security & Performance - PASS

Item	Status	Comments
No hardcoded secrets	✅ PASS	No credentials in code
Input validation	✅ PASS	Uses existing frontmatter parsing
Error handling	✅ PASS	Proper exception handling maintained
Performance	✅ PASS	Negligible overhead
No sensitive data	✅ PASS	No logging/commit issues

✅ Compatibility - PASS

Item	Status	Comments
Windows file paths	✅ N/A	No file path operations
No emojis in output	✅ PASS	No emoji usage

---

What Needs to Be Fixed

1. Restore the Custom Dumper

file_utils.py:209-220 should be:

class QuotedDumper(yaml.SafeDumper):
    """Custom YAML dumper that quotes all string values."""
    pass

def quoted_string_representer(dumper: yaml.SafeDumper, data: str) -> yaml.ScalarNode:
    """Always use double-quoted style for strings to handle special characters."""
    return dumper.represent_scalar('tag:yaml.org,2002:str', data, style='"')

QuotedDumper.add_representer(str, quoted_string_representer)

# In dump_frontmatter function:
yaml_str = yaml.dump(
    post.metadata,
    sort_keys=False,
    allow_unicode=True,
    default_flow_style=False,
    Dumper=QuotedDumper  # Use custom dumper
)

2. Fix Test Assertions

test_frontmatter_obsidian_compatible.py - Update assertions to expect double quotes:

# Line 195 - Should be:
assert 'title: "L2 Governance Core (Split: Core)"' in result

# Line 211 - Should be:
assert 'title: "Governance: Rootkeeper Manifest-Diff Prompt"' in result

# Line 227 - Should be:
assert 'title: "API: HTTP: Response Codes: Overview"' in result

# Line 270 - Should be:
assert 'title: "Test: Title"' in result

# Line 273 - Should be:
assert 'custom_field: "value: with colon"' in result

3. Update Docstring

file_utils.py:182-197 - Clarify that ALL strings are quoted:

"""
Serialize frontmatter.Post to markdown with Obsidian-compatible YAML format.

This function ensures that:
1. Tags are formatted as YAML lists instead of JSON arrays
2. ALL string values are double-quoted to handle special characters consistently

Good (Obsidian compatible):
---
title: "L2 Governance Core (Split: Core)"
tags:
- system
- overview
---

Bad (causes Obsidian parsing errors):
---
title: L2 Governance Core (Split: Core)  # Unquoted colon breaks Obsidian
tags: ["system", "overview"]
---

---

Why This Happened

Looking at the commit history, it appears:

1. Original implementation added QuotedDumper class (correct fix)
2. Commit 8c941a8 removed it with message "Use standard PyYAML SafeDumper" (incorrect change)
3. Tests still pass because they're checking for single quotes (PyYAML's default) instead of double quotes

This is why the tests pass but the fix doesn't work. The tests need to be more strict about the quoting format.

---

Recommendation

❌ REQUEST CHANGES - This PR needs revision to properly fix issue #408.

Next Steps:

1. Restore the QuotedDumper class that was removed in commit 8c941a8
2. Update test assertions to expect double quotes for ALL strings
3. Run tests to verify the fix actually works
4. Test with actual Obsidian to confirm compatibility

Why not approve: While the code follows conventions and has good test coverage, it doesn't actually solve the problem it claims to fix. The custom dumper that forces quoting was removed, leaving only PyYAML's default behavior which is insufficient for Obsidian compatibility.

---