Add Embedder Component#3526
Draft
Matts966 wants to merge 17 commits into
Draft
Conversation
Deploying vald with
|
| Latest commit: |
60ca0c1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0c47e395.vald.pages.dev |
| Branch Preview URL: | https://feature-embedder-all.vald.pages.dev |
Member
Author
|
/format |
Collaborator
|
[CHATOPS:HELP] ChatOps commands.
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds an Embedder API (proto + swagger), generated bindings, an OpenAI-backed LLM integration, a Vald-backed embedder service with metadata support, REST and gRPC handlers, HTTP router, config/CLI, CI OpenAI mock, E2E test extensions, and docs/dependency updates. ChangesEmbedder feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
✨ 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 |
github-actions
Bot
added
area/agent/sidecar
actions/backport/release/v1.7
actions/build/amd64
Collaborator
|
[FORMAT] Updating license headers and formatting go codes triggered by Matts966. |
Collaborator
|
[FORMAT] Failed to format. |
![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: 27
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apis/swagger/v1/embedder/embedder.swagger.json`:
- Around line 14-139: The OpenAPI spec only documents /commit, /embedding,
/insert, /search but the router (pkg/tools/embedder/router/router.go) exposes
additional public endpoints (/id/search, /linearsearch, /id/linearsearch,
/insert/multi, /update, /update/multi, /delete, /delete/multi, /index/create,
/index/save, /object/{id}); update the contract to be consistent by either
removing those extra routes from router.go if they should not be public, or add
corresponding operations to embedder.swagger.json (or to the proto if the
swagger is generated) describing request/response schemas and operationIds
(e.g., Embedder_IdSearch, Embedder_LinearSearch, Embedder_InsertMulti,
Embedder_Update, Embedder_UpdateMulti, Embedder_Delete, Embedder_DeleteMulti,
Embedder_IndexCreate, Embedder_IndexSave, Embedder_ObjectGet) so the published
docs match the router.
In `@pkg/tools/embedder/config/config_test.go`:
- Around line 17-18: Update the package doc comment at the top of config_test.go
to reference the correct package name: replace "Package setting stores all
server application settings" with a brief docstring that mentions package config
(e.g., "Package config stores all server application settings" or similar) so
the godoc reflects the actual package; locate the package comment immediately
above the line declaring package config in config_test.go and edit that sentence
only.
In `@pkg/tools/embedder/config/config.go`:
- Around line 17-18: The package doc comment at the top of config.go is
incorrect: it reads "Package setting" but the package is named config; update
the comment to start with "Package config" so it matches the package identifier
and follows Go documentation conventions (look for the file-level comment above
the package declaration in config.go).
- Around line 79-164: Delete the large commented-out FakeData block (the entire
commented function that constructs a Data instance and calls config.ToRawYaml)
from pkg/tools/embedder/config/config.go; if the sample is still needed, move
its content into a README example or a test fixture instead and remove any
now-unused imports such as fmt; locate the block by the FakeData function name,
the Data struct construction, and the config.ToRawYaml call to ensure you remove
the correct commented section.
In `@pkg/tools/embedder/handler/doc.go`:
- Line 17: Add a package-level documentation comment at the top of doc.go that
begins with "Package handler" and briefly describes the purpose and
responsibilities of the handler package (what it provides, key types or
behaviors). Update doc.go (which currently only contains "package handler") to
include this comment so the package is documented per project conventions.
In `@pkg/tools/embedder/handler/grpc/option.go`:
- Around line 41-52: The WithLLM option currently can panic and returns the
wrong error type: guard against nil deref by either assigning the llm to s.llm
(as other options do) or checking if s.embedder != nil before calling
s.embedder.SetLLM(llm); change the returned error to use the Vald option helpers
(e.g. errors.ErrInvalidOption("LLM", ...)) so the constructor can inspect
errors.ErrCriticalOption/errors.ErrOptionFailed correctly; make the same
error-fix for WithValdClient (use errors.ErrInvalidOption("ValdClient", ...));
remove or track the three Japanese TODO comments as issues (don’t leave them in
the merged code) and add a GoDoc comment above WithLLM similar to WithName to
explain its purpose.
In `@pkg/tools/embedder/handler/rest/handler.go`:
- Around line 152-161: CreateAndSaveIndex currently calls
h.agent.SaveIndex(r.Context(), nil); change it to pass an explicit non-nil empty
protobuf message (e.g. &payload.Empty{}) instead of nil to match other handlers
and avoid surprising implementations; update the call in CreateAndSaveIndex so
SaveIndex receives &payload.Empty{} while leaving CreateIndex(req) usage
unchanged.
In `@pkg/tools/embedder/handler/rest/option.go`:
- Around line 22-30: Change the REST handler option API to match the grpc
handler: update the exported Option type to be "func(*handler) error" and update
New to return "(Handler, error)"; add Godoc comments for Option and WithAgent;
modify WithAgent to validate a != nil and return
errors.NewErrInvalidOption("Agent", a) when nil before assigning to h.agent; and
make New iterate over provided opts, call each option and return any error
(mirroring the grpc handler's option validation flow) so nil/invalid options are
rejected early.
In `@pkg/tools/embedder/README.md`:
- Line 1: Replace the placeholder header in README.md ("# server sample") with
an embedder-specific README: add a brief one- or two-sentence overview of the
embedder component, usage instructions showing required configuration keys and
example values, a short "Run" section with the exact command(s) to start the
embedder, and an "API" or "CLI" examples section demonstrating common
requests/flags and expected responses; ensure the markdown is concise and
unambiguous so readers can operate the embedder without reading the source
(refer to this file's README.md and the embedder's config keys and executable
name when crafting examples).
In `@pkg/tools/embedder/router/option_test.go`:
- Around line 148-158: The test currently expects WithTimeout to accept and
store an invalid time string; instead update the test for the WithTimeout option
to assert that timeutil.Parse failures produce an error and that the T.timeout
field remains its zero value. Specifically, in the test case for "invalid time
string" (operating on the T struct and invoking WithTimeout), assert the
returned error is non-nil and that obj.timeout is empty/default rather than
"invalid time string"; mirror how successful cases still validate parsed values,
and ensure the test exercises the parsing path in
pkg/tools/embedder/router/option.go where WithTimeout calls timeutil.Parse.
In `@pkg/tools/embedder/router/option.go`:
- Around line 26-44: The Option type currently is func(*router) which cannot
return validation errors and WithTimeout accepts any string and defers parsing;
change Option to func(*router) error (matching other routers) so New can wrap
failures, update all option functions to return error, and in WithTimeout use
timeutil.Parse(timeout) to validate the value and on parse failure return
errors.ErrInvalidOption("Timeout", err) instead of storing the raw string;
ensure callers (e.g., New) propagate option errors and update the
TestWithTimeout/"invalid time string" to expect an ErrInvalidOption failure.
In `@pkg/tools/embedder/router/router_test.go`:
- Around line 74-76: The table-driven test constructs rest.New() (and
errgroup.Get()) outside the per-test closure which makes test table building
brittle if rest.New requires options or non-nil handlers; change the code so
each subtest creates its own REST instance inside the per-test closure (or call
rest.New with an explicit fake/mock handler option) and obtain the errgroup
inside the same closure (referencing rest.New, errgroup.Get and the local h
variable) so a single subtest failure won't panic the whole suite.
In `@pkg/tools/embedder/router/router.go`:
- Around line 136-144: The route entry named "Multiple Remove" for Pattern
"/delete/multi" currently allows both http.MethodDelete and http.MethodPost
which is inconsistent with the singular Remove handler and surprising for a
delete endpoint; either remove http.MethodPost from the Methods slice so only
http.MethodDelete is accepted, or if POST is required due to payload/body-size
constraints, add a clear comment above the route explaining why MultiRemove must
accept POST and ensure the handler name MultiRemove is annotated to reflect the
POST allowance.
- Around line 36-50: Change New so it validates required dependencies after
applying options and returns an error: update func New(opts ...Option) to return
(http.Handler, error), and after the options loop check that r.handler != nil
and r.eg != nil; if either is nil return nil and errors.ErrInvalidOption (or
wrap with context). Reference the router type and fields r.handler and r.eg and
ensure Option implementations like WithHandler / WithErrorGroup are used to set
them; update call sites to handle the (handler, error) return accordingly.
- Around line 51-169: The router in routing.WithRoutes in router.go exposes many
handlers (h.SearchByID, h.LinearSearch, h.LinearSearchByID, h.MultiInsert,
h.Update, h.MultiUpdate, h.Remove, h.MultiRemove, h.CreateIndex, h.SaveIndex,
h.GetObject, etc.) that are not present in the gRPC proto/apis/swagger contract
(which only defines Insert, Search, Embedding, Commit); either remove those
extra Route entries so the router only registers the four supported endpoints
(Insert, Search, Embedding, Commit) or update
apis/proto/v1/embedder/embedder.proto and
apis/swagger/v1/embedder/embedder.swagger.json to add matching RPCs/paths for
each extra REST endpoint, ensuring names and semantics match the handler methods
and the router entries exactly so SDKs/docs remain consistent.
In `@pkg/tools/embedder/service/embedder.go`:
- Around line 78-86: The Commit method on type embedder is verbose; replace its
body with a single-line return by directly returning the error from
e.aclient.CreateAndSaveIndex(ctx, in, opts...) — i.e., in function
embedder.Commit call e.aclient.CreateAndSaveIndex(...) and return its error
result immediately instead of assigning to _ and branching.
- Around line 41-66: New currently returns an embedder even when required
dependencies are nil and Insert dereferences an assumed vector (vec.Id = id)
which can be nil; update New to validate required fields (client, aclient, llm)
and return a clear error (e.g., errors.ErrInvalidOption or a new internal error)
if any are missing, change Insert (and Search/Commit callers) to check for a nil
vector returned from Embed and return an error instead of dereferencing it, and
address SetLLM's race by either removing the runtime setter or protecting e.llm
with synchronization (use atomic.Pointer[LLM] or a mutex) so concurrent calls to
Embed/SetLLM are safe; reference functions/receivers: New, (*embedder).SetLLM,
(*embedder).Insert, Embed (and callers Search/Commit) when applying the fixes.
- Around line 16-25: Replace the direct import of "google.golang.org/grpc" with
Vald's internal wrapper "github.com/vdaas/vald/internal/net/grpc" and update all
uses of grpc.CallOption in this file (notably the Commit method signatures and
any parameters/variables referencing grpc.CallOption) to use the re-exported
type from the internal package; specifically change the import and ensure the
Commit method(s) (and any other functions referencing grpc.CallOption) reference
the internal/net/grpc package symbol so the code uses the re-exported CallOption
type.
In `@pkg/tools/embedder/service/llm_option.go`:
- Around line 49-57: The WithToken functional option currently returns
errors.New("token is empty") on missing token; change it to return the
Vald-style critical option error instead so constructors treat this as a fatal
option — update the error return in WithToken (the func WithToken(token string)
OpenAIOption that sets o.token on *openAI) to return
errors.NewErrCriticalOption("token", token, nil) (or provide an appropriate
cause) when token == "" so callers receive a critical error rather than a plain
errors.New.
- Around line 24-46: Update the stale doc comment on the OpenAIOption type to
correctly describe OpenAI options (remove "ngt" reference) and change
WithOpenAIModel to validate the input string: inside WithOpenAIModel map mstr to
an openai.EmbeddingModel as now, but if no case matches return an OpenAIOption
that returns a non-nil error (fail-fast) instead of setting a zero-value model;
ensure defaultOpenAIOptions still uses WithOpenAIModel("adav2") and surface a
clear error message naming the unknown model string and valid choices when the
option is applied.
In `@pkg/tools/embedder/service/llm.go`:
- Around line 38-47: NewOpenAI currently constructs an openai.Client even when
o.token is empty, causing runtime 401s; update NewOpenAI to validate the token
before creating the client: after applying options (loop over
defaultOpenAIOptions and opts), if o.token is empty try to read a fallback (e.g.
VALD_OPENAI_API_KEY or config) and if still empty return
errors.ErrInvalidOption("token", nil) (or errors.ErrOptionFailed with
reflect.ValueOf(WithToken) as appropriate) so the function fails fast; reference
NewOpenAI, openAI.o.token, defaultOpenAIOptions and WithToken in the change.
- Around line 49-63: The Embed method should return an explicit error when
OpenAI returns no embedding instead of (nil, nil) and should enforce a fallback
timeout for the CreateEmbeddings call; modify openAI.Embed to (1) wrap the
CreateEmbeddings invocation with a context.WithTimeout if ctx has no deadline
(remember to defer cancel) and (2) after iterating embeddings.Data, if no
non-nil embedding found return a descriptive error (e.g., fmt.Errorf("no
embedding returned from OpenAI for input")) rather than nil, so callers
(embedder.Embed / Search) can handle the failure instead of dereferencing a nil
vector.
In `@pkg/tools/embedder/service/option.go`:
- Around line 31-61: Update the nil-check errors in WithValdClient,
WithAgentClient, and WithLLM to return the repository's structured option error
(e.g. errors.ErrInvalidOption("ValdClient", errors.ErrNilPointer),
errors.ErrInvalidOption("AgentClient", errors.ErrNilPointer),
errors.ErrInvalidOption("LLM", errors.ErrNilPointer)) instead of plain
errors.New strings so service.New can classify option failures; keep the option
name argument equal to the function name without the "With" prefix. Also fix the
package doc comment above Option to read "Option represents the functional
option for embedder" (correcting grammar and the copy-paste target).
In `@pkg/tools/embedder/usecase/agentd.go`:
- Around line 171-199: In run.Start, the error consumer hot-loops when any
source channel (oech, nech, sech) closes because receives yield zero values; fix
by using the two-value receive form for each case (e.g., val, ok := <-oech)
inside the select, and when ok is false set that channel variable to nil (oech =
nil) and continue so the select no longer chooses closed channels (alternatively
track closed-count and break when all three are closed); apply this change for
oech, nech, and sech in the goroutine started by r.eg.Go to stop the busy-loop
and allow the loop to exit when appropriate.
- Around line 212-215: The PostStop method is discarding the error returned by
r.ngt.Close(ctx); change it to handle and propagate the error instead of
ignoring it. Update PostStop (method name PostStop on type run) to call
r.ngt.Close(ctx), capture its error, and return it (or wrap it with context) so
callers can detect cleanup failures; do not assign the result to _. Ensure the
function signature remains func (r *run) PostStop(ctx context.Context) error and
that any error from r.ngt.Close(ctx) is returned.
- Around line 55-162: The New function currently builds an NGT service and
registers agent/vald servers; replace that with creation and wiring of the
embedder service and handler: instantiate service.Embedder (e.g., via
service.NewOpenAI(...) or appropriate constructor) providing LLM config and
Vald/agent clients, pass the embedder instance into handler.New using
handler.WithEmbedder and handler.WithLLM options, and register
embedder.RegisterEmbedderServer(srv, g) in grpcServerOptions instead of
agent.RegisterAgentServer/vald.RegisterValdServer; also change the fieldManager
constant from "vald-agent-index-controller" to "vald-embedder-index-controller"
and update the infometrics.New call (name and description) to embedder-specific
values (replace "agent_core_ngt_info" and "Agent NGT info" with embedder
identifiers).
In `@pkg/tools/embedder/usecase/embedder.go`:
- Around line 17-33: Remove the duplicated license header at the top of
embedder.go so the file contains a single license block and the package
declaration remains "package usecase"; then add a new doc.go in the same package
containing a package-level comment that starts with "Package usecase" describing
the package purpose (ensure the comment begins exactly with "Package usecase"
and the file declares package usecase). This ensures no duplicated boilerplate
in embedder.go and provides the required package documentation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79e9da3f-22f8-4208-bd0c-fb54696bc887
⛔ Files ignored due to path filters (4)
apis/grpc/v1/agent/sidecar/sidecar_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.go,!**/*_vtproto.pb.goapis/grpc/v1/embedder/embedder.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/embedder/embedder_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.go,!**/*_vtproto.pb.gorust/libs/proto/src/embedder.v1.tonic.rsis excluded by!**/*.tonic.rs
📒 Files selected for processing (24)
apis/proto/v1/embedder/embedder.protoapis/swagger/v1/embedder/embedder.swagger.jsonpkg/tools/embedder/README.mdpkg/tools/embedder/config/config.gopkg/tools/embedder/config/config_test.gopkg/tools/embedder/handler/doc.gopkg/tools/embedder/handler/grpc/handler.gopkg/tools/embedder/handler/grpc/option.gopkg/tools/embedder/handler/rest/handler.gopkg/tools/embedder/handler/rest/handler_test.gopkg/tools/embedder/handler/rest/option.gopkg/tools/embedder/handler/rest/option_test.gopkg/tools/embedder/router/option.gopkg/tools/embedder/router/option_test.gopkg/tools/embedder/router/router.gopkg/tools/embedder/router/router_test.gopkg/tools/embedder/service/doc.gopkg/tools/embedder/service/embedder.gopkg/tools/embedder/service/llm.gopkg/tools/embedder/service/llm_option.gopkg/tools/embedder/service/option.gopkg/tools/embedder/usecase/agentd.gopkg/tools/embedder/usecase/embedder.goqemu_make_20250512-042753_18331.core
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3526 +/- ##
==========================================
- Coverage 27.06% 26.14% -0.93%
==========================================
Files 527 586 +59
Lines 49064 51004 +1940
==========================================
+ Hits 13280 13333 +53
- Misses 34802 36680 +1878
- Partials 982 991 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
/format |
Collaborator
|
[FORMAT] Updating license headers and formatting go codes triggered by Matts966. |
Signed-off-by: Vdaas CI <vald@vdaas.org>
Member
Author
|
/format |
Member
Author
|
/format |
Collaborator
|
[FORMAT] Updating license headers and formatting go codes triggered by Matts966. |
Signed-off-by: Vdaas CI <vald@vdaas.org>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 25
♻️ Duplicate comments (2)
pkg/tools/embedder/service/option.go (1)
28-56: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace the ad-hoc nil causes with
errors.ErrNilPointer.
NewErrInvalidOptionis the right outer wrapper, but the innererrors.New("... is nil")values still introduce new untyped errors. Reusingerrors.ErrNilPointerkeeps option failures consistent with the rest of the repo and preserveserrors.Ischecks.Suggested change
func WithValdClient(c vald.Client) Option { return func(e *embedder) error { if c == nil { - return errors.NewErrInvalidOption("ValdClient", c, errors.New("vald client is nil")) + return errors.NewErrInvalidOption("ValdClient", c, errors.ErrNilPointer) } e.client = c return nil } } @@ func WithMetaClient(c MetaClient) Option { return func(e *embedder) error { if c == nil { - return errors.NewErrInvalidOption("MetaClient", c, errors.New("meta client is nil")) + return errors.NewErrInvalidOption("MetaClient", c, errors.ErrNilPointer) } e.mclient = c return nil } } @@ func WithLLM(c LLM) Option { return func(e *embedder) error { if c == nil { - return errors.NewErrInvalidOption("LLM", c, errors.New("llm is nil")) + return errors.NewErrInvalidOption("LLM", c, errors.ErrNilPointer) } e.llm = c return nil } }As per coding guidelines: "All new error types should go in
internal/errorswith anErrprefix."🤖 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 `@pkg/tools/embedder/service/option.go` around lines 28 - 56, Replace the ad-hoc nil error causes in the Option constructors WithValdClient, WithMetaClient, and WithLLM by using the shared sentinel errors.ErrNilPointer inside the errors.NewErrInvalidOption call instead of creating new errors with errors.New("... is nil"); locate the three functions (WithValdClient, WithMetaClient, WithLLM) and pass errors.ErrNilPointer as the inner cause to errors.NewErrInvalidOption to preserve consistent error typing and allow errors.Is checks to work.pkg/tools/embedder/router/router_test.go (1)
34-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove handler/errgroup construction into subtest scope to avoid brittle setup.
rest.New()anderrgroup.Get()are still built at top-level test execution scope. If either starts requiring stricter dependencies, the whole test fails before route checks run. Build them per subtest to isolate failures.🤖 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 `@pkg/tools/embedder/router/router_test.go` around lines 34 - 36, The test builds shared state at top-level (rest.New() and errgroup.Get()) making the test brittle; move the handler and errgroup construction into each subtest's scope so they are created per-case. Specifically, inside the subtest body create h := rest.New() and eg := errgroup.Get() and call got := New(WithHandler(h), WithErrGroup(eg)) before asserting gh, ok := got.(*mux.Router); remove any top-level h or errgroup.Get() usage so each t.Run uses its own fresh instances.
🤖 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 @.gitfiles:
- Line 615: Add unit test files for every new implementation file listed: create
*_test.go files for cmd/tools/embedder/main.go,
pkg/tools/embedder/handler/grpc/handler.go,
pkg/tools/embedder/handler/grpc/option.go and for each file under
pkg/tools/embedder/service/ and pkg/tools/embedder/usecase/. For each test file,
target the exported functions and methods (eg. the Serve/Handle methods in
grpc/handler.go, option parsing in option.go, and each public service/usecase
function), use table-driven tests for normal and error cases, and inject/mocк
dependencies (interfaces) to isolate logic; place tests in the same package or
package_name_test as appropriate and name files with the _test.go suffix so they
run under go test. Ensure coverage includes success, failure, and edge paths and
that any gRPC handlers are exercised via grpc/test utilities or mocked servers.
In @.github/workflows/e2e.v2.yaml:
- Around line 325-326: The fixed sleep should be replaced with a readiness poll
against the embedder's health endpoint instead of the hardcoded "sleep 10":
after launching the embedder with "nohup go run ./cmd/tools/embedder -c
/tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 &", implement a loop that
repeatedly curls http://127.0.0.1:3301/healthz (or /healthz) with a short
backoff until it returns success or a reasonable timeout is reached; if the
timeout expires, print /tmp/embedder-e2e.log and fail the step so downstream
"make e2e/v2" doesn’t hit connection refused on port 28081. Ensure the probe
uses --fail/exit codes so the workflow step can detect readiness reliably.
In `@apis/docs/v1/payload.md.tmpl`:
- Line 1206: The description text "count infos" is awkward; update the template
entries that mention Info.Index.Detail.CountsEntry (the line containing "|
counts | Info.Index.Detail.CountsEntry | repeated | count infos for each agent
|") to use natural singular phrasing such as "count information for each agent"
(also apply the same replacement at the other occurrence noted around the
Info.Index.Detail.CountsEntry entry referenced at the second location).
In `@apis/proto/v1/payload/payload.proto`:
- Line 546: Replace the awkward API-facing comment text "count infos" with
"count information" in the proto file comments (e.g., update the comment "//
count infos for each agent" and the other similar occurrence) so generated docs
read naturally; locate the comment strings in payload.proto (the ones mentioning
"count infos") and change them to "count information" while keeping the
surrounding comment context intact.
In `@cmd/tools/embedder/main.go`:
- Line 55: Replace the direct log.Fatal call in main with non-fatal logging and
explicit error propagation: extract the logic in main into a run() (or similar)
function that returns an error, change the log.Fatal(err, info.Get()) site to
use process-safe logging (e.g., logger.Error/Printf) to record the error and
context (info.Get()), and let main handle the returned error by exiting
explicitly (os.Exit with a code) or returning from the program after any
necessary cleanup; ensure you remove any log.Fatal usage and instead propagate
errors via the run()/main error handling path.
In `@hack/tools/openai-embedding-mock/main.go`:
- Line 21: The file imports the standard "log" package and calls log.Fatal,
which violates project rules; switch the import to the project's internal
logging (e.g., internal/log) and avoid using log.Fatal: replace the "log" import
with the internal/log package used across the repo, change the log.Fatal calls
(the one around the ListenAndServe call and the ones at lines 86-87) to an
appropriate internal/log error logging call (e.g., internal/log.Errorf or
internal/log.Error) and then explicitly call os.Exit(1) after logging to
preserve the termination behavior.
- Line 61: The handler currently ignores JSON decode errors with `_ =
json.NewDecoder(r.Body).Decode(&req)`, which lets malformed requests proceed;
change this to capture the error (e.g. `err :=
json.NewDecoder(r.Body).Decode(&req)`), and if err != nil log the error (use the
package logger or log.Printf) and return an HTTP error response (e.g.
http.Error(w, "invalid JSON", http.StatusBadRequest) or similar) so the mock
returns a non-200 and the failure is diagnosable; reference the variables
json.NewDecoder, req, r.Body and the handler's http.ResponseWriter to locate
where to add the check.
- Around line 81-83: The response currently calls json.NewEncoder(w).Encode(res)
which may start sending headers/body and then calls http.Error on encode failure
(so the 200 is already committed); fix by either setting the status header
before encoding (call w.Header().Set("Content-Type","application/json") and
w.WriteHeader(http.StatusOK) then json.NewEncoder(w).Encode(res)) or buffer the
JSON (encode into a bytes.Buffer first, check for encode errors, then call
w.Header().Set(...), w.WriteHeader(http.StatusOK) and io.Copy(w, &buf)); locate
the json.NewEncoder(w).Encode(res) call and apply one of these two approaches to
ensure correct status codes on error.
In `@pkg/tools/embedder/config/config_test.go`:
- Around line 27-65: TestNewConfig currently only tests the happy path; convert
it into a table-driven test that iterates cases (name, JSON content, wantErr
bool and optional wantErrSubstring) and for each case write the JSON to a temp
file, call NewConfig(path), then assert error presence/substring for failure
cases (unsupported provider, missing llm.openai.token) and on success assert
cfg.LLM.Provider == "openai" and cfg.LLM.OpenAI.Token == "test-token"; keep the
goleak.VerifyNone defer and use t.Run(case.name) for subtests to isolate cases.
In `@pkg/tools/embedder/config/config.go`:
- Around line 87-91: Replace the raw errors.New calls in
pkg/tools/embedder/config/config.go with typed errors from internal/errors: add
exported error variables ErrUnsupportedLLMProvider and ErrMissingOpenAIToken to
the internal/errors package, then in the validation branches that check
cfg.LLM.Provider and cfg.LLM.OpenAI.Token return those typed errors (wrap with
fmt.Errorf("%w: %s", internalerrors.ErrUnsupportedLLMProvider, cfg.LLM.Provider)
for the provider case if you want to include the value). Update the checks
referencing cfg.LLM.Provider and cfg.LLM.OpenAI.Token to import internal/errors
and return internalerrors.ErrMissingOpenAIToken for the missing token case (or
wrap similarly) so callers can match on the typed errors.
In `@pkg/tools/embedder/handler/grpc/handler.go`:
- Around line 39-53: New currently constructs a server without validating that
an embedder was injected, which leads to nil-derefs later; modify New to verify
that the created server (type server) has a non-nil embedder (the field set by
WithEmbedder / service.Embedder) after applying options and return a clear error
if it's missing (make the error descriptive like "missing required embedder
option" or reuse an appropriate errors.* type), so callers get a
construction-time failure instead of runtime panics in s.embedder.<Method>.
In `@pkg/tools/embedder/handler/grpc/option.go`:
- Around line 30-32: Replace the raw errors.New("embedder is nil") in the option
validation with a typed error from internal/errors (e.g.,
internal/errors.ErrNilEmbedder or a new ErrInvalidOption) and return that
instead; add the new Err-prefixed error constant to the internal/errors package
and import it into pkg/tools/embedder/handler/grpc/option.go, then update the
nil-check (the branch checking e == nil in the Option-related function) to
return that typed error so callers can classify option failures consistently.
In `@pkg/tools/embedder/handler/rest/handler_test.go`:
- Line 16: Add commented table-driven test skeletons for the embedder-specific
REST handler methods so their intent is tracked: create placeholder test blocks
(commented) for Embedding, InsertWithMetadata, UpdateWithMetadata,
UpsertWithMetadata, and RemoveWithMetadata immediately below the existing "//
NOT IMPLEMENTED BELOW" marker in handler_test.go; each stub should follow the
same pattern as the existing commented skeletons (test case struct, fields,
args, want, and t.Run loop) so reviewers see the planned tests for these new
handler surface methods.
In `@pkg/tools/embedder/handler/rest/handler.go`:
- Around line 119-121: The Exists method currently parses payload.Empty and
returns a hardcoded {"ok": true}, which is misleading; either implement a real
existence check by wiring handler.Exists to the embedder backend (call
h.embedder.Exists with the parsed request id and propagate the boolean/error
result, updating the gRPC service and proto to add an Exists RPC and
request/response messages), or change the handler name to Healthz/Ping (and
payload.Empty stay) and update routing/docs to reflect it's only a liveness
probe; update function handler.Exists (or rename it) and ensure the HTTP
response and docs match the chosen semantics.
- Around line 64-117: Handlers like Search, LinearSearch, Insert, Update,
Upsert, Remove, and Embedding declare pointer vars (e.g., var req
*embedderpb.SearchRequest) which remain nil for empty request bodies and cause
panics when passed to h.embedder.<Method>; change each handler to initialize the
request before calling json.Handler (e.g., req := new(embedderpb.SearchRequest))
so DecodeRequest will populate the struct safely, mirroring the nil-checking
pattern used in InsertWithMetadata/UpdateWithMetadata/UpsertWithMetadata.
In `@pkg/tools/embedder/handler/rest/option_test.go`:
- Around line 16-101: The test file contains only commented scaffolding so add
at least one implemented table-driven test for WithAgent: create TestWithAgent
that defines args (a grpc.Server), want (expected Option), a defaultCheckFunc
(using reflect.DeepEqual) and at least one test case which calls got :=
WithAgent(test.args.a) and asserts via checkFunc; keep the existing
goleak.VerifyNone setup and use the same test helper names (defaultCheckFunc,
TestWithAgent, WithAgent, Option) and place this implemented test above the "//
NOT IMPLEMENTED BELOW" marker.
In `@pkg/tools/embedder/service/embedder.go`:
- Around line 53-58: Replace ad-hoc errors.New calls used for nil checks with
centralized Err-style errors from internal/errors (e.g., use
internalerrors.ErrValdClientNil and internalerrors.ErrLLMNill or similarly named
Err identifiers) instead of strings; update the nil-checks on e.client and e.llm
in embedder.go (and the other validation sites flagged in this review) to return
those shared Err values, import the internal/errors package (alias if needed)
and ensure the Err names follow the project's Err* naming convention so all
occurrences (including the other validation paths referenced) use the same
centralized error symbols.
- Around line 160-166: The Remove handler currently forwards empty IDs to
downstream clients; update embedder.Remove (and similarly
embedder.RemoveWithMetadata) to validate the incoming request and its Id before
calling e.client.Remove: ensure req is non-nil and req.GetId() is not empty, and
if invalid return a clear fast-fail error (e.g., an InvalidArgument gRPC/status
error with a descriptive message) instead of delegating to the client.
- Around line 98-104: The Insert followed by setMetadata (e.Insert(...);
e.setMetadata(...)) can leave partial state when metadata fails; update these
WithMetadata flows to either perform a compensating rollback of the primary
write or return explicit partial-success semantics: after e.Insert returns a
location (loc), if e.setMetadata fails attempt a best-effort compensating call
to remove the inserted vector (e.g., call a delete/undo method such as e.Delete
or e.Remove by id) and log/metric the outcome, and if rollback is not possible
return a structured partial-success error (include the inserted id/loc and
metadata error) and emit an observability event. Apply the same pattern to the
other flows that call e.Insert then e.setMetadata (the other WithMetadata blocks
referenced) so primary writes are either rolled back or reported as partial
successes with clear metadata for retry/cleanup.
- Around line 198-201: The code currently only checks for nil embeddings but
allows zero-length slices to be wrapped into payload.Object_Vector, which can
cause downstream invalid-vector errors; update the checks in the
embedding-to-payload path (the code that returns &payload.Object_Vector{Vector:
vec}) to reject zero-length embeddings by adding a condition that returns an
error when len(vec) == 0 (in addition to vec == nil), so that the function
returns an error instead of constructing an Object_Vector from an empty slice.
In `@pkg/tools/embedder/service/llm.go`:
- Around line 40-77: The Embed method in openAI uses a hardcoded 30s timeout and
returns a string error for missing embeddings; extract the magic constant into a
package-level const (e.g., defaultEmbedTimeout = 30 * time.Second) or add a
WithTimeout OpenAIOption to NewOpenAI so callers can configure timeouts, and
replace the literal error returned in Embed ("openai: no embedding returned for
input") with a typed sentinel from internal/errors (e.g.,
errors.ErrOpenAINoEmbeddingReturned) so callers can use errors.Is; update
NewOpenAI/OpenAIOption handling and the Embed logic to use the new const/option
and the new sentinel error.
In `@pkg/tools/embedder/service/meta.go`:
- Around line 39-44: Replace the ad-hoc string error in NewMetaClient with a
package-level sentinel error defined in internal/errors using the Err prefix
(e.g., errors.ErrGRPCClientNotFound) and return that sentinel from NewMetaClient
when c is nil; update the metaClient constructor to reference that sentinel (and
any callers/tests that asserted the string) so other modules can use errors.Is
to detect this condition.
In `@pkg/tools/embedder/usecase/agentd.go`:
- Around line 119-177: In r.Start, reset the named return variable after
relaying to avoid stale errors and add a short comment about nil channels: after
the select branch that sends the error into ech (the "case ech <- err" path) set
err = nil to clear the previous error so it cannot be re-relayed on subsequent
iterations, and add a one-line comment near the mech/oech initialization (or
above the select) noting that nil channels block in select and therefore their
cases will never fire until set — this keeps the fan-in logic explicit and
maintainable for functions/methods using oech, vech, mech, sech and the named
err return.
- Around line 183-198: The Stop method on run currently ignores the error
returned by r.observability.Stop(ctx); change Stop to capture that error (e.g.,
obsErr := r.observability.Stop(ctx)) and include it in the shutdown error chain
using errors.Join (or similar) so it’s returned alongside errors from
r.metaClient.Stop, r.valdClient.Stop and r.server.Shutdown; ensure you import
errors if needed and consistently join obsErr with any subsequent errors before
returning.
In `@tests/v2/e2e/crud/embedder_test.go`:
- Around line 157-163: The test's OpEmbedderRemoveWithMetadata case is calling
RemoveWithMetadata with a RemoveRequest (Id, Config) but the proto should mirror
the other WithMetadata RPCs by accepting a RemoveWithMetadataRequest { Request,
Metadata }; fix the proto (change the RPC signature to
RemoveWithMetadata(RemoveWithMetadataRequest) and add the
RemoveWithMetadataRequest message containing a RemoveRequest request and
Metadata metadata), regenerate gRPC stubs, and update the test to pass the meta
wrapper (use embedderpb.NewEmbedderClient(conn).RemoveWithMetadata(ctx,
&embedderpb.RemoveWithMetadataRequest{Request: &embedderpb.RemoveRequest{Id: id,
Config: ...}, Metadata: meta}, ...) so the meta variable is used; alternatively,
if metadata is not needed, rename the operation to Remove and update references
to OpEmbedderRemoveWithMetadata accordingly.
---
Duplicate comments:
In `@pkg/tools/embedder/router/router_test.go`:
- Around line 34-36: The test builds shared state at top-level (rest.New() and
errgroup.Get()) making the test brittle; move the handler and errgroup
construction into each subtest's scope so they are created per-case.
Specifically, inside the subtest body create h := rest.New() and eg :=
errgroup.Get() and call got := New(WithHandler(h), WithErrGroup(eg)) before
asserting gh, ok := got.(*mux.Router); remove any top-level h or errgroup.Get()
usage so each t.Run uses its own fresh instances.
In `@pkg/tools/embedder/service/option.go`:
- Around line 28-56: Replace the ad-hoc nil error causes in the Option
constructors WithValdClient, WithMetaClient, and WithLLM by using the shared
sentinel errors.ErrNilPointer inside the errors.NewErrInvalidOption call instead
of creating new errors with errors.New("... is nil"); locate the three functions
(WithValdClient, WithMetaClient, WithLLM) and pass errors.ErrNilPointer as the
inner cause to errors.NewErrInvalidOption to preserve consistent error typing
and allow errors.Is checks to work.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45858d97-31da-4eca-bbce-e419378649bd
⛔ Files ignored due to path filters (14)
apis/grpc/v1/embedder/embedder.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/embedder/embedder.pb.json.gois excluded by!**/*.pb.json.goapis/grpc/v1/embedder/embedder_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.go,!**/*_vtproto.pb.goapis/grpc/v1/payload/payload.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/rpc/errdetails/error_details.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/index_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.go,!**/*_vtproto.pb.gogo.sumis excluded by!**/*.sum,!**/go.sumrust/bin/agent/Cargo.tomlis excluded by!**/bin/**rust/bin/meta/Cargo.tomlis excluded by!**/bin/**rust/libs/proto/src/embedder.v1.tonic.rsis excluded by!**/*.tonic.rsrust/libs/proto/src/embedder/v1/embedder.v1.serde.rsis excluded by!**/*.serde.rsrust/libs/proto/src/embedder/v1/embedder.v1.tonic.rsis excluded by!**/*.tonic.rsrust/libs/proto/src/vald/v1/vald.v1.tonic.rsis excluded by!**/*.tonic.rstests/v2/e2e/assets/embedder.yamlis excluded by!**/assets/**
📒 Files selected for processing (57)
.cspell.json.gitfiles.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/PULL_REQUEST_TEMPLATE.md.github/workflows/e2e.v2.yamlapis/docs/v1/docs.mdapis/docs/v1/index.mdapis/docs/v1/payload.md.tmplapis/proto/v1/embedder/embedder.protoapis/proto/v1/payload/payload.protoapis/proto/v1/rpc/errdetails/error_details.protoapis/proto/v1/vald/index.protoapis/swagger/v1/embedder/embedder.swagger.jsonapis/swagger/v1/vald/index.swagger.jsoncmd/tools/embedder/doc.gocmd/tools/embedder/main.gocmd/tools/embedder/sample.yamlgo.modhack/go.mod.defaulthack/tools/openai-embedding-mock/main.gopkg/tools/embedder/README.mdpkg/tools/embedder/config/config.gopkg/tools/embedder/config/config_test.gopkg/tools/embedder/handler/doc.gopkg/tools/embedder/handler/grpc/handler.gopkg/tools/embedder/handler/grpc/option.gopkg/tools/embedder/handler/rest/handler.gopkg/tools/embedder/handler/rest/handler_test.gopkg/tools/embedder/handler/rest/option.gopkg/tools/embedder/handler/rest/option_test.gopkg/tools/embedder/router/option.gopkg/tools/embedder/router/option_test.gopkg/tools/embedder/router/router.gopkg/tools/embedder/router/router_test.gopkg/tools/embedder/service/doc.gopkg/tools/embedder/service/embedder.gopkg/tools/embedder/service/llm.gopkg/tools/embedder/service/llm_option.gopkg/tools/embedder/service/meta.gopkg/tools/embedder/service/option.gopkg/tools/embedder/usecase/agentd.gopkg/tools/embedder/usecase/doc.gopkg/tools/embedder/usecase/embedder.gorust/libs/algorithm/Cargo.tomlrust/libs/algorithms/ngt/Cargo.tomlrust/libs/kvs/Cargo.tomlrust/libs/observability/Cargo.tomlrust/libs/proto/Cargo.tomlrust/libs/proto/src/embedder/v1/embedder.v1.rsrust/libs/proto/src/payload/v1/payload.v1.rsrust/libs/proto/src/rpc/v1/rpc.v1.rsrust/libs/vqueue/Cargo.tomltests/v2/e2e/config/config.gotests/v2/e2e/config/enums.gotests/v2/e2e/crud/embedder_test.gotests/v2/e2e/crud/strategy_test.go
| cmd/tools/cli/vdctl/main.go | ||
| cmd/tools/cli/vdctl/main_test.go | ||
| cmd/tools/embedder/doc.go | ||
| cmd/tools/embedder/main.go |
Contributor
There was a problem hiding this comment.
Several new embedder implementation files ship without matching unit tests.
From this manifest, cmd/tools/embedder/main.go, pkg/tools/embedder/handler/grpc/handler.go, pkg/tools/embedder/handler/grpc/option.go, the pkg/tools/embedder/service/*.go files, and the pkg/tools/embedder/usecase/*.go files do not have corresponding *_test.go entries. That leaves most of the new component’s core paths unverified in this PR.
As per coding guidelines: "Each implementation file must come with its unit test file with *_test.go postfix".
Also applies to: 2287-2305
🤖 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 @.gitfiles at line 615, Add unit test files for every new implementation file
listed: create *_test.go files for cmd/tools/embedder/main.go,
pkg/tools/embedder/handler/grpc/handler.go,
pkg/tools/embedder/handler/grpc/option.go and for each file under
pkg/tools/embedder/service/ and pkg/tools/embedder/usecase/. For each test file,
target the exported functions and methods (eg. the Serve/Handle methods in
grpc/handler.go, option parsing in option.go, and each public service/usecase
function), use table-driven tests for normal and error cases, and inject/mocк
dependencies (interfaces) to isolate logic; place tests in the same package or
package_name_test as appropriate and name files with the _test.go suffix so they
run under go test. Ensure coverage includes success, failure, and edge paths and
that any gRPC handlers are exercised via grpc/test utilities or mocked servers.
Comment on lines
+325
to
+326
| nohup go run ./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 & | ||
| sleep 10 |
Contributor
There was a problem hiding this comment.
Replace hardcoded sleep 10 with a readiness poll on the embedder's health endpoint.
The mock-OpenAI step correctly polls /healthz before proceeding, but the embedder start step uses a fixed 10 s sleep. Go compilation under a cold cache or a slow cluster can easily exceed this, causing the subsequent make e2e/v2 run to hit a "connection refused" error on port 28081. The embedder config already exposes a readiness endpoint at port 3301.
🛠️ Proposed fix
- nohup go run ./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 &
- sleep 10
+ nohup go run ./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 &
+ for i in $(seq 1 60); do
+ if curl -fsS http://127.0.0.1:3301/readiness >/dev/null 2>&1; then
+ break
+ fi
+ sleep 1
+ done
+ curl -fsS http://127.0.0.1:3301/readiness >/dev/null📝 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.
Suggested change
| nohup go run ./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 & | |
| sleep 10 | |
| nohup go run ./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 & | |
| for i in $(seq 1 60); do | |
| if curl -fsS http://127.0.0.1:3301/readiness >/dev/null 2>&1; then | |
| break | |
| fi | |
| sleep 1 | |
| done | |
| curl -fsS http://127.0.0.1:3301/readiness >/dev/null |
🤖 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 @.github/workflows/e2e.v2.yaml around lines 325 - 326, The fixed sleep should
be replaced with a readiness poll against the embedder's health endpoint instead
of the hardcoded "sleep 10": after launching the embedder with "nohup go run
./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 &",
implement a loop that repeatedly curls http://127.0.0.1:3301/healthz (or
/healthz) with a short backoff until it returns success or a reasonable timeout
is reached; if the timeout expires, print /tmp/embedder-e2e.log and fail the
step so downstream "make e2e/v2" doesn’t hit connection refused on port 28081.
Ensure the probe uses --fail/exit codes so the workflow step can detect
readiness reliably.
| | field | type | label | description | | ||
| | :---: | :--- | :---- | :---------- | | ||
| | counts | Info.Index.Detail.CountsEntry | repeated | count infos for each agents | | ||
| | counts | Info.Index.Detail.CountsEntry | repeated | count infos for each agent | |
Contributor
There was a problem hiding this comment.
Use natural singular phrasing in descriptions.
count infos is awkward in generated docs. Prefer count information for readability and consistency.
✏️ Suggested wording fix
- | counts | Info.Index.Detail.CountsEntry | repeated | count infos for each agent |
+ | counts | Info.Index.Detail.CountsEntry | repeated | count information for each agent |
...
- | details | Info.Index.StatisticsDetail.DetailsEntry | repeated | count infos for each agent |
+ | details | Info.Index.StatisticsDetail.DetailsEntry | repeated | count information for each agent |Also applies to: 1428-1428
🤖 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 `@apis/docs/v1/payload.md.tmpl` at line 1206, The description text "count
infos" is awkward; update the template entries that mention
Info.Index.Detail.CountsEntry (the line containing "| counts |
Info.Index.Detail.CountsEntry | repeated | count infos for each agent |") to use
natural singular phrasing such as "count information for each agent" (also apply
the same replacement at the other occurrence noted around the
Info.Index.Detail.CountsEntry entry referenced at the second location).
| // Represent the index count for each Agents message. | ||
| message Detail { | ||
| // count infos for each agents | ||
| // count infos for each agent |
Contributor
There was a problem hiding this comment.
Polish exported proto comment text.
count infos is unnatural in API-facing comments. Please use count information to avoid awkward generated docs.
✏️ Suggested wording fix
- // count infos for each agent
+ // count information for each agent
...
- // count infos for each agent
+ // count information for each agentAlso applies to: 605-605
🤖 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 `@apis/proto/v1/payload/payload.proto` at line 546, Replace the awkward
API-facing comment text "count infos" with "count information" in the proto file
comments (e.g., update the comment "// count infos for each agent" and the other
similar occurrence) so generated docs read naturally; locate the comment strings
in payload.proto (the ones mentioning "count infos") and change them to "count
information" while keeping the surrounding comment context intact.
| }), | ||
| ) | ||
| })(); err != nil { | ||
| log.Fatal(err, info.Get()) |
Contributor
There was a problem hiding this comment.
Avoid log.Fatal in production entrypoint.
Replace log.Fatal with non-fatal logging plus explicit exit/error propagation to align with repo error-handling policy.
As per coding guidelines: **/*.{go,rs}: Never use panic! or log.Fatal in production code. Errors must be propagated and handled gracefully.
🤖 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/tools/embedder/main.go` at line 55, Replace the direct log.Fatal call in
main with non-fatal logging and explicit error propagation: extract the logic in
main into a run() (or similar) function that returns an error, change the
log.Fatal(err, info.Get()) site to use process-safe logging (e.g.,
logger.Error/Printf) to record the error and context (info.Get()), and let main
handle the returned error by exiting explicitly (os.Exit with a code) or
returning from the program after any necessary cleanup; ensure you remove any
log.Fatal usage and instead propagate errors via the run()/main error handling
path.
Comment on lines
+40
to
+77
| func NewOpenAI(opts ...OpenAIOption) (OpenAI, error) { | ||
| o := &openAI{} | ||
| for _, opt := range append(defaultOpenAIOptions, opts...) { | ||
| if err := opt(o); err != nil { | ||
| return nil, errors.ErrOptionFailed(err, reflect.ValueOf(opt)) | ||
| } | ||
| } | ||
| if o.token == "" { | ||
| return nil, errors.NewErrInvalidOption("token", o.token, errors.New("token must not be empty")) | ||
| } | ||
| cfg := openai.DefaultConfig(o.token) | ||
| if o.baseURL != "" { | ||
| cfg.BaseURL = o.baseURL | ||
| } | ||
| o.client = openai.NewClientWithConfig(cfg) | ||
| return o, nil | ||
| } | ||
|
|
||
| func (o *openAI) Embed(ctx context.Context, doc string) ([]float32, error) { | ||
| if _, ok := ctx.Deadline(); !ok { | ||
| var cancel context.CancelFunc | ||
| ctx, cancel = context.WithTimeout(ctx, 30*time.Second) | ||
| defer cancel() | ||
| } | ||
| embeddings, err := o.client.CreateEmbeddings(ctx, openai.EmbeddingRequest{ | ||
| Model: o.model, | ||
| Input: doc, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, embedding := range embeddings.Data { | ||
| if embedding.Embedding != nil { | ||
| return embedding.Embedding, nil | ||
| } | ||
| } | ||
| return nil, errors.New("openai: no embedding returned for input") | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Past review concerns are addressed; one small follow-up worth considering.
Token validation, the 30s fallback timeout, and the explicit error on missing embeddings are all in place — this resolves the previous critical/major findings.
Two minor follow-ups to consider:
- The 30s fallback is a magic constant. If you anticipate exposing it via config (e.g., LLM-level timeout), pulling it into a package-level
const defaultEmbedTimeout = 30 * time.Second(or aWithTimeoutoption) is cheap and makes the contract grep-friendly. - Per Vald's convention, "no embedding returned" could be a typed sentinel in
internal/errors(e.g.,errors.ErrOpenAINoEmbeddingReturned) so callers canerrors.Isagainst it instead of string-matching. Not blocking, but consistent with the rest of the repo.
🤖 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 `@pkg/tools/embedder/service/llm.go` around lines 40 - 77, The Embed method in
openAI uses a hardcoded 30s timeout and returns a string error for missing
embeddings; extract the magic constant into a package-level const (e.g.,
defaultEmbedTimeout = 30 * time.Second) or add a WithTimeout OpenAIOption to
NewOpenAI so callers can configure timeouts, and replace the literal error
returned in Embed ("openai: no embedding returned for input") with a typed
sentinel from internal/errors (e.g., errors.ErrOpenAINoEmbeddingReturned) so
callers can use errors.Is; update NewOpenAI/OpenAIOption handling and the Embed
logic to use the new const/option and the new sentinel error.
Comment on lines
+39
to
+44
| func NewMetaClient(c vgrpc.Client) (MetaClient, error) { | ||
| if c == nil { | ||
| return nil, errors.New("grpc client is nil") | ||
| } | ||
| return &metaClient{client: c}, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Constructor and lifecycle look idiomatic.
NewMetaClient matches the standard Vald gRPC-client wrapper shape and rejects a nil client up front, which is the right call site to fail. As a minor consistency note, Vald typically defines such sentinel errors in internal/errors with an Err prefix (e.g., errors.ErrGRPCClientNotFound) so callers can errors.Is rather than string-match — worth aligning here if you intend other modules to handle this distinctly.
As per coding guidelines: "All new error types should go in internal/errors with an Err prefix."
🤖 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 `@pkg/tools/embedder/service/meta.go` around lines 39 - 44, Replace the ad-hoc
string error in NewMetaClient with a package-level sentinel error defined in
internal/errors using the Err prefix (e.g., errors.ErrGRPCClientNotFound) and
return that sentinel from NewMetaClient when c is nil; update the metaClient
constructor to reference that sentinel (and any callers/tests that asserted the
string) so other modules can use errors.Is to detect this condition.
Comment on lines
+119
to
+177
| func (r *run) Start(ctx context.Context) (<-chan error, error) { | ||
| ech := make(chan error, 4) | ||
| var oech, vech, mech, sech <-chan error | ||
| r.eg.Go(safety.RecoverFunc(func() (err error) { | ||
| defer close(ech) | ||
| if r.observability != nil { | ||
| oech = r.observability.Start(ctx) | ||
| } | ||
| vech, err = r.valdClient.Start(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if r.metaClient != nil { | ||
| mech, err = r.metaClient.Start(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| sech = r.server.ListenAndServe(ctx) | ||
| for { | ||
| var ok bool | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case err, ok = <-oech: | ||
| if !ok { | ||
| oech = nil | ||
| continue | ||
| } | ||
| case err, ok = <-vech: | ||
| if !ok { | ||
| vech = nil | ||
| continue | ||
| } | ||
| case err, ok = <-mech: | ||
| if !ok { | ||
| mech = nil | ||
| continue | ||
| } | ||
| case err, ok = <-sech: | ||
| if !ok { | ||
| sech = nil | ||
| continue | ||
| } | ||
| } | ||
| if oech == nil && vech == nil && mech == nil && sech == nil { | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case ech <- err: | ||
| } | ||
| } | ||
| } | ||
| })) | ||
| return ech, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Channel fan-in fix looks correct; one residual subtlety.
The closure-detection (ok=false → nil out the channel → continue) plus the "all four nil → return" terminator is the right shape and addresses the previous busy-loop. Two things worth keeping in mind:
erris a named return shared across iterations. After a successful relay (case ech <- err),errstays set until the next select case overwrites it. That's fine today because everycaseassignment overwriteserr, but if you ever add a path that doesn't, you risk re-relaying a stale error. Aerr = nilreset right aftercase ech <- err:would make the invariant explicit.mechandoechstart nil when meta/observability are disabled. Receiving from a nil channel blocks forever inselect, so the cases simply never fire — correct, but means the "all four nil" exit can only be reached whenvechandsechactually close. Worth a one-line comment for the next maintainer.
🤖 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 `@pkg/tools/embedder/usecase/agentd.go` around lines 119 - 177, In r.Start,
reset the named return variable after relaying to avoid stale errors and add a
short comment about nil channels: after the select branch that sends the error
into ech (the "case ech <- err" path) set err = nil to clear the previous error
so it cannot be re-relayed on subsequent iterations, and add a one-line comment
near the mech/oech initialization (or above the select) noting that nil channels
block in select and therefore their cases will never fire until set — this keeps
the fan-in logic explicit and maintainable for functions/methods using oech,
vech, mech, sech and the named err return.
Comment on lines
+183
to
+198
| func (r *run) Stop(ctx context.Context) error { | ||
| if r.observability != nil { | ||
| r.observability.Stop(ctx) | ||
| } | ||
| if r.metaClient != nil { | ||
| if err := r.metaClient.Stop(ctx); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if r.valdClient != nil { | ||
| if err := r.valdClient.Stop(ctx); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return r.server.Shutdown(ctx) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Observability.Stop signature.
ast-grep --pattern 'type Observability interface { $$$ }'
rg -nP 'Stop\s*\(\s*context\.Context\s*\)' internal/observability -C1Repository: vdaas/vald
Length of output: 413
Handle observability.Stop error return value.
Observability.Stop(ctx context.Context) returns an error, but the current code ignores it at line 187. This silently drops failures from the observability exporter during shutdown, contradicting Vald's error handling guideline. The other lifecycle methods (metaClient.Stop, valdClient.Stop) are properly handled; join observability.Stop errors with the shutdown chain using errors.Join or handle them consistently.
🤖 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 `@pkg/tools/embedder/usecase/agentd.go` around lines 183 - 198, The Stop method
on run currently ignores the error returned by r.observability.Stop(ctx); change
Stop to capture that error (e.g., obsErr := r.observability.Stop(ctx)) and
include it in the shutdown error chain using errors.Join (or similar) so it’s
returned alongside errors from r.metaClient.Stop, r.valdClient.Stop and
r.server.Shutdown; ensure you import errors if needed and consistently join
obsErr with any subsequent errors before returning.
Comment on lines
+157
to
+163
| case config.OpEmbedderRemoveWithMetadata: | ||
| return checkLocation(call(func(ctx context.Context, conn *grpc.ClientConn, copts ...grpc.CallOption) (any, error) { | ||
| return embedderpb.NewEmbedderClient(conn).RemoveWithMetadata(ctx, &embedderpb.RemoveRequest{ | ||
| Id: id, | ||
| Config: &payload.Remove_Config{SkipStrictExistCheck: true}, | ||
| }, copts...) | ||
| })) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether RemoveWithMetadataRequest type exists in Go generated proto and
# what argument type RemoveWithMetadata client method accepts.
rg -n "RemoveWithMetadata" apis/grpc/v1/embedder/ --type go -A 3
rg -n "RemoveWithMetadataRequest" apis/ --type go -l
rg -n "RemoveWithMetadata" apis/proto/v1/embedder/ -A 3Repository: vdaas/vald
Length of output: 5710
🏁 Script executed:
#!/bin/bash
# Check what request types the other WithMetadata operations accept in the proto
rg -n "rpc (InsertWithMetadata|UpdateWithMetadata|UpsertWithMetadata)" apis/proto/v1/embedder/embedder.proto -A 3
# Also check the generated Go code
rg -n "(InsertWithMetadata|UpdateWithMetadata|UpsertWithMetadata)" apis/grpc/v1/embedder/embedder_vtproto.pb.go -A 2 | head -40Repository: vdaas/vald
Length of output: 1905
Proto design inconsistency: RemoveWithMetadata should use a wrapper request type analogous to other WithMetadata operations.
The proto defines metadata transport wrapper types only for three of four WithMetadata operations:
| RPC | Request Type |
|---|---|
InsertWithMetadata |
InsertWithMetadataRequest{Request, Metadata} |
UpdateWithMetadata |
UpdateWithMetadataRequest{Request, Metadata} |
UpsertWithMetadata |
UpsertWithMetadataRequest{Request, Metadata} |
RemoveWithMetadata |
RemoveRequest{Id, Config} — no metadata field |
The proto definition (line 143 of apis/proto/v1/embedder/embedder.proto) declares RemoveWithMetadata(RemoveRequest) instead of RemoveWithMetadata(RemoveWithMetadataRequest). This breaks the established pattern and prevents metadata from being transmitted with remove operations despite the RPC name promising it.
The meta variable constructed on lines 104–108 in the test remains unused for the OpEmbedderRemoveWithMetadata case. Either define RemoveWithMetadataRequest in the proto (with Request and Metadata fields) or rename the operation to Remove.
🤖 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 `@tests/v2/e2e/crud/embedder_test.go` around lines 157 - 163, The test's
OpEmbedderRemoveWithMetadata case is calling RemoveWithMetadata with a
RemoveRequest (Id, Config) but the proto should mirror the other WithMetadata
RPCs by accepting a RemoveWithMetadataRequest { Request, Metadata }; fix the
proto (change the RPC signature to RemoveWithMetadata(RemoveWithMetadataRequest)
and add the RemoveWithMetadataRequest message containing a RemoveRequest request
and Metadata metadata), regenerate gRPC stubs, and update the test to pass the
meta wrapper (use embedderpb.NewEmbedderClient(conn).RemoveWithMetadata(ctx,
&embedderpb.RemoveWithMetadataRequest{Request: &embedderpb.RemoveRequest{Id: id,
Config: ...}, Metadata: meta}, ...) so the meta variable is used; alternatively,
if metadata is not needed, rename the operation to Remove and update references
to OpEmbedderRemoveWithMetadata accordingly.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| pkg/tools/embedder/router/option.go | ||
| pkg/tools/embedder/router/option_test.go | ||
| pkg/tools/embedder/router/router.go | ||
| pkg/tools/embedder/router/router_test.go |
Contributor
There was a problem hiding this comment.
[file name cspell] reported by reviewdog 🐶
Unknown word (devserver) Suggestions: [deserver, deserve, deserter, deserved, deserves]
| pkg/tools/embedder/service/doc.go | ||
| pkg/tools/embedder/service/embedder.go | ||
| pkg/tools/embedder/service/llm.go | ||
| pkg/tools/embedder/service/llm_option.go |
Contributor
There was a problem hiding this comment.
[file name cspell] reported by reviewdog 🐶
Unknown word (esbuild) Suggestions: [ebuild, tsbuild, TSBuild, build, rebuild]
| rust/libs/proto/src/discoverer/mod.rs | ||
| rust/libs/proto/src/discoverer/v1/discoverer.v1.tonic.rs | ||
| rust/libs/proto/src/discoverer/v1/mod.rs | ||
| rust/libs/proto/src/embedder.v1.tonic.rs |
Contributor
There was a problem hiding this comment.
[file name cspell] reported by reviewdog 🐶
Unknown word (sveltekit) Suggestions: [sveltest, sleekit, svelte, svelter, sveltely]
| rust/libs/proto/src/discoverer/v1/mod.rs | ||
| rust/libs/proto/src/embedder.v1.tonic.rs | ||
| rust/libs/proto/src/embedder/v1/embedder.v1.rs | ||
| rust/libs/proto/src/embedder/v1/embedder.v1.serde.rs |
Contributor
There was a problem hiding this comment.
[file name cspell] reported by reviewdog 🐶
Unknown word (tanstack) Suggestions: [daostack, jamstack, DAOstack, attack, tanta]
Member
Author
|
/format |
Collaborator
|
[FORMAT] Updating license headers and formatting go codes triggered by Matts966. |
Signed-off-by: Vdaas CI <vald@vdaas.org>
| addrs: | ||
| - "vald-gateway.default.svc.cluster.local:8081" | ||
| llm: | ||
| provider: openai |
Contributor
There was a problem hiding this comment.
[LanguageTool] reported by reviewdog 🐶
Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Suggestions: openai
Rule: https://community.languagetool.org/rule/show/ENGLISH_WORD_REPEAT_RULE?lang=en-US
Category: MISC
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
2 participants
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.
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit