fix: graph search silently dropping all results#937
Open
abhinav-m22 wants to merge 4 commits into
Open
Conversation
|
@abhinav-m22 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
|
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 skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an optional ChangessessionId propagation and resolution across graph extraction and retrieval
Sequence Diagram(s)sequenceDiagram
participant Extraction as graph-extract / temporal-graph-extract
participant Parse as parseGraphXml / parseTemporalGraphXml
participant Retrieval as GraphRetrieval.searchByEntities
participant Resolver as resolveSessionIds
participant KV
Extraction->>Extraction: build sessionByObsId from input observations
Extraction->>Parse: pass sessionByObsId map
Parse->>Parse: resolve sessionId for each entity node
Parse-->>Extraction: GraphNode[] with sessionId
Extraction->>KV: merge extracted nodes with sessionId
Retrieval->>Retrieval: build results, seed sessionId from node data
Retrieval->>Resolver: top-K results with sessionId hints
Resolver->>KV: verify hint in hinted session (fast path)
alt hint valid
KV-->>Resolver: observation found
else hint invalid or empty
Resolver->>KV: list known sessions (lazy, once)
loop scan sessions
Resolver->>KV: probe each session for observation
alt found
KV-->>Resolver: owning session located
end
end
end
Resolver->>Resolver: mutate result.sessionId with resolved value
Resolver-->>Retrieval: results with authoritative sessionIds
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/functions/temporal-graph.ts (1)
172-304:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd audit recording for
mem::temporal-graph-extractstate mutations.This function writes/updates graph nodes and edges but does not emit an audit entry, so state changes are not traceable in the audit stream.
Proposed fix
import { logger } from "../logger.js"; +import { recordAudit } from "./audit.js"; @@ logger.info("Temporal graph extraction complete", { nodes: nodes.length, edges: edges.length, }); + await recordAudit( + kv, + "observe", + "mem::temporal-graph-extract", + obsIds, + { + nodesExtracted: nodes.length, + edgesExtracted: edges.length, + }, + ); return { success: true, nodesAdded: nodes.length, edgesAdded: edges.length, };As per coding guidelines:
src/**/*.tsstate-changing operations should userecordAudit().🤖 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/temporal-graph.ts` around lines 172 - 304, The mem::temporal-graph-extract function performs multiple state mutations by calling kv.set() for graph nodes, graph edges, and graph edge history, but does not record any audit entries for these operations. Add recordAudit() calls after each kv.set() operation to ensure all state changes are traceable in the audit stream: one audit entry when nodes are created or updated (when setting KV.graphNodes), one when edges are created or updated (when setting KV.graphEdges), and one when edge history is recorded (when setting KV.graphEdgeHistory). Include appropriate context in each audit entry such as the action performed and the IDs affected.Source: Coding guidelines
🤖 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/graph-retrieval.ts`:
- Around line 93-95: The fallback resolution code in the sessions initialization
block does not handle potential exceptions from the kv.list call. Wrap the await
this.kv.list<Session>(KV.sessions) statement in a try-catch block to gracefully
handle list failures. In the catch block, set sessions to an empty collection as
a safe fallback instead of allowing the error to propagate. This allows
searchByEntities and expandFromChunks to continue with degraded functionality by
falling back to an unresolved sessionId ("") rather than aborting the entire
retrieval process.
---
Outside diff comments:
In `@src/functions/temporal-graph.ts`:
- Around line 172-304: The mem::temporal-graph-extract function performs
multiple state mutations by calling kv.set() for graph nodes, graph edges, and
graph edge history, but does not record any audit entries for these operations.
Add recordAudit() calls after each kv.set() operation to ensure all state
changes are traceable in the audit stream: one audit entry when nodes are
created or updated (when setting KV.graphNodes), one when edges are created or
updated (when setting KV.graphEdges), and one when edge history is recorded
(when setting KV.graphEdgeHistory). Include appropriate context in each audit
entry such as the action performed and the IDs affected.
🪄 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: 591030c8-8155-42c0-a913-84c88171a332
📒 Files selected for processing (6)
src/functions/graph-retrieval.tssrc/functions/graph.tssrc/functions/temporal-graph.tssrc/types.tstest/graph-retrieval.test.tstest/graph.test.ts
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.
sessionId: ""#925Graph search was finding the right observations and then throwing them all away.
GraphNodenever stored asessionId, sograph-retrieval.tshardcodedsessionId: ""on every result.enrichResultsthen looked them up viaKV.observations(""), got nothing, and silently dropped them — sographScorewas always null andgraphContextnever showed up. The whole graph retrieval path was effectively dead for observations.sessionIdonGraphNode, stamped at extract time (bothgraph-extractandtemporal-graph-extract)sessionId— they resolve via the same scan, so nothing needs a migration.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests