feat(graph): auto-trigger mem::graph-extract on mem::remember#568
Open
efenex wants to merge 3 commits into
Open
feat(graph): auto-trigger mem::graph-extract on mem::remember#568efenex wants to merge 3 commits into
efenex wants to merge 3 commits into
Conversation
mem::remember persists a Memory record and indexes it into BM25 + the vector store, but never fires the knowledge-graph extraction pipeline. As a result, content saved via /agentmemory/remember — including bulk imports like in-repo doc ingestion or post-hoc lesson crystallization — doesn't populate KV.graphNodes/Edges. mem::reflect's graph-clustering path then can't see this content and synthesizes insights only from lessons + semantic facts + crystals + graph (which itself is fed only by observation writes via triggers/events.ts). Add a fire-and-forget triggerVoid for mem::graph-extract right after the vector-index step, mirroring the observation pipeline's gate on isGraphExtractionEnabled(). Reuses the existing memoryToObservation helper (state/memory-utils.ts) so the wire format matches what graph-extract already expects. Wrapped in try/catch so a failure in the trigger setup never blocks the remember response. No new env flags — gated on the existing GRAPH_EXTRACTION_ENABLED. Operators who don't want auto-extraction from Memory writes simply keep that flag off; observation-path extraction (which already honors the same flag) is unaffected either way. Companion to scripts/rebuild-graph.sh (commit 865c8d3), which back-fills the graph for pre-V3-D content using a local LLM. With both in place, ongoing remember POSTs auto-feed the graph and the script is reserved for one-off bulk sweeps (or recovery after large imports done with extraction disabled).
|
@efenex is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughThe ChangesGraph Extraction Triggering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/functions/remember.ts (1)
123-133: ⚡ Quick winReduce the large WHAT-comment block.
Please trim this to a short intent note and rely on naming/function structure for behavior details.
As per coding guidelines, "Avoid code comments explaining WHAT — use clear naming instead".
🤖 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/functions/remember.ts` around lines 123 - 133, Replace the large explanatory comment in src/functions/remember.ts with a concise intent note: state that saved memories are enqueued for graph extraction (same behavior as observation writes) when GRAPH_EXTRACTION_ENABLED is true and that extraction is fire-and-forget to avoid blocking the remember response; keep references to the existing helpers/paths (the observation event path event::session::compressed, mem::reflect, graph-extract, and KV.graphNodes/Edges) only if needed for clarity, and rely on function/variable names to convey the detailed behavior instead of a long WHAT block.
🤖 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/functions/remember.ts`:
- Around line 135-144: The call to sdk.triggerVoid("mem::graph-extract", {
observations: [memoryToObservation(memory)] }) returns a Promise<void> so the
current try/catch around it won't catch async rejections; update the call in the
remember handler to either await the Promise (ensuring the enclosing function is
async) or append .catch(...) to the Promise and log via logger.warn including
memId: memory.id and the error message (err instanceof Error ? err.message :
String(err)) to mirror existing logging; ensure you reference sdk.triggerVoid,
memoryToObservation, and logger.warn when making the change.
---
Nitpick comments:
In `@src/functions/remember.ts`:
- Around line 123-133: Replace the large explanatory comment in
src/functions/remember.ts with a concise intent note: state that saved memories
are enqueued for graph extraction (same behavior as observation writes) when
GRAPH_EXTRACTION_ENABLED is true and that extraction is fire-and-forget to avoid
blocking the remember response; keep references to the existing helpers/paths
(the observation event path event::session::compressed, mem::reflect,
graph-extract, and KV.graphNodes/Edges) only if needed for clarity, and rely on
function/variable names to convey the detailed behavior instead of a long WHAT
block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
`sdk.triggerVoid()` returns `Promise<void>`, so the previous sync try/catch only caught synchronous throws (argument validation, etc.). Async rejections — provider timeout, engine queue full, downstream extraction failure — escaped as unhandledRejection. CodeRabbit caught this on rohitg00#568. Keep the sync guard (defense in depth) and add a Promise.catch() so either failure mode is logged-and-continued. Defensive type-narrow the return value before attaching .catch in case a future iii-sdk release changes triggerVoid's exact return shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/functions/remember.ts (1)
135-140: ⚡ Quick winRemove the WHAT-style inline explanation block.
These lines restate control flow behavior; keep intent-focused comments only where naming cannot express why.
As per coding guidelines, "Avoid code comments explaining WHAT — use clear naming instead".
🤖 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/functions/remember.ts` around lines 135 - 140, Remove the WHAT-style explanatory block comment that restates control flow (the multi-line comment above the triggerVoid call) and replace it with an intent-focused one-liner (or no comment) clarifying why we attach .catch() to the promise — e.g., "Ensure fire-and-forget handles both sync throws and async rejections" — near the triggerVoid invocation; update references around triggerVoid and the surrounding try/catch so naming and the concise note communicate the why without restating how the language works.
🤖 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.
Nitpick comments:
In `@src/functions/remember.ts`:
- Around line 135-140: Remove the WHAT-style explanatory block comment that
restates control flow (the multi-line comment above the triggerVoid call) and
replace it with an intent-focused one-liner (or no comment) clarifying why we
attach .catch() to the promise — e.g., "Ensure fire-and-forget handles both sync
throws and async rejections" — near the triggerVoid invocation; update
references around triggerVoid and the surrounding try/catch so naming and the
concise note communicate the why without restating how the language works.
CodeRabbit nitpick on rohitg00#568. The block explained what triggerVoid does in detail; the WHY (handle sync throw + async rejection) is captured in a single line. Naming + structure already convey the rest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/functions/remember.ts (1)
135-135: ⚡ Quick winRemove WHAT-style inline comment and let code carry intent.
Line 135 adds a behavior-explaining comment that conflicts with the repo style rule; please remove it (or rewrite as intent-focused naming only).
Suggested minimal diff
- // triggerVoid can throw synchronously or reject async — handle both. try {As per coding guidelines, "Avoid code comments explaining WHAT — use clear naming instead".
🤖 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/functions/remember.ts` at line 135, Remove the WHAT-style inline comment next to triggerVoid in remember.ts: either delete the comment entirely or replace it with an intent-focused element (e.g., rename the surrounding helper/variable to convey error-handling intent or extract the call into a clearly named function like handleTriggerVoidErrors/ensureTriggerVoidHandled) so the code self-documents that triggerVoid may throw synchronously or reject asynchronously without an explanatory inline comment.
🤖 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.
Nitpick comments:
In `@src/functions/remember.ts`:
- Line 135: Remove the WHAT-style inline comment next to triggerVoid in
remember.ts: either delete the comment entirely or replace it with an
intent-focused element (e.g., rename the surrounding helper/variable to convey
error-handling intent or extract the call into a clearly named function like
handleTriggerVoidErrors/ensureTriggerVoidHandled) so the code self-documents
that triggerVoid may throw synchronously or reject asynchronously without an
explanatory inline comment.
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
1 participant
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.
Summary
mem::remember(the explicit memory-save path used bymemory_saveMCP calls, plugin Stop hooks, JSONL imports, etc.) does NOT auto-triggermem::graph-extract. Observations do auto-trigger extraction whenGRAPH_EXTRACTION_ENABLED=true, but memories don't — so the knowledge graph grows from observation traffic only, leaving curated memories invisible to graph queries.This wires the same auto-trigger pattern on the memory write path: after a successful
mem::remember, firemem::graph-extractagainst the newly-saved memory id whenGRAPH_EXTRACTION_ENABLED=true. Best-effort; failure is logged-and-continued so a slow/down LLM provider can't block the save.Impact
Closes the gap between observation-driven and memory-driven graph growth. After this:
mem::lineage(withincludeGraph: true) sees the same node set whether a concept was extracted from observation traffic or written as a memory.mem::smart-searchgraph-score component lights up for curated memories.agentmemory graph-build(feat(graph): backfill graph from stored memories #538, if/when it lands) becomes a backfill tool for old data, not the only path for new data.Related
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit