fix(graph): auto-fire mem::graph-extract at session end (closes #210)#215
Merged
fix(graph): auto-fire mem::graph-extract at session end (closes #210)#215
Conversation
mem::graph-extract was registered and the REST endpoint at POST /agentmemory/graph/extract was live, but no internal caller invoked the function. Setting GRAPH_EXTRACTION_ENABLED=true populated nothing — the graph KV stayed empty unless users manually POSTed observations to the extract endpoint. Wire the function into event::session::stopped (alongside the existing mem::slot-reflect glue). Fire-and-forget via triggerVoid; the existing node-merge (name+type) and edge-merge (source|target|type) keys make re-runs idempotent. README pipeline diagram updated: graph extraction shown at the Stop/SessionEnd phase rather than implying it runs per PostToolUse. The previous wording was misleading — the BM25 + vector indices ARE written per-PostToolUse, but the graph never was. Note: mem::temporal-graph-extract has the same orphan status and is NOT addressed here. Tracking separately.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe changes automatically trigger graph extraction when a session ends. Previously, Changes
Sequence DiagramsequenceDiagram
participant Session
participant EventSystem as Event System
participant Handler as Session Stopped Handler
participant FeatureFlag as Feature Flag Check
participant ObsDB as Observation Store
participant GraphExtract as mem::graph-extract
participant KVStore as Graph KV
Session->>EventSystem: Session completes
EventSystem->>Handler: fire event::session::stopped
Handler->>FeatureFlag: isGraphExtractionEnabled()?
alt Flag Enabled
FeatureFlag-->>Handler: true
Handler->>ObsDB: Query observations for session
ObsDB-->>Handler: observations with title
Handler->>GraphExtract: sdk.triggerVoid("mem::graph-extract", {observations})
GraphExtract->>KVStore: Extract & merge nodes/edges
KVStore-->>GraphExtract: Stored (idempotent)
GraphExtract-->>Handler: Complete
else Flag Disabled
FeatureFlag-->>Handler: false
Handler-->>EventSystem: Skip extraction
end
Handler-->>EventSystem: Continue (slot reflection, etc.)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 2
🧹 Nitpick comments (1)
src/triggers/events.ts (1)
64-67: Tighten observation filtering before callingmem::graph-extract.Filtering only by truthy
titlecan pass malformed rows;mem::graph-extractbuilds prompts fromtitle/narrative/concepts/files/type.Proposed fix
- const compressed = observations.filter((o) => o.title); + const compressed = observations.filter( + (o): o is CompressedObservation => + typeof o.title === "string" && + o.title.trim().length > 0 && + typeof o.narrative === "string" && + Array.isArray(o.concepts), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/triggers/events.ts` around lines 64 - 67, The current filter only checks truthy title and can pass malformed observation rows to mem::graph-extract; update the filtering logic where compressed is built to validate required fields for each observation (e.g., in the observations.filter call that produces compressed) by ensuring title and type are non-empty strings, narrative is a string (if required), concepts is an array (and optionally non-empty), and files is an array of valid file entries before calling sdk.triggerVoid("mem::graph-extract", { observations: compressed }); this prevents malformed prompts by only sending well-shaped observation objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 514: The README uses the wrong config flag name: change the reference
from SLOT_REFLECT_ENABLED to AGENTMEMORY_REFLECT (or vice‑versa) so the
documentation matches the actual configuration variable; update the single
occurrence "-> Slot reflection (if SLOT_REFLECT_ENABLED=true)" to read "-> Slot
reflection (if AGENTMEMORY_REFLECT=true)" and ensure any nearby config examples
and descriptions consistently use AGENTMEMORY_REFLECT.
In `@src/triggers/events.ts`:
- Around line 59-63: Validate data.sessionId before using it to build the KV
scope: before calling isGraphExtractionEnabled()'s KV read (the
kv.list<CompressedObservation>(KV.observations(data.sessionId)) call) check that
data.sessionId is present, is a non-empty string (and matches any expected
pattern/UUID if applicable), and reject/skip the KV read (or throw a controlled
error) when validation fails; update the input validation where this handler is
registered (sdk.registerFunction) to enforce the same contract and add a
recordAudit() call for invalid/malformed sessionId events so malformed payloads
are recorded and the downstream KV read is never attempted.
---
Nitpick comments:
In `@src/triggers/events.ts`:
- Around line 64-67: The current filter only checks truthy title and can pass
malformed observation rows to mem::graph-extract; update the filtering logic
where compressed is built to validate required fields for each observation
(e.g., in the observations.filter call that produces compressed) by ensuring
title and type are non-empty strings, narrative is a string (if required),
concepts is an array (and optionally non-empty), and files is an array of valid
file entries before calling sdk.triggerVoid("mem::graph-extract", {
observations: compressed }); this prevents malformed prompts by only sending
well-shaped observation objects.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b064e7a6-440f-46a7-9af8-f4a82863463b
📒 Files selected for processing (3)
CHANGELOG.mdREADME.mdsrc/triggers/events.ts
| Stop / SessionEnd hook fires | ||
| -> Summarize session | ||
| -> Knowledge graph extraction (if GRAPH_EXTRACTION_ENABLED=true) | ||
| -> Slot reflection (if SLOT_REFLECT_ENABLED=true) |
There was a problem hiding this comment.
Use the correct slot reflection flag name.
Line 514 uses SLOT_REFLECT_ENABLED, but the config section documents AGENTMEMORY_REFLECT. This can cause users to enable the wrong flag.
Proposed fix
- -> Slot reflection (if SLOT_REFLECT_ENABLED=true)
+ -> Slot reflection (if AGENTMEMORY_REFLECT=true)📝 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
| -> Slot reflection (if SLOT_REFLECT_ENABLED=true) | |
| -> Slot reflection (if AGENTMEMORY_REFLECT=true) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 514, The README uses the wrong config flag name: change
the reference from SLOT_REFLECT_ENABLED to AGENTMEMORY_REFLECT (or vice‑versa)
so the documentation matches the actual configuration variable; update the
single occurrence "-> Slot reflection (if SLOT_REFLECT_ENABLED=true)" to read
"-> Slot reflection (if AGENTMEMORY_REFLECT=true)" and ensure any nearby config
examples and descriptions consistently use AGENTMEMORY_REFLECT.
Comment on lines
+59
to
+63
| if (isGraphExtractionEnabled()) { | ||
| try { | ||
| const observations = await kv.list<CompressedObservation>( | ||
| KV.observations(data.sessionId), | ||
| ); |
There was a problem hiding this comment.
Validate sessionId before KV scope reads.
The new path reads KV.observations(data.sessionId) without a runtime guard. A malformed payload can cause incorrect scope reads and noisy downstream triggers.
Proposed fix
if (isGraphExtractionEnabled()) {
+ if (typeof data.sessionId !== "string" || data.sessionId.trim().length === 0) {
+ logger.warn("graph-extract skipped: invalid sessionId", {
+ sessionId: String(data.sessionId),
+ });
+ return summary;
+ }
try {
const observations = await kv.list<CompressedObservation>(
KV.observations(data.sessionId),
);📝 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
| if (isGraphExtractionEnabled()) { | |
| try { | |
| const observations = await kv.list<CompressedObservation>( | |
| KV.observations(data.sessionId), | |
| ); | |
| if (isGraphExtractionEnabled()) { | |
| if (typeof data.sessionId !== "string" || data.sessionId.trim().length === 0) { | |
| logger.warn("graph-extract skipped: invalid sessionId", { | |
| sessionId: String(data.sessionId), | |
| }); | |
| return summary; | |
| } | |
| try { | |
| const observations = await kv.list<CompressedObservation>( | |
| KV.observations(data.sessionId), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/triggers/events.ts` around lines 59 - 63, Validate data.sessionId before
using it to build the KV scope: before calling isGraphExtractionEnabled()'s KV
read (the kv.list<CompressedObservation>(KV.observations(data.sessionId)) call)
check that data.sessionId is present, is a non-empty string (and matches any
expected pattern/UUID if applicable), and reject/skip the KV read (or throw a
controlled error) when validation fails; update the input validation where this
handler is registered (sdk.registerFunction) to enforce the same contract and
add a recordAudit() call for invalid/malformed sessionId events so malformed
payloads are recorded and the downstream KV read is never attempted.
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
Closes #210.
mem::graph-extractwas registered and exposed via REST, but never auto-invoked. Graph KV stayed empty even withGRAPH_EXTRACTION_ENABLED=trueunless users manuallyPOSTed to/agentmemory/graph/extract.Reporter @jco-analyst traced this thoroughly: zero callers in
observe.ts,compress.ts,consolidation-pipeline.ts,flow-compress.ts, orregisterEventTriggers. README pipeline diagram (-> Index in BM25 + vector + knowledge graph) implied per-PostToolUse extraction that didn't exist.Fix
Wire
mem::graph-extractintoevent::session::stopped— fire-and-forget, gated onisGraphExtractionEnabled(), mirrors the existingmem::slot-reflectglue.Idempotent on re-run via existing merge keys (
name+typefor nodes,source|target|typefor edges) atsrc/functions/graph.ts:115-149.README
Pipeline diagram split: BM25 + vector at PostToolUse (unchanged), graph extraction now correctly shown at Stop/SessionEnd phase.
Out of scope
mem::temporal-graph-extracthas the same orphan status — separate follow-upgraph-prune/decay/gc/clear(raised in the issue) — separate follow-up; graph KV will grow unbounded until those landTest plan
npm run build)GRAPH_EXTRACTION_ENABLED=true, run a session through Claude Code, end session,curl /agentmemory/graph/stats→ expect non-zerototalNodesSummary by CodeRabbit
New Features
Documentation