fix(slots): wire pinned slot injection into mem::context#288
Conversation
`renderPinnedContext` and `listPinnedSlots` were introduced in rohitg00#182 and tested in `test/slots.test.ts`, but no code path actually invokes them. `mem::context` (which `/agentmemory/session/start` calls, and which the `session-start.mjs` hook reads back to populate Claude Code's SessionStart context) reads profile / sessions / observations but never reads slots — so pinned content stays in KV and never reaches the conversation. rohitg00#182's description hedged with "pinned slots **can be** injected into SessionStart context" and described pinned slots as "**candidates for** SessionStart injection". The helpers and tests for them landed; the wiring did not. Reflection has the same shape of problem: `mem::slot-reflect` writes to `pending_items` / `session_patterns` / `project_context` on the Stop hook, but the next session never reads those slots, so the reflect → next-session loop is open. This adds the missing call at the top of `mem::context`, gated by `isSlotsEnabled()` so behaviour for `AGENTMEMORY_SLOTS=false` users is unchanged. Slot content is pushed as a `ContextBlock` with `recency = Date.now()` so pinned content ranks first in the budget-bounded selection — "always current" semantics. The existing `INJECT_CONTEXT` gate in `session-start.mjs` still decides whether the result actually reaches Claude Code stdout, so default-off users are unaffected on both flags. `test/context-slots.test.ts` covers: - pinned global slot lands in returned context - multiple pinned slots render in label-sorted order - unpinned slots are excluded even when they have content - empty pinned slots (the seeded defaults) are excluded - project-scoped slot shadows global slot with the same label - AGENTMEMORY_SLOTS=off path emits nothing Signed-off-by: wyh0626 <44987669+wyh0626@users.noreply.github.com>
|
@wyh0626 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughConditionally fetches and renders pinned slot content into the mem::context output when AGENTMEMORY_SLOTS is enabled; adds a memory block with token count and recency only if rendered content is non-empty. Tests verify enabled/disabled behavior and precedence between project and global slots. ChangesPinned Slots Context Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/functions/context.ts (1)
41-52: ⚡ Quick winConsider parallelizing independent KV reads.
The
listPinnedSlotscall and the profile fetch (line 54-56) are independent operations reading from different KV scopes. Per the coding guideline, these could be executed in parallel usingPromise.allto reduce latency.⚡ Proposed refactor to parallelize KV reads
- if (isSlotsEnabled()) { - const pinned = await listPinnedSlots(kv).catch(() => []); - const slotContent = renderPinnedContext(pinned); - if (slotContent) { - blocks.push({ - type: "memory", - content: slotContent, - tokens: estimateTokens(slotContent), - recency: Date.now(), - }); - } - } - - const profile = await kv - .get<ProjectProfile>(KV.profiles, data.project) - .catch(() => null); + const [pinnedSlots, profile] = await Promise.all([ + isSlotsEnabled() + ? listPinnedSlots(kv).catch(() => []) + : Promise.resolve([]), + kv.get<ProjectProfile>(KV.profiles, data.project).catch(() => null), + ]); + + if (pinnedSlots.length > 0) { + const slotContent = renderPinnedContext(pinnedSlots); + if (slotContent) { + blocks.push({ + type: "memory", + content: slotContent, + tokens: estimateTokens(slotContent), + recency: Date.now(), + }); + } + } + if (profile) {As per coding guidelines: "Use parallel operations where possible with Promise.all for independent kv writes/reads" for files in
src/functions/**/*.ts.🤖 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/context.ts` around lines 41 - 52, The two independent KV reads (listPinnedSlots and the profile fetch used just after this block) should run in parallel using Promise.all to reduce latency: call Promise.all([listPinnedSlots(kv), fetchProfile(kvOrScope)]) (replace fetchProfile with the actual profile-fetch function used in the file), then destructure the results into pinned and profile; keep the existing fallback for pinned (empty array) and handle a missing profile as before, then call renderPinnedContext(pinned) and push the memory block if slotContent exists; ensure you preserve estimateTokens(slotContent) and recency timestamp and maintain any existing error handling for each read (e.g., map the pinned promise to .catch(() => []) if you want that specific fallback).
🤖 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/context.ts`:
- Around line 41-52: The two independent KV reads (listPinnedSlots and the
profile fetch used just after this block) should run in parallel using
Promise.all to reduce latency: call Promise.all([listPinnedSlots(kv),
fetchProfile(kvOrScope)]) (replace fetchProfile with the actual profile-fetch
function used in the file), then destructure the results into pinned and
profile; keep the existing fallback for pinned (empty array) and handle a
missing profile as before, then call renderPinnedContext(pinned) and push the
memory block if slotContent exists; ensure you preserve
estimateTokens(slotContent) and recency timestamp and maintain any existing
error handling for each read (e.g., map the pinned promise to .catch(() => [])
if you want that specific fallback).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c97c886-7838-44a3-81bd-989a6f59df9c
📒 Files selected for processing (2)
src/functions/context.tstest/context-slots.test.ts
Per CodeRabbit feedback on rohitg00#288: `listPinnedSlots` and the profile fetch read independent KV scopes, so the project's `src/functions/**` guideline asks for `Promise.all`. When `AGENTMEMORY_SLOTS=false` the slot side short-circuits to `Promise.resolve([])`, so the existing flag gate is preserved and no extra KV traffic is introduced for opt-out users. Signed-off-by: wyh0626 <44987669+wyh0626@users.noreply.github.com>
|
Thanks @wyh0626 |
…290) Two field-reported fixes landed since v0.9.8: - #288 (closes #286) — wires pinned memory slots into mem::context so agentmemory pinned-slot content actually reaches SessionStart. The renderPinnedContext / listPinnedSlots helpers from #182 had zero callers; this lands the wiring behind isSlotsEnabled(). - #289 (closes #285) — MiniMax provider now reads MINIMAX_BASE_URL via the shared getEnvVar() loader (so ~/.agentmemory/.env values get picked up), and the default endpoint is bumped from the stale api.minimaxi.com to api.minimax.io/anthropic per MiniMax's current Anthropic-compatible docs. Bumping 0.9.8 -> 0.9.9 across the 8 standard files (package.json, packages/mcp/package.json, plugin/.claude-plugin/plugin.json, src/version.ts, src/types.ts ExportData literal, src/functions/export-import.ts supportedVersions, the export round-trip test expectation, and CHANGELOG.md). 868 / 868 tests pass.
…, #301) (#304) @flamerged reported three issues on a real production deployment: 1. (#301) v0.9.7's working-directory fix moved iii-config paths from ./data/... to /data/... so the named volume mount is actually reached. But iiidev/iii is distroless and runs as UID 65532, while `docker volume create` initializes the named volume mountpoint as root:root mode 755. Engine writes fail Permission denied (os error 13), the API silently buffers in RAM, every API call returns success, and state evaporates on every container restart — exactly what 0.9.7 set out to fix. Fix: ship a one-shot iii-init service in docker-compose.yml (busybox:1.36, ~4MB, exits in <100ms) that chowns /data to 65532:65532. iii-engine now has user: "65532:65532" and depends_on.iii-init.condition: service_completed_successfully. Verified live: pre-fix volume stayed 4.0K after API writes; post- fix volume grows to 44K with state_store.db/mem%3A*.bin files written through the named volume. 2. (#299) src/viewer/index.html ports detection hardcoded ':3113' as the fallback when window.location.port is empty (page served on 80/443 behind a reverse proxy). Every browser-side /agentmemory/* fetch went to <host>:3113, which is typically loopback-only on the self-hosted shape — the dashboard rendered cleanly but every panel showed the empty "first run" state. Fix: when neither ?port=N nor window.location.port is set, use window.location.origin as the REST base and window.location.host for the WebSocket URL — same-origin path works for both REST and live updates. Explicit ?port=N / non-default window.location.port paths unchanged. 3. (bundled) mem::context budget loop used `break` on first oversized block. With #288's new pinned-slot injection sorting first via recency: Date.now(), one fat pinned slot could starve every smaller block downstream that would have fit. Switched to `continue` so smaller blocks still slip into remaining budget. Total tokens still bounded by tokenBudget; only composition under contention changes. Bumping 0.9.9 -> 0.9.10 across the 8 standard files (package.json, packages/mcp/package.json, plugin/.claude-plugin/plugin.json, src/version.ts, src/types.ts ExportData literal, src/functions/export-import.ts supportedVersions, the export round-trip test expectation, and CHANGELOG.md). 868 / 868 tests pass. Build clean. Volume + viewer fixes verified end-to-end live.
Summary
Closes #286.
renderPinnedContextandlistPinnedSlotswere introduced in #182 and tested intest/slots.test.ts, but no code path actually invokes them.mem::context— the function/agentmemory/session/startcalls and thatsession-start.mjsreads back into Claude Code — reads profile / sessions / observations but never reads slots, so pinned content stays in KV and never reaches the conversation.#182's description hedged with "pinned slots can be injected into SessionStart context" and "pinned slots are candidates for SessionStart injection". The helpers and unit tests landed; the wiring didn't.
Fix
Add the
listPinnedSlots→renderPinnedContextcall at the top ofmem::context, gated byisSlotsEnabled():recency: Date.now()so the pinned block sorts to the top of the budget-bounded selection. Mirrors howisGraphExtractionEnabled()guardsmem::graph-extractafter #215.AGENTMEMORY_SLOTS=false(the default) andAGENTMEMORY_INJECT_CONTEXT=false(also the default) both keep the existing behaviour untouched — the gate composition matches the established opt-in convention (#138, #143, #160, #179).On cross-project visibility
This PR uses the same
listPinnedSlots(kv)reader as the existingmem::slot-list/memory_slot_list/memory_slot_getAPIs and themem::slot-reflectwriter. Project-scoped slot storage (KV.slots = "mem:slots") is keyed by label without a project prefix today, so cross-project visibility already exists via the slot list/get tools and via reflect. This PR doesn't change the underlying isolation model; if tighter per-project keying is wanted, that's an orthogonal concern in the slot storage layer.Verify
Before:
"context": ""(0 chars).After:
contextcontains thetool_guidelinesslot wrapped in<agentmemory-context>.Tests
6 cases in
test/context-slots.test.ts:AGENTMEMORY_SLOTS=offpath emits nothingnpm testfor the file: 6/6 pass. Pre-existing failures intest/mcp-standalone.test.tsandtest/fs-watcher.test.tsare unaffected by this change.Summary by CodeRabbit
Release Notes
New Features
Tests