Skip to content

feat: add persistent per-user agent memory option - #133

Open
OchnikBartek wants to merge 3 commits into
mainfrom
feat/agent-memory
Open

feat: add persistent per-user agent memory option#133
OchnikBartek wants to merge 3 commits into
mainfrom
feat/agent-memory

Conversation

@OchnikBartek

Copy link
Copy Markdown
Member

Summary

Adds an enable_memory template option (--memory CLI flag / "Agent Memory"
wizard step) that ships generated projects with persistent per-user agent memory:
a pydantic-ai-harness notebook stored in PostgreSQL, write/read/delete/search_memory
agent tools rendered as chat cards, a /api/v1/me/memory CRUD API and a
Settings → Memory page. PydanticAI + PostgreSQL only, activated at runtime with
ENABLE_MEMORY=true.

Added

  • Generator: --memory flag in fastapi_gen/cli.py, prompt_memory() and an
    "Agent Memory" step in prompts.py, enable_memory on ProjectConfig with
    framework validation (PydanticAI required; a Postgres guard kept as
    defense-in-depth), cookiecutter.json default and a VARIABLES.md entry.
  • Templated backend: app/agents/memory.py, app/db/memory_pool.py,
    app/schemas/user_memory.py, app/services/user_memory.py,
    app/api/routes/v1/me_memory.py, lifespan pool wiring, the ENABLE_MEMORY
    setting and a pydantic-ai-harness>=0.12.0 dependency, plus generated tests
    tests/test_memory.py and tests/api/test_me_memory.py.
  • MEMORY_GUIDANCE in app/agents/memory.py, extending the harness default
    guidance: questions about the memory itself must go through read_memory /
    search_memory rather than being answered from the injected notebook (so the
    read is visible as a chat card), and dates may only be written when actually
    known from the conversation or a tool.
  • Templated frontend: Settings → Memory page and components (memory-manager,
    memory-file-editor, memory-delete-dialog, memory-file-name), use-memory
    hook, memory-api.ts client, /api/me/memory[/file] proxy routes, memory tool
    cards (tool-results/memory.tsx), demo-replay labels/graph previews, and a new
    py-literal.ts parser lib used by the memory cards.
  • pydantic_ai_memory matrix config (memory + deep research + Slack) in
    tests/test_template_integration.py and enable_memory validation tests in
    tests/test_config.py.

Changed

  • agent_session.py template: Deps(user_id=…) and a per-turn memory capability
    under the flag; deep-research interstitial buffering extended to memory tools,
    with withheld text released before final_result when the run ends on that step
    (previously dropped); event.result.contentevent.part.content for the
    pydantic-ai v2 API.
  • agent_invocation.py template: Slack/Telegram channel invocations build the
    memory capability only for traffic mapped to a real user account, so anonymous
    channel traffic cannot collapse onto one shared memory scope.
  • tool-call-card.tsx template: memory tools use the NotebookPen icon —
    Brain already marks reasoning frames in demo-replay.tsx — the memory file
    name no longer duplicates into the collapsed bar, and the italic input hint
    gained pr-0.5 so truncate stops clipping the last glyph's overhang.
  • TestGeneratedDeepResearch::test_session_gates_buffer_on_research_tools now
    asserts on _INTERSTITIAL_TOOL_NAMES / made_interstitial_call instead of the
    removed made_research_call.
  • post_gen_project.py removes all memory modules (backend and frontend) when the
    flag is off.
  • Templated dependency bumps: pydantic-ai-slim >=2.18.0 across all provider
    variants, pydantic-ai-skills >=1.3.0, subagents-pydantic-ai >=0.2.10,
    summarization-pydantic-ai >=0.1.11; fastapi-pagination pinned <0.15.16
    (that release targets the FastAPI 0.137 get_body_field() signature the
    template's FastAPI pin stays below).

Testing

  • Ran uv run pytest tests/test_config.py — 79 passed.
  • Ran uv run pytest tests/test_template_integration.py -k "gates_buffer or memory"
    — 3 passed, including the new pydantic_ai_memory matrix config.
  • Ran the full uv run pytest — 676 passed, 3 failed; the deep-research buffering
    assertion was the only memory-related failure and is fixed in this branch.
  • Generated a memory-on project (PydanticAI + Postgres + Next.js + deep research):
    tests/test_memory.py, tests/api/test_me_memory.py and tests/test_agents.py
    — 63 passed, ruff check clean, tsc --noEmit and eslint clean.
  • Generated a memory-off project: no memory modules or references remain, backend
    and frontend gates clean.

Notes for Reviewers

  • Three harness internals are imported directly: _DEFAULT_GUIDANCE,
    normalize_filename and validate_store_path. None is re-exported publicly;
    normalize_filename in particular is load-bearing — the toolset only accepts
    flat *.md names, and a nested path written through the API makes
    search_memory raise for that whole scope, so the API canonicalizes on write.
  • The full suite has 2 remaining failures, both test_passes_ty[deepagents_pg*]:
    the deepagents package changed StateBackend.__init__ to take no arguments,
    which breaks a framework path this branch does not touch. Pre-existing drift,
    tracked separately.
  • The Postgres requirement check in config.py is currently unreachable (the only
    non-Postgres option is rejected earlier as "a database is required") and is kept
    with # pragma: no cover so a future database backend can't silently ship memory
    on a store the harness cannot use.
  • py-literal.ts is removed by the post-gen hook when memory is off; its only
    reference in the template is tool-results/memory.tsx, so nothing dangles.

@OchnikBartek
OchnikBartek requested a review from DEENUU1 July 30, 2026 09:24
@OchnikBartek OchnikBartek self-assigned this Jul 30, 2026
@OchnikBartek OchnikBartek added the enhancement New feature or request label Jul 30, 2026
deepagents 0.7.0 (released 2026-07-29) removed the `runtime` parameter it
had deprecated since 0.5.0, and narrowed `create_deep_agent(backend=...)`
to a backend instance — the factory form is gone. Generated deepagents
projects raised a TypeError at runtime and failed `ty check` in CI.

`StateBackend()` is valid on both 0.6.x and 0.7.x, so the dependency floor
stays where it is.

@DEENUU1 DEENUU1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the whole thing end to end — generator, backend, frontend, and the post-gen cleanup — and generated projects with memory on, memory off, and PydanticDeep to check the rendering. This is a solid feature and the design decisions are the right ones.

What I particularly liked: the tenant boundary is resolved from the authenticated user before the agent exists rather than through a ctx.deps callable, so there's no path where a missing or model-controlled user_id widens the scope. The deliberate absence of an in-memory fallback in memory_pool.py is the same instinct — memory being unavailable is better than faking persistence. MEMORY_GUIDANCE forcing recall through a real tool call so the user can see where the answer came from is a genuinely good product call, not just a technical one. And test_tool_names_match_harness_toolset pinning names the harness owns is the kind of test that pays for itself.

One thing to fix before merge, plus a few smaller ones:

The FunctionToolResultEvent.result.part rename is only done in one of three places. The other two are in the PydanticDeep branch and I confirmed they render as-is in a generated project:

  • template/{{cookiecutter.project_slug}}/backend/app/services/agent_session.py:2781 and :2787
  • template/{{cookiecutter.project_slug}}/backend/app/api/routes/v1/agent.py:234-236

Details in the inline comment. It's pre-existing rather than caused by this PR, but it's the same rename and the same afternoon's work, so I'd sweep all three here.

The rest is a retry-pile-up in the memory pool, a lost-edit case in the settings UI, and two nits.

tc = pending.get(tool_event.tool_call_id)
if tc is not None:
tc["result"] = str(tool_event.result.content)
tc["result"] = str(tool_event.part.content)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is right — FunctionToolResultEvent really did move to .part in pydantic-ai 2.x. The base class is now ToolResultEvent.part: ToolReturnPart | RetryPromptPart and there is no .result attribute at all any more.

The problem is that this is one of three identical sites and the other two are still on the old attribute. Both live in the PydanticDeep branch:

  • backend/app/services/agent_session.py:2781 and :2787str(tool_event.result.content)
  • backend/app/api/routes/v1/agent.py:234-236event.result.tool_name and str(event.result.content)

I generated a PydanticDeep project off this branch to be sure, and both render exactly as written. That variant isn't insulated from v2 either: pydantic-deep itself requires pydantic-ai-slim>=2.0.0, so the generated project resolves to v2 and every single tool call raises AttributeError: 'FunctionToolResultEvent' object has no attribute 'result' the moment a tool returns.

To be fair, this is pre-existing — the old >=1.80.0 floor was unbounded, so PydanticDeep already pulled v2 before this PR. But nothing is going to catch it on its own: the Template - PostgreSQL + PydanticDeep CI job generates the project and runs ruff + ty, and neither flags it (the event is reached through an Any-typed stream). Since you're already doing the v2 migration for this line, I'd finish the sweep:

# agent_session.py, pydantic-deep branch (~2781, ~2787)
tc["result"] = str(tool_event.part.content)
...
"content": str(tool_event.part.content),

# api/routes/v1/agent.py (~234-236)
"tool_name": event.part.tool_name,
"content": str(event.part.content),

While you're there: backend/pyproject.toml:180 still pins the pydantic-deep branch at pydantic-ai-slim[duckduckgo,web-fetch]>=1.80.0. Worth bumping to >=2.18.0 with the others so the declared floor matches what actually installs — right now the pin says v1 is fine and it very much isn't.

return None
if _memory_store is not None:
return _memory_store
if _last_attempt_at is not None and time.monotonic() - _last_attempt_at < _RETRY_COOLDOWN_SECS:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the lazy retry a lot — a database that happens to be down at boot shouldn't leave memory dead until the next deploy. There's one hole in it though.

_last_attempt_at is stamped at the start of the attempt (line 60), not when it finishes. A failing create_pool against an unreachable host can easily outlast the 10 second cooldown — a dropped SYN or a slow DNS failure will sit there for a while. So while attempt #1 is still hanging, every request arriving more than 10s in passes this cooldown check, calls init_memory_pool, and queues on _init_lock. And because the double-check inside the lock only tests _memory_pool is not None, each queued caller then runs its own full connect attempt. Instead of one attempt per 10s you get a serialized queue of them, each holding a request open for the duration.

Two small changes close it — stamp the timestamp when the attempt completes, and re-check the cooldown inside the lock so a queued caller bails instead of retrying:

async with _init_lock:
    if _memory_pool is not None:
        return _memory_pool
    # Re-check under the lock: a caller that queued behind a failed attempt
    # must not immediately launch its own.
    if _last_attempt_at is not None and time.monotonic() - _last_attempt_at < _RETRY_COOLDOWN_SECS:
        return None
    pool: asyncpg.Pool | None = None
    try:
        ...
    finally:
        _last_attempt_at = time.monotonic()

Passing an explicit timeout= to create_pool would also help, so a hung connect fails fast rather than holding the lock for the default.

Comment on lines +87 to +90
toast.error(errorMessage(e, "Failed to save file"));
// The version in hand may be stale; drop it so the next attempt reloads.
setEditor(null);
void refresh();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small, but it'll annoy people. Any failure here closes the dialog and throws away whatever the user typed.

For MEMORY_VERSION_CONFLICT that's exactly right — the version in hand is stale and they need a fresh load. But a 500, a dropped connection, or the backend restarting mid-save all land here too, and then the user loses a paragraph of notes with no way to get it back and no reason to expect it.

I'd scope the reset to the conflict case:

Suggested change
toast.error(errorMessage(e, "Failed to save file"));
// The version in hand may be stale; drop it so the next attempt reloads.
setEditor(null);
void refresh();
const code = e instanceof ApiError ? (e.data as { code?: string } | null)?.code : undefined;
toast.error(errorMessage(e, "Failed to save file"));
if (code === "MEMORY_VERSION_CONFLICT") {
// The version in hand is stale; drop it so the next attempt reloads.
setEditor(null);
void refresh();
}

Everything else keeps the dialog open so they can hit Save again.

"""All of one user's memory files."""

items: list[MemoryFileEntry]
total: int

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

total is set to len(items) in list_files, so once truncated is true it reports 100 rather than how many files the user actually has. Combined with the class docstring ("All of one user's memory files") it reads like a real count.

Nothing consumes it wrongly today — the UI uses the array length — but the next person who reaches for total to render "you have N memories" will get it wrong, and it'll be wrong only for the users with the most data. A renaming would ripple into memory-api.ts and the route tests, so I'd just make the field say what it is:

Suggested change
total: int
total: int
"""Number of files in ``items`` — not the store total when ``truncated``."""
truncated: bool = False

value={path}
onChange={(e) => onPathChange(e.target.value)}
placeholder="preferences.md"
maxLength={MAX_MEMORY_NAME_CHARS}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this caps what the user types at 80, but .md gets appended afterwards, so an exactly-80-character name canonicalizes to 83 and isValidMemoryName rejects it. The toast they get is "Use a flat file name — letters, digits, dots and dashes, no folders", which never mentions length, so it looks like the name is malformed rather than too long.

Cheapest fix is to say so in the message that fires:

toast.error("Use a flat file name up to 80 characters — letters, digits, dots and dashes, no folders.");

Edge case either way, just a confusing one when you hit it.

async def build_memory_capability(user_id: str) -> "Memory[Deps] | None":
"""Build the per-user Memory capability, or ``None`` when unavailable.

A static namespace (rather than a ``ctx.deps`` callable) keeps CLI and

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right call and I'm glad it's written down rather than left implicit. Resolving the namespace from the authenticated user at build time, instead of through a ctx.deps callable the way the harness also allows, means there is no code path where a missing or model-influenced user_id can silently widen the scope — the tenant boundary is decided before the agent object exists. Anyone extending this later will read the comment before they reach for ctx.deps, which is the whole point.

# Channel messages share the web chat's memory notebook, but only for
# traffic mapped to a real account — anonymous channel traffic must not
# collapse onto one shared memory scope.
invocation_user_id = kwargs.get("user_id")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, and easy to miss. If this had just mirrored the MCP line above it and passed kwargs.get("user_id") straight through, every unmapped Slack or Telegram sender would have collapsed onto a single user-None notebook — one person's notes injected into strangers' conversations, and written to by all of them. Gating on a real account and simply going without memory otherwise is the correct trade.

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@DEENUU1 DEENUU1 moved this to In review in Vstorm OSS Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

3 participants