Conversation
![orca-security-eu[bot]](https://avatars.githubusercontent.com/in/276244?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Infrastructure as Code |  0  0  0  0 |
View in Orca |
| Passed |
SAST | 0 0 0 0 |
View in Orca |
| Passed |
Secrets | 0 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
src/engram/_base_client.py
Outdated
| raise AuthenticationError(detail, status_code=401, body=data) | ||
|
|
||
| if response.status_code == 404: | ||
| detail = _extract_detail(data, "Not found") | ||
| raise NotFoundError(detail, status_code=404, body=data) |
There was a problem hiding this comment.
Could probably make the status_code default in these errors since they should always be consistent
src/engram/_models.py
Outdated
| from dataclasses import dataclass, field | ||
| from typing import Literal | ||
|
|
||
| # ── Request models ────────────────────────────────────────────────────── |
There was a problem hiding this comment.
If we're needing to split this file up with comments like this, we should probably just split them into separate files now
src/engram/_serialization.py
Outdated
| SearchResults, | ||
| ) | ||
|
|
||
| # ── Body builders ─────────────────────────────────────────────────────── |
There was a problem hiding this comment.
Similar to before, these should be separate files not separated by comments
src/engram/async_client.py
Outdated
| self.memories = AsyncMemories(self._transport) | ||
| self.runs = AsyncRuns(self._transport) | ||
|
|
||
| def build_request( |
There was a problem hiding this comment.
Is this needed anywhere now (are we always using the client.subclient.method() structure)? It might be cleaner to remove this and enforce that each subclient has to own a reference to the transport
src/engram/async_client.py
Outdated
| async def aclose(self) -> None: | ||
| if self._owns_http_client: | ||
| await self._http_client.aclose() | ||
| await self._transport._http_client.aclose() |
There was a problem hiding this comment.
This is fine, but now that we have the HttpTransport classes, I wonder whether this check belongs in there? So, make AsyncHttpTransport take an init arg http_client: httpx.AsyncClient | None and keep the _owns_http_client abstracted away in there? This can just be a AsyncHttpTransport.close() method then (which is either no-op or ._http_client.aclose() depending on whether the transport owns the client or not)
src/engram/client.py
Outdated
| @@ -24,4 +23,4 @@ | |||
| *, | |||
| base_url: str = DEFAULT_BASE_URL, | |||
| api_key: str | None = None, | |||
There was a problem hiding this comment.
shouldn't api key be required?
| *, | ||
| base_url: str = DEFAULT_BASE_URL, | ||
| api_key: str | None = None, | ||
| headers: Mapping[str, str] | None = None, |
There was a problem hiding this comment.
what would users pass in headers that we expose them?
src/engram/client.py
Outdated
There was a problem hiding this comment.
we sure we wanna allow passing http client? Seems like implementation details of the client, and later it's harder to refactor if it can be passed by users
src/engram/_http.py
Outdated
| detail = _extract_detail(data, "Authentication failed") | ||
| raise AuthenticationError(detail, body=data) | ||
|
|
||
| if response.status_code == 404: |
There was a problem hiding this comment.
do we need this granular error handling, maybe just APIError for all so we don't need to maintain this?
There was a problem hiding this comment.
I think AuthenticationError feels okay as a distinct category of error, agree though that we don't want to go too granular beyond that.
|
|
||
| run_id: str | ||
| status: str | ||
| error: str | None = None |
There was a problem hiding this comment.
just to confirm, is run.error user-facing? If yes, then we should be more careful what we put there in server
There was a problem hiding this comment.
Yes, this will be the error that we surface to developer users.
3 participants
Implements the core Python SDK client for the Engram memory API, covering all five endpoints:
client.memories.add()— create memories from plain text, conversations, or pre-extracted contentclient.memories.get()/delete()— fetch or remove a specific memoryclient.memories.search()— vector, BM25, or hybrid search with configurable retrievalclient.runs.get()/wait()— check pipeline run status or poll until completionBoth sync (
EngramClient) and async (AsyncEngramClient) clients are supported with identical APIs. The content serialization matches the backend's type-discriminated envelope format ({type: "string", content: ...}).