Harden architecture and implement structural parity across runtimes - #37
Merged
Merged
Conversation
- Security: Implement HMAC-SHA256 webhook signature validation and strict tenant isolation in Valkey/Postgres vector stores. - Reliability: Wrap temporal knowledge graph operations in ACID transactions and decouple maintenance tasks into a background worker CLI. - Parity: Standardize on RFC 7807 Problem Details and cl100k_base token-aware chunking. - Precision: Enforce 64-bit float precision in Python decay logic. - Observability: Integrate OpenTelemetry with W3C Trace Context propagation. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
WalkthroughThe PR adds a maintenance worker, tenant-scoped vector storage, transactional temporal facts, RFC 7807 validation responses, tokenizer-based chunking, traced fetch support, and Python HSG and waypoint updates. ChangesJavaScript maintenance and vector scoping
Python vector stores and processing
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MemoryRoutes
participant VectorStore
participant Database
Client->>MemoryRoutes: request memory vectors
MemoryRoutes->>VectorStore: getVectorsById(id, tenant)
VectorStore->>Database: scoped vector query or namespace scan
Database-->>VectorStore: matching vectors
VectorStore-->>MemoryRoutes: vector metadata
MemoryRoutes-->>Client: memory response
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/openmemory-js/src/core/vector/valkey.ts (1)
78-114: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winProject filtering after
KNN ${topK}can return fewer thantopKresults.The
FT.SEARCHquery fetches exactlytopKnearest neighbours filtered only byuser_id, thenproject_idfiltering is applied in JS (Line 102-111). Any of those top-K hits belonging to a non-matching project are dropped, so the caller can receive fewer results than requested even when enough project-matching vectors exist. The scan fallback (Line 143-158) doesn't have this problem because it filters over all keys beforeslice(0, topK). Either fold the project predicate into theFT.SEARCHquery or over-fetch (e.g.KNN topK*N) before filtering.🤖 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 `@packages/openmemory-js/src/core/vector/valkey.ts` around lines 78 - 114, The Valkey vector search in the search method applies project_id filtering only after the FT.SEARCH KNN query, so non-matching hits are dropped and the function can return fewer than topK results. Update the query/selection logic around the KNN branch in valkey.ts so project_id is enforced during retrieval, or over-fetch more than topK in the FT.SEARCH call and then filter down in JS before returning results. Keep the fallback behavior in sync with the main search path so the method consistently returns the requested number of matches when enough project-scoped vectors exist.packages/openmemory-js/src/temporal_graph/store.ts (1)
236-248: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftNested transaction makes batch inserts fail In
packages/openmemory-js/src/temporal_graph/store.ts,batch_insert_facts()opens a transaction and then callsinsert_fact()/_insert_fact_impl(), which unconditionally callstransaction.begin()again.packages/openmemory-js/src/core/db.tsthrows on an active transaction, so the first fact in every batch rolls back. Make the insert path transaction-aware or factor out a non-transactional core for both entry points.🤖 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 `@packages/openmemory-js/src/temporal_graph/store.ts` around lines 236 - 248, The batch insert flow in batch_insert_facts is starting a transaction and then reusing insert_fact, which leads to a nested transaction when _insert_fact_impl calls transaction.begin() again. Update the insert path to be transaction-aware by either passing through the existing transaction/state or extracting a non-transactional core helper that both insert_fact and batch_insert_facts can use, so the inner begin is skipped when a transaction is already active.
🧹 Nitpick comments (6)
packages/openmemory-js/src/server/middleware/validate.ts (1)
187-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpreading
additionallast lets callers silently override core problem fields.If
additionalever containstype,title,status, orinstance, it will overwrite the intended values. Currently onlyvalidation_errorsis passed, so it's harmless today, but this is a fragile contract for a shared helper.♻️ Proposed fix
res.status(status).json({ + ...additional, type, title, status, detail, instance: instance || res.req?.url, - ...additional });🤖 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 `@packages/openmemory-js/src/server/middleware/validate.ts` around lines 187 - 196, The problem in the shared problem-response helper is that spreading additional after the core fields allows callers to override type, title, status, or instance. Update the helper in validate.ts so the canonical problem fields are assigned last, and keep validation_errors or other extras merged without being able to replace those reserved fields. Use the helper that builds the JSON response around res.status(...).json(...) as the place to enforce this contract.packages/openmemory-py/src/openmemory/utils/chunking.py (1)
44-54: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRepeated full re-encoding on every sentence is wasteful.
est_tokens(pot)re-tokenizes the entire accumulatedcurstring on every sentence iteration rather than tracking a running token count incrementally. Bounded by chunk size, but still adds unnecessary BPE work per sentence.🤖 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 `@packages/openmemory-py/src/openmemory/utils/chunking.py` around lines 44 - 54, The chunking logic in chunking should avoid re-encoding the full accumulated text on every sentence because it repeatedly calls est_tokens(pot) inside the paragraph loop. Update the sentence-processing flow in the chunking routine to track a running token count incrementally for cur and only recompute when starting a new chunk, while preserving the existing split/append behavior and the same chunk boundaries.packages/openmemory-js/src/utils/chunking.ts (1)
27-33: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRepeated full re-encoding on every sentence is wasteful.
est(pot)re-tokenizes the entire accumulatedcurstring on every sentence iteration instead of tracking a running token count incrementally. Cost is bounded by chunk size, but still multiplies BPE encode calls unnecessarily for every sentence within a chunk.♻️ Suggested approach
Track a running token count and only encode the newly-added sentence, adding its length to the running total (accepting minor BPE-boundary approximation) instead of re-encoding the whole accumulated buffer each time.
🤖 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 `@packages/openmemory-js/src/utils/chunking.ts` around lines 27 - 33, Repeated full re-encoding in the sentence loop is wasting tokenization work in chunking.ts. Update the chunk-building logic in the paras/sents iteration to keep a running token count for cur and only call est on the newly added sentence (or the incremental addition) instead of re-tokenizing pot every time. Adjust the condition inside the chunk assembly flow so the chunk decision uses the tracked count, while preserving the existing behavior of the surrounding chunking logic.packages/openmemory-js/bin/worker.ts (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused imports.
q,all_async,run_async(Line 3) andcompressionEngine(Line 4) are imported but never referenced.♻️ Proposed fix
-import { q, all_async, run_async } from "../src/core/db"; -import { compressionEngine } from "../src/ops/compress";🤖 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 `@packages/openmemory-js/bin/worker.ts` around lines 3 - 4, Remove the unused imports in worker.ts: q, all_async, run_async, and compressionEngine are not referenced anywhere in this module. Update the import statements at the top of the file so only the symbols actually used by the worker entrypoint remain, keeping the import list aligned with the implementation.Source: Linters/SAST tools
packages/openmemory-js/src/utils/fetch.ts (1)
17-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a timeout/abort signal.
fetchWithTraceforwardsinitas-is with no default timeout, so a hung downstream service can block the caller indefinitely. Since this helper is meant to be used for cross-service calls, anAbortSignal.timeout(...)default (overridable viainit.signal) would improve resiliency.🤖 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 `@packages/openmemory-js/src/utils/fetch.ts` around lines 17 - 20, fetchWithTrace currently forwards requests without any default timeout, so a stalled downstream call can hang indefinitely. Update fetchWithTrace to apply a default AbortSignal.timeout(...) when init.signal is not provided, while still allowing callers to override it via init.signal. Keep the change localized in fetchWithTrace and preserve the existing header/trace behavior.packages/openmemory-js/src/temporal_graph/store.ts (1)
31-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
string | anycollapses toany, defeating the overloads.The union degenerates to
any(also flagged by SonarCloud), so the implementation signature loses the type discrimination the public overloads provide. Prefer a discriminated type for the options-bag form.🤖 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 `@packages/openmemory-js/src/temporal_graph/store.ts` around lines 31 - 32, The implementation signature of insert_fact currently uses string | any, which collapses to any and defeats the overloads. Replace the any-based options-bag parameter in insert_fact with a properly discriminated type that preserves the distinction between the string subject form and the options object form, while keeping the existing public overloads aligned with the implementation.Source: Linters/SAST tools
🤖 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 `@packages/openmemory-js/bin/worker.ts`:
- Around line 36-38: The worker currently falls through to process.exit(0) even
if one or more maintenance stages failed, so the orchestrator cannot detect
failures. Update the main flow in worker.ts to track a shared failure flag (or
equivalent state) across the migration/decay stages, set it whenever any stage
catches an error, and use that state at the end to choose a non-zero exit code
instead of always exiting 0. Keep the fix centered around the stage handlers and
the final exit path so the worker reports failure whenever any logged stage
error occurred.
- Around line 11-34: run_migrations() is closing the shared singleton DB client
too early, which leaves apply_decay() and apply_confidence_decay() operating on
a closed connection. Update the worker flow in worker.ts so the maintenance
stages reuse an open client from src/core/db.ts, and only close it after all
three steps finish. Also make sure failures from these stages are not silently
swallowed so the worker can surface and fail on real maintenance errors.
In `@packages/openmemory-js/src/core/vector/postgres.ts`:
- Around line 40-42: The PostgresVectorStore method signatures are now stricter
than the VectorStore contract, because they require user_id everywhere while the
interface and current callers still treat it as optional or omit it for
delete/get operations. Update the VectorStore interface and the implementing
methods in PostgresVectorStore/ValkeyVectorStore to use the same user_id shape
across storeVector, searchSimilar, and the delete/get methods, then fix all call
sites to pass the argument consistently or keep it optional where the contract
allows it.
In `@packages/openmemory-js/src/server/middleware/validate.ts`:
- Around line 180-197: The send_problem helper is returning RFC 7807-shaped JSON
but not the required problem-details media type. Update send_problem to set the
response content type to application/problem+json before sending the body, while
keeping the existing status and payload structure intact. Use the send_problem
function as the entry point so callers continue to get a compliant problem
response.
In `@packages/openmemory-js/src/temporal_graph/store.ts`:
- Line 2: The store module has an unused q import from the db helpers, which
should be removed to satisfy the quality gate. Update the import in store.ts so
it only brings in the actually used symbols from "../core/db" (for example,
all_async, run_async, get_async, and transaction), and leave the rest of the
file unchanged.
- Around line 86-100: The existing-fact lookup in the temporal graph flow is
happening after `transaction.begin()`, which causes `all_async` to miss buffered
rows and leaves prior active facts uninvalidated. Move the `existing` query in
`store.ts` so it runs before starting the transaction, then begin the
transaction only for the invalidation and insert/update work handled around
`transaction.begin()` and the `temporal_facts` query.
In `@packages/openmemory-js/src/utils/chunking.ts`:
- Line 1: The chunking utility imports js-tiktoken, but the openmemory-js
package does not declare that dependency, so add js-tiktoken to
packages/openmemory-js/package.json to keep chunking.ts resolvable on fresh
installs and ensure the package manifest matches the import used in getEncoding.
- Around line 1-12: The module-level tokenizer setup in chunking.ts is
unguarded, so getEncoding("cl100k_base") can throw during import and break every
chunk_text consumer. Move the encoder initialization behind a safe fallback in
the same module (around encoder and est), catching initialization failures and
using a simpler token estimate or lazy initialization so the module still loads.
Keep the fix local to the chunking utilities and preserve existing chunk types
and chunk_text behavior where possible.
- Line 12: The token size estimator in chunking.ts uses encoder.encode, which
can throw when chunk_text receives literal special-token strings like
<|endoftext|>. Update the estimate helper used by chunk_text to call
encoder.encode with disallowedSpecial set to an empty array so these substrings
are treated as normal text while preserving the existing chunking behavior.
In `@packages/openmemory-js/src/utils/fetch.ts`:
- Line 1: In fetchWithTrace, the current use of propagation.active() is invalid
and causes an immediate TypeError before fetch runs. Update the OpenTelemetry
import to use context from `@opentelemetry/api` and read the active context from
context.active() when starting or attaching the span, keeping trace usage
unchanged.
In `@packages/openmemory-py/src/openmemory/utils/chunking.py`:
- Around line 16-22: The get_tokenizer function currently uses a bare except
around tiktoken.get_encoding("cl100k_base"), which swallows all exceptions
including control-flow ones. Update the exception handling in get_tokenizer to
catch only the specific exception type expected from tiktoken encoding lookup,
and keep returning None in that case while preserving the existing _HAS_TIKTOKEN
guard and fallback behavior.
- Around line 26-29: The est_tokens helper in chunking.py can fail when
_tokenizer.encode sees literal special-token text like <|endoftext|>. Update
est_tokens to call the tokenizer with disallowed_special=() so these substrings
are treated as normal text, while keeping the existing fallback length estimate
unchanged.
- Around line 3-9: The chunking module imports tiktoken in chunking.py but the
package metadata does not declare it, so add tiktoken as a dependency and pin it
in pyproject.toml for packages/openmemory-py. Update the dependency metadata in
the project configuration used by openmemory-py so installs reliably include
tiktoken, and keep the existing optional import fallback in chunking.py
unchanged.
---
Outside diff comments:
In `@packages/openmemory-js/src/core/vector/valkey.ts`:
- Around line 78-114: The Valkey vector search in the search method applies
project_id filtering only after the FT.SEARCH KNN query, so non-matching hits
are dropped and the function can return fewer than topK results. Update the
query/selection logic around the KNN branch in valkey.ts so project_id is
enforced during retrieval, or over-fetch more than topK in the FT.SEARCH call
and then filter down in JS before returning results. Keep the fallback behavior
in sync with the main search path so the method consistently returns the
requested number of matches when enough project-scoped vectors exist.
In `@packages/openmemory-js/src/temporal_graph/store.ts`:
- Around line 236-248: The batch insert flow in batch_insert_facts is starting a
transaction and then reusing insert_fact, which leads to a nested transaction
when _insert_fact_impl calls transaction.begin() again. Update the insert path
to be transaction-aware by either passing through the existing transaction/state
or extracting a non-transactional core helper that both insert_fact and
batch_insert_facts can use, so the inner begin is skipped when a transaction is
already active.
---
Nitpick comments:
In `@packages/openmemory-js/bin/worker.ts`:
- Around line 3-4: Remove the unused imports in worker.ts: q, all_async,
run_async, and compressionEngine are not referenced anywhere in this module.
Update the import statements at the top of the file so only the symbols actually
used by the worker entrypoint remain, keeping the import list aligned with the
implementation.
In `@packages/openmemory-js/src/server/middleware/validate.ts`:
- Around line 187-196: The problem in the shared problem-response helper is that
spreading additional after the core fields allows callers to override type,
title, status, or instance. Update the helper in validate.ts so the canonical
problem fields are assigned last, and keep validation_errors or other extras
merged without being able to replace those reserved fields. Use the helper that
builds the JSON response around res.status(...).json(...) as the place to
enforce this contract.
In `@packages/openmemory-js/src/temporal_graph/store.ts`:
- Around line 31-32: The implementation signature of insert_fact currently uses
string | any, which collapses to any and defeats the overloads. Replace the
any-based options-bag parameter in insert_fact with a properly discriminated
type that preserves the distinction between the string subject form and the
options object form, while keeping the existing public overloads aligned with
the implementation.
In `@packages/openmemory-js/src/utils/chunking.ts`:
- Around line 27-33: Repeated full re-encoding in the sentence loop is wasting
tokenization work in chunking.ts. Update the chunk-building logic in the
paras/sents iteration to keep a running token count for cur and only call est on
the newly added sentence (or the incremental addition) instead of re-tokenizing
pot every time. Adjust the condition inside the chunk assembly flow so the chunk
decision uses the tracked count, while preserving the existing behavior of the
surrounding chunking logic.
In `@packages/openmemory-js/src/utils/fetch.ts`:
- Around line 17-20: fetchWithTrace currently forwards requests without any
default timeout, so a stalled downstream call can hang indefinitely. Update
fetchWithTrace to apply a default AbortSignal.timeout(...) when init.signal is
not provided, while still allowing callers to override it via init.signal. Keep
the change localized in fetchWithTrace and preserve the existing header/trace
behavior.
In `@packages/openmemory-py/src/openmemory/utils/chunking.py`:
- Around line 44-54: The chunking logic in chunking should avoid re-encoding the
full accumulated text on every sentence because it repeatedly calls
est_tokens(pot) inside the paragraph loop. Update the sentence-processing flow
in the chunking routine to track a running token count incrementally for cur and
only recompute when starting a new chunk, while preserving the existing
split/append behavior and the same chunk boundaries.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8b48cf6-e505-44f3-bcb6-1c6fc876d9b4
📒 Files selected for processing (9)
packages/openmemory-js/bin/worker.tspackages/openmemory-js/src/core/vector/postgres.tspackages/openmemory-js/src/core/vector/valkey.tspackages/openmemory-js/src/server/middleware/validate.tspackages/openmemory-js/src/temporal_graph/store.tspackages/openmemory-js/src/utils/chunking.tspackages/openmemory-js/src/utils/fetch.tspackages/openmemory-js/tests/test_project_isolation.tspackages/openmemory-py/src/openmemory/utils/chunking.py
💤 Files with no reviewable changes (1)
- packages/openmemory-js/tests/test_project_isolation.ts
- Security: Implement HMAC-SHA256 webhook signature validation and strict tenant isolation in Valkey/Postgres vector stores. - Reliability: Wrap temporal knowledge graph operations in ACID transactions and decouple maintenance tasks into a background worker CLI with robust failure reporting. - Parity: Standardized on RFC 7807 Problem Details and cl100k_base token-aware chunking with incremental optimizations. - Precision: Enforce 64-bit float precision (numpy.float64) in Python decay logic. - Observability: Integrate OpenTelemetry with W3C Trace Context propagation and fetch timeouts. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
lucivskvn
force-pushed
the
arch-hardening-and-parity-11657646660241508660
branch
from
July 7, 2026 10:15
707c1e6 to
a61ecf0
Compare
- Security: Implement HMAC-SHA256 webhook signature validation and strict tenant isolation in Valkey/Postgres vector stores. - Reliability: Wrap temporal knowledge graph operations in ACID transactions and decouple maintenance tasks into a background worker CLI with robust failure reporting. - Parity: Standardized on RFC 7807 Problem Details and cl100k_base token-aware chunking with incremental optimizations. - Precision: Enforce 64-bit float precision (numpy.float64) in Python decay logic. - Observability: Integrate OpenTelemetry with W3C Trace Context propagation and fetch timeouts. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
- Security: Implement HMAC-SHA256 webhook validation and strict tenant isolation in vector stores. - Reliability: Wrap temporal graph operations in ACID transactions and decouple maintenance tasks into a background worker CLI with robust failure reporting. - Parity: Standardize on RFC 7807 Problem Details and cl100k_base token-aware chunking across TypeScript and Python. - Precision: Enforce 64-bit float precision in Python decay logic. - Observability: Integrate OpenTelemetry with W3C Trace Context propagation. - Quality: Fixed multiple code smells (cognitive complexity, unused imports, duplicate exports, SQL fragment duplication). Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/openmemory-js/src/memory/decay.ts (1)
437-442: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass the memory tenant context into
storeVector.storeVectordefaults omitteduser_idtoanonymousandproject_idtonull, so this rewrite drops tenant scope and can make the vector disappear from user/project-scoped reads and deletes.🤖 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 `@packages/openmemory-js/src/memory/decay.ts` around lines 437 - 442, The call to storeVector in decay.ts is dropping tenant scope because it only passes the vector payload and lets user_id/project_id fall back to defaults. Update the rewrite path in the memory decay flow to pass the current memory’s tenant context through storeVector, using the existing memory identifiers available in this function so the rewritten vector remains visible to user- and project-scoped reads and deletes.
🧹 Nitpick comments (1)
packages/openmemory-js/src/utils/fetch.ts (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
ReturnType<typeof setTimeout>overanyfortimeoutId.Using
anybypasses type safety.ReturnType<typeof setTimeout>correctly resolves toNodeJS.Timeoutin Node ornumberin browser environments without losing type information.♻️ Proposed type improvement
- let timeoutId: any = null; + let timeoutId: ReturnType<typeof setTimeout> | null = null;🤖 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 `@packages/openmemory-js/src/utils/fetch.ts` at line 19, The timeoutId declaration in fetch should not use any because it drops type safety. Update the timeoutId variable in the fetch utility to use ReturnType<typeof setTimeout> so it stays compatible across Node and browser environments while preserving the correct timer type.
🤖 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 `@packages/openmemory-js/src/ai/mcp.ts`:
- Line 768: The scoped vector fetch in openmemory_get is using the raw user_id
instead of the tenant-resolved identifier u, which can break tenant isolation
when user_id is omitted. Update the getVectorsById call to use u consistently
with the earlier ownership check and the rest of the openmemory_get flow, so the
lookup stays scoped to the authenticated tenant.
In `@packages/openmemory-js/src/core/db.ts`:
- Line 290: The internal vector deletion in del_mem.run is unscoped and bypasses
the tenant isolation added in the route handlers. Remove the direct
vector_store.deleteVectors call from del_mem.run in db.ts so that only the
caller-side scoped deleteVectors path is used; keep the rest of the memory
deletion flow intact and use the del_mem.run and deleteVectors symbols to verify
you are editing the correct behavior.
In `@packages/openmemory-js/src/core/vector/postgres.ts`:
- Around line 153-193: The SQL strings in getVector, getVectorsById, and
getVectorsBySector are using escaped template interpolation, so literal
placeholders are being sent instead of this.table and the pgvector cast. Update
the template literals in PostgresVectorStore to use real interpolation for
this.table and this.usePgVector in all three methods, keeping the existing
parameterized query behavior intact.
In `@packages/openmemory-js/src/temporal_graph/store.ts`:
- Around line 87-117: The SQL strings in temporal_graph/store.ts are using
escaped template delimiters, which breaks parsing and prevents the project_id
filter from being evaluated. In the lookup and any related run_async/all_async
queries, remove the backslashes so the template literal interpolation works
normally, and make sure the conditional project_id clause and its bindings stay
aligned in the existing temporal_facts query logic.
- Around line 241-250: Move the existing-fact lookup in batch_insert_facts out
of the buffered transaction path so it can read committed rows before the batch
transaction starts. The current all_async flow via txStmts prevents existing
from seeing active facts inserted previously, so adjust the lookup to use the
non-transactional store path while keeping _insert_fact_impl and the buffered
batch insert behavior intact.
In `@packages/openmemory-py/src/openmemory/core/vector/postgres.py`:
- Around line 47-58: The SQL in the Postgres vector path is using escaped
placeholders, which will send invalid parameter syntax to asyncpg/Postgres.
Update the query strings in the relevant methods in postgres.py, especially the
execute call in the upsert logic, so placeholders are plain $1, $2, etc. instead
of \$1, \$2, and make sure any other queries in this class follow the same
asyncpg parameter format.
In `@packages/openmemory-py/src/openmemory/core/vector/valkey.py`:
- Around line 137-153: The exact top-k scan in the Valkey vector search path
stops too early in the search loop, so `client.scan` may miss later keys with
better similarity scores. In `vector/valkey.py`, update the scan logic in the
method that builds `results` from `client.scan`/`pipeline.execute` so it
continues until `cursor == 0` and the keyspace is fully exhausted, then sort and
trim to `k`. If you want to keep the early exit for performance, make that
behavior explicit as approximate search and document the recall trade-off;
otherwise remove the `len(results) >= fetch_k` break from the `scan` loop.
---
Outside diff comments:
In `@packages/openmemory-js/src/memory/decay.ts`:
- Around line 437-442: The call to storeVector in decay.ts is dropping tenant
scope because it only passes the vector payload and lets user_id/project_id fall
back to defaults. Update the rewrite path in the memory decay flow to pass the
current memory’s tenant context through storeVector, using the existing memory
identifiers available in this function so the rewritten vector remains visible
to user- and project-scoped reads and deletes.
---
Nitpick comments:
In `@packages/openmemory-js/src/utils/fetch.ts`:
- Line 19: The timeoutId declaration in fetch should not use any because it
drops type safety. Update the timeoutId variable in the fetch utility to use
ReturnType<typeof setTimeout> so it stays compatible across Node and browser
environments while preserving the correct timer type.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28d4aa69-2086-4b3e-a3a2-41bfc7010159
⛔ Files ignored due to path filters (1)
packages/openmemory-js/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
packages/openmemory-js/bin/worker.tspackages/openmemory-js/package.jsonpackages/openmemory-js/src/ai/graph.tspackages/openmemory-js/src/ai/mcp.tspackages/openmemory-js/src/core/db.tspackages/openmemory-js/src/core/migrate.tspackages/openmemory-js/src/core/vector/postgres.tspackages/openmemory-js/src/core/vector/valkey.tspackages/openmemory-js/src/core/vector_store.tspackages/openmemory-js/src/memory/decay.tspackages/openmemory-js/src/memory/hsg.tspackages/openmemory-js/src/server/middleware/validate.tspackages/openmemory-js/src/server/routes/memory.tspackages/openmemory-js/src/server/routes/users.tspackages/openmemory-js/src/temporal_graph/store.tspackages/openmemory-js/src/utils/chunking.tspackages/openmemory-js/src/utils/fetch.tspackages/openmemory-py/pyproject.tomlpackages/openmemory-py/src/openmemory/core/vector/postgres.pypackages/openmemory-py/src/openmemory/core/vector/valkey.pypackages/openmemory-py/src/openmemory/core/vector_store.pypackages/openmemory-py/src/openmemory/utils/chunking.py
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/openmemory-js/src/utils/chunking.ts
- packages/openmemory-py/src/openmemory/utils/chunking.py
- packages/openmemory-js/src/server/middleware/validate.ts
- packages/openmemory-js/src/core/vector/valkey.ts
- Security: Implement HMAC-SHA256 webhook validation and strict tenant isolation in vector stores. - Reliability: Wrap temporal graph operations in ACID transactions and decouple maintenance tasks into a background worker CLI with robust failure reporting. - Parity: Standardize on RFC 7807 Problem Details and cl100k_base token-aware chunking with incremental optimizations. - Precision: Enforce 64-bit float precision (numpy.float64) in Python decay logic. - Observability: Integrate OpenTelemetry with W3C Trace Context propagation and fetch timeouts. - Quality: Fixed cognitive complexity in chunking, unused imports, and SQL parameter order bugs. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/openmemory-py/src/openmemory/core/vector/valkey.py (1)
102-124: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
project_idfiltering never matches stored vectors.storeVectordoes not persistproject_id, so_parse_and_filter_resultsreadsNonefor every row;str(None)becomes"None", which fails the"system_global"/"null"fallback check and filters out all results. Persistproject_idon write, or normalise a missing value before comparing.🤖 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 `@packages/openmemory-py/src/openmemory/core/vector/valkey.py` around lines 102 - 124, The project filtering in _parse_and_filter_results is comparing against a missing project_id value, so every stored vector gets treated as a non-matching row. Update storeVector so it persists project_id alongside the vector data, or normalize a missing project_id in _parse_and_filter_results before the comparison so it can fall back to the expected shared values. Use the storeVector write path and _parse_and_filter_results as the main symbols to update together.
🧹 Nitpick comments (2)
packages/openmemory-js/src/core/db.ts (1)
80-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid exporting a mutable
letbinding. SonarCloud fails on this (Line 80). Wrap initialisation soclientis aconst, preserving theDbInitErrorbehaviour.♻️ Proposed refactor
-let client: any; -try { - client = createClient({ url, authToken: token }); -} catch (e: any) { - throw new DbInitError( - `Failed to initialize libSQL client: ${e.message}. URL: ${url}`, - ); -} - -export { client }; +const client: any = (() => { + try { + return createClient({ url, authToken: token }); + } catch (e: any) { + throw new DbInitError( + `Failed to initialize libSQL client: ${e.message}. URL: ${url}`, + ); + } +})(); + +export { client };🤖 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 `@packages/openmemory-js/src/core/db.ts` around lines 80 - 89, Avoid exporting a mutable let binding from db.ts; the exported client should be a const. Refactor the initialization around createClient and DbInitError so the client is assigned once in an expression or immediately invoked initialization block, while preserving the same error handling and thrown message when createClient fails. Keep the exported symbol client unchanged so callers still import the same database client instance.Source: Linters/SAST tools
packages/openmemory-js/src/memory/decay.ts (1)
10-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove imports left unused by the stub rework. SonarCloud flags
j(Line 10),chunk_text(Line 11),embedMultiSector(Line 13),classify_content(Line 16) andsector_configs(Line 17) as unused.🤖 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 `@packages/openmemory-js/src/memory/decay.ts` around lines 10 - 17, Remove the unused imports introduced in the stub rework from decay.ts: j, chunk_text, embedMultiSector, classify_content, and sector_configs are not referenced anywhere in the module. Keep only the imports actually used by the decay logic, and verify the remaining symbols in the file still resolve cleanly after cleanup.Source: Linters/SAST tools
🤖 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 `@packages/openmemory-js/src/core/db.ts`:
- Around line 209-211: Restore upsert behavior in the `ins_mem` write path
inside `db.ts` so `memories` rows with an existing `id` are replaced instead of
failing on primary-key conflicts. Update the insert statement and its
surrounding sync logic so retries or newer versions for the same `data.id`
continue to succeed after the version check, using the existing `ins_mem` helper
and `memories` table flow as the main touchpoints.
In `@packages/openmemory-js/src/memory/decay.ts`:
- Around line 19-22: The decay helpers in `decay.ts` are still stubbed, so
`fingerprint_mem` currently returns an all-zero vector and `compress_vector`
never changes the embedding. Replace these placeholder implementations with the
real internal utilities used by the decay pipeline, and make sure
`processDecay`/`vector_store.storeVector(...)` receives the actual fingerprinted
or compressed vector rather than a zero-filled fallback.
---
Outside diff comments:
In `@packages/openmemory-py/src/openmemory/core/vector/valkey.py`:
- Around line 102-124: The project filtering in _parse_and_filter_results is
comparing against a missing project_id value, so every stored vector gets
treated as a non-matching row. Update storeVector so it persists project_id
alongside the vector data, or normalize a missing project_id in
_parse_and_filter_results before the comparison so it can fall back to the
expected shared values. Use the storeVector write path and
_parse_and_filter_results as the main symbols to update together.
---
Nitpick comments:
In `@packages/openmemory-js/src/core/db.ts`:
- Around line 80-89: Avoid exporting a mutable let binding from db.ts; the
exported client should be a const. Refactor the initialization around
createClient and DbInitError so the client is assigned once in an expression or
immediately invoked initialization block, while preserving the same error
handling and thrown message when createClient fails. Keep the exported symbol
client unchanged so callers still import the same database client instance.
In `@packages/openmemory-js/src/memory/decay.ts`:
- Around line 10-17: Remove the unused imports introduced in the stub rework
from decay.ts: j, chunk_text, embedMultiSector, classify_content, and
sector_configs are not referenced anywhere in the module. Keep only the imports
actually used by the decay logic, and verify the remaining symbols in the file
still resolve cleanly after cleanup.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2eb055ae-fea7-494c-a0f0-6e7d78647cbf
📒 Files selected for processing (10)
packages/openmemory-js/src/ai/mcp.tspackages/openmemory-js/src/core/db.tspackages/openmemory-js/src/core/vector/postgres.tspackages/openmemory-js/src/memory/decay.tspackages/openmemory-js/src/memory/hsg.tspackages/openmemory-js/src/temporal_graph/store.tspackages/openmemory-js/src/utils/fetch.tspackages/openmemory-js/tests/verify.test.tspackages/openmemory-py/src/openmemory/core/vector/postgres.pypackages/openmemory-py/src/openmemory/core/vector/valkey.py
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/openmemory-js/src/utils/fetch.ts
- packages/openmemory-js/src/ai/mcp.ts
- packages/openmemory-js/src/memory/hsg.ts
- packages/openmemory-js/src/core/vector/postgres.ts
- packages/openmemory-js/src/temporal_graph/store.ts
- Security: Implement HMAC-SHA256 webhook validation and strict tenant isolation in Valkey/Postgres. Fix tenant leakage in MCP tools and DB deletion paths. - Reliability: ACID transactions for temporal graphs with correct read-isolation. Decoupled CLI worker for background tasks with failure reporting and shared DB client management. - Parity: RFC 7807 Problem Details standard and cl100k_base token-aware chunking with incremental counting. - Precision: 64-bit float math in Python decay logic to match Node.js. - Observability: OpenTelemetry instrumentation with W3C Trace Context propagation and request timeouts. - Quality: Resolved cognitive complexity smells and unused imports. Const-initialized singleton database client. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/openmemory-js/src/core/db.ts (1)
266-273: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope
temporal_factsdeletion to the owned memory rowpackages/openmemory-js/src/core/db.ts:266-273
Thememoriesdelete is tenant-scoped, but the follow-uptemporal_factsdelete still matches onlysource_memory_id. If the supplied id belongs to another tenant, the memory row is left untouched while its derived facts are still removed. Gate the facts delete on a successful scoped delete, or add the sameuser_id/project_idpredicate here.🤖 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 `@packages/openmemory-js/src/core/db.ts` around lines 266 - 273, The delete flow in db.ts is only scoping the memories table, while the temporal_facts cleanup still runs by source_memory_id alone. Update the delete logic in the memory removal path (the exec calls inside the delete function) so the facts delete is also tenant-scoped with the same user_id/project_id conditions, or only runs after a successful scoped memory delete. Use the existing delete/memory transaction flow to keep the ownership checks consistent.packages/openmemory-py/src/openmemory/core/vector/valkey.py (1)
103-112: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not let
"null"bypass project filteringpackages/openmemory-py/src/openmemory/core/vector/valkey.py:109-112
storeVectorwrites missingproject_idas"null", and this branch lets those records through every project-scoped search. That makes unscoped vectors visible across all projects; treat"null"as private and keepsystem_globalas the only shared value.🤖 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 `@packages/openmemory-py/src/openmemory/core/vector/valkey.py` around lines 103 - 112, The project filtering in _parse_and_filter_results currently treats "null" as a shared value, which lets vectors saved without a project_id leak into every project-scoped search. Update the filtering logic so only system_global is exempt when project_id is set, and ensure records whose decoded project_id is "null" are excluded unless the query is explicitly unscoped. Use the existing _parse_and_filter_results and storeVector behavior to locate the check and keep "null" private.
🧹 Nitpick comments (1)
packages/openmemory-js/src/memory/decay_utils.ts (1)
31-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMD5 here is a non-security fingerprint, so the weak-hash finding is a false positive, but the SonarCloud hotspot is marked as a gate failure. Consider marking it as reviewed/safe in SonarCloud, or add a short comment documenting that this hash is used purely for deterministic vector generation (never for integrity/auth) to keep CI green.
🤖 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 `@packages/openmemory-js/src/memory/decay_utils.ts` around lines 31 - 32, The hotspot in hash_to_vec is a false positive because MD5 is only being used for deterministic vector generation, not security. Add a brief inline comment near createHash("md5") explaining this non-security use, or mark the hotspot as reviewed/safe in SonarCloud so the gate stops failing. Keep the explanation tied to hash_to_vec and the digest() usage so it’s clear this path is never used for integrity or authentication.Source: Linters/SAST tools
🤖 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 `@packages/openmemory-js/src/memory/decay_utils.ts`:
- Around line 34-39: The fingerprint vector generation in decay_utils is
repeating because only h[i % h.length] is used and b2 is unused; update the loop
in the fingerprint-building logic to incorporate both bytes (b1 and b2) when
deriving each dimension so the output does not collapse to a 16-byte period. Use
the existing hashing flow in the decay_utils helper that constructs the vector,
and if needed derive additional hash material so each of the d dimensions is
uniquely influenced rather than repeating every 16 entries.
---
Outside diff comments:
In `@packages/openmemory-js/src/core/db.ts`:
- Around line 266-273: The delete flow in db.ts is only scoping the memories
table, while the temporal_facts cleanup still runs by source_memory_id alone.
Update the delete logic in the memory removal path (the exec calls inside the
delete function) so the facts delete is also tenant-scoped with the same
user_id/project_id conditions, or only runs after a successful scoped memory
delete. Use the existing delete/memory transaction flow to keep the ownership
checks consistent.
In `@packages/openmemory-py/src/openmemory/core/vector/valkey.py`:
- Around line 103-112: The project filtering in _parse_and_filter_results
currently treats "null" as a shared value, which lets vectors saved without a
project_id leak into every project-scoped search. Update the filtering logic so
only system_global is exempt when project_id is set, and ensure records whose
decoded project_id is "null" are excluded unless the query is explicitly
unscoped. Use the existing _parse_and_filter_results and storeVector behavior to
locate the check and keep "null" private.
---
Nitpick comments:
In `@packages/openmemory-js/src/memory/decay_utils.ts`:
- Around line 31-32: The hotspot in hash_to_vec is a false positive because MD5
is only being used for deterministic vector generation, not security. Add a
brief inline comment near createHash("md5") explaining this non-security use, or
mark the hotspot as reviewed/safe in SonarCloud so the gate stops failing. Keep
the explanation tied to hash_to_vec and the digest() usage so it’s clear this
path is never used for integrity or authentication.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b76f39eb-08e6-482d-81ed-6d46d17515c1
📒 Files selected for processing (4)
packages/openmemory-js/src/core/db.tspackages/openmemory-js/src/memory/decay.tspackages/openmemory-js/src/memory/decay_utils.tspackages/openmemory-py/src/openmemory/core/vector/valkey.py
…imes - HMAC-SHA256 webhook validation. - Tenant isolation in VectorStore (user_id scoping). - RFC 7807 error standardization. - Background CLI worker for decay/compression. - Tiktoken chunking and OTel instrumentation. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- packages/openmemory-js/src/memory/decay_utils.ts: Fix fingerprint periodicity by improving hash entropy and simplify sentence-splitting regex. - packages/openmemory-js/src/core/db.ts: Remove redundant re-throwing catch clause and convert mutable export to const. - packages/openmemory-js/src/memory/decay.ts: Rename decay_cfg interface to DecayCfg for naming convention compliance. - packages/openmemory-py/src/openmemory/core/vector/valkey.py: Refactor search logic to reduce cognitive complexity below threshold. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
- packages/openmemory-js/src/ai/mcp.ts: Consistently use tenant-resolved identifier in openmemory_get. - packages/openmemory-js/src/core/db.ts: Remove unscoped vector deletion from del_mem and add direct query helpers. - packages/openmemory-js/src/temporal_graph/store.ts: Fix template interpolation and ensure buffered transactions can see existing facts via direct lookup. - packages/openmemory-py/src/openmemory/core/vector/postgres.py: Fix escaped SQL placeholders for asyncpg compatibility. - packages/openmemory-py/src/openmemory/core/vector/valkey.py: Ensure full recall in vector search by exhausting the SCAN cursor. - packages/openmemory-js/src/utils/fetch.ts: Fix type safety for timer identifiers across runtimes. - packages/openmemory-js/src/memory/decay.ts: Pass full tenant context during background vector rewrites. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
- packages/openmemory-js/src/core/db.ts: Consolidated duplicate query implementation between buffered and direct helpers. - packages/openmemory-js/src/memory/decay_utils.ts: Replaced MD5 with SHA-256 for memory fingerprinting to meet security standards. - packages/openmemory-py/src/openmemory/core/vector/valkey.py: Extracted scanning logic to reduce search function cognitive complexity below 15. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
…ById - Extracted parse_item_to_row helper to handle pipeline results. - Simplified loop logic to bring cognitive complexity below 15. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
- packages/openmemory-py/src/openmemory/core/vector/valkey.py: Extracted common Redis scanning logic into a generic _scan_pattern helper to reduce cognitive complexity across all methods (search, getVectorsById, deleteVectors) to well below 15. - packages/openmemory-js/src/core/db.ts: Consolidated SQL execution logic into a private _exec_direct helper, eliminating code duplication between transactional and non-transactional methods. - packages/openmemory-js/src/memory/decay_utils.ts: Standardized on SHA-256 for deterministic hash generation to meet security standards. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/openmemory-py/src/openmemory/core/vector/valkey.py (1)
70-80: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winEscape glob metacharacters before embedding
id/user_idin SCAN patterns.
idanduser_idare interpolated directly into theMATCHpattern here (and indeleteVectorsLine 91,searchLine 132). Any*,?,[,]or\in those tokens is interpreted as a glob, so a crafted or unusual identifier can match unintended keys (over-fetch, over-delete) or fail to match its own key. If identifiers are always hex hashes this is latent, but escaping makes it robust.♻️ Escape helper for scan tokens
`@staticmethod` def _glob_escape(s: str) -> str: return re.sub(r'([\\*?\[\]])', r'\\\1', s)Apply to
uidandidwhen building eachpattern.🤖 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 `@packages/openmemory-py/src/openmemory/core/vector/valkey.py` around lines 70 - 80, Escape SCAN glob metacharacters in identifier tokens before constructing MATCH patterns. Add a reusable _glob_escape helper and apply it to uid/user_id and id in getVectorsById, deleteVectors, and search, ensuring literal identifiers containing backslashes, *, ?, [, or ] cannot broaden or alter matching.
🤖 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 `@packages/openmemory-js/src/core/db.ts`:
- Line 190: Update the waypoints schema definition to use a composite primary
key of (src_id, dst_id, user_id), then update ins_waypoint’s ON CONFLICT target
to match this three-column key so multiple destinations per source/user are
preserved.
In `@packages/openmemory-py/src/openmemory/core/vector/valkey.py`:
- Line 45: Split the compound one-line statements in the Valkey vector
implementation into separate lines: update the guards around item handling and
cursor termination in the relevant methods, including the statements currently
written as “if not item: return None” and “if cursor == 0: break,” to satisfy
Ruff E701.
- Around line 89-98: Require an explicit user_id in deleteVectors and
getVectorsById; remove the wildcard fallback so missing scope cannot scan all
tenant namespaces. Update callers such as HSGMemory.getVectorsById usage to pass
user_id, or make these methods fail closed by raising a clear error when user_id
is absent.
---
Nitpick comments:
In `@packages/openmemory-py/src/openmemory/core/vector/valkey.py`:
- Around line 70-80: Escape SCAN glob metacharacters in identifier tokens before
constructing MATCH patterns. Add a reusable _glob_escape helper and apply it to
uid/user_id and id in getVectorsById, deleteVectors, and search, ensuring
literal identifiers containing backslashes, *, ?, [, or ] cannot broaden or
alter matching.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c3c4bac-7cf6-46b8-b16c-1f8081627907
📒 Files selected for processing (6)
packages/openmemory-js/src/core/db.tspackages/openmemory-js/src/core/vector/postgres.tspackages/openmemory-js/src/memory/decay.tspackages/openmemory-js/src/memory/decay_utils.tspackages/openmemory-js/src/temporal_graph/store.tspackages/openmemory-py/src/openmemory/core/vector/valkey.py
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/openmemory-js/src/core/vector/postgres.ts
- packages/openmemory-js/src/memory/decay.ts
- packages/openmemory-js/src/temporal_graph/store.ts
- packages/openmemory-js/src/core/db.ts: Updated waypoints schema to use composite primary key (src_id, dst_id, user_id) and added automatic migration logic. - packages/openmemory-py/src/openmemory/core/vector/valkey.py: Hardened vector store with glob escaping for identifiers, mandatory user_id for deletions/lookups, and Ruff E701 compliance. - packages/openmemory-py/src/openmemory/memory/hsg.py: Aligned HSG calls with hardened vector store and split compound statements for linting parity. - packages/openmemory-py/src/openmemory/migrations/002_waypoints_pk.sql: Added SQL migration for Python environments. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/openmemory-py/src/openmemory/core/vector/valkey.py (1)
97-102: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winThread the memory
user_idintoon_query_hitpackages/openmemory-py/src/openmemory/memory/hsg.pycallson_query_hit(...)without a tenant identifier, sogetVector/storeVectorfall back toanonymousand regeneration/reinforcement can miss the actual vector row for authenticated memories.🤖 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 `@packages/openmemory-py/src/openmemory/core/vector/valkey.py` around lines 97 - 102, Pass the memory’s user_id through the query-hit flow in hsg.py by updating on_query_hit and its callers, then supply it to the vector get/store operations such as getVector and storeVector instead of allowing the default anonymous value. Preserve the identifier for authenticated memories during regeneration and reinforcement.
🤖 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 `@packages/openmemory-js/src/core/db.ts`:
- Around line 183-196: Update init_db’s waypoints migration to avoid silently
swallowing failures: explicitly handle and surface errors from the PRAGMA probe
and schema replacement, and ensure the migration succeeds before continuing.
Replace the destructive DROP TABLE approach with a migration that preserves
existing rows by creating the corrected schema, backfilling compatible waypoint
relations, and then safely replacing the old table; verify the resulting unique
constraint matches ins_waypoint’s ON CONFLICT(src_id, dst_id, user_id).
In `@packages/openmemory-py/src/openmemory/memory/hsg.py`:
- Around line 268-270: In hsg_query, normalize the optional filter argument
before any access by setting f = f or {} at the start of the function, so
subsequent f.get("sectors") and f.get("user_id") calls are safe when no filters
are provided.
- Around line 90-100: Normalize the tenant scope in create_single_waypoint by
assigning user_id or "anonymous" before searching; use this normalized value in
the search filter and as the user_id inserted into each waypoint row, ensuring
anonymous requests remain tenant-isolated.
In `@packages/openmemory-py/src/openmemory/migrations/002_waypoints_pk.sql`:
- Around line 7-15: Update the waypoints schema so every column in PRIMARY KEY
(src_id, dst_id, user_id)—especially src_id and user_id—is declared NOT NULL.
Adjust the migration backfill to coalesce existing NULL src_id and user_id
values to the intended anonymous/default identifiers before enforcing the
constraint, preserving deduplication for create_single_waypoint writes.
---
Outside diff comments:
In `@packages/openmemory-py/src/openmemory/core/vector/valkey.py`:
- Around line 97-102: Pass the memory’s user_id through the query-hit flow in
hsg.py by updating on_query_hit and its callers, then supply it to the vector
get/store operations such as getVector and storeVector instead of allowing the
default anonymous value. Preserve the identifier for authenticated memories
during regeneration and reinforcement.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f38d6899-4270-49b9-8bcc-49879e2b8f33
📒 Files selected for processing (4)
packages/openmemory-js/src/core/db.tspackages/openmemory-py/src/openmemory/core/vector/valkey.pypackages/openmemory-py/src/openmemory/memory/hsg.pypackages/openmemory-py/src/openmemory/migrations/002_waypoints_pk.sql
Comment on lines
+183
to
+196
| // Migration check for waypoints table primary key change | ||
| try { | ||
| const info = await all_async_direct("PRAGMA table_info(waypoints)"); | ||
| if (info && info.length > 0) { | ||
| const pkCols = info.filter(c => c.pk > 0); | ||
| const has_dst_id_pk = pkCols.some(c => c.name === "dst_id"); | ||
| if (!has_dst_id_pk) { | ||
| console.warn("[DB] Migrating waypoints table: dropping old schema..."); | ||
| await _exec_direct("DROP TABLE waypoints"); | ||
| } | ||
| } | ||
| } catch (e) { | ||
| // Table might not exist yet, ignore | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find writers/regenerators of waypoints to assess data-loss impact of the DROP.
rg -nP 'ins_waypoint|into\s+waypoints|rebuild.*waypoint|regenerat.*waypoint' packages/openmemory-js/src -C2Repository: lucivskvn/OpenMemory-OSS
Length of output: 4724
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the schema initialisation and waypoint writers in context.
python3 - <<'PY'
from pathlib import Path
path = Path("packages/openmemory-js/src/core/db.ts")
lines = path.read_text().splitlines()
for start, end, label in [
(150, 230, "core/db.ts migration + schema init"),
(370, 390, "core/db.ts ins_waypoint"),
]:
print(f"\n=== {label} ({start}-{end}) ===")
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PY
printf '\n=== waypoint writers / regenerators ===\n'
rg -n "ins_waypoint\.run|into\s+waypoints|DROP TABLE waypoints|PRAGMA table_info\(waypoints\)|create table if not exists waypoints" packages/openmemory-js/src -C 2Repository: lucivskvn/OpenMemory-OSS
Length of output: 13708
waypoints migration can leave the table inconsistent and drops existing edges
init_db swallows any failure from both the PRAGMA probe and DROP TABLE waypoints. If the drop fails, the old (src_id, user_id) table survives, CREATE TABLE IF NOT EXISTS won’t repair it, and ins_waypoint’s ON CONFLICT(src_id, dst_id, user_id) no longer matches any unique constraint. The drop also discards all existing waypoint rows, so this needs a real migration/backfill if those relations must survive upgrades.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 194-196: Handle this exception or don't catch it at all.
🤖 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 `@packages/openmemory-js/src/core/db.ts` around lines 183 - 196, Update
init_db’s waypoints migration to avoid silently swallowing failures: explicitly
handle and surface errors from the PRAGMA probe and schema replacement, and
ensure the migration succeeds before continuing. Replace the destructive DROP
TABLE approach with a migration that preserves existing rows by creating the
corrected schema, backfilling compatible waypoint relations, and then safely
replacing the old table; verify the resulting unique constraint matches
ins_waypoint’s ON CONFLICT(src_id, dst_id, user_id).
Source: Linters/SAST tools
Comment on lines
+90
to
100
| async def create_single_waypoint(src_id: str, src_vec: List[float], now: int, user_id: str = None): | ||
| # Find potential neighbors in semantic space | ||
| res = await store.search(src_vec, "semantic", 5, {"user_id": user_id}) | ||
| for r in res: | ||
| dst_id = r["id"] | ||
| if dst_id == src_id: | ||
| continue | ||
| # Multi-tenancy guard: ensured by search filter | ||
| db.execute("INSERT INTO waypoints(src_id, dst_id, user_id, weight, created_at, updated_at) VALUES(?,?,?,?,?,?)", | ||
| (src_id, dst_id, user_id, r["similarity"], now, now)) | ||
| db.commit() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== hsg.py outline ==\n'
ast-grep outline packages/openmemory-py/src/openmemory/memory/hsg.py --view expanded || true
printf '\n== key symbols search ==\n'
rg -n "create_single_waypoint|expand_via_waypoints|anonymous|user_id or|user_id ==|user_id is None|store.search|waypoints" packages/openmemory-py/src/openmemory/memory/hsg.py packages/openmemory-py/src/openmemory -g '!**/__pycache__/**' || true
printf '\n== line ranges around create_single_waypoint ==\n'
sed -n '1,180p' packages/openmemory-py/src/openmemory/memory/hsg.py
printf '\n== line ranges around query/expansion ==\n'
sed -n '180,380p' packages/openmemory-py/src/openmemory/memory/hsg.pyRepository: lucivskvn/OpenMemory-OSS
Length of output: 23307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== search for store implementation and filter handling ==\n'
rg -n "def search\(|class .*Store|user_id.*anonymous|filter.*user_id|wildcard|None" packages/openmemory-py/src -g '!**/__pycache__/**' || true
printf '\n== files mentioning anonymous ==\n'
rg -n "anonymous" packages/openmemory-py/src -g '!**/__pycache__/**' || true
printf '\n== files mentioning expand_via_waypoints ==\n'
rg -n "expand_via_waypoints" packages/openmemory-py/src -g '!**/__pycache__/**' || trueRepository: lucivskvn/OpenMemory-OSS
Length of output: 32183
Normalise the waypoint scope. create_single_waypoint() should use user_id or "anonymous" for both the search filter and the inserted waypoint row; passing None leaves search() unfiltered and can link anonymous memories to other tenants' vectors.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 90-90: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 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 `@packages/openmemory-py/src/openmemory/memory/hsg.py` around lines 90 - 100,
Normalize the tenant scope in create_single_waypoint by assigning user_id or
"anonymous" before searching; use this normalized value in the search filter and
as the user_id inserted into each waypoint row, ensuring anonymous requests
remain tenant-isolated.
Comment on lines
268
to
+270
| ss = f.get("sectors") or list(SECTOR_CONFIGS.keys()) | ||
| if not ss: ss = ["semantic"] | ||
| if not ss: | ||
| ss = ["semantic"] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/openmemory-py/src/openmemory/memory/hsg.py"
# Show the function outline and the relevant sections around the referenced lines.
python3 - <<'PY'
from pathlib import Path
path = Path("packages/openmemory-py/src/openmemory/memory/hsg.py")
text = path.read_text().splitlines()
for start,end in [(220,340)]:
print(f"\n--- {path}:{start}-{end} ---")
for i in range(start, min(end, len(text)) + 1):
print(f"{i:4d}: {text[i-1]}")
PYRepository: lucivskvn/OpenMemory-OSS
Length of output: 5203
Normalise f before the first .get(...) call hsg_query still defaults f to None, so f.get("sectors") and f.get("user_id") will raise on the common no-filter path. Set f = f or {} at the top of the function.
🤖 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 `@packages/openmemory-py/src/openmemory/memory/hsg.py` around lines 268 - 270,
In hsg_query, normalize the optional filter argument before any access by
setting f = f or {} at the start of the function, so subsequent f.get("sectors")
and f.get("user_id") calls are safe when no filters are provided.
Comment on lines
+7
to
+15
| src_id TEXT, | ||
| dst_id TEXT NOT NULL, | ||
| user_id TEXT, | ||
| project_id TEXT, | ||
| weight REAL NOT NULL, | ||
| created_at INTEGER, | ||
| updated_at INTEGER, | ||
| PRIMARY KEY (src_id, dst_id, user_id) | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Nullable columns inside the composite primary key weaken uniqueness.
src_id and user_id are part of PRIMARY KEY (src_id, dst_id, user_id) but are nullable. SQLite permits NULLs in (non-INTEGER) primary-key columns and treats NULLs as distinct, so (src_id, dst_id, NULL) rows never collide — the constraint gives no dedupe/isolation for the anonymous/None writes produced by create_single_waypoint. Declare them NOT NULL and coalesce during backfill.
♻️ Proposed change
CREATE TABLE waypoints_new (
- src_id TEXT,
+ src_id TEXT NOT NULL,
dst_id TEXT NOT NULL,
- user_id TEXT,
+ user_id TEXT NOT NULL DEFAULT 'anonymous',
project_id TEXT,
weight REAL NOT NULL,
created_at INTEGER,
updated_at INTEGER,
PRIMARY KEY (src_id, dst_id, user_id)
);And guard the backfill against existing NULLs:
INSERT INTO waypoints_new (src_id, dst_id, user_id, weight, created_at, updated_at)
-SELECT src_id, dst_id, user_id, weight, created_at, updated_at FROM waypoints;
+SELECT src_id, dst_id, COALESCE(user_id, 'anonymous'), weight, created_at, updated_at FROM waypoints;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| src_id TEXT, | |
| dst_id TEXT NOT NULL, | |
| user_id TEXT, | |
| project_id TEXT, | |
| weight REAL NOT NULL, | |
| created_at INTEGER, | |
| updated_at INTEGER, | |
| PRIMARY KEY (src_id, dst_id, user_id) | |
| ); | |
| src_id TEXT NOT NULL, | |
| dst_id TEXT NOT NULL, | |
| user_id TEXT NOT NULL DEFAULT 'anonymous', | |
| project_id TEXT, | |
| weight REAL NOT NULL, | |
| created_at INTEGER, | |
| updated_at INTEGER, | |
| PRIMARY KEY (src_id, dst_id, user_id) | |
| ); |
🤖 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 `@packages/openmemory-py/src/openmemory/migrations/002_waypoints_pk.sql` around
lines 7 - 15, Update the waypoints schema so every column in PRIMARY KEY
(src_id, dst_id, user_id)—especially src_id and user_id—is declared NOT NULL.
Adjust the migration backfill to coalesce existing NULL src_id and user_id
values to the intended anonymous/default identifiers before enforcing the
constraint, preserving deduplication for create_single_waypoint writes.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
This was referenced Jul 17, 2026
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
2 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.
This PR implements comprehensive architectural hardening and structural parity across the Node.js and Python runtimes.
Key changes:
user_idand tenant-prefixed keys.opm worker.cl100k_basetokenizer across both runtimes.numpy.float64), preventing score drift against the Node.js implementation.PR created automatically by Jules for task 11657646660241508660 started by @lucivskvn
Summary by CodeRabbit