fix: memory search fails open instead of crashing chat completions#1288
fix: memory search fails open instead of crashing chat completions#1288aloktripathi1 wants to merge 2 commits into
Conversation
…tions
SupermemoryOpenAIWrapper wraps client.chat.completions.create(). With
default configuration, every call triggers a network request to
api.supermemory.ai via add_system_prompt() -> supermemory_profile_search().
That call chain had no error handling, so any Supermemory outage,
timeout, or network hiccup crashed the entire chat completion, even
though the underlying LLM call was completely unaffected.
The memory-write path (add_memory_tool) already failed open correctly
in this same file; the memory-search path (which runs on every call
by default) had no equivalent protection. The original test for this
scenario even documented the intended behavior in its own comment
('Should not raise exception, should fall back gracefully') while its
assertion checked the opposite.
Both async and sync paths now catch SupermemoryNetworkError and
SupermemoryAPIError (warn-level log) plus any other exception
(error-level log), and fall through to the original unmodified
messages instead of propagating the failure.
11 new tests covering both paths: expected Supermemory errors, an
unexpected exception, the happy-path regression (memory injection
still works normally), and black-box repros matching the filed issue.
35 passed, 11 skipped (pre-existing, gated on a live API key).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbaff205ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ): | ||
| """Exact repro: the real search path hits a connection error at the | ||
| HTTP layer, and ``create()`` must still return successfully.""" | ||
| import aiohttp |
There was a problem hiding this comment.
In the documented/default dev setup, aiohttp is only declared under the optional async extra in pyproject.toml, not the dev dependency group, so running the test suite after uv sync --dev can fail with ModuleNotFoundError before exercising the requests fallback that production supports. This black-box test should either use pytest.importorskip("aiohttp")/an aiohttp extra in dev dependencies, or structure the repro so the fallback path remains testable without the optional package.
Useful? React with 👍 / 👎.
…box tests
aiohttp is only declared under the optional `async` extra, not the dev
dependency group, so the black-box repros failed with ModuleNotFoundError
under the default `uv sync --dev` setup before exercising anything.
- Async repro now uses pytest.importorskip("aiohttp"), skipping cleanly
when the extra is absent (it exercises the aiohttp code path).
- Sync repro forces the requests fallback by making `import aiohttp` fail
via sys.modules, so it runs in the default dev setup and actually covers
the requests path production uses when aiohttp is not installed.
Verified both ways: with aiohttp -> 35 passed, 11 skipped; without aiohttp
-> 34 passed, 12 skipped (async repro skips, sync repro passes via requests).
1 participant
Closes #1287
SupermemoryOpenAIWrapper wraps client.chat.completions.create(). With
default configuration, every call triggers a network request to
api.supermemory.ai via add_system_prompt() -> supermemory_profile_search().
That call chain had no error handling, so any Supermemory outage, timeout,
or network hiccup crashed the entire chat completion, even though the
underlying LLM call was completely unaffected.
The memory-write path (add_memory_tool) already failed open correctly in
this same file. The memory-search path (which runs on every call by
default) had no equivalent protection. The pre-existing test for this
scenario actually documented the intended behavior in its own comment
("Should not raise exception, should fall back gracefully") while its
assertion checked the opposite -- I've fixed that test to match its own
stated intent.
Both async and sync paths now catch SupermemoryNetworkError and
SupermemoryAPIError (logged at warn, since these are expected failure
modes) plus any other exception (logged at error), and fall through to
the original unmodified messages instead of propagating the failure.
11 new tests: both expected Supermemory error types, an unexpected
exception, happy-path regression tests confirming memory injection is
completely unchanged when Supermemory succeeds, and black-box repros
matching the crash from the filed issue. 35 passed, 11 skipped
(pre-existing, gated on a live SUPERMEMORY_API_KEY, unrelated to this
change).