Rationalize memory-tree summary filenames and stamp core version on wiki artifacts (#1457)

src/openhuman/memory/tree/content_store/compose.rs

@@ -39,10 +39,13 @@
 use chrono::{DateTime, Utc};
 
 use crate::openhuman::memory::tree::content_store::paths::{
-    sanitize_filename, slugify_source_id, SummaryTreeKind,
+    slugify_source_id, summary_filename, SummaryTreeKind,
 };
 use crate::openhuman::memory::tree::types::{Chunk, SourceKind};
 
+pub const MEMORY_ARTIFACT_FORMAT: u32 = 2;
+pub const OPENHUMAN_CORE_VERSION: &str = env!("CARGO_PKG_VERSION");
+
 /// Build the canonical Obsidian `source/<slug>` tag for a given
 /// `source_id`. Used to seed the `tags:` block on every chunk and
 /// every source-tree summary so the Obsidian graph view can filter by
@@ -405,7 +408,7 @@ fn build_summary_front_matter(r: &SummaryComposeInput<'_>) -> String {
                 .and_then(|slot| slot.as_ref())
             {
                 Some(b) => b.clone(),
-                None => sanitize_filename(id),
+                None => summary_filename(id),
             };
             let wikilink = format!("[[{}]]", basename);
             fm.push_str(&format!("  - {}\n", yaml_scalar(&wikilink)));
@@ -415,6 +418,14 @@ fn build_summary_front_matter(r: &SummaryComposeInput<'_>) -> String {
     fm.push_str(&format!("time_range_start: {trs}\n"));
     fm.push_str(&format!("time_range_end: {tre}\n"));
     fm.push_str(&format!("sealed_at: {sealed}\n"));
+    fm.push_str(&format!(
+        "openhuman_core_version: {}\n",
+        yaml_scalar(OPENHUMAN_CORE_VERSION)
+    ));
+    fm.push_str(&format!(
+        "memory_artifact_format: {}\n",
+        MEMORY_ARTIFACT_FORMAT
+    ));
 
     // aliases: human-readable title
     let alias = build_summary_alias(r);
@@ -506,7 +517,64 @@ fn scope_short_label(scope: &str) -> String {
 /// Reuses the generic [`rewrite_tags`] function — the front-matter structure
 /// is identical for both chunk and summary `.md` files.
 pub fn rewrite_summary_tags(file_bytes: &[u8], new_tags: &[String]) -> Result<Vec<u8>, String> {
-    rewrite_tags(file_bytes, new_tags)
+    let rewritten = rewrite_tags(file_bytes, new_tags)?;
+    let content =
+        std::str::from_utf8(&rewritten).map_err(|e| format!("file is not valid UTF-8: {e}"))?;
+    let (front_matter, body) = split_front_matter(content)
+        .ok_or_else(|| "cannot find front-matter delimiters".to_string())?;
+    let front_matter = upsert_summary_provenance(front_matter);
+
+    let mut out = Vec::with_capacity(front_matter.len() + body.len());
+    out.extend_from_slice(front_matter.as_bytes());
+    out.extend_from_slice(body.as_bytes());
+    Ok(out)
+}
+
+fn upsert_summary_provenance(front_matter: &str) -> String {
+    let mut lines: Vec<String> = Vec::new();
+    let mut inserted = false;
+
+    for raw in front_matter.lines() {
+        if raw.starts_with("openhuman_core_version: ")
+            || raw.starts_with("memory_artifact_format: ")
+        {
+            continue;
+        }
+        if !inserted && raw == "aliases:" {
+            lines.push(format!(
+                "openhuman_core_version: {}",
+                yaml_scalar(OPENHUMAN_CORE_VERSION)
+            ));
+            lines.push(format!(
+                "memory_artifact_format: {}",
+                MEMORY_ARTIFACT_FORMAT
+            ));
+            inserted = true;
+        }
+        lines.push(raw.to_string());
+    }
+
+    if !inserted {
+        let insert_at = lines
+            .iter()
+            .rposition(|line| line == "---")
+            .unwrap_or(lines.len());
+        lines.insert(
+            insert_at,
+            format!(
+                "openhuman_core_version: {}",
+                yaml_scalar(OPENHUMAN_CORE_VERSION)
+            ),
+        );
+        lines.insert(
+            insert_at + 1,
+            format!("memory_artifact_format: {}", MEMORY_ARTIFACT_FORMAT),
+        );
+    }
+
+    let mut result = lines.join("\n");
+    result.push('\n');
+    result
 }
 
 /// Split a file into `(front_matter, body)` at the second `---` delimiter.
@@ -843,6 +911,20 @@ mod tests {
         );
         assert!(fm.contains("level: 1"), "must have level");
         assert!(fm.contains("child_count: 2"), "must have child_count");
+        assert!(
+            fm.contains(&format!(
+                "openhuman_core_version: {}",
+                OPENHUMAN_CORE_VERSION
+            )),
+            "must stamp the core version"
+        );
+        assert!(
+            fm.contains(&format!(
+                "memory_artifact_format: {}",
+                MEMORY_ARTIFACT_FORMAT
+            )),
+            "must stamp the artifact format epoch"
+        );
         assert!(
             fm.contains("  - \"[[child-1]]\""),
             "must list child ids as Obsidian wikilinks; got:\n{fm}"
@@ -1103,7 +1185,32 @@ mod tests {
         assert!(rewritten_str.contains("  - person/Alice-Smith"));
         assert!(rewritten_str.contains("  - topic/Memory"));
         assert!(!rewritten_str.contains("tags: []"));
+        assert!(rewritten_str.contains(&format!(
+            "openhuman_core_version: {}",
+            OPENHUMAN_CORE_VERSION
+        )));
+        assert!(rewritten_str.contains(&format!(
+            "memory_artifact_format: {}",
+            MEMORY_ARTIFACT_FORMAT
+        )));
         // Body must be unchanged
         assert!(rewritten_str.ends_with("summary body text"));
     }
+
+    #[test]
+    fn rewrite_summary_tags_backfills_missing_provenance() {
+        let file =
+            b"---\nid: legacy\nkind: summary\ntags: []\naliases:\n  - legacy\n---\nlegacy body";
+        let rewritten = rewrite_summary_tags(file, &["person/Alice".to_string()]).unwrap();
+        let rewritten_str = std::str::from_utf8(&rewritten).unwrap();
+        assert!(rewritten_str.contains(&format!(
+            "openhuman_core_version: {}",
+            OPENHUMAN_CORE_VERSION
+        )));
+        assert!(rewritten_str.contains(&format!(
+            "memory_artifact_format: {}",
+            MEMORY_ARTIFACT_FORMAT
+        )));
+        assert!(rewritten_str.ends_with("legacy body"));
+    }
 }

src/openhuman/memory/tree/content_store/paths.rs

@@ -61,8 +61,15 @@ pub const WIKI_PREFIX: &str = "wiki";
 /// `scope_slug` must already be slugified by the caller (use [`slugify_source_id`] or
 /// a per-kind variant). A trailing `.md` on `summary_id` is stripped if present.
 ///
-/// The `summary_id` is sanitized into a filesystem-safe filename by replacing
-/// characters illegal on Windows (`:`, `\`, `*`, `?`, `"`, `<`, `>`, `|`) with `-`.
+/// New summaries use the explicit basename contract implemented by
+/// [`summary_filename`]:
+/// - current canonical ids: `summary:{13-digit-ms}:L{level}-{tail}`
+///   → `summary-{13-digit-ms}-L{level}-{tail}.md`
+/// - legacy ids: `summary:L{level}:{rest}`
+///   → `summary-L{level}-{rest}.md`
+///
+/// Unknown / malformed ids fall back to [`sanitize_filename`] so existing vaults
+/// remain readable even if they contain older experimental shapes.
 pub fn summary_rel_path(
     tree_kind: SummaryTreeKind,
     scope_slug: &str,
@@ -70,10 +77,7 @@ pub fn summary_rel_path(
     summary_id: &str,
     date_for_global: Option<DateTime<Utc>>,
 ) -> String {
-    // Strip a trailing `.md` from summary_id if accidentally included.
-    let id = summary_id.strip_suffix(".md").unwrap_or(summary_id);
-    // Sanitize to a cross-platform filename (colons are illegal on Windows NTFS).
-    let filename = sanitize_filename(id);
+    let filename = summary_filename(summary_id);
 
     match tree_kind {
         SummaryTreeKind::Source => {
@@ -108,6 +112,60 @@ pub fn summary_rel_path(
     }
 }
 
+/// Convert a summary id into the canonical on-disk basename stem (without
+/// `.md`).
+///
+/// This keeps summary filenames independent from the generic "replace illegal
+/// characters" fallback so new writes follow one documented convention, while
+/// legacy ids still map to their historical names.
+pub(crate) fn summary_filename(summary_id: &str) -> String {
+    let id = summary_id.strip_suffix(".md").unwrap_or(summary_id);
+
+    if let Some(rest) = id.strip_prefix("summary:") {
+        if let Some((ms, suffix)) = rest.split_once(':') {
+            // Canonical fast-path: only accept ms-first ids whose
+            // `L<level>-<tail>` suffix has a numeric level and a tail
+            // free of filesystem-illegal characters. Without the tail
+            // check a malformed canonical-looking id like
+            // `summary:1700000000000:L2-a/b` would smuggle a `/` into
+            // the basename and split the file across multiple path
+            // components when joined onto the L<level>/ directory.
+            if let Some((level, tail)) = suffix.split_once('-') {
+                let level_is_numeric = level.starts_with('L')
+                    && level.len() > 1
+                    && level[1..].chars().all(|c| c.is_ascii_digit());
+                let tail_is_safe = !tail.is_empty()
+                    && !tail
+                        .chars()
+                        .any(|c| matches!(c, '\\' | '/' | ':' | '*' | '?' | '"' | '<' | '>' | '|'));
+                if ms.len() == 13
+                    && ms.chars().all(|c| c.is_ascii_digit())
+                    && level_is_numeric
+                    && tail_is_safe
+                {
+                    return format!("summary-{ms}-{level}-{tail}");
+                }
+            }
+        }
+
+        if let Some((level, tail)) = rest.split_once(':') {
+            // Legacy ms-less ids (`summary:L<n>:<rest>`). Require strict
+            // `L<digits>` for the level segment so a malicious input
+            // like `summary:L1/2:abc` or `summary:L../../x:tail` cannot
+            // inject `/` into the returned basename. Anything else
+            // falls through to the generic sanitiser below.
+            let level_is_numeric = level.starts_with('L')
+                && level.len() > 1
+                && level[1..].chars().all(|c| c.is_ascii_digit());
+            if level_is_numeric && !tail.is_empty() {
+                return format!("summary-{level}-{}", sanitize_filename(tail));
+            }
+        }
+    }
+
+    sanitize_filename(id)
+}
+
 /// Replace characters that are illegal in filenames on Windows NTFS with `-`.
 ///
 /// Illegal characters: `\`, `/`, `:`, `*`, `?`, `"`, `<`, `>`, `|`.
@@ -116,8 +174,10 @@ pub fn summary_rel_path(
 ///
 /// Exposed at crate scope so [`super::compose`] can convert structured IDs
 /// like `summary:L1:UUID` into the basename used by [`summary_rel_path`]
-/// (`summary-L1-UUID`) when emitting Obsidian wikilinks. This keeps a single
-/// source of truth for the id→filename mapping.
+/// when no summary-specific mapping applies. Summary ids should prefer
+/// [`summary_filename`] so new writes follow the documented basename
+/// contract instead of relying on punctuation replacement as an accident of
+/// the implementation.
 pub(crate) fn sanitize_filename(s: &str) -> String {
     s.chars()
         .map(|c| match c {
@@ -424,6 +484,21 @@ mod tests {
         );
     }
 
+    #[test]
+    fn summary_rel_path_current_ids_keep_time_first_basename() {
+        let p = summary_rel_path(
+            SummaryTreeKind::Source,
+            "slack-eng",
+            2,
+            "summary:1700000000000:L2-deadbeef",
+            None,
+        );
+        assert_eq!(
+            p,
+            "wiki/summaries/source-slack-eng/L2/summary-1700000000000-L2-deadbeef.md"
+        );
+    }
+
     #[test]
     fn summary_rel_path_global() {
         use chrono::TimeZone;
@@ -466,6 +541,76 @@ mod tests {
         assert_eq!(p, "wiki/summaries/topic-entity-slug/L2/summary-L2-foo.md");
     }
 
+    #[test]
+    fn summary_filename_preserves_legacy_level_first_shape() {
+        assert_eq!(
+            summary_filename("summary:L3:legacy-uuid"),
+            "summary-L3-legacy-uuid"
+        );
+    }
+
+    #[test]
+    fn summary_filename_rejects_canonical_shape_with_path_separators() {
+        // A canonical-looking id whose tail contains `/` must NOT be
+        // returned verbatim — that would smuggle a directory separator
+        // into the basename. Fall back to the generic sanitiser so the
+        // illegal char is replaced with `-`.
+        let basename = summary_filename("summary:1700000000000:L2-a/b");
+        assert!(
+            !basename.contains('/'),
+            "basename must not contain a path separator; got {basename}"
+        );
+        // Generic-fallback shape: sanitize_filename replaces both `:` and `/`.
+        assert_eq!(basename, "summary-1700000000000-L2-a-b");
+    }
+
+    #[test]
+    fn summary_filename_rejects_canonical_shape_with_non_numeric_level() {
+        // Level segment must be `L<digits>`. Anything else (`Lxyz`,
+        // `L-1`, …) is not the canonical contract — fall back to the
+        // generic sanitiser instead of accepting verbatim.
+        let basename = summary_filename("summary:1700000000000:Lxyz-tail");
+        assert_eq!(basename, "summary-1700000000000-Lxyz-tail");
+    }
+
+    #[test]
+    fn summary_filename_legacy_branch_rejects_path_separator_in_level() {
+        // Legacy `summary:L<n>:<rest>` branch must also enforce strict
+        // `L<digits>` for the level segment — otherwise an input like
+        // `summary:L1/2:abc` would produce `summary-L1/2-abc` and
+        // smuggle a `/` into the basename. Falls through to the
+        // generic sanitiser, which replaces `/` with `-`.
+        let basename = summary_filename("summary:L1/2:abc");
+        assert!(
+            !basename.contains('/'),
+            "basename must not contain a path separator; got {basename}"
+        );
+        assert_eq!(basename, "summary-L1-2-abc");
+    }
+
+    #[test]
+    fn summary_filename_legacy_branch_rejects_traversal_in_level() {
+        // `summary:L../../x:tail` must NOT produce
+        // `summary-L../../x-tail` — the legacy branch requires
+        // `L<digits>` and falls through to `sanitize_filename` when
+        // the level segment is non-numeric. Without `/` in the
+        // resulting basename the dots are inert characters, not
+        // directory components.
+        let basename = summary_filename("summary:L../../x:tail");
+        assert!(
+            !basename.contains('/'),
+            "basename must not contain a path separator; got {basename}"
+        );
+    }
+
+    #[test]
+    fn summary_filename_falls_back_for_unknown_shapes() {
+        assert_eq!(
+            summary_filename("summary:experimental:value:tail"),
+            "summary-experimental-value-tail"
+        );
+    }
+
     #[test]
     fn summary_rel_path_global_falls_back_to_sentinel_without_date() {
         // Caller bug to omit date for Global, but a path utility shouldn't