Feature/local setup#201
Conversation
|
✅ Staging Deployment Report
🟢 Staging is live and healthy! Test your changes at the staging URL above. Ready to ship? Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the benchmarking system and ports several core agents (Classifier, Code, Image, Judge, etc.) from Python to Go to ensure feature parity. It introduces a robust tracing and metrics collector for LLM calls, adds multimodal vision support across multiple providers, and enhances the Neo4j storage layer with relationship-based event tracking and vector search. Feedback identifies several critical issues, including a missing method in the Go implementation that will cause compilation errors, logic flaws in Neo4j event updates and creation that could lead to data duplication or loss, and incorrect handling of image data for Gemini and Claude vision APIs. Additionally, improvements are needed for database performance via indexing and optimizing API call placement within retry logic.
| incoming := temporalFieldsFromContent(itemStr) | ||
| existing := temporalFieldsFromMatch(match) | ||
| if sameTemporalEvent(incoming, existing) { | ||
| ops = append(ops, weaver.Operation{Type: weaver.OperationNoop, Content: itemStr, EmbeddingID: match.EmbeddingID(), Reason: "Existing temporal event is unchanged."}) |
There was a problem hiding this comment.
The EmbeddingID() method appears to be missing from the graph.Event struct definition in internal/graph/types.go. This will cause a compilation error. Please ensure the method is implemented (e.g., returning a combination of date and event name) or access the identifying fields directly.
| func (s *Neo4jTemporalStore) initSchema(ctx context.Context) { | ||
| constraints := []string{ | ||
| "CREATE CONSTRAINT user_id_unique IF NOT EXISTS FOR (u:User) REQUIRE u.user_id IS UNIQUE", | ||
| "CREATE CONSTRAINT date_val_unique IF NOT EXISTS FOR (d:Date) REQUIRE d.date IS UNIQUE", |
There was a problem hiding this comment.
The SearchEventsByEmbedding method relies on vector.similarity.cosine, which requires a vector index for efficient execution. Without an index, Neo4j will perform a full scan of all relationships, leading to significant performance degradation as the database grows. Consider adding vector index creation to initSchema.
| MATCH (u:User {user_id: $user_id}) | ||
| -[r:HAS_EVENT]-> | ||
| (d:Date {date: $date_str}) |
There was a problem hiding this comment.
The MATCH clause in updateEventOnce is missing a filter for the specific event name. As currently implemented, it will match and update all HAS_EVENT relationships between the user and the specified date, which will overwrite unrelated events if multiple events exist on the same day.
| if imageURL != "" { | ||
| parts = append(parts, map[string]any{ | ||
| "inline_data": map[string]string{"mime_type": "image/jpeg", "data": imageURL}, | ||
| }) |
There was a problem hiding this comment.
The Gemini API's inline_data field expects base64-encoded image data in the data field, not a URL. If imageURL is a web URL, this request will fail. You should either fetch and encode the image or use a supported method for providing URLs if available.
| contentBlocks = append(contentBlocks, map[string]any{ | ||
| "type": "image", "source": map[string]string{"type": "url", "url": imageURL}, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The Anthropic (Claude) Messages API does not support type: "url" for image sources. It requires type: "base64" with the image data. This implementation will fail when provided with a web URL.
|
|
||
| func (s *Neo4jTemporalStore) CreateEvent(ctx context.Context, userID string, date string, event Event) error { | ||
| return s.upsertEvent(ctx, userID, date, event) | ||
| return s.withRetry(ctx, func() error { |
There was a problem hiding this comment.
The embedding API call (s.embedder.Embed) is performed inside the withRetry loop (via createEventOnce). If the database operation fails and triggers a retry, it will result in redundant and potentially costly embedding requests. The embedding should be generated once before entering the retry logic.
| _, err := neo4j.ExecuteQuery(ctx, s.driver, ` | ||
| MERGE (u:User {user_id: $user_id}) | ||
| MERGE (d:Date {date: $date_str}) | ||
| CREATE (u)-[r:HAS_EVENT]->(d) |
There was a problem hiding this comment.
Using CREATE for the relationship will result in duplicate HAS_EVENT relationships if CreateEvent is called multiple times for the same event. Consider using MERGE with the event_name property to ensure idempotency and data integrity.
| MATCH (u:User {user_id: $user_id}) | ||
| -[r:HAS_EVENT]-> | ||
| (d:Date) | ||
| WHERE toLower(r.event_name) = toLower($event_name) |
There was a problem hiding this comment.
Changing from CONTAINS to an exact equality check (=) is a regression in search flexibility. Users searching for a partial event name will no longer find relevant results. Reverting to CONTAINS is recommended for better usability.
|
| Filename | Overview |
|---|---|
| scripts/xmem.js | Adds ensureVirtualenv() to verify pip availability after venv creation and repair it via ensurepip; cleanly replaces the old inline venv check in runSetup. |
| xmem-go/internal/api/auth.go | Only change is swapping encoding/json for goccy/go-json; pre-existing validateJWT never checks the exp claim, so expired tokens authenticate indefinitely. |
| xmem-go/internal/models/models.go | NewRegistry now returns (ChatModel, error) instead of panicking silently when no provider is configured; HTTP/2 transport and multi-provider support look correct. |
| xmem-go/internal/agents/temporal.go | New isValidDate uses daysInMonth map for tighter validation but sets February to 29, accepting 02-29 in non-leap years when paired with any Year value. |
| xmem-go/internal/agents/judge.go | Adds JudgeTemporal method using the new TemporalStore field; deduplication, fallback paths, and LLM parse fallback all look correct. |
| xmem-go/internal/graph/neo4j.go | Adds embedder-backed cosine-similarity search for temporal events with keyword fallback; retry wrapper, schema init, and CRUD operations all look sound. |
| xmem-go/cmd/xmem/bootstrap.go | Refactors embedder/temporal store initialisation with proper production vs. dev guards; NewRegistry error is now propagated correctly. |
| xmem-go/cmd/smoke_test/trace.go | New cloud-connected tracing binary (build-ignored); PrintSummary reads mc.entries and mc.pipelineTimings without holding the mutex, creating a data race when called concurrently with ongoing agent calls. |
| benchmark.py | Major overhaul: removes in-memory fakes and per-agent micro-benchmarks in favour of full-pipeline tracing with per-agent token metrics and cost estimates. |
| xmem-go/internal/agents/base.go | Extracts shared callModel helper with a 45s per-call timeout; clean separation from the deleted monolithic agents.go. |
| src/prompts/temporal.py | Adds today -> use CONTEXT_DATE resolution rule to the temporal system prompt; change is minimal and consistent with the new few-shot example. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[npm run setup] --> B{venv Python exists?}
B -- No --> C[Create .venv via python -m venv]
C --> D{pip available?}
B -- Yes --> D
D -- No --> E[Run ensurepip --upgrade]
E --> F{pip now available?}
F -- No --> G[FAIL: reinstall Python]
F -- Yes --> H[pip install dependencies]
D -- Yes --> H
H --> I[Setup complete]
Comments Outside Diff (3)
-
xmem-go/internal/api/auth.go, line 41-72 (link)JWT expiry (
exp) never validatedvalidateJWTverifies the HMAC signature and decodes the payload, but never inspects theexpclaim. Any token that was legitimately issued in the past will continue to authenticate users indefinitely after it expires. An attacker who captures an old token (e.g., from logs or a leaked header) can reuse it forever. -
xmem-go/cmd/smoke_test/trace.go, line 1150-1227 (link)PrintSummaryreads shared state without holding the mutexmc.entriesandmc.pipelineTimingsare iterated withoutmc.mu.Lock(), whileRecordandRecordPipelinemutate those same fields under the lock on other goroutines. This is a data race even though the file carries//go:build ignore— running the trace tool alongside concurrent agent calls will produce non-deterministic output or a panic. -
xmem-go/internal/agents/temporal.go, line 13-16 (link)daysInMonthtreats February as always 29 daysThe map hard-codes February as
29, soisValidDateaccepts02-29for any input, including years that are not leap years. Because theEventstruct tracksYearseparately, a non-leap-year event could be stored with an invalid date (e.g.,Year: "2025",Date: "02-29") and later fail to parse or mismatch when correlated with calendar lookups.
Reviews (1): Last reviewed commit: "Repaired incomplete local virtualenv set..." | Re-trigger Greptile
1 participant
Repaired virtual env setup while setting up local