Repository navigation
Feat/vector search - #1
Conversation
📝 WalkthroughWalkthroughThe PR extends the log system with semantic search via vector embeddings. A new ChangesVector Embedding and Semantic Search
Sequence DiagramsequenceDiagram
participant Client
participant LogService
participant OllamaEmbedder
participant Store
participant Ollama as Ollama API
rect rgba(0, 100, 200, 0.5)
note right of Client: SendLog (embedding on ingest)
Client->>LogService: SendLog(Message)
LogService->>OllamaEmbedder: Embed(Message)
OllamaEmbedder->>Ollama: POST /api/embed {model, input}
Ollama-->>OllamaEmbedder: {Embeddings: [[...]]}
OllamaEmbedder-->>LogService: []float32
LogService->>Store: InsertLog(req, embedding)
Store-->>LogService: ok
LogService-->>Client: SendLogResponse
end
rect rgba(200, 100, 0, 0.5)
note right of Client: SimilarLogs (vector distance search)
Client->>LogService: SimilarLogs(query, limit)
LogService->>OllamaEmbedder: Embed(query)
OllamaEmbedder->>Ollama: POST /api/embed {model, input}
Ollama-->>OllamaEmbedder: {Embeddings: [[...]]}
OllamaEmbedder-->>LogService: []float32
LogService->>Store: SimilarLogs(embedding, limit)
Store-->>LogService: []*pb.Log (ordered by distance)
LogService-->>Client: GetLogsResponse
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@cmd/server/main.go`:
- Around line 26-28: The OllamaEmbedder instantiation at line 26 is hardcoded
with no fallback mechanism, and the synchronous embedding call in SendLog
(internal/grpc/log_service.go) blocks log ingestion on any Ollama failure. To
make this resilient, create a configurable embedder selection mechanism in the
main function that allows choosing between the OllamaEmbedder and a no-op
fallback embedder via a command-line flag or configuration option. Pass the
selected embedder to grpc.NewLogService at line 28, and optionally refactor the
SendLog method to decouple embedding from persistence by making it asynchronous
or queue-based so that log storage succeeds even if embedding fails.
In `@internal/embeddings/ollama.go`:
- Around line 34-59: Remove the error suppression in the json.Marshal call and
handle the error return value appropriately. Add validation of the HTTP response
status code (check resp.StatusCode) immediately after the resp, err assignment
and before attempting to decode the response body to prevent panics from error
responses. After decoding the response into res with json.NewDecoder, validate
that res.Embeddings is not nil and contains at least one element before
accessing res.Embeddings[0] to prevent index out of bounds panics. Additionally,
remove all the debug print statements (the three fmt.Println calls) before
returning the embeddings.
- Around line 32-43: The HTTP client instantiated in the embedding call (the
`client := &http.Client{}` line) has no timeout configured, which allows slow or
stuck Ollama services to block indefinitely. Set the Timeout field on the
http.Client struct to an appropriate duration value (such as 30 seconds) when
creating the client to ensure that embedding requests complete or fail within a
reasonable timeframe.
In `@internal/grpc/log_service.go`:
- Around line 36-42: The SendLog method returns an error immediately when
s.embedder.Embed fails, preventing log persistence in s.store.InsertLog from
occurring. To make embedding best-effort, catch the error from s.embedder.Embed
when creating the embedding for req.Message, log the embedding failure for
observability, but allow the function to continue and call s.store.InsertLog
with the log data anyway (potentially with a nil or empty embedding value). This
ensures logs are persisted to storage even when the embedder is unavailable or
experiencing issues, rather than blocking the entire ingestion pipeline.
In `@internal/storage/postgres.go`:
- Around line 29-32: In the InsertLog method of the Store type, add validation
for the embedding dimension before attempting to create the pgvector. Check that
the length of the embedding slice matches the expected dimension (768 as per
your database schema). If the dimension is incorrect, return an error
immediately with a clear message describing the expected and actual dimensions.
This validation should occur before the pgvector.NewVector call to fail fast and
provide better error messages.
In `@Makefile`:
- Line 1: In the `.PHONY` declaration on the first line, remove the duplicate
`proto` target and correct the misspelled `migate-create` to `migrate-create`.
The corrected line should list each phony target exactly once: proto,
migrate-up, migrate-down, and migrate-create.
🪄 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: 7e1ebd4c-92e5-4465-b6ef-0ea982b88d70
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
Makefilecmd/server/main.godocker-compose.ymlgo.modinternal/embeddings/embedder.gointernal/embeddings/ollama.gointernal/grpc/log_service.gointernal/storage/postgres.gomigrations/20260617124840_add_embeddings.down.sqlmigrations/20260617124840_add_embeddings.up.sql
| embedder := embeddings.NewOllamaEmbedder("nomic-embed-text") | ||
|
|
||
| logService := grpc.NewLogService(store, embedder) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== SendLog call order (Embed before InsertLog) ==="
rg -n -C3 'Embed\(|InsertLog\(|return nil, err' internal/grpc/log_service.go
echo
echo "=== Hardcoded Ollama endpoint/model usage ==="
rg -n -C2 'localhost:11434|nomic-embed-text|NewOllamaEmbedder\(' internal/embeddings/ollama.go cmd/server/main.go
echo
echo "=== Check whether config exposes embedding toggles/endpoint/model ==="
fd -i 'config*.go' internal/config | xargs -r rg -n -C2 'embedding|embed|ollama|model|endpoint|enabled'Repository: Aneeshie/Loom
Length of output: 1606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking for other embedder implementations ==="
find . -type f -name "*.go" | xargs grep -l "Embedder" | head -20
echo
echo "=== Embedder interface definition ==="
rg -n "type.*Embedder.*interface|func.*Embed\(" --type go -A 3
echo
echo "=== Error handling in SendLog and broadcast ==="
rg -n "SendLog|broadcast" internal/grpc/log_service.go -B 2 -A 8
echo
echo "=== Checking for async/queue patterns ==="
rg -n "goroutine|channel|async|queue|worker" cmd/server/main.go internal/grpc/log_service.goRepository: Aneeshie/Loom
Length of output: 1870
Embedding wiring makes log ingestion brittle to Ollama outages.
At Line 26 and Line 28, the service uses a single hardcoded Ollama embedder. In SendLog (internal/grpc/log_service.go:36–38), embedding is called synchronously before persistence, and any Ollama failure immediately returns an error to the client, blocking log ingestion entirely.
The codebase has no fallback implementation, configurable toggle, async mechanism, or alternative embedder. Only OllamaEmbedder exists, hardcoded to localhost:11434 with model nomic-embed-text, and no configuration surface exposes these values or resilience options.
Make this path resilient—for example: provide a no-op/fallback embedder option (configurable via flag or config), or decouple embedding from persistence (async pattern or queue-based).
🤖 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 `@cmd/server/main.go` around lines 26 - 28, The OllamaEmbedder instantiation at
line 26 is hardcoded with no fallback mechanism, and the synchronous embedding
call in SendLog (internal/grpc/log_service.go) blocks log ingestion on any
Ollama failure. To make this resilient, create a configurable embedder selection
mechanism in the main function that allows choosing between the OllamaEmbedder
and a no-op fallback embedder via a command-line flag or configuration option.
Pass the selected embedder to grpc.NewLogService at line 28, and optionally
refactor the SendLog method to decouple embedding from persistence by making it
asynchronous or queue-based so that log storage succeeds even if embedding
fails.
| client := &http.Client{} | ||
| payload := embedRequest{Model: e.model, Input: text} | ||
| body, _ := json.Marshal(payload) | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, "POST", "http://localhost:11434/api/embed", bytes.NewBuffer(body)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := client.Do(req) | ||
| if err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "ollama.go" | grep -E "embeddings|embed" | head -20Repository: Aneeshie/Loom
Length of output: 89
🏁 Script executed:
cat -n ./internal/embeddings/ollama.goRepository: Aneeshie/Loom
Length of output: 1668
🏁 Script executed:
rg "SendLog" --type go -B 2 -A 2Repository: Aneeshie/Loom
Length of output: 10245
🏁 Script executed:
cat -n ./internal/grpc/log_service.go | head -40Repository: Aneeshie/Loom
Length of output: 1128
Add an explicit HTTP timeout for embedding calls.
The HTTP client instantiated on each call has no timeout, so a slow or stuck Ollama service can block the SendLog handler indefinitely and exhaust request handling capacity.
Suggested fix
import (
"bytes"
"context"
"encoding/json"
"fmt"
"net/http"
+ "time"
)
@@
type OllamaEmbedder struct {
model string
+ client *http.Client
}
@@
return &OllamaEmbedder{
- model: model,
+ model: model,
+ client: &http.Client{Timeout: 10 * time.Second},
}
}
@@
- client := &http.Client{}
@@
- resp, err := client.Do(req)
+ resp, err := e.client.Do(req)🤖 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 `@internal/embeddings/ollama.go` around lines 32 - 43, The HTTP client
instantiated in the embedding call (the `client := &http.Client{}` line) has no
timeout configured, which allows slow or stuck Ollama services to block
indefinitely. Set the Timeout field on the http.Client struct to an appropriate
duration value (such as 30 seconds) when creating the client to ensure that
embedding requests complete or fail within a reasonable timeframe.
| body, _ := json.Marshal(payload) | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, "POST", "http://localhost:11434/api/embed", bytes.NewBuffer(body)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| defer resp.Body.Close() | ||
|
|
||
| var res embedResponse | ||
|
|
||
| if err := json.NewDecoder(resp.Body).Decode(&res); err != nil { | ||
| return nil, err | ||
| } | ||
| fmt.Println("this is before returning") | ||
|
|
||
| fmt.Println(res.Model) | ||
| fmt.Println(len(res.Embeddings[0])) | ||
|
|
||
| return res.Embeddings[0], nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "ollama.go" | head -20Repository: Aneeshie/Loom
Length of output: 89
🏁 Script executed:
fd -t f "ollama\.go"Repository: Aneeshie/Loom
Length of output: 87
🏁 Script executed:
cat -n internal/embeddings/ollama.goRepository: Aneeshie/Loom
Length of output: 1668
Guard error responses and empty embeddings before indexing.
This code has critical safety issues. Line 34 silently ignores json.Marshal errors, and lines 57 and 59 will panic when res.Embeddings is empty or nil. Additionally, there is no HTTP status code validation before attempting to decode the response, meaning error responses from Ollama could result in a panic when accessing res.Embeddings[0].
Suggested fix
- body, _ := json.Marshal(payload)
+ body, err := json.Marshal(payload)
+ if err != nil {
+ return nil, err
+ }
@@
resp, err := client.Do(req)
if err != nil {
return nil, err
}
@@
+ if resp.StatusCode != http.StatusOK {
+ return nil, fmt.Errorf("ollama embed returned status %d", resp.StatusCode)
+ }
+
var res embedResponse
@@
- return res.Embeddings[0], nil
+ if len(res.Embeddings) == 0 || len(res.Embeddings[0]) == 0 {
+ return nil, fmt.Errorf("ollama embed returned no embeddings")
+ }
+
+ return res.Embeddings[0], nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| body, _ := json.Marshal(payload) | |
| req, err := http.NewRequestWithContext(ctx, "POST", "http://localhost:11434/api/embed", bytes.NewBuffer(body)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| req.Header.Set("Content-Type", "application/json") | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer resp.Body.Close() | |
| var res embedResponse | |
| if err := json.NewDecoder(resp.Body).Decode(&res); err != nil { | |
| return nil, err | |
| } | |
| fmt.Println("this is before returning") | |
| fmt.Println(res.Model) | |
| fmt.Println(len(res.Embeddings[0])) | |
| return res.Embeddings[0], nil | |
| body, err := json.Marshal(payload) | |
| if err != nil { | |
| return nil, err | |
| } | |
| req, err := http.NewRequestWithContext(ctx, "POST", "http://localhost:11434/api/embed", bytes.NewBuffer(body)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| req.Header.Set("Content-Type", "application/json") | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if resp.StatusCode != http.StatusOK { | |
| return nil, fmt.Errorf("ollama embed returned status %d", resp.StatusCode) | |
| } | |
| defer resp.Body.Close() | |
| var res embedResponse | |
| if err := json.NewDecoder(resp.Body).Decode(&res); err != nil { | |
| return nil, err | |
| } | |
| fmt.Println("this is before returning") | |
| fmt.Println(res.Model) | |
| fmt.Println(len(res.Embeddings[0])) | |
| if len(res.Embeddings) == 0 || len(res.Embeddings[0]) == 0 { | |
| return nil, fmt.Errorf("ollama embed returned no embeddings") | |
| } | |
| return res.Embeddings[0], nil |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 47-47: Error return value of resp.Body.Close is not checked
(errcheck)
🤖 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 `@internal/embeddings/ollama.go` around lines 34 - 59, Remove the error
suppression in the json.Marshal call and handle the error return value
appropriately. Add validation of the HTTP response status code (check
resp.StatusCode) immediately after the resp, err assignment and before
attempting to decode the response body to prevent panics from error responses.
After decoding the response into res with json.NewDecoder, validate that
res.Embeddings is not nil and contains at least one element before accessing
res.Embeddings[0] to prevent index out of bounds panics. Additionally, remove
all the debug print statements (the three fmt.Println calls) before returning
the embeddings.
| embedding, err := s.embedder.Embed(ctx, req.Message) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| err = s.store.InsertLog(ctx, req, embedding) | ||
|
|
There was a problem hiding this comment.
Embedding failures currently take down log ingestion.
SendLog short-circuits on embedder error, so any Ollama outage/latency event prevents logs from being stored at all. For the ingest path, consider making embedding best-effort (or async) and keeping primary log persistence available.
🤖 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 `@internal/grpc/log_service.go` around lines 36 - 42, The SendLog method
returns an error immediately when s.embedder.Embed fails, preventing log
persistence in s.store.InsertLog from occurring. To make embedding best-effort,
catch the error from s.embedder.Embed when creating the embedding for
req.Message, log the embedding failure for observability, but allow the function
to continue and call s.store.InsertLog with the log data anyway (potentially
with a nil or empty embedding value). This ensures logs are persisted to storage
even when the embedder is unavailable or experiencing issues, rather than
blocking the entire ingestion pipeline.
| func (s *Store) InsertLog(ctx context.Context, req *pb.SendLogRequest, embedding []float32) error { | ||
|
|
||
| vec := pgvector.NewVector(embedding) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "postgres.go" | head -20Repository: Aneeshie/Loom
Length of output: 88
🏁 Script executed:
rg "func.*InsertLog" -A 10 -B 2Repository: Aneeshie/Loom
Length of output: 664
🏁 Script executed:
rg "768" --type goRepository: Aneeshie/Loom
Length of output: 39
🏁 Script executed:
rg "embedding.*dimension\|dimension.*embedding" --type goRepository: Aneeshie/Loom
Length of output: 39
🏁 Script executed:
rg "embedding" -B 3 -A 3 internal/storage/postgres.goRepository: Aneeshie/Loom
Length of output: 328
🏁 Script executed:
find . -type f -name "*.proto" -o -name "*.pb.go" | head -20Repository: Aneeshie/Loom
Length of output: 116
🏁 Script executed:
rg "SendLogRequest" -B 5 -A 5Repository: Aneeshie/Loom
Length of output: 13085
🏁 Script executed:
rg "pgvector\|NewVector" -B 2 -A 5Repository: Aneeshie/Loom
Length of output: 39
🏁 Script executed:
cat -n internal/storage/postgres.go | head -50Repository: Aneeshie/Loom
Length of output: 1188
🏁 Script executed:
rg "Embed\|embedder" -B 3 -A 10 internal/grpc/log_service.goRepository: Aneeshie/Loom
Length of output: 39
🏁 Script executed:
find . -type f -name "*.go" -path "*/embedder*" -o -name "*embed*" | grep -v proto | grep -v pb.goRepository: Aneeshie/Loom
Length of output: 215
🏁 Script executed:
rg "NewVector\|pgvector" -B 5 -A 5Repository: Aneeshie/Loom
Length of output: 39
🏁 Script executed:
cat -n internal/embeddings/embedder.goRepository: Aneeshie/Loom
Length of output: 232
🏁 Script executed:
cat -n internal/grpc/log_service.goRepository: Aneeshie/Loom
Length of output: 3618
🏁 Script executed:
cat -n migrations/20260617124840_add_embeddings.up.sqlRepository: Aneeshie/Loom
Length of output: 192
🏁 Script executed:
rg "dimension\|size\|len" migrations/ -B 2 -A 2Repository: Aneeshie/Loom
Length of output: 39
🏁 Script executed:
rg "Embed\(ctx\|type.*Embedder" -B 2 -A 15 --type goRepository: Aneeshie/Loom
Length of output: 39
🏁 Script executed:
find . -type f -name "*.go" -exec grep -l "Embedder\|Embed" {} \; | head -20Repository: Aneeshie/Loom
Length of output: 198
🏁 Script executed:
rg "pgvector\|NewVector" --type go -B 3 -A 3Repository: Aneeshie/Loom
Length of output: 39
🏁 Script executed:
cat -n internal/storage/postgres.go | tail -30Repository: Aneeshie/Loom
Length of output: 565
🏁 Script executed:
cat -n internal/embeddings/ollama.goRepository: Aneeshie/Loom
Length of output: 1668
🏁 Script executed:
cat -n cmd/server/main.go | head -80Repository: Aneeshie/Loom
Length of output: 902
🏁 Script executed:
rg "InsertLog" --type go -B 2 -A 2Repository: Aneeshie/Loom
Length of output: 560
Add embedding dimension validation to prevent DB-level errors.
InsertLog does not validate the embedding dimension before database write. The schema enforces VECTOR(768), and while pgvector will eventually reject invalid sizes, catching this earlier with a fail-fast check provides clearer error messages and avoids unnecessary DB round-trips.
Suggested fix
func (s *Store) InsertLog(ctx context.Context, req *pb.SendLogRequest, embedding []float32) error {
+ if len(embedding) != 768 {
+ return fmt.Errorf("invalid embedding dimension: got %d, want 768", len(embedding))
+ }
vec := pgvector.NewVector(embedding)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *Store) InsertLog(ctx context.Context, req *pb.SendLogRequest, embedding []float32) error { | |
| vec := pgvector.NewVector(embedding) | |
| func (s *Store) InsertLog(ctx context.Context, req *pb.SendLogRequest, embedding []float32) error { | |
| if len(embedding) != 768 { | |
| return fmt.Errorf("invalid embedding dimension: got %d, want 768", len(embedding)) | |
| } | |
| vec := pgvector.NewVector(embedding) |
🤖 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 `@internal/storage/postgres.go` around lines 29 - 32, In the InsertLog method
of the Store type, add validation for the embedding dimension before attempting
to create the pgvector. Check that the length of the embedding slice matches the
expected dimension (768 as per your database schema). If the dimension is
incorrect, return an error immediately with a clear message describing the
expected and actual dimensions. This validation should occur before the
pgvector.NewVector call to fail fast and provide better error messages.
| @@ -1,4 +1,8 @@ | |||
| .PHONY: proto migrate-up migrate-down proto | |||
| .PHONY: proto migrate-up migrate-down proto migate-create | |||
There was a problem hiding this comment.
Fix .PHONY target list typo for migrate-create.
Line 1 includes migate-create (misspelled) and omits migrate-create, so the new target is not guaranteed to run if a same-named file exists. Also proto is duplicated.
Suggested patch
-.PHONY: proto migrate-up migrate-down proto migate-create
+.PHONY: proto migrate-up migrate-down migrate-create📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .PHONY: proto migrate-up migrate-down proto migate-create | |
| .PHONY: proto migrate-up migrate-down migrate-create |
🤖 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 `@Makefile` at line 1, In the `.PHONY` declaration on the first line, remove
the duplicate `proto` target and correct the misspelled `migate-create` to
`migrate-create`. The corrected line should list each phony target exactly once:
proto, migrate-up, migrate-down, and migrate-create.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmd/query/main.go (1)
132-146: 💤 Low valueConsider adding "no logs found" message for empty results.
The main
GetLogspath prints "no logs found" whenresp.Logsis nil (line 89-94), butrunSemanticSearchsilently outputs nothing. For consistency:func runSemanticSearch(client pb.LogServiceClient, query string, limit int64) { resp, err := client.SimilarLogs(context.Background(), &pb.SimilarLogsRequest{Query: query, Limit: limit}) if err != nil { log.Fatal(err) } + if len(resp.Logs) == 0 { + fmt.Println("no similar logs found") + return + } + for _, logEntry := range resp.Logs {🤖 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 `@cmd/query/main.go` around lines 132 - 146, The runSemanticSearch function does not handle the case when no logs are returned, unlike the GetLogs path which prints a "no logs found" message. After calling client.SimilarLogs and checking for errors, add a check to verify if resp.Logs is empty or nil. If it is empty, print a "no logs found" message using fmt.Println. If logs exist, proceed with the existing loop that prints each log entry. This ensures consistent user feedback across both search paths.
🤖 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 `@internal/agent/agent.go`:
- Around line 41-43: The condition checking if a line is empty by comparing it
directly to an empty string only catches exact empty strings and misses lines
containing only whitespace characters like spaces or tabs. Replace the condition
`line == ""` with `strings.TrimSpace(line) == ""` in the loop where lines are
being validated before they flow into ParseLogLine and embedding/storage. This
ensures that all whitespace-only lines are properly skipped from further
processing.
In `@internal/storage/postgres.go`:
- Line 170: Remove the debug print statements that were added during
development, specifically the fmt.Println call containing "entered similar logs"
around line 170 and any related distance logging debug statements in the
vicinity of lines 222-226 in the postgres.go file. These are development
artifacts that should not be included in production code and will cause
unnecessary log clutter.
- Around line 172-182: The SQL query in the postgres.go file currently applies
LIMIT before any distance threshold filtering, causing fewer results than
requested when some logs exceed the threshold. Add a WHERE clause to the SELECT
query that filters results based on the distance threshold (likely comparing the
embedding distance calculation to a threshold value), ensuring LIMIT applies
only to qualifying results. Then remove the application-side threshold check
that currently exists around lines 218-220 where results are filtered after
retrieval.
- Line 168: The SimilarLogs function does not validate the limit parameter,
while the GetLogs function enforces bounds checking (1-100). Add limit
validation at the beginning of the SimilarLogs function to ensure the limit
parameter is between 1 and 100 (inclusive), and return an appropriate error if
the validation fails, similar to how GetLogs handles invalid limits. This
ensures consistent behavior across both methods.
---
Nitpick comments:
In `@cmd/query/main.go`:
- Around line 132-146: The runSemanticSearch function does not handle the case
when no logs are returned, unlike the GetLogs path which prints a "no logs
found" message. After calling client.SimilarLogs and checking for errors, add a
check to verify if resp.Logs is empty or nil. If it is empty, print a "no logs
found" message using fmt.Println. If logs exist, proceed with the existing loop
that prints each log entry. This ensures consistent user feedback across both
search paths.
🪄 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: 208c0e50-74a7-4fcf-a7ac-343c6ed13b22
⛔ Files ignored due to path filters (3)
proto/log.pb.gois excluded by!**/*.pb.goproto/log_grpc.pb.gois excluded by!**/*.pb.gotestdata/test.logis excluded by!**/*.log
📒 Files selected for processing (6)
cmd/query/main.gocmd/server/main.gointernal/agent/agent.gointernal/grpc/log_service.gointernal/storage/postgres.goproto/log.proto
💤 Files with no reviewable changes (1)
- cmd/server/main.go
| if line == "" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Skip whitespace-only lines, not just exact empty strings.
line == "" misses lines like " "/\t, which still flow into ParseLogLine and then embedding/storage. Use strings.TrimSpace(line) == "" here.
Suggested patch
import (
"bufio"
"context"
"fmt"
"log"
"os"
+ "strings"
"time"
@@
- if line == "" {
+ if strings.TrimSpace(line) == "" {
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if line == "" { | |
| continue | |
| } | |
| import ( | |
| "bufio" | |
| "context" | |
| "fmt" | |
| "log" | |
| "os" | |
| "strings" | |
| "time" | |
| ) | |
| // ... other code ... | |
| if strings.TrimSpace(line) == "" { | |
| continue | |
| } |
🤖 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 `@internal/agent/agent.go` around lines 41 - 43, The condition checking if a
line is empty by comparing it directly to an empty string only catches exact
empty strings and misses lines containing only whitespace characters like spaces
or tabs. Replace the condition `line == ""` with `strings.TrimSpace(line) == ""`
in the loop where lines are being validated before they flow into ParseLogLine
and embedding/storage. This ensures that all whitespace-only lines are properly
skipped from further processing.
|
|
||
| } | ||
|
|
||
| func (s *Store) SimilarLogs(ctx context.Context, embedding []float32, limit int64) ([]*pb.Log, error) { |
There was a problem hiding this comment.
Add limit validation for consistency with GetLogs.
GetLogs validates limit (1-100), but SimilarLogs accepts any value including zero or negative numbers. Consider adding similar bounds checking.
func (s *Store) SimilarLogs(ctx context.Context, embedding []float32, limit int64) ([]*pb.Log, error) {
+ if limit < 1 || limit > 100 {
+ return nil, fmt.Errorf("limit should be between 1 and 100")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *Store) SimilarLogs(ctx context.Context, embedding []float32, limit int64) ([]*pb.Log, error) { | |
| func (s *Store) SimilarLogs(ctx context.Context, embedding []float32, limit int64) ([]*pb.Log, error) { | |
| if limit < 1 || limit > 100 { | |
| return nil, fmt.Errorf("limit should be between 1 and 100") | |
| } |
🤖 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 `@internal/storage/postgres.go` at line 168, The SimilarLogs function does not
validate the limit parameter, while the GetLogs function enforces bounds
checking (1-100). Add limit validation at the beginning of the SimilarLogs
function to ensure the limit parameter is between 1 and 100 (inclusive), and
return an appropriate error if the validation fails, similar to how GetLogs
handles invalid limits. This ensures consistent behavior across both methods.
| query := `SELECT | ||
| id, | ||
| service_name, | ||
| host, | ||
| level, | ||
| message, | ||
| timestamp, | ||
| embedding <=> $1 AS distance | ||
| FROM logs | ||
| ORDER BY distance | ||
| LIMIT $2` |
There was a problem hiding this comment.
Move threshold filter to SQL WHERE clause for correct limit behavior.
Currently, LIMIT is applied before threshold filtering, so requesting 10 logs may return fewer if some exceed the threshold. Moving the filter to SQL ensures the limit applies to qualifying results.
query := `SELECT
id,
service_name,
host,
level,
message,
timestamp,
embedding <=> $1 AS distance
FROM logs
+ WHERE embedding <=> $1 <= $2
ORDER BY distance
- LIMIT $2`
+ LIMIT $3`
rows, err := s.db.Query(
ctx,
query,
pgvector.NewVector(embedding),
+ SIMILARITY_THRESHOLD,
limit,
)Then remove the application-side check (lines 218-220).
Also applies to: 218-220
🤖 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 `@internal/storage/postgres.go` around lines 172 - 182, The SQL query in the
postgres.go file currently applies LIMIT before any distance threshold
filtering, causing fewer results than requested when some logs exceed the
threshold. Add a WHERE clause to the SELECT query that filters results based on
the distance threshold (likely comparing the embedding distance calculation to a
threshold value), ensuring LIMIT applies only to qualifying results. Then remove
the application-side threshold check that currently exists around lines 218-220
where results are filtered after retrieval.
Overview
This PR introduces the foundation for semantic search in Loom by generating and storing vector embeddings for every ingested log.
Changes
Embedding Infrastructure
EmbedderinterfaceOllamaEmbedderimplementation/api/embedendpointnomic-embed-textDatabase
embedding VECTOR(768)column to logs tableLog Ingestion
SendLogArchitecture
LogServiceValidation
Why
This lays the groundwork for semantic log retrieval, allowing future queries such as:
loom-query --similar "database timeout"Adds semantic search support to Loom.
Logs are now embedded during ingestion and stored using pgvector.
Users can search for semantically similar logs through the query CLI.
Features
Example
loom-query --similar "database timeout"
Results are ranked by vector similarity instead of exact keyword matches.
Future Work