feat(memory): add is_user_autosave_key detector for per-turn user message keys#5631
Conversation
b002cc7 to
82e1207
Compare
Agent Review — Ready to MergeComprehension summary: This PR adds a pure detection function Thank you, @vernonstinebaker. Clean additive change with complete test coverage. What was reviewed and verified:
Security/performance assessment:
CI Status: The Quality Gate's Dependency note: This PR depends on #5633 (CI: suppress wasmtime RUSTSEC-2026-04-09 audit batch). PR #5632 depends on this PR. This PR is ready for maintainer merge.
|
|
This PR has merge conflicts with master. Could you rebase to resolve them? Once conflicts are cleared, this is agent-approved and ready to merge. |
Agent Session Report — Conflict Resolution
SummaryWhat: Adds Why: Per-turn user message auto-saves re-inject prior context verbatim on recall, causing exponential bloat. This pure detection function prepares for context-path filtering in a follow-up PR. No runtime behavior change. Blast radius: None on merge. The function is additive and uncalled until the follow-up PR. Conflict Root CauseUpstream merged a large crate-extraction refactor between this PR's base and now. Resolution
ValidationSecurity / PerformanceNo security impact. No performance impact. Pure additive function with no callers yet. Notes for maintainer
|
Agent Review — DRY AcknowledgmentAll findings from this PR were fully covered by the prior review (@theonlyhennygod). Summary of state:
This PR is agent-approved and awaiting maintainer merge. |
Per-turn user messages (user_msg, user_msg_<uuid>) are auto-saved for consolidation but must not be re-injected into memory recall context. Each recalled entry embeds prior context verbatim, causing exponential bloat across turns. Wire is_user_autosave_key (from zeroclaw-labs#5631) into all three context paths: - crates/zeroclaw-runtime/src/agent/loop_.rs build_context() - crates/zeroclaw-runtime/src/agent/memory_loader.rs DefaultMemoryLoader - crates/zeroclaw-channels/src/orchestrator/mod.rs should_skip_memory_context_entry() Fix pre-existing test that used a user_msg_real key as the pass-through case (now correctly filtered). Add dedicated tests for each path. Stacked on zeroclaw-labs#5631.
Per-turn user messages (user_msg, user_msg_<uuid>) are auto-saved for consolidation but must not be re-injected into memory recall context. Each recalled entry embeds prior context verbatim, causing exponential bloat across turns. Wire is_user_autosave_key (from zeroclaw-labs#5631) into all three context paths: - crates/zeroclaw-runtime/src/agent/loop_.rs build_context() - crates/zeroclaw-runtime/src/agent/memory_loader.rs DefaultMemoryLoader - crates/zeroclaw-channels/src/orchestrator/mod.rs should_skip_memory_context_entry() Fix pre-existing test that used a user_msg_real key as the pass-through case (now correctly filtered). Add dedicated tests for each path. Stacked on zeroclaw-labs#5631.
…sage keys Raw per-turn user messages are auto-saved under the 'user_msg' / 'user_msg_*' key prefix. Re-injecting these into context assembly causes exponential bloat: each recalled entry contains prior generations' full context verbatim, growing unboundedly across sessions. Add is_user_autosave_key() as the read-path counterpart to the existing is_assistant_autosave_key(), so all three context-building callers can consistently skip them. Adjacent to is_assistant_autosave_key in zeroclaw-memory/src/lib.rs. Includes a unit test covering the canonical patterns.
Add a module-level doc table listing both reserved auto-save key prefixes (assistant_resp_* and user_msg_*) alongside their detection functions and the context-building paths that honour them. Provides a single reference point for contributors adding new auto-save prefixes or writing memory tests.
fe137fa to
049aeb5
Compare
WareWolf-MoonWall
left a comment
There was a problem hiding this comment.
PR Review — #5631 feat(memory): add is_user_autosave_key detector for per-turn user message keys
Reviewing on the current HEAD. I read the full diff, the PR description, and the prior
review thread. No formal reviews exist; no inline threads; no active blocks. First review.
What this change does
Adds a single pub fn is_user_autosave_key(key: &str) -> bool to
crates/zeroclaw-memory/src/lib.rs, placed directly adjacent to the existing
is_assistant_autosave_key(). Adds a module-level reserved key prefix doc table
documenting both assistant_resp* and user_msg* namespaces. Adds a unit test covering
the canonical patterns. No callers are wired in this PR — this is the foundation commit
for #5632.
✅ Commendation
The implementation matches is_assistant_autosave_key() exactly in structure — same
normalization (trim + to_ascii_lowercase), same exact-match + prefix-match pattern,
same placement. When two functions enforce the same contract for symmetric key families,
structural symmetry is correctness evidence: a reader can verify the logic by diff
against the established sibling, and the test mirrors the existing
assistant_autosave_key_detection_matches_legacy_patterns test case-for-case. That
parity is intentional and right.
The cross-prefix negative assertion — assert!(!is_user_autosave_key("assistant_resp_1234")) —
is worth naming explicitly as good work. It closes a subtle gap: a reviewer scanning the
test can immediately confirm the function does not accidentally match the other reserved
family. Same pattern exists in the assistant test with assert!(!is_assistant_autosave_key("user_msg_1234")). Both directions covered. ✅
The risk label is correct. crates/zeroclaw-memory/src/lib.rs is not in any of the
explicitly listed high-risk paths in AGENTS.md (crates/zeroclaw-runtime/src/**,
crates/zeroclaw-gateway/src/**, crates/zeroclaw-tools/src/**,
.github/workflows/**). A pure additive function with no callers carries no runtime
blast radius. risk: low / size: XS is an accurate snapshot. ✅
🔵 Advisory — module-level doc preamble claims behavior that lands in #5632, not here
The doc table preamble reads:
Any memory stored under these keys will be excluded from context assembly by all
three context-building paths (build_context,DefaultMemoryLoader, and
should_skip_memory_context_entry).
The PR description correctly states "the detection function has no callers yet." At the
point #5631 merges, that claim is accurate for assistant_resp* (already wired) but not
yet accurate for user_msg* — the wiring lands in #5632. Someone reading rustdoc
between the two merges would see a promise that user_msg* keys are filtered from
context, but the three callers won't enforce it yet.
The simplest fix is to frame the preamble as the reserved-namespace contract rather than
a behavioral claim — e.g., "these prefixes are reserved for the auto-save system and
must not be used for semantic memories" — and let the detection function docstrings
explain when and where they are enforced. The behavioral claim could also carry a forward
reference: "enforced by downstream callers; see #5632 / is_user_autosave_key."
This does not need to block merge. The PR description is honest about the no-callers
state, the function itself is correct, and the window between the two merges is short. I'm
raising it so the doc stays accurate at each merge point, per FND-006 §4.2 (Public API
Surface as a Promise).
Gates
All pass per stated validation evidence: cargo fmt --all -- --check,
cargo clippy --all-targets -- -D warnings, cargo test (821 passed, 0 failed, 7
ignored). CI checks shown as passing on the PR. ✅
Merge note
#5631 must land before #5632. After this merges, the #5632 branch rebases cleanly and
its diff reduces to only the three context-path wiring files plus their tests.
Verdict
Approved. No active blocks. The function is correct, the tests cover the right
cases, the placement and structure are consistent with the existing pattern, and the risk
label accurately reflects a pure additive change. The doc preamble observation above is
advisory only — worth addressing in a follow-up doc commit or as part of the #5632 merge
if convenient, but not a reason to hold this PR.
|
Hey @vernonstinebaker — pulling this into the v0.7.4 milestone as part of our contributor triage sweep. Your PR is approved and in the merge queue (must land before #5632). Thank you for the contribution! 🙌 |
singlerider
left a comment
There was a problem hiding this comment.
Reviewed. Clean additive implementation — symmetric mirror of is_assistant_autosave_key, well-tested, no issues.
Summary
masterfor all contributions):masteruser_msg,user_msg_<uuid>) embed prior conversation context verbatim. Re-injecting these into memory recall causes exponential bloat as each recalled entry carries forward all prior generations.is_user_autosave_key()tocrates/zeroclaw-memory/src/lib.rsmatchinguser_msganduser_msg_<uuid>patterns (case-insensitive), with full unit test coverage. Added a module-level reserved key prefix doc table documenting theuser_msg*andassistant_resp*reserved namespaces and their detection functions.Label Snapshot (required)
risk: low):risk: low— pure additive function and doc comments, no behavior changesize: XS):size: XS— ~34 lines added across two commitsmemory,testsmemory: autosaveChange Metadata
featurememoryDependencies
Linked Issue
is_user_autosave_keyinto all context-building pathsValidation Evidence (required)
user_autosave_key_detection_matches_per_turn_patternsSecurity Impact (required)
NoNoNoNoPrivacy and Data Hygiene (required)
passuser_msg,user_msg_1234,USER_MSG_abcd)Compatibility / Migration
YesNoNoi18n Follow-Through (required when docs or user-facing wording changes)
No— the doc table added is Rust API documentation (doc comments), not user-facing strings or prose docs; no locale files affectedHuman Verification (required)
user_msg,user_msg_<uuid>(any case), rejectsuser_message,assistant_resp_*,telegram_*user_message≠user_msg)Side Effects / Blast Radius (required)
Agent Collaboration Notes (recommended)
is_assistant_autosave_keypattern; added reserved-prefix doc table to document the contract centrallyis_assistant_autosave_keyAGENTS.md+CONTRIBUTING.md): YesRollback Plan (required)
git revert <sha>— two commits, no side effectsRisks and Mitigations
None.