feat(visibility): surface insights + semantic facts in mem::context and smart-search#591
Closed
kmariunas wants to merge 1 commit into
Closed
Conversation
…nd smart-search (rohitg00#590) Closes rohitg00#590. Same trust-shock pattern fixed for lessons in rohitg00#458 + rohitg00#473: KV.insights and KV.semantic are produced by consolidate-pipeline/reflect but had no agent-facing surface in v0.9.21. ## smart-search (mirrors rohitg00#473) - New `includeInsights` param (default true) - Delegates to existing `mem::insight-search` via `sdk.trigger` — no duplicate scoring logic, future tweaks auto-propagate - Insights come back in a separate `insights` field, not merged into `results`. Existing consumers reading `result.results` / `result.lessons` are unaffected - Content truncated to 240 chars for compact preview (same convention as lessons) - Best-effort: catches + warns on failure, falls back to empty list ## mem::context (mirrors rohitg00#458) - Reads `KV.insights` and `KV.semantic` in parallel with existing slots/profile/lessons reads - **Insights** filtered identically to lessons (`!deleted && (!project || project === current)`), ranked `(matching ? 1.5 : 1) × confidence`, capped at top 10, rendered as `## Distilled Insights` block - **Semantic facts** have no direct `project` field; membership derived from `sourceSessionIds` via the already-fetched `allSessions` map. Filter logic: include when any source session matches the current project OR when the fact has no resolvable source sessions (cross-project knowledge / global). Excludes facts whose sources are entirely in other projects. Ranks `strength × confidence × project-relevance-boost`, capped at top 10, rendered as `## Architectural Facts` block - Block recency tracks `lastReinforcedAt || updatedAt` so hot entries survive when budget tightens (same pattern as the lessons block) ## Tests - `test/smart-search.test.ts` — 6 new cases under `insight inclusion (rohitg00#590)` mirroring the lesson inclusion suite: payload shape, content truncation, `includeInsights:false` opt-out, project forwarding, error tolerance, non-success response tolerance - `test/context-insights-semantic.test.ts` — 12 new cases split across two `describe` blocks: insight block injection (presence, absence, project ranking, cross-project isolation, deleted, top-10 cap, confidence rendering) + semantic block injection (project-source inclusion, global inclusion, cross-project exclusion, project-boost ranking, top-10 cap) Empirical motivation: on a local stack with consolidation working, 109 semantic facts + 58 insights accumulated over a day's sessions, but none were retrievable by an agent during a session — only via direct REST calls. The patch closes that loop. Signed-off-by: Karolis Mariunas <kmariunass@gmail.com>
|
@kmariunas 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 PR adds insights and semantic memories to ChangesInsights and Semantic Facts Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/smart-search.test.ts (1)
36-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
test/smart-search.test.tsSDK mocking withvi.mock("iii-sdk").
test/smart-search.test.tsdefines a localmockSdk()shim (function mockSdk() { ... }) and assignssdk = mockSdk();, but it never uses the mandatedvi.mock("iii-sdk")approach. Update the test to mockiii-sdkviavi.mock("iii-sdk"), providing a mocksdk.triggerandkv.get/set/listinstead of the custom shim.🤖 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/smart-search.test.ts` around lines 36 - 52, Replace the local shim mockSdk() and any direct assignment sdk = mockSdk() with a vi.mock("iii-sdk") call that returns the mocked module shape used in tests: export an sdk object with trigger (async function) and a kv namespace implementing get, set, and list; ensure the mocked trigger behavior matches the previous mockSdk.trigger contract and that tests import/require the module so they use the vi.mock implementation rather than the local mockSdk function; update references to registerFunction/registerTrigger if present to instead drive behavior via the mocked sdk.trigger and kv.get/set/list implementations.
🧹 Nitpick comments (2)
src/functions/context.ts (1)
142-147: ⚡ Quick winRemove WHAT-level explanatory comments and rely on naming.
Line 142-147 and Line 244-250 add narrative behavior comments. In
src/this should be expressed through naming/extraction instead of explanatory WHAT-comments.As per coding guidelines,
src/**/*.{ts,tsx}: Avoid code comments explaining WHAT — use clear naming instead.Also applies to: 244-250
🤖 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 142 - 147, Remove the WHAT-level narrative comments around the insights logic (the block mentioning "Insights — mirrors the lessons round-trip closure" and the similar comment at the later block) and encode the behavior in names or small extracted helpers instead; e.g., rename the variable/constant or wrap the logic in a clearly named function like computeProjectScopedInsights or rankAndCapInsights (references: mem::reflect, memory_insight_list, and the insights-ranking logic) so the code reads self-descriptively without explanatory WHAT-comments.src/functions/smart-search.ts (1)
90-95: ⚡ Quick winDrop WHAT-level inline commentary in
src/code.Line 90-95 explains behavior rather than encoding intent in code structure/naming.
As per coding guidelines,
src/**/*.{ts,tsx}: 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/smart-search.ts` around lines 90 - 95, The inline comment describing WHAT the block does should be removed and its intent encoded in code: extract the parallel side-channel work into a clearly named helper like runBestEffortSideChannelLookups (or runSideChannelLookupsInParallel) that invokes hybridSearch, lessonRecall, and insightSearch, returns empty fallbacks on errors, and documents only WHY if needed; replace the comment with that function call so the code's name and return shape communicate the behavior instead of the inline WHAT-level commentary.
🤖 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/smart-search.ts`:
- Around line 33-34: Validate includeInsights and includeLessons immediately at
the handler boundary by checking their types explicitly (e.g., const
insightsEnabled = typeof includeInsights === 'boolean' ? includeInsights : false
and const lessonsEnabled = typeof includeLessons === 'boolean' ? includeLessons
: false) and use insightsEnabled / lessonsEnabled for all subsequent branching
instead of trusting the raw params; update the handler parameter processing and
any branches that currently read includeInsights or includeLessons so only a
literal boolean true enables the feature.
In `@test/context-insights-semantic.test.ts`:
- Around line 33-43: The test currently fabricates an SDK object and force-casts
it to import("iii-sdk").ISdk in wireContext; instead, call vi.mock("iii-sdk") at
top of the test file and use the mocked SDK to set up registerFunction and
trigger behaviours, then pass that mocked SDK into registerContextFunction
(instead of the casted object). Specifically: remove the "as unknown as
import('iii-sdk').ISdk" cast and the handcrafted sdk object, replace with the
vi.mock'd module’s mock (set registerFunction = vi.fn(...) to capture the
ContextHandler like you do now, and add a mock implementation for sdk.trigger),
ensure the mockKV provides get/set/list and is passed in unchanged, then call
registerContextFunction(sdkMock, kv, budget) and use the captured handler; this
follows the repo guideline and enables proper testing of sdk.trigger and kv
interactions.
---
Outside diff comments:
In `@test/smart-search.test.ts`:
- Around line 36-52: Replace the local shim mockSdk() and any direct assignment
sdk = mockSdk() with a vi.mock("iii-sdk") call that returns the mocked module
shape used in tests: export an sdk object with trigger (async function) and a kv
namespace implementing get, set, and list; ensure the mocked trigger behavior
matches the previous mockSdk.trigger contract and that tests import/require the
module so they use the vi.mock implementation rather than the local mockSdk
function; update references to registerFunction/registerTrigger if present to
instead drive behavior via the mocked sdk.trigger and kv.get/set/list
implementations.
---
Nitpick comments:
In `@src/functions/context.ts`:
- Around line 142-147: Remove the WHAT-level narrative comments around the
insights logic (the block mentioning "Insights — mirrors the lessons round-trip
closure" and the similar comment at the later block) and encode the behavior in
names or small extracted helpers instead; e.g., rename the variable/constant or
wrap the logic in a clearly named function like computeProjectScopedInsights or
rankAndCapInsights (references: mem::reflect, memory_insight_list, and the
insights-ranking logic) so the code reads self-descriptively without explanatory
WHAT-comments.
In `@src/functions/smart-search.ts`:
- Around line 90-95: The inline comment describing WHAT the block does should be
removed and its intent encoded in code: extract the parallel side-channel work
into a clearly named helper like runBestEffortSideChannelLookups (or
runSideChannelLookupsInParallel) that invokes hybridSearch, lessonRecall, and
insightSearch, returns empty fallbacks on errors, and documents only WHY if
needed; replace the comment with that function call so the code's name and
return shape communicate the behavior instead of the inline WHAT-level
commentary.
🪄 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: 5f3ae1e3-a17c-4a2e-aa74-ef7b775ebbd9
📒 Files selected for processing (5)
src/functions/context.tssrc/functions/smart-search.tssrc/types.tstest/context-insights-semantic.test.tstest/smart-search.test.ts
Comment on lines
+33
to
34
| includeInsights?: boolean; | ||
| }) => { |
Contributor
There was a problem hiding this comment.
Validate includeInsights as a boolean at the handler boundary.
Line 88 treats every value except literal false as enabled (e.g., "false" or 0 still enables insights). Add an explicit type guard for includeInsights (and ideally includeLessons) before branching.
As per coding guidelines, src/**/*.{ts,tsx}: Perform input validation at system boundaries (MCP handlers and REST endpoints).
Also applies to: 87-89
🤖 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/smart-search.ts` around lines 33 - 34, Validate includeInsights
and includeLessons immediately at the handler boundary by checking their types
explicitly (e.g., const insightsEnabled = typeof includeInsights === 'boolean' ?
includeInsights : false and const lessonsEnabled = typeof includeLessons ===
'boolean' ? includeLessons : false) and use insightsEnabled / lessonsEnabled for
all subsequent branching instead of trusting the raw params; update the handler
parameter processing and any branches that currently read includeInsights or
includeLessons so only a literal boolean true enables the feature.
Comment on lines
+33
to
+43
| function wireContext(kv: ReturnType<typeof mockKV>, budget = 4000) { | ||
| let handler: ContextHandler | undefined; | ||
| const sdk = { | ||
| registerFunction: vi.fn((id: string, cb: ContextHandler) => { | ||
| if (id === "mem::context") handler = cb; | ||
| }), | ||
| } as unknown as import("iii-sdk").ISdk; | ||
| registerContextFunction(sdk, kv as never, budget); | ||
| if (!handler) throw new Error("mem::context not registered"); | ||
| return handler; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify required iii-sdk mock pattern in changed tests
rg -n --type=ts 'vi\.mock\("iii-sdk"\)' test/context-insights-semantic.test.ts test/smart-search.test.ts
rg -n --type=ts 'as unknown as import\("iii-sdk"\)\.ISdk|function mockSdk\(' test/context-insights-semantic.test.ts test/smart-search.test.tsRepository: rohitg00/agentmemory
Length of output: 198
Use vi.mock("iii-sdk") in test/context-insights-semantic.test.ts (avoid ISdk casting).
test/context-insights-semantic.test.ts casts a handcrafted object to import("iii-sdk").ISdk (as unknown as import("iii-sdk").ISdk at line 39), and it doesn’t use the required vi.mock("iii-sdk") test mocking pattern for sdk.trigger / kv.get/set/list. Refactor the test to mock iii-sdk via vi.mock("iii-sdk") like the repo’s guideline expects.
🤖 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/context-insights-semantic.test.ts` around lines 33 - 43, The test
currently fabricates an SDK object and force-casts it to import("iii-sdk").ISdk
in wireContext; instead, call vi.mock("iii-sdk") at top of the test file and use
the mocked SDK to set up registerFunction and trigger behaviours, then pass that
mocked SDK into registerContextFunction (instead of the casted object).
Specifically: remove the "as unknown as import('iii-sdk').ISdk" cast and the
handcrafted sdk object, replace with the vi.mock'd module’s mock (set
registerFunction = vi.fn(...) to capture the ContextHandler like you do now, and
add a mock implementation for sdk.trigger), ensure the mockKV provides
get/set/list and is passed in unchanged, then call
registerContextFunction(sdkMock, kv, budget) and use the captured handler; this
follows the repo guideline and enables proper testing of sdk.trigger and kv
interactions.
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.
Closes #590. Same pattern as #458 + #473, applied to
KV.insightsandKV.semantic.smart-search: newincludeInsights(default true), parallelmem::insight-search, returnsinsightsfield — direct mirror of the lesson piggyback in #473.mem::context: readsKV.insights+KV.semanticin parallel with existing reads.!deleted && (!project || project === current)), ranked(matching ? 1.5 : 1) × confidence, top 10,## Distilled Insightsblock.sourceSessionIdsvia the already-fetchedallSessionsmap. Cross-project facts surface as global; sources entirely in other projects are excluded. Rankedstrength × confidence × boost, top 10,## Architectural Factsblock.+18 tests (smart-search +6, new
test/context-insights-semantic.test.ts+12). All pass. No existing test broken.