feat(knowledge): introduce local Orama persistence (feature-flagged)#1015
feat(knowledge): introduce local Orama persistence (feature-flagged)#10153kin0x wants to merge 1 commit intoGitlawb:mainfrom
Conversation
|
Thanks for narrowing this down from the earlier broader knowledge/RAG direction. This is a much better shape from a trust-boundary perspective because it is local-only and feature-flagged. Before a real review, though, this needs a refresh:
Could you rebase onto latest
Not blocking the direction conceptually, just asking for a clean reviewable head first. |
|
please rebase to main and fix conflicts |
bf8cb4f to
ad07527
Compare
|
Build Fix Summary: Module Resolution & Circular Dependencies The build failure in the CI (bun run smoke) was caused by a module resolution error following the architectural changes made to break a circular dependency.
The PR is now in a "Green" state and ready for review. |
|
hello @techbrewboss @jatmn kindly have a look too when you have time. |
Give me about an hour or so. Got you |
|
Review note from the current head:
Can we initialize Orama before the first knowledge graph write/search, and add a feature-flagged test that proves the |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for the follow-up and rebase. I did a targeted current-head review of ad07527, focused on the feature-flagged Orama path, persistence behavior, and async knowledge/arc call sites.
Review scope: Targeted review of the new local Orama persistence path and its feature-flag behavior.
Verdict: Needs changes
Blocking issue:
- The Orama backend appears to never initialize.
src/utils/knowledgeGraph.tsexportsinitOrama(cwd), but on the current head it has no call site. The write/search branches are all guarded byisOramaEnabled() && oramaDb, so withOPENCLAUDE_KNOWLEDGE_ORAMA=1,oramaDbstill remainsnullunless someone manually callsinitOrama(). That meansaddGlobalEntity(),addGlobalSummary(),searchGlobalGraph(), andgetOrchestratedMemory()silently stay on the JSON path instead of exercising the new Orama persistence/search path.
What I checked:
- Current head:
ad07527 rg "initOrama\("only finds the exported function definition insrc/utils/knowledgeGraph.ts.- The Orama insert/search branches all require
oramaDbto already be non-null. - Existing tests cover the async JSON-path behavior, but I do not see a feature-flagged regression test proving
OPENCLAUDE_KNOWLEDGE_ORAMA=1creates/restores/searches the.oramapersistence file.
Please initialize Orama before the first feature-flagged write/search path, and add focused coverage that proves:
- default behavior remains JSON-only when the flag is unset
- with
OPENCLAUDE_KNOWLEDGE_ORAMA=1, the Orama store is initialized, persisted, restored, and actually used for search
Non-blocking note:
- I like that this version is local-only and feature-flagged. Once initialization and coverage are fixed, the remaining review can stay focused on persistence path safety and async compatibility.
- Added @orama/orama and persistence plugin. - Implemented optional local-only Orama backend in knowledgeGraph.ts. - Gated Orama logic behind OPENCLAUDE_KNOWLEDGE_ORAMA=1. - Converted knowledge and conversation arc functions to async. - Fixed circular dependency between knowledgeGraph and sessionStorage by moving getProjectsDir to envUtils. - Updated all call sites and tests to handle async Knowledge API. - Verified build and tests pass on latest main.
ad07527 to
02e7bd9
Compare
|
Thank you for the detailed review! I have addressed the blocking issues regarding initialization and added the requested test coverage. Key Changes:
Verification:
|
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Scope: Targeted re-review of the current Orama head (02e7bd9) against the earlier initialization blocker and the new async/persistence paths.
Verdict: Needs changes
Good progress: the original blocker I saw earlier is mostly addressed now. initOrama() is no longer dead code; it is reached from the Orama-enabled insert/search paths, and the focused knowledge/conversation tests pass locally after installing the new locked dependencies.
Blocking issues:
finalizeArcTurn()did not finish the async conversion. It still returnsvoidand callsaddGlobalSummary(summaryContent, keywords)withoutawait, whilequery.tsstill callsfinalizeArcTurn()withoutawait. SinceaddGlobalSummary()now owns async Orama init/insert/save work, the turn summary can be fire-and-forgeted and race with the next loop/process shutdown. Please makefinalizeArcTurn()async, awaitaddGlobalSummary(), update thequery.tscall site, and make the test assert the awaited path intentionally./knowledge clear/resetGlobalGraph()only clears the JSON graph path. WithOPENCLAUDE_KNOWLEDGE_ORAMA=1, the newknowledge.oramapersistence file and in-memoryoramaDbcan survive the clear path, so cleared memory can still be restored/searched later. Please clear the Orama persistence file and in-memory DB as part of the reset/clear flow, and add a regression test with the Orama flag enabled.
Verification I ran:
bun install --frozen-lockfilebun test src/utils/knowledgeGraph.test.ts src/utils/conversationArc.test.ts src/commands/knowledge/knowledge.test.tspassed: 25/25
Happy to re-review once those two persistence/async semantics are tightened.
Summary
Impact
Testing
Notes
gating).
@Vasanthdev2004
@gnanam1990
@kevincodex1