fix(graph): wire session-end → graph extraction + repair status (#666)#698
Merged
Conversation
Three concrete bugs reported by @LeonemFu, all verified: 1. `event::session::stopped` is a dead subscriber. The handler in `src/triggers/events.ts` listens on `agentmemory.session.stopped` but nothing in the codebase publishes that topic. `api::session::end` just calls `kv.update` and returns — so every session ends with `mem::summarize`, `mem::slot-reflect`, and `mem::graph-extract` skipped. Result: graph stays empty no matter how many sessions the user runs. Fix: `api::session::end` now calls `sdk.triggerVoid("event::session::stopped", { sessionId })` after the KV update so the existing pipeline fires. HTTP response stays fast (kv.update is synchronous, downstream work fans out without blocking). 2. `agentmemory status` shows Memories/Observations = 0. CLI used `/agentmemory/export` to derive counts but that endpoint times out (>5s) on iii-engine's file-based KV under concurrent `kv.list()` pressure — even on small datasets (121 observations in the reporter's case). Promise.all swallowed the timeout and fell back to 0. Fix: status now reads `/memories` for memory count and sums `sessions[].observationCount` for observation count. No longer depends on the slow path. 3. Viewer's "Build Graph" button posts to `/agentmemory/graph/build` which returned 404 — the endpoint was never registered. Fix: new `api::graph-build` iterates all sessions, lists each session's compressed observations (filter on `title` field), and feeds them through `mem::graph-extract` in configurable batches (default 25, capped at 100). Returns `{ success, sessions, batches, nodes, edges }` for the viewer's progress UI. Test coverage: 11 new assertions in `test/session-end-triggers-graph.test.ts` covering the wiring across all three fixes (no runtime mocks — pure source-string assertions matching the pattern used by `test/memories-pagination.test.ts`). Consistency: REST endpoint count bumped 124 → 125 in `src/index.ts`, README, and AGENTS.md per the AGENTS.md consistency rules. Closes #666.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR implements issue ChangesGraph Backfill and Session Lifecycle Event
Sequence DiagramsequenceDiagram
participant Session as api::session::end
participant KV as KV (state store)
participant SDK as sdk.triggerVoid
participant GraphBuild as POST /agentmemory/graph/build
participant MemExtract as mem::graph-extract
participant Status as agentmemory status (runStatus)
participant Memories as /memories?count=true
Session->>KV: kv.update(mark session completed)
KV-->>Session: update result
Session->>SDK: triggerVoid("event::session::stopped", { sessionId })
SDK-->>Session: non-blocking return / error handling
GraphBuild->>KV: list sessions
KV-->>GraphBuild: sessions[]
GraphBuild->>KV: load compressed observations per session
KV-->>GraphBuild: observations (filter o.title != '')
GraphBuild->>MemExtract: process batch (1–100)
MemExtract-->>GraphBuild: { nodes, edges } or error
GraphBuild-->>GraphBuild: accumulate totals, log per-batch warnings
Status->>Memories: fetch memories + sessions
Memories-->>Status: memoriesRes, sessionsRes
Status-->>Status: obsCount = sum(sessions[].observationCount)
Status-->>Status: memCount = memoriesRes.latestCount || memoriesRes.total
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli.ts (1)
1161-1167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
memories?count=truefor status to avoid large payload fetches.
statusonly needs a count, but this path fetches full memory rows and can still degrade under large datasets.Proposed fix
- apiFetch<any>(base, "memories"), + apiFetch<any>(base, "memories?count=true"), @@ - const memCount = Array.isArray(memoriesRes?.memories) ? memoriesRes.memories.length : 0; + const memCount = Number(memoriesRes?.total) || 0;Also applies to: 1187-1187
🤖 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/cli.ts` around lines 1161 - 1167, The status call currently fetches full memory rows causing heavy payloads; update the apiFetch calls that request "memories" (the ones that populate memoriesRes at the Promise.all and the later single call around line 1187) to use the count-only endpoint "memories?count=true" so the server returns only a count. Keep the variable names (memoriesRes) and other parallel requests unchanged, and ensure any downstream code that expects a count reads the count field from the response rather than iterating full rows.test/session-end-triggers-graph.test.ts (1)
1-87: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftUse behavior tests with the repo’s
vi.mock('iii-sdk')pattern instead of source-regex tests.This file currently validates string patterns in source code, which is brittle and misses runtime contracts. Please align with the established mock-based function tests for trigger registration and call behavior.
As per coding guidelines
test/**/*.test.ts: "Mock pattern: vi.mock('iii-sdk') with mock sdk.trigger, kv.get/set/list in tests".🤖 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 `@test/session-end-triggers-graph.test.ts` around lines 1 - 87, Replace fragile source-regex assertions with behavior tests using the repo-wide vi.mock('iii-sdk') pattern: vi.mock('iii-sdk') and provide mocks for sdk.trigger/triggerVoid, registerFunction, kv.update/kv.list/kv.get, and apiFetch; import the module under test after setting up mocks, then invoke the exported handler functions and assert behavior. For api::session::end test, mock kv.update to succeed and verify sdk.triggerVoid was called after kv.update with "event::session::stopped" and payload containing sessionId and that the call order respects update→trigger. For api::graph-build, mock kv.list to return session IDs and observations, call the registered api::graph-build handler (via the mocked registerFunction capture), assert registerFunction was called with "api::graph-build", the handler calls kv.list<Session> and kv.list<CompressedObservation>, and sdk.trigger is invoked with function_id "mem::graph-extract" for each batch; simulate batchSize override to assert Math.min(100, Number(batchSize)) behavior by invoking handler with different batchSize inputs. For CLI status tests, mock apiFetch to return memoriesRes and sessionList, require the CLI module after mocks, call the status function, and assert apiFetch used "memories" (not "export"), observation count derived from sessionList[].observationCount via reduce, and memCount read from memoriesRes.memories.length.
🧹 Nitpick comments (1)
src/triggers/api.ts (1)
614-617: ⚡ Quick winReplace WHAT-comments with intent-focused naming/logging.
These new comments narrate implementation details; prefer self-explanatory code and brief intent-only notes.
As per coding guidelines
**/*.ts: "Use clear naming instead of code comments explaining WHAT — avoid comments that describe what code does".Also applies to: 1395-1398
🤖 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/triggers/api.ts` around lines 614 - 617, Replace the explanatory "WHAT" comment block about the fan-out of the session-stopped lifecycle with intent-focused naming and minimal logging: remove the multi-line implementation-narration and instead ensure the code that publishes/subscribes to the agentmemory.session.stopped topic (and the fan-out handler in this file) is given a clear identifier (e.g., publishSessionStoppedEvent or handleSessionStoppedFanout) and add a single-line intent comment like "// publish session-stopped lifecycle events (non-blocking)" or a concise log message; also ensure subscribers in events.ts remain tied to agentmemory.session.stopped so behavior is unchanged.
🤖 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/triggers/api.ts`:
- Around line 618-619: The call to sdk.triggerVoid("event::session::stopped", {
sessionId }) should be guarded so a thrown error doesn't convert a successful
session-end (after kv.update) into a failure; wrap the sdk.triggerVoid(...)
invocation in a try/catch, catch and log the error (or swallow it) and ensure
the function still returns { status_code: 200, body: { success: true } } when
kv.update succeeded; reference the sdk.triggerVoid call and the sessionId
variable so you modify that exact call site.
In `@test/session-end-triggers-graph.test.ts`:
- Around line 52-54: The test "filters observations that have a title
(compressed only)" uses an impossible regex checking `o.title === "string"`;
update the assertion on `api` to match the actual code pattern `typeof o.title
=== "string" && o.title.length > 0` (i.e., change the regex in the expect call
in that test to look for `typeof o\.title === "string" && o\.title\.length > 0`)
so the test validates the intended behavior.
---
Outside diff comments:
In `@src/cli.ts`:
- Around line 1161-1167: The status call currently fetches full memory rows
causing heavy payloads; update the apiFetch calls that request "memories" (the
ones that populate memoriesRes at the Promise.all and the later single call
around line 1187) to use the count-only endpoint "memories?count=true" so the
server returns only a count. Keep the variable names (memoriesRes) and other
parallel requests unchanged, and ensure any downstream code that expects a count
reads the count field from the response rather than iterating full rows.
In `@test/session-end-triggers-graph.test.ts`:
- Around line 1-87: Replace fragile source-regex assertions with behavior tests
using the repo-wide vi.mock('iii-sdk') pattern: vi.mock('iii-sdk') and provide
mocks for sdk.trigger/triggerVoid, registerFunction, kv.update/kv.list/kv.get,
and apiFetch; import the module under test after setting up mocks, then invoke
the exported handler functions and assert behavior. For api::session::end test,
mock kv.update to succeed and verify sdk.triggerVoid was called after kv.update
with "event::session::stopped" and payload containing sessionId and that the
call order respects update→trigger. For api::graph-build, mock kv.list to return
session IDs and observations, call the registered api::graph-build handler (via
the mocked registerFunction capture), assert registerFunction was called with
"api::graph-build", the handler calls kv.list<Session> and
kv.list<CompressedObservation>, and sdk.trigger is invoked with function_id
"mem::graph-extract" for each batch; simulate batchSize override to assert
Math.min(100, Number(batchSize)) behavior by invoking handler with different
batchSize inputs. For CLI status tests, mock apiFetch to return memoriesRes and
sessionList, require the CLI module after mocks, call the status function, and
assert apiFetch used "memories" (not "export"), observation count derived from
sessionList[].observationCount via reduce, and memCount read from
memoriesRes.memories.length.
---
Nitpick comments:
In `@src/triggers/api.ts`:
- Around line 614-617: Replace the explanatory "WHAT" comment block about the
fan-out of the session-stopped lifecycle with intent-focused naming and minimal
logging: remove the multi-line implementation-narration and instead ensure the
code that publishes/subscribes to the agentmemory.session.stopped topic (and the
fan-out handler in this file) is given a clear identifier (e.g.,
publishSessionStoppedEvent or handleSessionStoppedFanout) and add a single-line
intent comment like "// publish session-stopped lifecycle events (non-blocking)"
or a concise log message; also ensure subscribers in events.ts remain tied to
agentmemory.session.stopped so behavior is unchanged.
🪄 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: 80d0918e-c53d-471b-a83c-992d48a53a7e
📒 Files selected for processing (6)
AGENTS.mdREADME.mdsrc/cli.tssrc/index.tssrc/triggers/api.tstest/session-end-triggers-graph.test.ts
Review fixes on #698: 1. api::session::end now wraps sdk.triggerVoid in try/catch so a thrown error in the fan-out path doesn't convert a successful kv.update into a 5xx response. kv.update already landed; the client should see success regardless of whether the downstream subscriber registration is healthy. Errors are logged via logger.warn for diagnostics. 2. Trimmed the multi-line WHAT/WHY comment block in api::session::end down to a single-line intent comment. The historical context belongs in the PR description, not in source. 3. cli.ts `agentmemory status` now uses /memories?count=true (the count-only endpoint added in #544) instead of fetching full rows. For deployments with 8K+ memories the count-only path is constant- time-ish on the server. memCount reads from latestCount (falling back to total) — matches the count endpoint shape. 4. test/session-end-triggers-graph.test.ts: - Tightened the "filters observations with title" regex to assert `typeof o.title === "string"` not just `o.title === "string"` (latter matched the source as a substring but didn't verify the typeof guard, which is the actual contract). - Updated the CLI status assertions for the count=true shape. Skipped (review item: rewrite tests as runtime vi.mock'd behavior tests): scope creep. test/memories-pagination.test.ts uses the same source-string pattern across the repo for trigger registration checks; switching test style is its own PR and the existing assertions are sufficient to catch regressions on these three specific changes.
Contributor
|
Actionable comments posted: 0 |
rohitg00
added a commit
that referenced
this pull request
May 28, 2026
Bumps version across 9 files + adds CHANGELOG entry summarizing the 18 commits since v0.9.22. Highlights: - GitHub Copilot CLI first-class support (#534) — plugin + hooks + MCP with LSP-style Content-Length framing on the standalone stdio transport. - Five new MCP adapters: Warp, Cline, Continue, Zed, Droid (#677); ADAPTERS count 11 → 17. - Three silent DX bugs fixed: graph extraction never fired on session end (#666 / #698), status reported zero memories (#666), consolidation defaulted off even with an LLM provider configured (#612 / #696). - Nine telemetry hooks switched to fire-and-forget so they don't block Claude Code's next-prompt boundary (#573 / #688). - Hook project field now sends repo basename instead of full filesystem path so auto-injected context isn't silently filtered out (#474 / #687). - Local-LLM docs: Ollama / LM Studio / vLLM section added (#671 / #697). Version-bump files: package.json, plugin/.claude-plugin/plugin.json, plugin/plugin.json, plugin/.codex-plugin/plugin.json, packages/mcp/package.json, src/version.ts, src/types.ts, src/functions/export-import.ts, test/export-import.test.ts.
rohitg00
added a commit
that referenced
this pull request
May 28, 2026
* chore(release): v0.9.23 Bumps version across 9 files + adds CHANGELOG entry summarizing the 18 commits since v0.9.22. Highlights: - GitHub Copilot CLI first-class support (#534) — plugin + hooks + MCP with LSP-style Content-Length framing on the standalone stdio transport. - Five new MCP adapters: Warp, Cline, Continue, Zed, Droid (#677); ADAPTERS count 11 → 17. - Three silent DX bugs fixed: graph extraction never fired on session end (#666 / #698), status reported zero memories (#666), consolidation defaulted off even with an LLM provider configured (#612 / #696). - Nine telemetry hooks switched to fire-and-forget so they don't block Claude Code's next-prompt boundary (#573 / #688). - Hook project field now sends repo basename instead of full filesystem path so auto-injected context isn't silently filtered out (#474 / #687). - Local-LLM docs: Ollama / LM Studio / vLLM section added (#671 / #697). Version-bump files: package.json, plugin/.claude-plugin/plugin.json, plugin/plugin.json, plugin/.codex-plugin/plugin.json, packages/mcp/package.json, src/version.ts, src/types.ts, src/functions/export-import.ts, test/export-import.test.ts. * chore(release): add #701 + #709 to v0.9.23 CHANGELOG
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
Three concrete bugs from #666, all verified line-by-line. Reporter @LeonemFu did very clean root-cause analysis — went straight to the cause for each.
Bug 1: Knowledge graph stays empty forever
The
event::session::stoppedhandler insrc/triggers/events.ts:47-81listens on theagentmemory.session.stoppedtopic and is responsible for firingmem::summarize+mem::slot-reflect+mem::graph-extractwhen a session ends. Nothing in the codebase publishes that topic.api::session::endjust runskv.update(endedAt, status=completed)and returns. So the handler is a dead subscriber and graphs / lessons / crystals never materialize.Confirmed by 3 independent reporters in the thread:
Fix:
api::session::endnow callssdk.triggerVoid("event::session::stopped", { sessionId })after the KV update so the existing pipeline fires. HTTP response stays fast —kv.updateis synchronous, downstream summarize / graph-extract fans out without blocking.Bug 2:
agentmemory statusshows Memories/Observations = 0src/cli.ts:1161-1183doesPromise.all([health, sessions, graph/stats, export, config/flags])then readsobsCount = memoriesRes?.observations?.length || 0. The problem:/agentmemory/exportconsistently times out (>5s) on iii-engine's file-based KV under concurrentkv.list()pressure — even on small datasets (121 observations / 26 sessions in the reporter's case). ThePromise.allswallows the timeout and falls back to 0.Fix: Switch the slow fetch to
/memories(already paginated per #544) and derive observation count fromsessionList.reduce((sum, s) => sum + (s.observationCount || 0), 0). Status no longer depends on the slow path.Bug 3: Viewer "Build Graph" button → 404
src/viewer/index.html:1608and:2000bothapiPost('graph/build', {})when the graph is empty or the user clicks "Rebuild". The endpoint was never registered on the server.Fix: New
api::graph-build(registered atPOST /agentmemory/graph/build) iterates every session, lists its compressed observations (filter ontitlefield — uncompressed obs aren't ready for extraction), and feeds them throughmem::graph-extractin batches (default 25, configurable via{ batchSize: N }, capped at 100). Returns{ success, sessions, batches, nodes, edges }for the viewer's progress UI.Test plan
npx vitest run test/session-end-triggers-graph.test.ts— 11/11 pass (3 groups: triggerVoid wiring, graph-build endpoint shape, cli no longer depends on /export)npx vitest run— 1278/1278 pass (integration.test.ts skipped pre-existing, consistency.test.ts now requires the bumped 125 endpoint count which is updated in src/index.ts + README.md + AGENTS.md)npm run buildcleanConsistency
Per AGENTS.md consistency rules — REST endpoint count bumped 124 → 125 in
src/index.ts, README.md, AGENTS.md.Closes #666.
Summary by CodeRabbit
Documentation
New Features
Bug Fixes / Improvements
Tests