Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Deploying vald with
|
| Latest commit: |
78b051f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://061991f3.vald.pages.dev |
| Branch Preview URL: | https://doc-add-agents-md-9898591043.vald.pages.dev |
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
bot
added
actions/backport/release/v1.7
actions/build/amd64
|
Important Review skippedToo many files! This PR contains 280 files, which is 130 over the limit of 150. You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a new repository-level documentation file Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 78-80: Replace the redundant phrase "API interfaces" in the
AGENTS.md languages bullet with a concise term like "APIs" (or simply
"interfaces"); locate the line containing "**API interfaces**" and change it to
"**APIs**" to remove the tautology while preserving meaning.
- Around line 24-155: The MD linter errors are caused by missing blank lines
around headings and fenced code blocks (e.g., the "Start Local Cluster (k3d):",
"Deploy Vald (Helm):", "Run E2E Tests (V2):", "Clean Up:", "6.1 gRPC &
Bidirectional Streaming (Critical)" headings and the surrounding
```bash```/```go``` fences). Fix by inserting a single blank line before each
heading and both before and after each fenced code block (and between the
preceding list/table and the next heading) so that every code fence and heading
is separated by blank lines, ensuring the numbered lists and tables remain
separated from code blocks to satisfy MD022/MD031/MD058 rules.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 83-84: The documentation contains a typo in the Agent description:
change the phrase "State-full (requires PV)" to "Stateful (requires PV)" in the
AGENTS.md entry for "Agent (NGT)" so the wording is correct and consistent with
other docs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 162-223: The Markdown headings (e.g., "Go", "Rust", "Kubernetes &
Helm (YAML)", "Dockerfile", "Makefiles", "GitHub Actions", "6.2 gRPC &
Bidirectional Streaming", "6.3 Error Handling Strategy (gRPC Codes)", and "6.4
Configuration & Tuning") each need a single blank line inserted immediately
after the heading line to satisfy MD022; edit AGENTS.md and add one empty line
after each of those heading lines so the linter no longer fails.
kpango
force-pushed
the
doc/add-agents-md-9898591043237542042
branch
3 times, most recently
from
dad9c66 to
5a1dd7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Line 1: Fix the typographical error in the AGENTS.md header: replace "Tesm"
with "Team" in the line "# You are The Vald Development Tesm's Co-Worker" so the
header reads "# You are The Vald Development Team's Co-Worker".
- Around line 248-257: Update the "Repository Map" entry for `pkg/` to
accurately describe that `pkg/` contains internal service implementations rather
than stable public SDKs: replace "Public SDKs and libraries" with a note that
`pkg/` houses service code such as agent handlers, gateway handlers, usecases,
and configuration types, and add a short warning that these are not guaranteed
as a stable public API surface to prevent AI or developers from assuming
stability.
5a1dd7a to
1134105
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Line 292: Update the grammatical error in the AGENTS.md entry for the cmd/
directory: replace the phrase "`Entry points for each components (Agent,
Gateway, Discoverer, etc.).`" with "`Entry points for each component (Agent,
Gateway, Discoverer, etc.).`" so the singular "component" is used; modify the
line containing the `cmd/` description accordingly.
- Around line 35-42: The ordered list in AGENTS.md is broken by an extra blank
line after the "2. **Cross-Language Dependency:**" heading which causes a MD029
linter violation; fix it by removing the blank line immediately following the
"2. **Cross-Language Dependency:**" line so the two sub-bullets remain part of
item 2 (ensure the sub-bullets "- Does a change in Go `apis/proto`..." and "-
Does a change in Rust `libs/`..." remain directly under that heading with
correct indentation), then run the Markdown linter to confirm MD029 is resolved.
- Around line 59-66: Update the two unclear checklist items: replace "Use
`internal/` Libraries As you can: Do not use standard `log` or `errors` or
`sync` or `strings`. Use `github.com/vdaas/vald/internal/**`." with a clearer
sentence like "Prefer internal packages whenever possible: avoid using standard
`log`, `errors`, `sync`, or `strings` and use
`github.com/vdaas/vald/internal/**` instead." and replace "Handle gRPC Errors:
Please do well handled gRPC Richer Error Model." with "Handle gRPC errors using
the gRPC richer error model and provide proper error handling and propagation."
Ensure these changes target the checklist items titled "Use `internal/`
Libraries As you can" and "Handle gRPC Errors" in the MANDATORY PATTERNS
section.
- Around line 193-204: The placeholders in the make examples use curly braces
which can be mistaken for shell parameter expansion; update the examples for the
Vald deploy/delete commands (the make invocation lines using VERSION and
HELM_VALUES) to use conventional angle-bracket placeholders like <version> and
<pr-number> (e.g., change VERSION={vX.Y.Z} and VERSION={pr-XXXX} to
VERSION=<version> and VERSION=<pr-number>) so examples for the make
k8s/vald/deploy and make k8s/vald/delete commands are unambiguous.
bbb3b5a to
5b9a150
Compare
Matts966
left a comment
Matts966
left a comment
There was a problem hiding this comment.
Adding how to write option.go and update helm can help?
LGTM!
|
Thats sounds great, can you add it to the GitHub issues?
|
kpango
force-pushed
the
doc/add-agents-md-9898591043237542042
branch
from
975b076 to
9b3d197
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 35-38: The nested bullets under list item "2. Cross-Language
Dependency:" are not indented, breaking the ordered list (MD029); fix by
indenting the two sub-bullets under "2." (the lines starting with "- Does a
change in Go `apis/proto`..." and "- Does a change in Rust `libs/`...") so they
are children of item 2 (e.g., add consistent two-space or four-space
indentation), preserving the ordered list numbering for subsequent items.
69f0894 to
556262b
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/test/data/hdf5/hdf5.go (1)
183-189:⚠️ Potential issue | 🟠 MajorPre-existing bug:
ris zero-length but indexed — guaranteed panic.Not introduced by this PR, but worth noting:
ris allocated withmake([][]float32, 0), yet the loop writes tor[x][y]. This will panic on any non-empty neighbors slice. Additionally, inner slices are never allocated.If this code path is ever exercised, it needs something like:
Proposed fix
case "neighbors": l := d.GetNeighbors() - r := make([][]float32, 0) - for x := range l { - for y, z := range l[x] { + r := make([][]float32, len(l)) + for x := range l { + r[x] = make([]float32, len(l[x])) + for y, z := range l[x] { r[x][y] = float32(z) } }hack/benchmark/src/singleflight/singleflight_bench_test.go (1)
207-208:⚠️ Potential issue | 🟡 MinorPre-existing typo:
singlefligh→singleflightin CSV filenames.Both
varianceCSVandaverageCSVare missing the trailingt. Since you're already in this function, might be worth fixing.Proposed fix
- varianceCSV = "vald_internal_singlefligh_variance.csv" - averageCSV = "vald_internal_singlefligh_average.csv" + varianceCSV = "vald_internal_singleflight_variance.csv" + averageCSV = "vald_internal_singleflight_average.csv"pkg/agent/internal/kvs/kvs_test.go (1)
1790-1790: 🧹 Nitpick | 🔵 TrivialUnimplemented tests for
bidi.CloseandgetShardIDremain.These are still stubbed out below the
// NOT IMPLEMENTED BELOWmarker. Not a blocker for this mechanical refactor PR, but worth tracking.Would you like me to open an issue to track implementing the
Test_bidi_CloseandTest_getShardIDtest cases?pkg/agent/core/ngt/handler/grpc/index_test.go (1)
2558-2569:⚠️ Potential issue | 🟡 MinorIndexProperty test no longer verifies a non-nil detail value.
The current type comparison is tautological and will pass even when the detail value is nil. This weakens the assertion and can let regressions slip.
✅ Strengthen the assertion (non-nil + optional value check)
- if _, ok := gotRes.Details[name]; ok { - if gotType != wantType { - return errors.Errorf("got_type: \"%s\", want_type: \"%s\"", gotType, wantType) - } - } else { - return errors.Errorf("do not exists key: %s", name) - } + prop, ok := gotRes.Details[name] + if !ok || prop == nil { + return errors.Errorf("missing property for key: %s", name) + } + if wantProp := w.wantRes.Details[name]; wantProp != nil && !reflect.DeepEqual(prop, wantProp) { + return errors.Errorf("got: \"%#v\", want: \"%#v\"", prop, wantProp) + }
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Line 46: Replace the ambiguous heading text "Use `internal/` Libraries As you
can:" with a clearer directive that conveys preference and examples; for the
existing phrase (the string "Use `internal/` Libraries As you can:"), change it
to something like "Prefer Vald's `internal/` packages over standard packages"
and include the explicit guidance that standard packages "log, errors, sync, or
strings" should be replaced with "github.com/vdaas/vald/internal/**" where
possible, ensuring the wording is concise and unambiguous.
- Around line 200-210: The list entry for `tests/v2/e2e` is missing a trailing
period causing inconsistency; update the markdown under "## 5. 🗺 Repository Map
& Dependency Graph" so the `tests/v2/e2e` bullet ends with a period (matching
the other entries) to maintain consistent punctuation and parsing by agents.
In `@hack/benchmark/core/benchmark/strategy/util.go`:
- Around line 52-53: The inner loops that build the `train` slices never execute
because `train` is created with length 0 (make(..., 0, ...)) and the code uses
`len(train)` in the loop condition; change the loop to iterate over the intended
number of items (use `cap(train)` or compute a `count :=
dataset.TrainSize()/bulkInsertCnt` and loop `for j := 0; j < count; j++`) so
training data is actually populated before calling `BulkInsert`; apply this fix
in both insertAndCreateIndex32 (the `train := make([][]float32, 0,
dataset.TrainSize()/bulkInsertCnt)` block) and insertAndCreateIndex64 (the
analogous `train := make([][]float32, 0, dataset.TrainSize()/bulkInsertCnt)`
block).
In `@hack/helm/schema/crd/main.go`:
- Around line 25-26: Remove the redundant standard library import alias strings0
("strings") and replace usages of strings0.Builder with Vald's internal wrapper
by importing and using internal/strings.Builder; update any occurrences
(including the other spots around the file noted) to reference the internal
Builder type instead of strings0 so the code uses the project's internal strings
implementation consistently.
In `@internal/db/storage/blob/s3/writer/writer.go`:
- Around line 116-121: The code incorrectly calls new() with string values
causing compilation errors; add a small shared helper func stringPtr(s string)
*string (e.g., in internal/db/storage/blob/s3/ptr.go) and update the S3 code to
use it: replace new(w.bucket), new(key), new(w.contentType) in the
s3manager.UploadInput construction in writer.go and replace new(r.bucket),
new(key), new("bytes=...") in the getObject method in reader.go with
stringPtr(...) so s3manager.UploadInput and the GetObject calls receive *string
correctly.
In `@internal/errors/errors_test.go`:
- Around line 1327-1332: In uncomparableErr.Error(), remove the stray space
before the method call so use str.WriteString(e.msg) and return the built string
directly by replacing return fmt.Sprint(str.String()) with return str.String();
update the same pattern if present in the companion production method (e.g., in
errors.go) to avoid the redundant fmt.Sprint wrapper and keep the formatting
clean.
- Around line 472-474: Replace the invalid compile-time use of
reflect.TypeFor[untyped nil]() in the test builder with runtime
reflect.TypeOf(nil) so the constructed wantErr matches
ErrInvalidTypeConversion's runtime-value signature (update the anonymous func
that sets wantErr accordingly); also fix the String() implementation on the
error type by removing the stray space in "str .WriteString(e.msg)" to
"str.WriteString(e.msg)" and return str.String() directly instead of wrapping it
in fmt.Sprint(str.String()).
In `@internal/k8s/client/client.go`:
- Around line 279-282: The code uses invalid Go syntax `new(true)` for boolean
pointer fields (e.g., in the Patch call where Force: new(true) and in rotator
where Controller: new(true)); add a small helper `boolPtr(v bool) *bool { return
&v }` (or reintroduce `k8s.io/utils/ptr` and use `ptr.To(true)`) and replace
every `new(true)` with `boolPtr(true)` (target symbols: s.client.Patch call
where Force is set, and the Controller field in the rotator code) so the boolean
pointer fields are constructed correctly and the code compiles.
In `@internal/k8s/vald/mirror/api/v1/target_types.go`:
- Line 47: The Status and its timestamp are being serialized even when unset,
producing zero-time values; update the JSON tags so unset timestamps (and
optionally Status) are omitted: add `omitempty` to the LastTransitionTime json
tag on the MirrorTargetStatus struct (and restore `omitempty` on the parent
Status field in the MirrorTarget type if you want to avoid serializing an empty
status object) so zero-value times are not emitted when controllers do not set
them.
In `@pkg/index/job/readreplica/rotate/service/rotator.go`:
- Line 213: The code incorrectly uses Go's new with a value (new(true)) which
won't compile; replace each occurrence (e.g., the Controller field in the struct
literal where Controller: new(true), and the second occurrence around line 261)
with ptr.To(true) and ensure the pointer utility package is imported (alias ptr
from "k8s.io/utils/pointer" or similar) so Controller receives a *bool via
ptr.To(true).
In `@rust/libs/algorithm/Cargo.toml`:
- Line 27: The Cargo.toml currently pins tonic to "0.14.4" which isn't
published; update the tonic dependency declaration (the line containing tonic =
"0.14.4") to either use the published crate version "0.14.3" or switch to a git
dependency referencing the hyperium/tonic repository and v0.14.4 tag (e.g.,
replace the version string with a git spec) and ensure the same change is
applied consistently across the workspace so builds resolve correctly.
a501d8e to
3ba4874
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
hack/benchmark/src/singleflight/singleflight_bench_test.go (1)
207-208:⚠️ Potential issue | 🟡 MinorPre-existing typo:
singlefligh→singleflightin CSV filenames.Both
varianceCSVandaverageCSVare missing the trailingt. Since you're already touching this function, worth fixing to avoid confusing output artifacts.Proposed fix
- varianceCSV = "vald_internal_singlefligh_variance.csv" - averageCSV = "vald_internal_singlefligh_average.csv" + varianceCSV = "vald_internal_singleflight_variance.csv" + averageCSV = "vald_internal_singleflight_average.csv"hack/helm/schema/crd/main.go (1)
256-274:⚠️ Potential issue | 🟠 MajorUse
internal/stringsconsistently throughout the function for performance and Vald compliance.Line 264 uses
strings0.Builderwhile lines 265 and 271 use unqualifiedstrings.Splitandstrings.TrimRight. Beyond the inconsistency, the function has two performance issues:
- Lines 257–261: Indent construction via
+=loop is O(n). Replace withstrings.Repeat.- Lines 270–273: Second branch still accumulates via
result +=inside a loop (O(n²)), while the first branch usesstrings.Builder. Both branches must usestrings.Builderfor consistency and performance.Since
internal/stringsis already imported and re-exportsBuilder,Repeat,Split, andTrimRight, refactor to use it throughout:Suggested refactor
func indent(text string, nindent int) string { - indent := "" - - for range nindent { - indent += " " - } + indent := strings.Repeat(" ", nindent) if text[len(text)-1:] == "\n" { - var result strings0.Builder + var result strings.Builder for _, j := range strings.Split(text[:len(text)-1], "\n") { result.WriteString(indent + j + "\n") } return result.String() } - result := "" + var result strings.Builder for _, j := range strings.Split(strings.TrimRight(text, "\n"), "\n") { - result += indent + j + "\n" + result.WriteString(indent + j + "\n") } - return result[:len(result)-1] + s := result.String() + return s[:len(s)-1] }This eliminates the confusing
strings0alias, makes both branches consistent, and aligns with Vald's requirement to use internal packages for common functionality..github/actions/scan-docker-image/action.yaml (1)
31-31: 🧹 Nitpick | 🔵 TrivialPre-existing:
trivy-action@masteris unpinned.Not introduced by this PR, but
aquasecurity/trivy-action@masteron lines 31 and 39 lacks a SHA pin—unlike thecodeql-actionreference you just hardened. Consider pinning it to a specific commit hash in a follow-up for consistent supply-chain posture across this composite action.internal/sync/singleflight/singleflight_test.go (1)
251-261: 🧹 Nitpick | 🔵 TrivialMixed
wg.Goand manualAdd/Doneon the same WaitGroup is fragile.The first goroutine uses
wg.Go(func() { ... })(line 251), while the second (lines 256–261) uses the manualwg.Add(1)/wg.Done()pattern on the samewg. This works ifGointernally callsAdd(1)before spawning, but mixing idioms on the same group is easy to miscount during future edits.If the
time.Sleepordering is the reason for keeping the manual pattern, consider wrapping the sleep inside thewg.Goclosure instead:Suggested consolidation
- wg.Go(func() { + wg.Go(func() { got, gotShared, err = g.Do(a.ctx, a.key, fn1) }) // call with the same key but with another function - wg.Add(1) - time.Sleep(time.Millisecond * 100) - go func() { + wg.Go(func() { + time.Sleep(time.Millisecond * 100) got1, gotShared1, err1 = g.Do(a.ctx, a.key, fn2) - wg.Done() - }() + })hack/benchmark/core/benchmark/strategy/util.go (1)
45-48:⚠️ Potential issue | 🟡 MinorCapacity of
idsis inflated by a factor ofbulkInsertCnt.
idsis allocated asmake([]uint, 0, dataset.TrainSize()*bulkInsertCnt). WithbulkInsertCnt = 1000, each iteration insertsTrainSize()/1000elements, so the total across 1000 iterations isTrainSize(). The capacity should bedataset.TrainSize(), notdataset.TrainSize()*bulkInsertCnt. Same issue exists at line 80.pkg/agent/core/ngt/handler/grpc/insert_test.go (2)
811-862:⚠️ Potential issue | 🟡 MinorDuplicate test case: "Boundary Value Testing case 4.2" appears twice.
Lines 759–810 and 811–862 define identical test cases with the same name, args, fields, and expected error. This means the same scenario runs twice, wasting cycles and masking what should be a distinct test (e.g., case 5 per the comment plan at line 117, which describes NaN testing as case 5 for float32). One of these should be removed or replaced with the intended distinct case.
2758-2773:⚠️ Potential issue | 🔴 CriticalBug:
errors.Joinreturn value is discarded —joindErris alwaysnil.
errors.Joinreturns a new error; it does not mutate its arguments. The result is never assigned back, so the wrapped status error always has anilcause. This silently breaks error matching in tests that rely ongenAlreadyExistsJoinedErr.🐛 Proposed fix
genAlreadyExistsJoinedErr := func(uuids []string, req *payload.Insert_MultiRequest, name, ip string) error { - var joindErr error + var joinedErr error for _, uuid := range uuids { - errors.Join(joindErr, errors.ErrUUIDAlreadyExists(uuid)) + joinedErr = errors.Join(joinedErr, errors.ErrUUIDAlreadyExists(uuid)) } return status.WrapWithAlreadyExists(fmt.Sprintf("MultiInsert API uuids %v already exists", uuids), - joindErr, + joinedErr, &errdetails.RequestInfo{
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 169-175: The fenced code block under the "#### Level 1: Unit Tests
(Fast)" heading in AGENTS.md has an extra trailing blank line; edit that code
block (the one containing "make test" and "make test/rust") and remove the blank
line so the closing ``` directly follows the last command, ensuring the code
block has no trailing blank line.
- Around line 140-165: Remove the trailing empty line inside the fenced bash
code block under the "Deployment & Operations" section so the closing ```
immediately follows the last make command; locate the code block containing the
make commands (e.g., "make VERSION=<version> k8s/vald/deploy
HELM_VALUES=example/helm/values.yaml" and the example lines) and delete the
blank line before the final fence.
In `@internal/db/storage/blob/s3/reader/reader.go`:
- Around line 169-176: The code incorrectly uses new(...) with string
expressions (e.g., constructing rng and passing new(r.bucket), new(key)) which
doesn't compile; change these to create *string values using aws.String(...) (or
a local helper that returns *string) and use that for Range, Bucket, and Key
when calling r.service.GetObjectWithContext; update the rng variable creation
(used in log.Debugf and GetObjectInput.Range) and the Bucket/Key arguments to
use aws.String so GetObjectWithContext, r.bucket, and key are passed as *string
values.
In `@internal/sync/semaphore/semaphore_bench_test.go`:
- Around line 86-93: The benchmark currently discards the error returned by
sem.Acquire(context.Background(), size) which can mask failures and skew
results; modify the loop that calls sem.Acquire to capture its returned error
(from Acquire) and handle it by failing the benchmark on error (e.g., call
b.Fatal or b.Fatalf) so any Acquire failure aborts the benchmark; keep the
sem.Release(size) calls unchanged and continue using N and size as before.
98d96cf to
8d75adb
Compare
github-actions
bot
added
area/discoverer
area/helm
area/agent/sidecar
size/XXXL
and removed
size/XXL
labels
kpango
force-pushed
the
doc/add-agents-md-9898591043237542042
branch
11 times, most recently
from
6f45ffa to
edfea50
Compare
|
|
kpango
force-pushed
the
doc/add-agents-md-9898591043237542042
branch
from
a63879c to
6df12da
Compare
Signed-off-by: kpango <kpango@vdaas.org>
kpango
force-pushed
the
doc/add-agents-md-9898591043237542042
branch
from
6df12da to
78b051f
Compare
5 participants
Add AGENTS.md for AI Co-Worker standardization
AGENTS.mdandGEMINI.mdconcepts into a single comprehensive guide.k3d,helm, ande2e/v2targets.PR created automatically by Jules for task 9898591043237542042 started by @kpango
Summary by CodeRabbit