Skip to content

fix(memory): replace yaml_quote with sanitize_hint in consolidation frontmatter writers#1388

Open
ralf003 wants to merge 2 commits into
alibaba:mainfrom
ralf003:fix/consolidation-frontmatter-sanitize
Open

fix(memory): replace yaml_quote with sanitize_hint in consolidation frontmatter writers#1388
ralf003 wants to merge 2 commits into
alibaba:mainfrom
ralf003:fix/consolidation-frontmatter-sanitize

Conversation

@ralf003

@ralf003 ralf003 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Apply the sanitize_hint pattern from PR #1154 to the consolidation module frontmatter writers (fact.rs, episode.rs).

Problem

PR #1154 established that sanitize_hint() is the correct way to sanitize frontmatter values: only replace newlines and ASCII control chars with spaces, without YAML double-quoting or backslash escaping.

The consolidation module had two non-compliant sanitizers:

  1. fact.rs yaml_quote(): Applied full YAML double-quoting + backslash escaping + newline replacement — asymmetric with the hand-rolled frontmatter readers that use split_once(": ") + trim_matches('"') and do NOT interpret YAML escapes.

  2. episode.rs sanitize(): Replaced " with ' — mangles quotes instead of preserving them, and still misses control char handling.

Root Cause

The hand-rolled frontmatter readers (parse_frontmatter_flat in memory_observe/memory_sovereignty/memory_summary_tool/session_context/session_history/user_profile) use simple string splitting and do not interpret YAML escapes. YAML-style escaping on the write side is therefore asymmetric, causing:

  • Round-trip regression on Windows paths containing backslashes (doubled by yaml_quote)
  • Data loss when quotes are replaced with different characters

Changes

fact.rs

  • Replace yaml_quote() with sanitize_hint() (same implementation as memory_observe.rs)
  • Update to_markdown() to call sanitize_hint instead of yaml_quote
  • Add 10 unit tests covering: normal text, hashes, colons, quotes, backslashes, newlines, CRLF, empty strings, control chars, and two integration tests (title with newlines, path with backslashes)

episode.rs

  • Update sanitize() to match sanitize_hint() pattern (no more ' replacement)

Testing

All 188 tests pass including 10 new tests: cargo test --lib on Alibaba Cloud Linux.

Related

…rontmatter writers

Replace yaml_quote() in fact.rs and quote-replacing sanitize() in
episode.rs with sanitize_hint() that only replaces newlines and ASCII
control chars with spaces, without YAML double-quoting or backslash
escaping.

The hand-rolled frontmatter readers (parse_frontmatter_flat /
extract_title_and_description) use split_once(": ") +
trim_matches('"') and do NOT interpret YAML escapes, so YAML-style
escaping is asymmetric and causes a round-trip regression on Windows
paths containing backslashes.

This aligns with the standard established in PR 1154.

Changes:
- fact.rs: replace yaml_quote() with sanitize_hint()
- episode.rs: update sanitize() to match sanitize_hint() pattern
- Add 10 unit tests: 8 for sanitize_hint edge cases plus 2 for
  title/path with special chars in to_markdown()
@shiloong

shiloong commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Code review

Found 1 issue:

  1. The two new "integration" tests (fact_title_with_special_chars, fact_path_with_backslashes) assert only md.contains(...) on the serialized markdown and never feed the output back through the real frontmatter readers (parse_frontmatter_flat / extract_title_and_description) to confirm the original title/path survives the write→read round-trip. This is the exact gap flagged on the sibling PR fix(agent-memory): yaml-escape hint values in memory_observe and add max_hint_bytes config #1154 (the gap that let the backslash-doubling regression slip through there). A to_markdown()parse_frontmatter_flat → assert-equal test would catch any trim_matches('"') / escape surprise for the special-char cases this PR claims to fix, including CRLF and control-char values. (CLAUDE.md says: "验证优先: 每次修改必须附带验证方法——运行测试、构建检查或可观察的行为变化。没有验证的代码不算完成。" and "目标驱动执行: 将任务转成可验证目标——先写复现测试→让测试通过→验证无回归。")

#[test]
fn fact_title_with_special_chars() {
let f = ConsolidatedFact::new(
"sid",
FactCategory::Lesson,
"Use const\nnot var".into(),
"Details".into(),
"mem_edit".into(),
vec![],
0.7,
);
let md = f.to_markdown();
assert!(md.contains("title: Use const not var"));
}
#[test]
fn fact_path_with_backslashes() {
let f = ConsolidatedFact::new(
"sid",
FactCategory::WorkingContext,
"Work".into(),
"Details".into(),
"mem_write".into(),
vec!["C:\\Users\\admin\\file.txt".into()],
0.5,
);
let md = f.to_markdown();
assert!(md.contains("C:\\Users\\admin\\file.txt"));
}

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.


ralf003 added a commit to ralf003/anolisa that referenced this pull request Jul 8, 2026
Add frontmatter write->read round-trip assertions to
fact_title_with_special_chars and fact_path_with_backslashes tests:
- Extract title/paths from to_markdown() output and verify
  they survive the write->read cycle without escaping artifacts.
- Verify no YAML double-quoting artifacts are present.
- Addresses review feedback from shiloong on PR alibaba#1388.
@ralf003

ralf003 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@shiloong thanks for the review! Addressed in 1c4ee60:

  • fact_title_with_special_chars: added round-trip extraction of the title: field from to_markdown() output and assert-equal against the sanitized input. Also asserts no YAML double-quoting artifact (title: \").

  • fact_path_with_backslashes: added round-trip extraction of related_paths list items from frontmatter and assert-equal. Also asserts no YAML double-quoting artifact on the path string.

Both tests pass locally (14/14 in consolidation::fact::tests).

Add frontmatter write->read round-trip assertions to
fact_title_with_special_chars and fact_path_with_backslashes tests:
- Extract title/paths from to_markdown() output and verify
  they survive the write->read cycle without escaping artifacts.
- Verify no YAML double-quoting artifacts are present.
- Addresses review feedback from shiloong on PR alibaba#1388.
@ralf003 ralf003 force-pushed the fix/consolidation-frontmatter-sanitize branch from 1c4ee60 to 53b99e3 Compare July 8, 2026 17:35
@ralf003

ralf003 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

CI fix: cargo fmt --check failure resolved. Amended commit to apply rustfmt formatting on the new round-trip test code in fact.rs. Force-pushed to \ ix/consolidation-frontmatter-sanitize.

The formatting issue was: multi-line method chains, argument wrapping, and comment formatting in the newly added assertions didn't match the upstream rustfmt config.

@ralf003

ralf003 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@shiloong friendly ping — the round-trip test feedback from your review has been addressed in 1c4ee60, and CI is all green now. Would you mind taking another look when you have a moment? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants