Merge branch 'issue/848-consolidator-portable-path'

CHANGELOG.md

@@ -34,6 +34,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   shape preserved) before validation, so the run and its other pages
   survive; a path `ensure_portable` still rejects after sanitizing is
   skipped with a warning instead of failing the batch. (#847)
+- Per-session consolidation (`consolidate_session_multi`) had the same
+  Windows-illegal-path defect as `ai-memory bootstrap` (#847): an
+  LLM-produced page path containing a character like `:` passed the
+  deliberately tolerant `PagePath::new` and only failed later at
+  `ensure_portable` inside the atomic wiki write batch, losing every other
+  page from that session's consolidation run. The path is now sanitized
+  the same way bootstrap's is, consistently across rule-routing, per-user
+  slot placement, and the session-anchor comparison, before validation;
+  a path `ensure_portable` still rejects after sanitizing is skipped with
+  a warning instead of failing the batch. (#848)
 - The Windows release checksum (`ai-memory-windows-x86_64.zip.sha256`) is now
   written with a LF terminator instead of CRLF. `Out-File`'s Windows line
   ending made `sha256sum -c` fail with `No such file or directory` — the CR

crates/ai-memory-consolidate/src/bootstrap.rs

@@ -44,6 +44,8 @@ use sha2::{Digest, Sha256};
 use thiserror::Error;
 use tracing::{debug, info, warn};
 
+use crate::path_sanitize::slugify_page_path;
+
 /// Rough characters-per-token estimate used for budget enforcement.
 /// 4 is the standard heuristic for English prose (cl100k, gpt-4
 /// tokenizer family). Don't rely on it for billing math — it's
@@ -1434,42 +1436,6 @@ fn insert_bootstrap_page(
     pages_by_path.insert(page.path.clone(), page);
 }
 
-/// Filename characters Windows refuses, mirroring
-/// `ai_memory_core::ids`'s reserved-char set, plus `\` — `PagePath::new`
-/// already rejects a literal backslash anywhere in the raw path (it reads as
-/// a separator), so a component containing one must be cleaned before
-/// `PagePath::new` ever sees it, not after.
-const PATH_ILLEGAL_CHARS: &[char] = &['<', '>', ':', '"', '|', '?', '*', '\\'];
-
-/// Clean a model-produced page path so it survives `PagePath::new` and
-/// `ensure_portable`.
-///
-/// The LLM sometimes echoes free text — a conventional-commit subject like
-/// `build(sandbox): orchestrate` — straight into a page path. That passes
-/// `PagePath::new` (deliberately tolerant; see its doc comment) but fails
-/// `ensure_portable`, which `Wiki::apply_batch` enforces atomically: one bad
-/// path there aborts every page in the batch, not just its own (#847).
-/// Replace every Windows-illegal character and ASCII control byte in each
-/// `/`-separated component with `-`, keeping the `dir/subdir/name.md` shape
-/// intact so the model's intended layout survives.
-fn slugify_page_path(raw: &str) -> String {
-    raw.split('/')
-        .map(|segment| {
-            segment
-                .chars()
-                .map(|c| {
-                    if PATH_ILLEGAL_CHARS.contains(&c) || (c as u32) < 0x20 {
-                        '-'
-                    } else {
-                        c
-                    }
-                })
-                .collect::<String>()
-        })
-        .collect::<Vec<_>>()
-        .join("/")
-}
-
 // --------------------------------------------------------------------
 // Manifest rendering
 // --------------------------------------------------------------------
@@ -2365,22 +2331,9 @@ mod tests {
     // Windows-illegal path sanitization (#847)
     // ----------------------------------------------------------------
 
-    #[test]
-    fn slugify_page_path_replaces_illegal_chars_and_keeps_slashes() {
-        assert_eq!(
-            slugify_page_path("concepts/build(sandbox): orchestrate the run.md"),
-            "concepts/build(sandbox)- orchestrate the run.md"
-        );
-        assert_eq!(
-            slugify_page_path("a/b<c>d:e\"f|g?h*i\\j.md"),
-            "a/b-c-d-e-f-g-h-i-j.md"
-        );
-        assert_eq!(
-            slugify_page_path("concepts/clean-path.md"),
-            "concepts/clean-path.md",
-            "an already-portable path must be left unchanged"
-        );
-    }
+    // `slugify_page_path` itself is unit-tested alongside its definition in
+    // `crate::path_sanitize`; this remaining test exercises the bootstrap
+    // write loop's use of it end to end.
 
     /// A batch with one page whose path contains a Windows-illegal `:`
     /// (copied verbatim from a conventional-commit subject, e.g.

crates/ai-memory-consolidate/src/consolidator.rs

@@ -17,6 +17,7 @@ use ai_memory_wiki::{AdmissionContext, AdmissionOp, Wiki, WritePageRequest};
 use thiserror::Error;
 use tracing::{debug, info, warn};
 
+use crate::path_sanitize::slugify_page_path;
 use crate::projection::{ObservationProjectionConfig, project_observations};
 use crate::types::{
     ConsolidatedBatch, ConsolidatedPage, ConsolidationOutcome, Relations, SlotKind,
@@ -621,6 +622,19 @@ impl Consolidator {
                 );
                 continue;
             }
+            // Final guard for whatever `slugify_page_path` in `build_update`
+            // can't fix (dot-segments, reserved DOS device names, `.git`,
+            // ...), mirroring bootstrap's #847 fix: skip this one page
+            // rather than let `Wiki::apply_batch`'s atomic `ensure_portable`
+            // check abort every other page in the batch (#848).
+            if let Err(e) = req.path.ensure_portable() {
+                warn!(
+                    path = %req.path.as_str(),
+                    error = %e,
+                    "skipped consolidation page update: path is not portable",
+                );
+                continue;
+            }
             requests.push(req);
             outcomes_preview.push(outcome);
         }
@@ -689,7 +703,17 @@ fn build_update(
         let slug = slugify_for_rule(&effective_title);
         format!("_rules/{slug}.md")
     } else {
-        upd.path.clone()
+        // The LLM sometimes echoes free text straight into a page path (a
+        // conventional-commit subject like `build(sandbox): orchestrate`).
+        // That passes `PagePath::new` (deliberately tolerant) but fails
+        // `ensure_portable`, which `Wiki::apply_batch` enforces atomically —
+        // one bad path there would abort every page in this batch, not just
+        // its own (#848, same class as bootstrap's #847). Sanitize before
+        // `PagePath::new` so every downstream use of `path` (rule routing
+        // already produces a safe slug above, slot placement, and the
+        // `req.path == anchor` comparison in `consolidate_session_multi`)
+        // sees this one, consistent, sanitized value.
+        slugify_page_path(&upd.path)
     };
     let path = PagePath::new(final_path)?;
     let tier = upd.tier;
@@ -2861,6 +2885,90 @@ mod tests {
         }
     }
 
+    /// A batch with one page whose LLM-produced path contains a
+    /// Windows-illegal `:` (copied verbatim from a conventional-commit
+    /// subject, e.g. `build(sandbox): orchestrate`) must not abort the whole
+    /// run: `build_update` sanitizes the path in place (same class of fix as
+    /// bootstrap's #847) and the batch's sibling valid page survives. Before
+    /// the fix, the bad path passed `PagePath::new` (deliberately tolerant)
+    /// and only failed later at `ensure_portable` inside `Wiki::apply_batch`,
+    /// which is atomic — one bad page there lost every page in the batch
+    /// (#848).
+    #[tokio::test]
+    async fn batch_with_illegal_char_path_is_sanitized_not_aborted() {
+        let tmp = tempfile::tempdir().unwrap();
+        let (store, wiki, session, ws, proj) = batch_fixture(tmp.path()).await;
+        let response = serde_json::json!({
+            "rationale": "one bad path, one good",
+            "updates": [
+                {
+                    "path": "concepts/build(sandbox): orchestrate the run.md",
+                    "tier": "semantic",
+                    "kind": "fact",
+                    "title": "Bad path page",
+                    "body_markdown": "Bad path body.",
+                    "tags": []
+                },
+                {
+                    "path": "concepts/good.md",
+                    "tier": "semantic",
+                    "kind": "fact",
+                    "title": "Good path page",
+                    "body_markdown": "Good path body.",
+                    "tags": []
+                }
+            ]
+        });
+
+        let outcomes = Consolidator::new(
+            store.reader.clone(),
+            store.writer.clone(),
+            wiki.clone(),
+            Arc::new(ScriptedLlm(response)),
+            ws,
+            proj,
+        )
+        .consolidate_session_multi(
+            session,
+            false,
+            ai_memory_core::ActorContext::anonymous(),
+            None,
+            None,
+        )
+        .await
+        .expect("a sanitizable bad path must not fail (or abort) the whole batch");
+
+        assert_eq!(
+            outcomes.len(),
+            2,
+            "both pages, including the sanitized one, must be written"
+        );
+
+        let sanitized_path = outcomes
+            .iter()
+            .find(|o| o.path.as_str().starts_with("concepts/build"))
+            .expect("the offending page must still be written, under a sanitized path")
+            .path
+            .clone();
+        assert!(
+            !sanitized_path.as_str().contains(':'),
+            "the sanitized path must not contain the Windows-illegal `:`: {}",
+            sanitized_path.as_str()
+        );
+        assert!(
+            sanitized_path.ensure_portable().is_ok(),
+            "the sanitized path must pass the portability check"
+        );
+
+        let good = wiki
+            .read_page(ws, proj, &PagePath::new("concepts/good.md").unwrap())
+            .unwrap();
+        assert_eq!(good.frontmatter["title"], "Good path page");
+
+        let bad = wiki.read_page(ws, proj, &sanitized_path).unwrap();
+        assert_eq!(bad.frontmatter["title"], "Bad path page");
+    }
+
     /// A batch whose single update targets `path` — the model chooses this
     /// string, and `build_update` keeps it verbatim for non-Rule kinds.
     fn batch_targeting(path: &str, body: &str) -> serde_json::Value {

crates/ai-memory-consolidate/src/lib.rs

@@ -22,6 +22,7 @@ pub mod entropy_filter;
 pub mod experience;
 pub mod keep_tokens;
 pub mod lint;
+mod path_sanitize;
 pub mod projection;
 pub mod sweep;
 pub mod types;

crates/ai-memory-consolidate/src/path_sanitize.rs

@@ -0,0 +1,67 @@
+//! Shared model-path sanitization for LLM-produced wiki paths.
+//!
+//! Both bootstrap (#847) and per-session consolidation (#848) accept a
+//! page path straight from LLM structured output and hand it to
+//! `Wiki::apply_batch`, which is atomic: a single path that fails
+//! `PagePath::ensure_portable` at write time aborts every page in that
+//! batch, not just its own. `PagePath::new` is deliberately tolerant (see
+//! its doc comment) and does not catch this, so callers must sanitize the
+//! raw model path themselves before constructing a `PagePath`.
+
+/// Filename characters Windows refuses, mirroring
+/// `ai_memory_core::ids`'s reserved-char set, plus `\` — `PagePath::new`
+/// already rejects a literal backslash anywhere in the raw path (it reads as
+/// a separator), so a component containing one must be cleaned before
+/// `PagePath::new` ever sees it, not after.
+pub(crate) const PATH_ILLEGAL_CHARS: &[char] = &['<', '>', ':', '"', '|', '?', '*', '\\'];
+
+/// Clean a model-produced page path so it survives `PagePath::new` and
+/// `ensure_portable`.
+///
+/// The LLM sometimes echoes free text — a conventional-commit subject like
+/// `build(sandbox): orchestrate` — straight into a page path. That passes
+/// `PagePath::new` (deliberately tolerant; see its doc comment) but fails
+/// `ensure_portable`, which `Wiki::apply_batch` enforces atomically: one bad
+/// path there aborts every page in the batch, not just its own (#847, #848).
+/// Replace every Windows-illegal character and ASCII control byte in each
+/// `/`-separated component with `-`, keeping the `dir/subdir/name.md` shape
+/// intact so the model's intended layout survives.
+pub(crate) fn slugify_page_path(raw: &str) -> String {
+    raw.split('/')
+        .map(|segment| {
+            segment
+                .chars()
+                .map(|c| {
+                    if PATH_ILLEGAL_CHARS.contains(&c) || (c as u32) < 0x20 {
+                        '-'
+                    } else {
+                        c
+                    }
+                })
+                .collect::<String>()
+        })
+        .collect::<Vec<_>>()
+        .join("/")
+}
+
+#[cfg(test)]
+mod tests {
+    use super::slugify_page_path;
+
+    #[test]
+    fn slugify_page_path_replaces_illegal_chars_and_keeps_slashes() {
+        assert_eq!(
+            slugify_page_path("concepts/build(sandbox): orchestrate the run.md"),
+            "concepts/build(sandbox)- orchestrate the run.md"
+        );
+        assert_eq!(
+            slugify_page_path("a/b<c>d:e\"f|g?h*i\\j.md"),
+            "a/b-c-d-e-f-g-h-i-j.md"
+        );
+        assert_eq!(
+            slugify_page_path("concepts/clean-path.md"),
+            "concepts/clean-path.md",
+            "an already-portable path must be left unchanged"
+        );
+    }
+}