feat(graph): auto-trigger mem::graph-extract on mem::remember#568
Open
efenex wants to merge 1 commit into
Open
feat(graph): auto-trigger mem::graph-extract on mem::remember#568efenex wants to merge 1 commit into
efenex wants to merge 1 commit into
Conversation
|
@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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe ChangesGraph Extraction Triggering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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
efenex
added a commit
to efenex/agentmemory
that referenced
this pull request
May 20, 2026
`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.
efenex
added a commit
to efenex/agentmemory
that referenced
this pull request
May 20, 2026
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.
efenex
added a commit
to efenex/agentmemory
that referenced
this pull request
May 21, 2026
CodeRabbit re-flagged the one-line note that survived rohitg00#568's earlier trim. The surrounding sync try/catch + async .catch() already make the intent self-evident. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mem::remember already feeds observations into the graph; saved memories skipped it, so mem::reflect's graph-clustering path never saw them. Trigger graph extraction (fire-and-forget, Void action) on remember too, gated on GRAPH_EXTRACTION_ENABLED. Rebased on current main; uses sdk.trigger + TriggerAction.Void() (sdk.triggerVoid was removed in rohitg00#773). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
efenex
force-pushed
the
feat/v3-d-graph-extract-on-remember
branch
from
ee8d844 to
bdd25ca
Compare
Contributor
Author
|
Rebased on current |
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.
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