Skill memory + unified task/conversation framework (adaptive session_tag routing) - #139
Merged
Merged
Conversation
…ion evolution
Blocking/regression fixes:
- migrate_procedural_to_skill.sql: ALTER steps to TEXT before textification
(assigning text to a json column fails at plan time with 42804 and aborted
the whole migration on every existing database); normalize legacy free-form
entry_type values into {workflow, guide, script}
- procedural entry_type validation: lenient normalize on read (to_pydantic
runs validators via from_attributes; legacy rows like 'process' must not
crash reads) while the write path stays strict in tool_validators
- startup schema-drift guard: fail fast with the exact migration scripts to
run when an existing DB is missing ORM columns (create_all never ALTERs);
README gains the upgrade-migration runbook
- Redis procedural index: detect a stale pre-skill index (no
description_embedding attribute), drop index-only and rebuild so skill
search doesn't silently fall back to PostgreSQL forever
Data lifecycle (PII/erasure):
- conversation_message + skill_experience now covered by the user/client
memory purge paths (delete_by_user_id / delete_by_client_id)
- distilled conversation turns are pruned after
MIRIX_CONVERSATION_RETENTION_DAYS (default 30, 0 = keep forever) at the end
of each procedural distillation pass
- skill experiences carry client attribution (created_by_id audit column)
Interface:
- SDK search()/search_all_users(): search_method now defaults to None and is
omitted from the request so the server per-type default (procedural ->
hybrid) applies; docstrings document hybrid; local retrieve_memory mirrors
the same per-type resolution
- restore DELETE /memory/procedural/{memory_id} for parity with the other
memory types (skill writes remain evolution-only, no public write routes)
- drop the unreachable memory-agent tool_calls[:1] truncation and the unused
AgentTriggerStateManager.get_state
Structure/hygiene:
- shared self-evicting KeyedLocks registry replaces three hand-rolled
dict[key, asyncio.Lock] registries (fixes the unbounded raw-memory lock
growth)
- get_server singleton moved to mirix.server.server; services no longer
import the REST module
- shared owner_org + _ingest_session_turns helpers replace duplicated blocks;
roadmap jargon (Goal-2/3, C3/C4) replaced with domain terms
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e endpoints
Codex review findings on the previous fix batch:
- DELETE /users/{user_id}/memories and DELETE /clients/{client_id}/memories
performed an irreversible cross-table hard delete (now including verbatim
conversation transcripts and skill experiences) with NO authentication and
no tenant check — any caller who knew a foreign user_id/client_id could
erase that tenant's data. Both endpoints now resolve the caller via
JWT/Client API Key and 404 on cross-organization targets (same response as
a missing id, so they can't be used as an existence oracle).
- Align the migration SQL entry_type mapping with normalize_entry_type():
any legacy value containing "how" maps to 'guide' in both places, so the
read-time normalizer and the migrated column can never disagree.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ry erasure
Codex re-review of the previous fix:
- DELETE /clients/{client_id}/memories compared the target to None, but
client_manager.get_client_by_id RAISES NoResultFound on a miss (it never
returns None), so a missing client_id fell through to a 500 while a
cross-org client_id returned 404 — a distinguishable response that leaks
client-id existence. Catch the miss and return the same 404 as the
cross-org case, matching the user endpoint.
- test_deletion_apis: pass the test client's API key on the two now-
authenticated /memories DELETE calls.
Not changed (out of scope): the keyless X-Client-ID auth mode is the
platform's documented trusted-network model used by the official SDK
(remote_client sends X-Client-ID when constructed without an api_key) and
by every memory endpoint; tightening it is a platform-wide auth-model
decision, not part of this feature.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…configurable transcript budgets
The distiller prompt has always treated tool errors/retries as strong
learning signals (signal_type: tool_error), but the ingestion seam dropped
every non-user/assistant message — the distiller never saw the work process
it was designed to learn from. Close that design/implementation gap:
- conversation_message role space: 'user' | 'assistant' | 'tool' (Literal
change only; the ORM column is a plain String, no DB migration needed)
- _extract_conversation_turns keeps tool results (role 'tool'/'function',
name-prefixed) and serializes assistant tool_calls (OpenAI shape) into the
turn content; a pure tool-call assistant turn with empty content still
yields a turn; [{"type":"text","text":...}] parts flatten to clean text
- role-collapse branches label tool/function messages [TOOL] instead of
mislabeling them [ASSISTANT] for the meta-agent path
- transcript budgets are env-configurable and sized for tool-heavy sessions:
MIRIX_DISTILLER_MAX_TRANSCRIPT_CHARS (default 80000, was hardcoded 16000)
and MIRIX_DISTILLER_MAX_MESSAGE_CHARS (default 8000, was 2000)
- remove the orphaned round-based session_distiller.txt prompt (zero
references; the production distiller uses auto_dream_agent/procedural.txt)
- README + SDK docstring document how to pass tool activity
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd malformed input Two failure modes found while self-reviewing the tool-activity change: - extraction ran OUTSIDE the additive try/except in _ingest_session_turns, so a malformed message shape could abort the primary memory add; it now runs inside the guard (degrades to a missed session, never a dropped add) - one oversized turn (typically a huge tool result) failed the whole batch's Pydantic validation in record_turns, silently dropping every turn of the request; extraction now truncates at the store's per-row content cap with a visible marker Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fix collisions Two defects in migrate_procedural_to_skill.sql, both reproduced end-to-end against real Postgres 15/16 before the fix: 1. Step 2 slugification aborted the WHOLE migration on any row whose summary slugs to '' (whitespace-only, punctuation-only): NULLIF assigned NULL into the NOT NULL name column before the fallback UPDATE could run — NOT NULL is checked per row at assignment time, so the two-statement "set NULL, then patch" shape can never work. Slug and fallback are now one COALESCE in a single statement. 2. Step 2b's single-pass -N suffixing could mint a name that collides with a PRE-EXISTING slug (two 'task' rows produce 'task-2' while a 'task 2' summary already slugged to 'task-2'), failing the final UNIQUE constraint. The dedup now loops until no row needs a rename; terminates because renamed names strictly lengthen each pass. Verified against postgres:16-alpine with adversarial legacy data (whitespace/ punctuation/NULL summaries, 3-way slug collisions crossing a pre-existing suffixed name, legacy entry_type variants, >60-char summaries, cross-org isolation) and on an empty table: 13/13 assertions pass. Also close the two test gaps flagged in the ship-readiness review: - _ingest_session_turns isolation guard rails (store failure, extraction failure, no-session_id skip, session stamping + turn recording) — pins the "additive write must never abort the primary memory add" contract. - Distiller _render_transcript budgets: per-turn cap marker, whole-transcript head/tail elision, and the untouched-within-budget case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e; enforce scope on all read fallbacks
Procedural skills created by the evolution pipeline carried no
filter_tags scope tag, while /memory/search filters reads with
filter_tags->>'scope' IN (<client.read_scopes>) — a NULL tag never
matches, so on Postgres deployments skill retrieval always returned 0.
The bug was masked on SQLite dev machines because the in-memory bm25
fallback ignored the scopes it was handed. Reproduced live on Postgres
(3 skills stored, 0 returned; tagging one row by hand made exactly that
row appear).
Write side (root cause): run_experience_evolution now constructs the
ProceduralMemoryAgent with filter_tags={"scope": actor.write_scope} via
the new pure scope_filter_tags() helper (warning when the actor has no
write_scope). The evolution path is the single producer of skills and
bypasses the REST layer's scope injection, so it must stamp the scope
itself; skill_create reads it back off the agent instance and skill_edit
preserves it. Intended side effect: the evolution agent's block loading
narrows to the same scope the normal add-flow child already uses.
Read side (defense in depth, family-wide): the SQLite in-memory bm25
fallback and fuzzy_match candidate loads in all five document-memory
managers (procedural, episodic, semantic, resource, knowledge_vault)
now mirror their own scoped base_query — organization_id plus
apply_filter_tags_sqlalchemy(filter_tags, scopes) — instead of
filtering on user_id alone. Redis search paths now build the
filter/scope clause when scopes are supplied without filter_tags
(previously the scope filter was silently dropped).
Backfill: scripts/migrate_backfill_procedural_scope.sql reconstructs
the scope for existing rows from client_id -> clients.write_scope,
handling SQL NULL / JSON scalar 'null' / non-object filter_tags (the
ORM's JSON column stores Python None as JSON 'null', which jsonb_set
rejects — hit on live data). Idempotent; unresolvable rows (no client
or read-only client) are left untouched and counted in a NOTICE.
README gains the runbook line. Verified against a live Postgres: 13
rows backfilled, re-run UPDATE 0, previously-invisible skills returned
by bm25 and hybrid search.
Tests (15 new in tests/test_scope_read_consistency.py): per-manager
bm25/fuzzy scope filtering on a forced-SQLite hermetic engine with
scopes=None/[scope]/[] cases, a constructor-spy test pinning that
run_experience_evolution passes the actor-derived stamp (fails if the
wiring is removed — verified), insert->scoped-read round trip,
scope_filter_tags unit tests, and migration text assertions. Suite:
697 passed.
Intentionally unchanged: /memory/components and
get_total_number_of_items remain unscoped (endpoint-wide overview
semantics shared by all memory types); Redis scopes=[] empty-list gap
(pre-existing, Redis off by default). Follow-up candidate: extract a
shared scoped candidate-query builder so the five managers cannot
drift apart again.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… memory
Add a per-session `session_tag` ("task" | "conversation") that routes the
memory machinery by input type, so procedural/skill memory and the
episodic/semantic conversation memory coexist in ONE framework instead of
every session paying for every memory type.
- task -> procedural skill distillation (auto_dream mode="procedural");
inline episodic/semantic extraction suppressed (skill-only).
- conversation -> episodic/semantic extracted as before; procedural batch
trigger never fires; experience consolidation stays
operator-invoked (auto_dream mode="experience").
- Caller declares the tag on ingest (AddMemoryRequest.session_tag). Unset falls
back to DEFAULT_SESSION_TAG="conversation" so legacy/untagged callers are
safe (no spurious skill distillation).
Mechanism: the tag rides on filter_tags (already set on the agent instance and
read in trigger_memory_update) so no Message-schema or agent.step change is
needed. The distiller gate (list_sealed_undistilled_sessions) is FAIL-OPEN:
it excludes only sessions EXPLICITLY tagged "conversation", so a missing tag
never zeroes out task skill-building.
Adds a nullable conversation_message.session_tag column + migration.
Tested on Azure (gpt-5.2 agent): LOCOMO-10 overall 81.23% (=81.3% ref, zero
regression); ALFWorld SkillOpt frozen-134 68.7% with full gpt-5.2 skill
distillation vs 52.2% no-skill baseline (+16.5).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
richard-peng-xia seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Skill memory + unified task/conversation framework
This PR delivers a procedural skill memory system and a unified framework
that lets task-style and conversation-style inputs share one memory stack, each
engaging only the machinery it needs. Tested end-to-end on ALFWorld (task)
and LOCOMO (conversation).
1. Skill (procedural) memory
Session transcripts are distilled into
SkillExperiencerecords and evolved intoreusable skills (name / description / instructions / triggers / examples), stored
in
procedural_memoryand retrieved top-k at act time. (Core skill system from#136.)
2. Unified framework — adaptive
session_tagroutingA per-session
session_tagroutes the memory machinery by input type soprocedural/skill memory and episodic/semantic conversation memory coexist in one
framework instead of every session paying for every memory type:
task(e.g. ALFWorld)conversation(e.g. LOCOMO)auto_dream(mode="procedural")auto_dream(mode="experience")AddMemoryRequest.session_tag); unset →DEFAULT_SESSION_TAG="conversation", so existing/untagged callers areunchanged and never get spurious skill distillation.
filter_tags(already stamped at ingest and read intrigger_memory_update), so noMessage-schema oragent.stepchange isneeded.
explicitly tagged
conversation, so a missing tag can never zero out taskskill-building.
conversation_message.session_tagcolumn + migration(
scripts/migrate_add_conversation_message_session_tag.sql); legacy rows stayNULL and skill-eligible.
3. Tested — scores on both regimes
Azure OpenAI; agent policy gpt-5.2, memory gpt-4.1-mini unless noted.
Conversation — LOCOMO-10 (routing must not regress dialogue memory):
→ zero regression (−0.07 overall).
conversationsessions extractepisodic/semantic normally; the procedural trigger stays off (LOCOMO QA never
reads procedural).
Task — ALFWorld (SkillOpt split, frozen test = 134; gpt-5.2 agent):
→
taskrouting builds + retrieves skills correctly; with a strong distiller theskills capture real failure modes (anti-loop / disambiguation / bounded-search)
and lift success +16.5 over the no-skill baseline.
Compatibility
Backward compatible: existing rows keep
session_tag = NULLand stayskill-eligible (fail-open). Run the migration once on existing deployments; fresh
databases get the column via
create_all.