feat(memory): recall and store memory around chat completions#190
feat(memory): recall and store memory around chat completions#190dpoulopoulos wants to merge 4 commits into
Conversation
GitHub Actions
Inactive
github-actions
Bot
added
the
missing-template
WalkthroughAdds platform-mode persistent memory for chat completions. The gateway recalls and injects facts before dispatch, then persists user-assistant exchanges after streaming and non-streaming completions, with configuration, platform API handling, and tests. ChangesPlatform Memory Integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/unit/test_memory_platform.py (1)
25-80: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd regression tests for malformed timeout config values.
Please add tests where
memory_recall_timeout_ms/memory_remember_timeout_msare non-numeric, so best-effort semantics stay protected against config drift.Also applies to: 86-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_memory_platform.py` around lines 25 - 80, Add new regression test cases to verify that non-numeric values for `memory_recall_timeout_ms` and `memory_remember_timeout_ms` configuration parameters are handled gracefully. Create test functions that pass malformed timeout values (such as strings or None) through the _config() helper function and verify that the _recall_platform_memory function still executes successfully without raising exceptions, following the same pattern as the existing test functions like test_recall_non_200_yields_no_facts and test_recall_timeout_is_swallowed to ensure best-effort semantics are protected against configuration drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gateway/api/routes/_pipeline.py`:
- Around line 1461-1462: The asyncio.create_task call for on_settled_text is
fire-and-forget and lacks exception handling, which causes unhandled task
exceptions to leak runtime warnings. Add a done callback to the task created by
asyncio.create_task that suppresses any exceptions raised by on_settled_text,
ensuring memory persistence failures remain silent and non-disruptive. Reference
the asyncio.create_task line with on_settled_text and add a callback using
add_done_callback that catches and silently ignores any exceptions from the
completed task.
In `@src/gateway/api/routes/_platform.py`:
- Around line 602-603: The timeout parsing on line 602 and line 639 using
int(config.platform.get("memory_recall_timeout_ms", 2000)) can raise ValueError
exceptions if the config value is not a valid integer, breaking the "never block
chat" contract. For both occurrences where memory_recall_timeout_ms is parsed,
wrap the int() conversion in a try-except block to safely handle invalid input,
catching ValueError and returning the default fallback value of 2000 when
parsing fails. This ensures misconfiguration does not cause chat functionality
to break.
In `@src/gateway/api/routes/chat.py`:
- Around line 361-364: The recall operation in the memory check block lacks
error handling, which means any exception raised by _recall_platform_memory
(such as invalid timeout parsing in its config handling) will cause the entire
request to fail. Wrap the _recall_platform_memory call and the subsequent
inject_memory_facts call in a try-except block so that any exceptions are caught
and logged, allowing the request to proceed without memory injection rather than
breaking the completion entirely. This ensures memory recall failures remain
best-effort and do not disrupt provider dispatch.
---
Nitpick comments:
In `@tests/unit/test_memory_platform.py`:
- Around line 25-80: Add new regression test cases to verify that non-numeric
values for `memory_recall_timeout_ms` and `memory_remember_timeout_ms`
configuration parameters are handled gracefully. Create test functions that pass
malformed timeout values (such as strings or None) through the _config() helper
function and verify that the _recall_platform_memory function still executes
successfully without raising exceptions, following the same pattern as the
existing test functions like test_recall_non_200_yields_no_facts and
test_recall_timeout_is_swallowed to ensure best-effort semantics are protected
against configuration drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eee16f2c-1ddb-4b1b-a076-e50f57f07112
📒 Files selected for processing (8)
src/gateway/api/routes/_helpers.pysrc/gateway/api/routes/_pipeline.pysrc/gateway/api/routes/_platform.pysrc/gateway/api/routes/chat.pysrc/gateway/core/config.pytests/unit/test_memory_inject.pytests/unit/test_memory_platform.pytests/unit/test_stream_memory_capture.py
github-actions
Bot
removed
the
missing-template
dpoulopoulos
left a comment
dpoulopoulos
left a comment
There was a problem hiding this comment.
Adds opt-in persistent memory to platform-mode chat completions: a new memory_enabled flag (off by default) gates a best-effort recall-before-dispatch + remember-after flow. Message helpers inject recalled facts into the system prompt and build the turn to store; two platform clients (_recall_platform_memory, _remember_platform_memory) talk to /gateway/memory/{recall,remember}; a streaming wrapper accumulates assistant text and fires a fire-and-forget remember on normal completion. Both streaming and non-streaming paths are wired. Standalone mode is unaffected.
0aee46f to
f98dbd5
Compare
GitHub Actions
Inactive
|
Thanks for the careful review. Pushed fixups addressing the best-effort gaps:
Two points I left as-is, with reasoning:
ruff, mypy (strict), and the full unit suite are green. |
dpoulopoulos
left a comment
dpoulopoulos
left a comment
There was a problem hiding this comment.
Adds opt-in, best-effort persistent memory to platform-mode chat completions, gated by a new memory_enabled config flag (OTARI_MEMORY_ENABLED, default off). Before dispatch the gateway recalls facts from the platform (/gateway/memory/recall) and injects them into the system prompt; after a completion it stores the new turn fire-and-forget (/gateway/memory/remember) for both non-streaming (background task) and streaming (a capture wrapper that accumulates assistant text and persists only on normal completion). Standalone mode and the /messages and /responses endpoints are unaffected. This is a re-review after the author addressed the previous round of feedback.
Prior feedback status: resolved. All actionable items from the previous review (CodeRabbit's 3 plus the prior [major]/[minor] review's 5) are addressed in the current code: the narrow (TimeoutException, NetworkError) catch was broadened to httpx.HTTPError in both recall and remember; a _coerce_timeout_ms helper guards timeout parsing; the recall call site in chat.py is wrapped in a best-effort try/except; the streaming create_task now has an exception-swallowing done-callback; and regression tests for malformed timeouts, generic httpx errors, and a raising settled-callback were added. Two low-priority items the prior review explicitly flagged as by-design/low-risk remain open (recall latency, multimodal-system-content edge case). Verified: ruff, mypy (strict), and all 32 new unit tests pass. No web UI exists for this backend feature, so Playwright verification is not applicable.
f98dbd5 to
7c5fb36
Compare
GitHub Actions
Inactive
|
Pushed fixups for the second-round feedback (folded into their original commits):
Left as-is (both flagged low-risk/by-design in review):
ruff, mypy (strict), and the memory + config test suites are green. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gateway/core/config.py`:
- Around line 410-415: The PLATFORM_MEMORY_RECALL_TIMEOUT_MS and
PLATFORM_MEMORY_REMEMBER_TIMEOUT_MS configuration entries are registered with
int casting, which causes invalid environment values to raise an exception
during config load and prevent the best-effort fallback behavior in
_coerce_timeout_ms from working. Change the type casting for these two entries
from int to str to defer validation, allowing the configuration to load
successfully and let _coerce_timeout_ms handle the validation and fallback to
defaults when encountering non-numeric values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17d7ddae-5fb9-4684-b2c5-6dfddb10ecd6
📒 Files selected for processing (10)
src/gateway/api/routes/_helpers.pysrc/gateway/api/routes/_pipeline.pysrc/gateway/api/routes/_platform.pysrc/gateway/api/routes/chat.pysrc/gateway/core/config.pytests/integration/test_config_env_loading.pytests/integration/test_platform_mode_chat.pytests/unit/test_memory_inject.pytests/unit/test_memory_platform.pytests/unit/test_stream_memory_capture.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/unit/test_memory_platform.py
- tests/unit/test_memory_inject.py
- src/gateway/api/routes/_helpers.py
- src/gateway/api/routes/_platform.py
- src/gateway/api/routes/chat.py
- src/gateway/api/routes/_pipeline.py
| # Best-effort memory timeouts (platform mode). Recall is on the hot path | ||
| # (added to time-to-first-token); remember is fire-and-forget after the | ||
| # completion. Both fall back to their defaults on a missing or invalid | ||
| # value (see _coerce_timeout_ms in routes/_platform.py). | ||
| "PLATFORM_MEMORY_RECALL_TIMEOUT_MS": ("memory_recall_timeout_ms", int), | ||
| "PLATFORM_MEMORY_REMEMBER_TIMEOUT_MS": ("memory_remember_timeout_ms", int), |
There was a problem hiding this comment.
Handle invalid PLATFORM_MEMORY_*_TIMEOUT_MS values without crashing startup.
These vars are registered with int casting here, so a non-numeric env value raises during config load (Line 431) and prevents startup. That bypasses the intended best-effort fallback behavior in _coerce_timeout_ms(...).
Suggested fix
@@
- for env_name, (field_name, caster) in env_mappings.items():
+ memory_timeout_envs = {
+ "PLATFORM_MEMORY_RECALL_TIMEOUT_MS",
+ "PLATFORM_MEMORY_REMEMBER_TIMEOUT_MS",
+ }
+ for env_name, (field_name, caster) in env_mappings.items():
value = os.getenv(env_name)
if value is None or value == "":
continue
- platform[field_name] = caster(value)
+ try:
+ platform[field_name] = caster(value)
+ except ValueError:
+ if env_name in memory_timeout_envs:
+ # Let downstream _coerce_timeout_ms apply defaults.
+ platform[field_name] = value
+ continue
+ raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gateway/core/config.py` around lines 410 - 415, The
PLATFORM_MEMORY_RECALL_TIMEOUT_MS and PLATFORM_MEMORY_REMEMBER_TIMEOUT_MS
configuration entries are registered with int casting, which causes invalid
environment values to raise an exception during config load and prevent the
best-effort fallback behavior in _coerce_timeout_ms from working. Change the
type casting for these two entries from int to str to defer validation, allowing
the configuration to load successfully and let _coerce_timeout_ms handle the
validation and fallback to defaults when encountering non-numeric values.
A master switch (default off) for persistent memory in platform mode. When off, the gateway makes no memory calls and behavior is unchanged; per-workspace enablement is still controlled by the platform. Refs #189 Signed-off-by: Dimitris Poulopoulos <dimitris.a.poulopoulos@gmail.com>
…pers _recall_platform_memory / _remember_platform_memory call the platform's /gateway/memory/* endpoints, modeled on the usage reporter: they swallow timeouts and errors so a slow or unavailable memory service never breaks a chat. inject_memory_facts prepends recalled facts to the system message (mirroring inject_purpose_hints); build_remember_messages assembles the minimal new turn to store. Helpers are unit-tested and not yet wired into the request path. Refs #189 Signed-off-by: Dimitris Poulopoulos <dimitris.a.poulopoulos@gmail.com>
Gated by OTARI_MEMORY_ENABLED in platform mode: recall runs once before dispatch and injects facts into the system message (covering every path, streaming and not), and the new turn is stored fire-and-forget afterwards. Non-streaming uses a background task; streaming uses _stream_with_memory_capture to accumulate the assistant text and remember only on normal completion (skipped on disconnect). The capture wrapper is unit-tested. No behavior change when the flag is off. Closes #189 Signed-off-by: Dimitris Poulopoulos <dimitris.a.poulopoulos@gmail.com>
7c5fb36 to
5a08d63
Compare
GitHub Actions
Inactive
njbrake
left a comment
njbrake
left a comment
There was a problem hiding this comment.
Hi @dpoulopoulos , tagging as request changes to make sure we chat before it gets merged, I have an open question about the background for this PR in the Github Issue: #189 (comment)
Thanks!
GitHub Actions
Failure
Read the memory_enabled flag off the resolve response and gate recall/remember on it, so the platform is authoritative for per-workspace enablement. The gateway's own switch becomes a self-hosted opt-out that defaults on, removing the silent-off footgun where memory was enabled on the platform but the gateway never called it. The flag defaults to true when a platform omits it, so recall never silently stops. Also raise the default recall timeout from 2000ms to 8000ms. Signed-off-by: Dimitris Poulopoulos <dimitris@mozilla.ai>
73e0b5b to
2c80c54
Compare
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/test_hybrid_mode_chat.py`:
- Around line 1699-1701: Update the comment above the memory_calls assertion in
the integration test to state that the gateway’s local opt-out is off by
default, while preserving the explanation that the platform’s workspace memory
setting prevents recall and remember from running.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 307a5169-7ce4-4bc6-b45e-0825b74c19d2
📒 Files selected for processing (7)
docs/hybrid-mode-protocol.mdsrc/gateway/api/routes/_platform.pysrc/gateway/api/routes/chat.pysrc/gateway/core/config.pytests/integration/test_hybrid_mode_chat.pytests/unit/test_memory_platform.pytests/unit/test_resolve_payload_parsing.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/test_memory_platform.py
- src/gateway/api/routes/_platform.py
- src/gateway/api/routes/chat.py
| # The platform reports the workspace has memory off, so neither recall nor remember runs | ||
| # even though the gateway's local opt-out is on (default). | ||
| assert memory_calls == [] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the local opt-out wording.
memory_enabled defaults to enabled here, so the gateway’s local opt-out is off, not “on.” Please update this comment to match the configuration contract and avoid confusing future maintenance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/test_hybrid_mode_chat.py` around lines 1699 - 1701, Update
the comment above the memory_calls assertion in the integration test to state
that the gateway’s local opt-out is off by default, while preserving the
explanation that the platform’s workspace memory setting prevents recall and
remember from running.
There was a problem hiding this comment.
Pull request overview
Adds best-effort “platform memory” to hybrid/platform-mode chat completions so the gateway can (1) recall remembered facts before dispatch and (2) persist new facts after the assistant responds, without making memory a hard dependency.
Changes:
- Introduces platform memory recall/remember HTTP helpers (with configurable timeouts) and a
memory_enabledgating flag carried on the platform resolve response. - Wires memory recall into the chat request normalization path and memory remember into both non-streaming and streaming completion flows (streaming captures assistant text on normal completion).
- Adds unit + integration test coverage for payload parsing, recall/remember behavior, injection helpers, and streaming settlement behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/gateway/core/config.py |
Adds memory_enabled config flag and platform env overrides for memory timeouts. |
src/gateway/api/routes/chat.py |
Injects pre-dispatch recall and schedules post-completion remember for chat completions. |
src/gateway/api/routes/_platform.py |
Extends resolve schema with memory_enabled and implements recall/remember client calls. |
src/gateway/api/routes/_pipeline.py |
Adds streaming wrapper to capture assistant text and trigger remember on normal stream completion. |
src/gateway/api/routes/_helpers.py |
Adds helpers to inject recalled facts into system messages and to build remember payloads. |
docs/hybrid-mode-protocol.md |
Documents the new optional memory_enabled resolve field. |
tests/integration/test_config_env_loading.py |
Verifies platform memory timeout env overrides load correctly. |
tests/integration/test_hybrid_mode_chat.py |
End-to-end integration tests for memory enablement gating, recall injection, and remember writes. |
tests/unit/test_memory_inject.py |
Unit tests for memory injection and remember-message construction helpers. |
tests/unit/test_memory_platform.py |
Unit tests for platform memory client behavior (timeouts/errors/malformed payloads). |
tests/unit/test_resolve_payload_parsing.py |
Unit tests for parsing memory_enabled from resolve payloads with backward-compatible defaults. |
tests/unit/test_stream_memory_capture.py |
Unit tests for streaming text capture and settled-callback behavior. |
| ], | ||
| "memory_enabled": false // optional; whether the workspace has memory enabled | ||
| } |
| request_id=str(payload["request_id"]), | ||
| fallback_enabled=bool(payload.get("fallback_enabled", False)), | ||
| attempts=attempts, | ||
| # Absent on older platforms; default True so recall still runs (see ResolvedRoute). | ||
| memory_enabled=bool(payload.get("memory_enabled", True)), | ||
| ) |
| api_key=str(payload["api_key"]), | ||
| managed=bool(payload.get("managed", False)), | ||
| ) | ||
| ], | ||
| memory_enabled=bool(payload.get("memory_enabled", True)), | ||
| ) |
| memory_enabled: bool = Field( | ||
| default=True, | ||
| description=( | ||
| "Self-hosted opt-out for persistent memory (platform mode). The platform is " | ||
| "authoritative for per-workspace enablement (carried on the resolve response), " | ||
| "so leave this on (the default) to follow the platform: memory is used only for " | ||
| "workspaces that enabled it. Set it false to hard-disable all memory calls from " | ||
| "this gateway regardless of platform config. Recall runs before dispatch, so a " | ||
| "slow memory service adds up to its timeout to time-to-first-token; the storing " | ||
| "call after a completion is fire-and-forget. Both timeouts are tunable via the " | ||
| "platform settings 'memory_recall_timeout_ms' (default 8000) and " | ||
| "'memory_remember_timeout_ms' (default 10000), or their PLATFORM_* env aliases." | ||
| ), | ||
| ) |
| Mirrors :func:`gateway.services.mcp_loop.inject_purpose_hints`: augments an existing | ||
| system message, or inserts one when absent. Returns a new list; the input is not | ||
| mutated. A no-op when ``facts`` is empty. | ||
| """ |
| exc = task.exception() | ||
| if exc is not None: | ||
| logger.warning("Background memory write failed: %s", exc) |
khaledosman
left a comment
khaledosman
left a comment
There was a problem hiding this comment.
Reviewed the full diff plus enclosing code, and cross-checked the wire contract against the companion platform PR (mozilla-ai/otari-ai#1148): endpoint paths, X-Gateway-Token/X-User-Token headers, and the {"query"} -> {"facts"} / {"messages"} shapes all match. Streaming disconnect handling is correct (no remember on aborted or errored streams), standalone mode is untouched, and test coverage is solid.
Findings are inline. The two that most need a decision before merge: the PR description still says memory is off by default while the code defaults memory_enabled to on, and the absent-field default of memory_enabled=true makes every hybrid request against a pre-memory platform pay a wasted recall round-trip.
One doc gap that has no diff line to anchor on: docs/configuration.md has an otari.ai env-var table listing PLATFORM_RESOLVE_TIMEOUT_MS etc.; the new PLATFORM_MEMORY_RECALL_TIMEOUT_MS / PLATFORM_MEMORY_REMEMBER_TIMEOUT_MS (and OTARI_MEMORY_ENABLED) should be added there.
This review was created by Claude Code.
| default=True, | ||
| description="Enable the /v1/files upload/storage endpoints (standalone mode).", | ||
| ) | ||
| memory_enabled: bool = Field( |
There was a problem hiding this comment.
The PR description says memory is "off by default (OTARI_MEMORY_ENABLED)" with "no behavior change for existing deployments", but this defaults to True. The last commit moved to the platform-authoritative design; the description was never updated. Since PRs are squash-merged and the description drives review, please update it. Note the behavior-change claim is also no longer accurate for hybrid deployments against a platform without otari-ai#1148 (see the ResolvedRoute.memory_enabled comment).
| # the gateway uses this to decide whether to make the hot-path recall call at all. | ||
| # Defaults to True so a gateway pointed at an older platform (which omits the field) | ||
| # still attempts recall, relying on the platform's empty-when-off short-circuit. | ||
| memory_enabled: bool = True |
There was a problem hiding this comment.
Defaulting to True when the resolve response omits memory_enabled makes every hybrid chat completion against a pre-memory platform POST /gateway/memory/recall and get a 404: a wasted hot-path round-trip per request. The stated rationale (never silently stop recalling) only protects a platform that has the memory endpoints but omits the flag, and that platform can't exist: otari-ai#1148 ships the endpoints and the resolve field together, and its response schema always includes memory_enabled. Default to False on absence; it removes the per-request call with no downside. (Applies to the doc paragraph in docs/hybrid-mode-protocol.md too.)
| block = "\n".join([MEMORY_FACTS_HEADER, *(f"- {fact}" for fact in facts)]) | ||
| out = list(messages) | ||
| if out and isinstance(out[0], dict) and out[0].get("role") == "system": | ||
| existing = out[0].get("content") or "" |
There was a problem hiding this comment.
OpenAI allows system content as a list of content parts. When content is a list, f"{existing}\n\n{block}" stringifies the Python list repr into the system prompt. Inherited from inject_purpose_hints, but worth fixing here: guard with isinstance(existing, str) and otherwise insert a separate system message (or append a text part).
| if not platform_base_url or not query.strip(): | ||
| return [] | ||
|
|
||
| timeout_ms = _coerce_timeout_ms(config.platform.get("memory_recall_timeout_ms", 8000), 8000) |
There was a problem hiding this comment.
8s default on the time-to-first-token path is a lot for a best-effort feature: if the platform hangs (rather than failing fast), every request stalls up to 8s until it recovers, with no circuit breaker. The PR body promises memory never "noticeably slows" a chat; a ~2000ms default fits that much better. Operators who need more can raise PLATFORM_MEMORY_RECALL_TIMEOUT_MS.
| "PLATFORM_USAGE_MAX_RETRIES": ("usage_max_retries", int), | ||
| # Best-effort memory timeouts (platform mode). Recall is on the hot path | ||
| # (added to time-to-first-token); remember is fire-and-forget after the | ||
| # completion. Both fall back to their defaults on a missing or invalid |
There was a problem hiding this comment.
Inaccurate for env values: PLATFORM_MEMORY_RECALL_TIMEOUT_MS=abc crashes at startup via int(value) in the loop below, before _coerce_timeout_ms ever runs. The fallback only covers YAML-sourced values. Fail-fast on a bad env var matches the other PLATFORM_* ints, so just scope the comment to config-file values.
| # (added to time-to-first-token); remember is fire-and-forget after the | ||
| # completion. Both fall back to their defaults on a missing or invalid | ||
| # value (see _coerce_timeout_ms in routes/_platform.py). | ||
| "PLATFORM_MEMORY_RECALL_TIMEOUT_MS": ("memory_recall_timeout_ms", int), |
There was a problem hiding this comment.
docs/configuration.md documents the other PLATFORM_* variables in the otari.ai table; add PLATFORM_MEMORY_RECALL_TIMEOUT_MS and PLATFORM_MEMORY_REMEMBER_TIMEOUT_MS there, and document OTARI_MEMORY_ENABLED.
| headers={"Authorization": "Bearer user_test_token"}, | ||
| ) | ||
|
|
||
| reset_config() |
There was a problem hiding this comment.
reset_config()/reset_db() aren't in a try/finally, so if the request raises, the global config/db override leaks into later tests. This test (and the next one) also duplicates the platform_client fixture but with the legacy mode="platform" instead of the file's mode="hybrid". Extending platform_client to take config kwargs (or a small factory fixture with teardown after yield) fixes both.
| completed = True | ||
| finally: | ||
| if completed and parts: | ||
| task = asyncio.create_task(on_settled_text("".join(parts))) |
There was a problem hiding this comment.
asyncio holds only a weak reference to tasks, so this fire-and-forget write can be garbage-collected mid-flight; the done-callback doesn't prevent that. The existing _report_platform_usage calls share the pattern, so low priority, but the standard fix is one line: keep tasks in a module-level set and task.add_done_callback(_TASKS.discard).
| completed = False | ||
| try: | ||
| async for chunk in stream: | ||
| piece = extract_text(chunk) |
There was a problem hiding this comment.
In tool-loop streaming this accumulates content deltas across all tool iterations, so the remembered "assistant reply" concatenates intermediate assistant turns, while the non-streaming path stores only the final message's content. Probably tolerable since the platform's fact extraction is LLM-based, but the inconsistency is worth a deliberate decision (or a note here).
Description
Wires persistent memory into chat completions in platform mode: the gateway recalls relevant remembered context before the model answers and stores durable new facts afterward. It is off by default (
OTARI_MEMORY_ENABLED), best-effort, and covers both streaming and non-streaming completions.The point is that assistants built on the gateway feel continuous across sessions instead of stateless. The platform owns configuration, storage, privacy, and accounting (companion PR
mozilla-ai/otari-ai#1148); the gateway just brackets each completion with recall + remember.What's in it:
OTARI_MEMORY_ENABLEDmaster switch (default off → no behavior change for existing deployments).No memory behavior or added latency in standalone mode.
PR Type
Relevant issues
Closes #189
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test).uv run python scripts/generate_openapi.py). (No contract change: memory is internal, the spec is unchanged.)AI Usage
AI Model/Tool used: Claude Code (Claude Opus 4.8)
Any additional AI details you'd like to share:
The memory integration was implemented end-to-end with Claude Code; the human author directed the work and reviews the result.
Summary
memory_enabledflags (with operator override support).memory_enabled.Technical notes
/gateway/memory/recall, then recalled facts are injected as system “memory facts” content ahead of the user prompt./gateway/memory/rememberwrite using the latest user turn + assistant reply.PLATFORM_MEMORY_RECALL_TIMEOUT_MSandPLATFORM_MEMORY_REMEMBER_TIMEOUT_MSare parsed safely with defaults (recall defaulting to 8s; remember defaulting to 10s when malformed).